434e6b68 | fix(routers): accept catalog ids that begin with the router's own slug | Amal | 2026-08-18 | ↗ GitHub |
commit body Saving OpenRouter's "openrouter/auto" (or Vercel's "vercel/v0-1.5-md")
used to 400 the entire profile PATCH, and the Word add-in sent a
different model string than the web app for the same stored selection.
WHY THIS MATTERS
Router catalogs are not namespaced away from the router's own brand:
OpenRouter really lists "openrouter/auto" and Vercel lists "vercel/v0-*"
models. The backend validator stripped a leading "<router>/" from every
submitted id BEFORE checking the vendor/model shape, so "openrouter/auto"
became "auto", failed the shape check, was dropped, tripped the
length-mismatch guard, and the whole settings save failed - a user could
not select those models at all.
WHAT IS THE CANONICAL STORED FORM
The database row stores the router's RAW catalog id (what the gateway's
/models endpoint returns), and the app-level model id is always
"<router-slug>/<catalog-id>" built by plain prefixing - so OpenRouter's
"openrouter/auto" travels as "openrouter/openrouter/auto". The adapter
strips exactly one namespace segment before calling upstream, which
round-trips this correctly (pinned by new llmModels tests).
HOW IT WORKS
- backend normalizeRouterModels (and the frontend settings typeahead's
new normalizeTypedModelId twin) strip the router prefix only when the
remainder is still a full vendor/model id; otherwise the raw id is
validated and stored verbatim.
- the Word add-in's ModelToggle no longer "defensively" strips an inner
router prefix when building option ids; both clients now share the
same prefix-verbatim option builders, and a new cross-package parity
test (frontend/src/wordAddin/catalogParity.test.ts) pins that the two
clients emit the identical composer model string for the same stored
selection.
Tests: userRouterModels.test.ts (PATCH accepts "openrouter/auto" /
"vercel/v0-1.5-md", still canonicalizes composer-form ids),
RouterSettingsSection.test.tsx (typing "openrouter/auto" + Enter adds it
verbatim), llmModels.test.ts (resolveModel + single-segment strip),
catalogParity.test.ts (web/add-in composer-string parity). The backend
and UI tests fail against the previous unconditional strip.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
af64aca5 | fix(routers): enforce the user's saved selection on the env-key spend path | Amal | 2026-08-18 | ↗ GitHub |
commit body resolveModel accepts router-prefixed ids by SHAPE alone, so any
authenticated user could hand-craft a chat/title/tabular request naming
any `openrouter/x/y` or `vercel/x/y` model and it would run - on the
operator's env key when one is configured.
WHY THIS MATTERS (what a cost-abuse vector via unvalidated model ids is)
On server-key deployments the operator pays per token for whatever model
a request names. The Settings UI curates a small allowlist per user, but
nothing at request time consulted it: the model string in the request
body went straight to the gateway. A user (or a leaked JWT) could point
every request at the most expensive frontier model on the gateway's
catalog and run up the operator's bill - a classic unvalidated-
identifier cost-abuse vector. First-party models don't have this
problem because resolveModel checks them against a closed catalog.
HOW IT WORKS
- routerModels.resolveRequestedModel is the single request-time choke
point: resolve the id as before, then, only for router-prefixed
results, require membership in that user's saved user_router_models
selection; otherwise warn and degrade to the caller's default -
exactly the path an invalid model id already takes. BYOK users get the
same rule (uniform semantics; their saved list is one Settings save
away).
- runLLMStream (chat, project chat, Word chat, tabular streams) awaits
it where it previously called resolveModel inline.
- getUserModelSettings applies the same membership guard to the stored
title/tabular preferences using the selections it already fetched, so
the profile row can't smuggle an unselected router model either.
Tests: streamingModelAllowlist.test.ts (saved member passes through to
the adapter; non-member falls back to the default, with and without a
BYOK key; first-party models skip the lookup) and userSettings.test.ts
(stored router preference outside the selection falls back). Verified by
stashing this fix: 4 of the new tests fail on the ported code because
the unselected router model reaches the adapter / settings unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
cf00ee86 | fix(routers): tolerate a missing user_router_models table on older databases | Amal | 2026-08-18 | ↗ GitHub |
commit body A deploy-before-migrate window used to 500 GET /user/profile and every
chat/title/tabular request, because the new router-selection read threw
and nothing downstream caught it.
WHY THIS MATTERS
Rolling deploys routinely run new application code against a database
that has not received the newest migration yet. This codebase already
plans for that: selectProfile has a whole fallback cascade for profile
columns that don't exist yet (42703 undefined_column). The router-models
read had no such tolerance, so the profile page and the assistant were
both bricked until 20260818_01 landed - the new feature's absence took
down old features.
HOW IT WORKS
getUserRouterModels now recognizes the two shapes a missing relation
takes on this stack - Postgres undefined_table (SQLSTATE 42P01) and
PostgREST's schema-cache miss (PGRST205 naming user_router_models, the
table-level analog of the 42703 column shape selectProfile checks) -
and returns an empty selection, warning once per process so operators
see the migration is outstanding. Every other error still throws:
fail-open is deliberately scoped to "the table does not exist yet",
not to arbitrary database failures.
Tests: routerModels.test.ts - both missing-table shapes resolve to []
(fail on the ported code, which rejects), and an unrelated error code
still rejects, pinning the narrow scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
09757c44 | fix(settings/routers): Enter adds the typed model id, never a lookalike | Amal | 2026-08-18 | ↗ GitHub |
commit body Typing the complete, valid id "qwen/qwen-2" and pressing Enter used to
silently add "qwen/qwen-2.5-72b-instruct" instead - the catalog row that
substring-matched the query.
WHY THIS MATTERS
This combobox is the write path for the router allowlist that the
backend now enforces at request time, so what lands in it decides which
models a user can run at all. An Enter that swaps the intended id for a
lookalike writes the wrong model into that allowlist without the user
noticing - the labels differ by a truncated suffix - and with per-token
pricing varying by orders of magnitude between such neighbors, "close"
is not good enough.
WHAT AN IMPLICIT HIGHLIGHT IS - AND WHY IT'S GONE
onChange used to point the active row at index 0 whenever anything
matched, and Enter preferred the active row. That means merely TYPING
claimed a highlight the user never asked for. Now the highlight only
ever follows an explicit gesture - ArrowDown/ArrowUp or pointer hover -
including when the list is opened from the chevron.
HOW ENTER RESOLVES NOW
1. explicit highlight → add that catalog row (unchanged);
2. no highlight + id-shaped text → add the typed id verbatim
(normalizeTypedModelId, sharing F1's slug-preserving rule);
3. no highlight + non-id text → silent no-op, so Enter mid-search never
errors and never adds anything.
The "Press Enter to add this model ID." hint now appears whenever the
typed text is id-shaped - previously it only showed at zero matches,
exactly the case where the trap couldn't fire.
Tests: RouterSettingsSection.test.tsx - the exact qwen scenario (typed
id added verbatim, no aria-activedescendant after typing, hint visible
alongside matches) and the Enter-no-op case. Verified by stashing the
fix: both fail on the ported code (the first adds the 72b lookalike).
Arrow-key + Enter selection keeps its existing passing test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
728c4a9f | fix(composer): three-state model availability - never brick on unknown keys | Amal | 2026-08-18 | ↗ GitHub |
commit body The ported availability gate treated "we don't know the key state" the
same as "there are no keys": the Word add-in's composer stuck on
"No API Key" after one flaky WKWebView preflight, and the web toggle
flashed "No API Key" on every page load while the profile was in flight
(a render test even asserted the flash).
WHAT FAIL-OPEN VS FAIL-CLOSED MEANS HERE
The key-status preflight is a UX hint, not the security boundary - the
backend authoritatively rejects any model it has no key for (and, since
the allowlist commit, any router model outside the user's selection).
Failing CLOSED on a hint means one dropped fetch blocks sends the
backend would have accepted, with no recovery except reloading the
pane. Failing OPEN on an UNKNOWN state costs nothing: the worst case is
a request the backend answers with a clear error. So unknown now fails
open, while a successfully LOADED status still gates exactly as before
- fail-closed is preserved where the information is real.
HOW IT WORKS - three states in both clients
- loading: neutral, disabled trigger showing the selected model's label
(never "No API Key"); submits are not blocked.
- loaded: unchanged - filter models by configured providers, show
"No API Key" when a real loaded status has none.
- failed: retry once with backoff (loadWithRetry in the add-in; an
inline retry in UserProfileContext on the web), then console.warn and
fail open. The web context exposes `apiKeysDegraded` so ChatInput,
TRChatPanel and TabularReviewView pass `undefined` (unknown) instead
of the fallback profile's all-false key map to their gates.
Tests: composerAvailability.test.ts (add-in isModelAvailable fails open
on null, still gates on loaded state; loadWithRetry retries once,
reports the final error, resolves null) and ModelToggle.render.test.tsx
(loading renders neutral+disabled; unknown-after-failure renders
enabled without "No API Key"; loaded no-keys still says "No API Key").
The old test that ENSHRINED the flash (asserting "No API Key" for
apiKeys-undefined) is rewritten. Verified pre-fix via stash: 3 tests
fail on the ported fail-closed code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
3d43970e | fix(routers): hardening batch - adapter, catalog route, and stale selections | Amal | 2026-08-18 | ↗ GitHub |
commit body Five smaller router-stream fixes that share one theme: the edges of the
feature (proxies, truncated streams, renamed ids, removed selections)
must degrade predictably instead of silently doing the wrong thing.
1. CATALOG ROUTE HONORS OPENROUTER_BASE_URL
The chat adapter already routes through the override (proxies, test
doubles, region mirrors); the catalog route hardcoded the public URL,
so a proxied deployment listed models it could not reach. The route
now reads the env per request, exactly like the Vercel route.
2. TRUNCATED TOOL-CALL ARGUMENTS FAIL THE STREAM
WHY THIS MATTERS: tool-call arguments stream as JSON fragments; when
the connection dies mid-call the fragment can never parse. Coercing
it to {} EXECUTED the tool - a side-effecting document edit could run
with empty input because a proxy hiccuped. parseToolCalls now throws
a descriptive error into the same failure path as a mid-stream
{"error"} chunk. Absent arguments ("" - parameter-less tools) still
mean {}: only present-but-unparseable input is fatal.
3. FLUSH THE DECODER AND RESIDUAL SSE LINE AT END-OF-STREAM
HOW SSE FRAMING BREAKS: the parser split on "\n", so a proxy that
closes the connection without a trailing newline stranded the final
"data:" line - and its content delta - in the buffer. On done, the
loop now flushes the TextDecoder (multi-byte sequences included) and
processes the residual line through the same extracted line handler.
4. LEGACY_MODEL_IDS KEEPS RENAMED STATIC IDS WORKING
gemini-3.1-flash-lite-preview → gemini-3.5-flash-lite and
gpt-5.4-lite → gpt-5.4-mini are mapped on read in backend
resolveModel, in the settings/models page, and in useSelectedModel -
a preference saved before the rename resolves to the model's new id
instead of silently degrading to the fallback.
5. STALE ROUTER SELECTIONS RESET TO THE DEFAULT
useSelectedModel now takes the loaded router lists; a stored
`openrouter/*`/`vercel/*` selection no longer in them resets to the
default model (persisted), mirroring how invalid first-party ids are
replaced on read - the composer no longer silently sends an id the
backend will reject-and-degrade. While the lists are loading, the
selection is left untouched.
Tests (each fails on the ported code, verified via stash - 4 backend +
3 frontend failures): models.test.ts (base-URL override),
openrouter.test.ts (truncated arguments reject and runTools is never
called; final newline-less delta is delivered), llmModels.test.ts
(legacy ids resolve to their new ids), settings models page.test.tsx
(legacy titleModel renders the renamed option), useSelectedModel.test.ts
(missing router selection resets, present one survives, loading leaves
it alone).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
f465ea32 | fix(db/routers): concurrency lock for replacing a router's model selection | Amal | 2026-08-18 | ↗ GitHub |
commit body Migration 20260818_01 is already MERGED, so it may have run on a real
deployment. A migration that has run is history: editing it in place changes
nothing on the databases that already applied it, while quietly disagreeing
with what they actually contain. The lock therefore ships FORWARD as a new
dated step, 20260819_01_harden_replace_user_router_models.sql, which re-runs
`create or replace function` with the hardened body. 20260818_01 is left
byte-for-byte as merged. schema.sql - the fresh-install path - carries the
hardened definition directly, so both real installation paths converge and
the schema-drift check stays green.
1. ADVISORY TRANSACTION LOCK IN replace_user_router_models
WHAT AN ADVISORY TRANSACTION LOCK IS: Postgres lets an application
take a lock on an arbitrary number it chooses (pg_advisory_xact_lock)
rather than on a table or row. Sessions that pick the same number
queue behind each other; everyone else is untouched; the lock
releases itself at commit/rollback, so it cannot leak.
WHY IT'S NEEDED: the function replaces a selection by delete+insert.
Two overlapping PATCHes for the same user+router could interleave -
both delete, both insert - and the second insert dies on the
(user_id, router, model_id) unique constraint as a 500. Locking on
hashtext(user_id:router) serializes exactly those two requests
(last writer wins) with zero effect on other users or routers.
2. THE LEGACY DATA COPY IS LEFT ALONE, DELIBERATELY
An earlier draft of this fix also sanitized 20260818_01's copy of the
old user_profiles.openrouter_models array (btrim, drop values the new
CHECK constraints reject, de-duplicate, cap at 50). That improvement
cannot be shipped forward and is no longer reachable:
- It only ever mattered as a way to stop 20260818_01 itself from
aborting on messy legacy data. 20260818_01 now runs first on every
deployment regardless, so a sanitized re-run in a later migration
could only execute AFTER the unsanitized one had already succeeded
- where it is a guaranteed no-op - or never, because a failed
migration stops the deploy before any later file is reached.
- The copy is guarded by `if exists (... column openrouter_models)`,
and no migration in this repository ever creates that column. It is
a shim for an unreleased intermediate state, so on every normal
deployment the guard is false and the block does nothing at all.
Residual risk, recorded rather than hidden: a database that really does
carry the legacy column with values the new constraints reject will
fail on 20260818_01, and that can now only be resolved by cleaning the
data - not by us. No such database is known to exist.
3. HONEST 400 FOR OVERSIZED PAYLOADS
A >50-model PATCH used to be truncated by the normalizer and then
reported as "invalid or duplicate model ID". The route now checks
the cap first and says "can include at most 50 models".
Validation: the hardened function body is unchanged from the version that
was executed against a live Postgres inside a rolled-back transaction when
this fix was first written - only its delivery vehicle moved, from an
in-place edit of 20260818_01 to a new migration. The function block in the
new migration and in schema.sql is diffed byte-identical. The cap message is
pinned by
userRouterModels.test.ts ("reports the 50-model cap"), which fails on the
ported code with the misleading message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
c03145f3 | test(word-addin): cross-package drift guard for the hand-mirrored catalog | Amal | 2026-08-18 | ↗ GitHub |
commit body The Word add-in mirrors the web app's model catalog by hand
(word-addin/src/taskpane/lib/modelCatalog.ts carries a "keep in sync"
comment and nothing enforcing it). This adds the enforcement.
WHY THIS MATTERS
Two hand-maintained copies of the same catalog WILL drift: an id gets
renamed on the web, a label is tweaked in the add-in, a defensive strip
sneaks into one client's option builder (exactly what the F1 commit
removed). Every such drift ships two different products - a model the
web offers that the add-in doesn't, a composer that sends a different
model string for the same stored selection - and nothing red-flags it
until a user notices. A shared package is the real fix; extracting one
is out of scope for this stack and flagged as a follow-up in the PR
body. Until then, CI has to catch the drift.
HOW IT WORKS
frontend/src/wordAddin/catalogParity.test.ts imports BOTH catalogs (the
frontend test runner transforms the add-in's TypeScript across the
package boundary; the add-in's modelCatalog imports its ApiKeyStatus
type from the office-free api/client so the compile graph stays clean)
and pins:
- the add-in's STATIC_MODELS equal the web MODELS (id, label, group);
- both clients share DEFAULT_MODEL_ID;
- modelDisplayName renders identical output for every shared id,
router-namespaced and ollama forms included;
- the option builders emit the identical composer model id for the same
stored selection (including router-slug catalog ids);
- availability gating agrees: for each provider configured alone, every
shared id is available in the add-in iff it is available on the web.
Sanity-checked by hand-editing one add-in label ("GPT 5.5 Turbo") and
watching the suite fail; the composer-string case is the same assertion
that pinned the F1 divergence.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
e414b530 | fix(composer): a degraded profile must not wipe the saved model | Amal | 2026-08-18 | ↗ GitHub |
commit body Found by adversarial review of this stack before it was pushed: one
dropped GET /user/profile permanently cleared the user's composer model
selection.
WHY THIS MATTERS
The stale-selection reset added earlier in this branch is destructive by
design - when a stored `openrouter/*` / `vercel/*` id is absent from the
user's saved router lists, useSelectedModel rewrites localStorage to the
default so the composer never sends an id the backend would reject. That
is right when the lists are TRUE. It is wrong when they are UNKNOWN.
HOW THE WIPE HAPPENED
UserProfileContext answers a failed profile fetch (after its one retry)
with a local fallback profile: `apiKeysDegraded` goes true and every
field is a placeholder, including `openRouterModels: []` and
`vercelModels: []`. ChatInput passed those empty arrays into
useSelectedModel exactly as if they had loaded, the reset effect saw the
stored router id "missing from the saved lists", and persisted the
default over it. The user's pick was gone for good - reconnecting could
not bring it back, because the evidence had been overwritten.
THE FIX
ChatInput passes `null` (the hook's "still loading, leave it alone"
signal) whenever the profile is degraded, so the reset runs only on a
profile the server actually answered. A healthy profile that genuinely
reports no saved router models still resets, unchanged. The
`apiKeysDegraded` doc comment is widened to say what it really means:
NOTHING on a degraded profile is an answer, so no normalization may key
off it.
Not fixed here, by design: a database that predates the
20260818_01 migration answers with real empty lists (the deploy-before-
migrate tolerance in getUserRouterModels), so a router selection does
reset there. That is a genuine "you have no saved router models" state
from the server's point of view, and the selection would not work
anyway until the migration lands.
Tests: ChatInput.modelSelection.test.tsx renders the real hook behind a
degraded and a healthy profile. Pre-fix the degraded case fails with
`Expected "openrouter/pricy/frontier" / Received "gemini-3-flash-preview"`
in localStorage; the healthy-empty case passes before and after, pinning
that the reset itself was not weakened.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
dd7cd96d | fix(routers): a tool call with no arguments needs a clean end-of-stream | Amal | 2026-08-18 | ↗ GitHub |
commit body Second pass of the adversarial review on this stack: the truncated-
arguments guard added earlier in the branch left one hole open, and it
is the hole that runs a side-effecting tool.
WHY THIS MATTERS
"Arguments present but unparseable" already fails the stream. But the
hole was the case with NO argument bytes at all. The carve-out read
`if (partial.arguments.trim())` - so an empty string skipped every check
and the call executed with `input: {}`. A stream that died right after
the `function.name` delta and before the first `{` therefore ran
delete_document (or any tool whose no-argument form is meaningful) with
empty input, which is exactly the failure mode the guard was written to
prevent.
WHY THE EMPTY STRING CANNOT DECIDE THIS
A parameter-less tool also streams "" - the two cases are byte-identical
in the accumulated arguments. The only thing that tells them apart is
whether the upstream ever said why it stopped. So the carve-out is
re-keyed on a termination signal instead of on the payload:
`endedCleanly` is set when the SSE `[DONE]` sentinel arrives or when a
choice carries `finish_reason: "tool_calls"`. Empty arguments plus a
clean end still mean {}; empty arguments after a silent socket close now
throw the same descriptive error as malformed JSON.
Handling `[DONE]` as a signal rather than a skip also tightened the line
parser: the sentinel and an empty `data:` line are now separate branches
instead of one combined early return.
Tests: openrouter.test.ts gains the died-before-arguments case (no
[DONE], no finish_reason - must reject and never call runTools) and a
parameter-less-tool case that pins {} still executes when the stream
terminated cleanly. Pre-fix the first test does not merely fail its
assertion - it proves the bug, erroring with "results is not iterable"
because runTools WAS invoked for the aborted call.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
3849a329 | fix(chat): resolve the request's model inside the stream's error boundary | Amal | 2026-08-18 | ↗ GitHub |
commit body Adversarial review of this stack, pre-push: the router allowlist check
added earlier in the branch sat one line above the try block it belonged
in.
WHY THIS MATTERS
runLLMStream's `try` is not decoration - it is the whole error contract
of an SSE turn. Inside it, a failure flushes the partial assistant text,
pushes an `{ type: "error" }` event that the route persists with the
turn, and throws AssistantStreamError carrying both. Outside it, a
failure is a bare rejection: no error event, no flushed text, and the
client is left with a stream that simply stops.
`resolveRequestedModel` reads user_router_models. That is a database
call, so it can fail for every ordinary reason a database call fails -
a statement timeout, a connection reset, a paused project. Placed above
the try, one such blip took the un-instrumented path.
HOW IT WORKS NOW
The call moves inside the try, immediately after the abort check.
Behavior on the happy path is identical (the resolved model is still
what reaches streamChatWithTools); the only change is which machinery
handles a failure - now the same one that handles a mid-stream adapter
error.
Tests: streamingModelAllowlist.test.ts gains a case whose
user_router_models read returns a 57014 statement-timeout error and
asserts the rejection is an AssistantStreamError carrying an error
event. Pre-fix it fails with "expected { code: '57014', ... } to be an
instance of AssistantStreamError" - the raw PostgREST error object
escaping, which is exactly the bug.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
8fdeac67 | fix(chat): an explicitly requested router model fails loudly, not silently | Amal | 2026-08-18 | ↗ GitHub |
commit body Adversarial review of this stack, pre-push: the allowlist choke point
protected the operator's wallet but lied to the user.
WHY THIS MATTERS
When a request body names `openrouter/pricy/frontier` and that model is
not in the user's saved selection, the previous behaviour was to answer
anyway - on gemini-3-flash-preview - with nothing in the response saying
so. The user asked one model a question and got another model's answer,
attributed to the one they picked. That is worse than an error: it is
undetectable from the client, and it makes model comparison, cost
reasoning, and bug reports meaningless.
WHY A SILENT FALLBACK IS STILL RIGHT ELSEWHERE
There are two different situations behind "model not in selection":
- The user named it in THIS request. They are present, they can act, and
the wrong answer is expensive. Tell them.
- A stored preference names it (title_model / tabular_model, resolved by
getUserModelSettings). The user set it long ago, is not watching, and
the alternative to degrading is bricking every background title
generation. Degrade and warn the operator.
So `resolveRequestedModel` gains an explicit `onOutsideSelection` mode
rather than one hard-coded policy. runLLMStream - reached from chat,
project chat and Word chat with the request body's model - passes
"throw"; getUserModelSettings keeps its own silent guard for stored
preferences, untouched. Tabular reaches runLLMStream with a preference
getUserModelSettings has ALREADY normalized, so it cannot trip the throw.
WHAT THE USER SEES
"Model openrouter/pricy/frontier is not in your saved OpenRouter models
- add it in Settings → BYOK → Routers." Because R3 moved the resolution
inside the stream's try block, that message arrives as a normal SSE
error event and is persisted with the turn, not as a dead socket.
Tests: streamingModelAllowlist.test.ts's two fallback cases become
rejection cases (and assert the adapter is never called), plus a new
case pinning the message onto the stream's error event;
routerModels.test.ts pins both modes directly. Pre-fix, all four fail -
"promise resolved 'gemini-3-flash-preview' instead of rejecting" for the
unit case and "promise resolved undefined instead of rejecting" for the
stream cases.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
a63a615e | fix(word-addin): map retired model ids, and pin that mapping in the guard | Amal | 2026-08-18 | ↗ GitHub |
commit body Adversarial review of this stack, pre-push: the rename-compat fix landed
in three places and missed the fourth.
WHY THIS MATTERS
The Word pane and the web composer store the user's model choice under
the SAME localStorage key, "mike.selectedModel" - same key name, same
value shape, written by whichever client the user touched last. The
hardening batch earlier in this branch taught the backend, the settings
page and the web composer to map retired ids (gemini-3.1-flash-lite-
preview → gemini-3.5-flash-lite, gpt-5.4-lite → gpt-5.4-mini) on read.
The add-in was not taught, so the same stored string resolved to the
renamed model in one client and to the fallback in the other.
THE PORT
`LEGACY_MODEL_IDS` and `canonicalModelId` are mirrored into the add-in's
modelCatalog, and its useSelectedModel canonicalizes both on read (the
stored value) and on write (an id handed in by the picker) - the same
two points the web hook does.
WHY THE GUARD MATTERS MORE THAN THE PORT
This class of bug is not "someone forgot"; it is structural. The add-in
mirrors the web catalog BY HAND, so every future rename can drift again.
catalogParity.test.ts already compared the model rows, labels, default
id and key gating; it now also compares LEGACY_MODEL_IDS entry-for-entry
and the accepted-id surface (isAllowedModelId) across the two packages.
To compare the accepted-id surface honestly rather than restating the
rule in the test, the web hook's private `isAllowed` becomes an exported
`isAllowedModelId` - the same name the add-in already uses.
Tests: the two new parity cases. Pre-fix the legacy-id case fails with
"expected undefined to deeply equal { gemini-3.1-flash-lite-preview: ... }"
- the add-in simply had no such export. The accepted-id case passes
before and after; it is a guard against the NEXT divergence, not a proof
of this one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
57cf3b11 | fix(routers): four small edges - error shape, lock key, typeahead feedback | Amal | 2026-08-18 | ↗ GitHub |
commit body The low-severity residue of the adversarial review of this stack,
batched because each is a few lines and none changes a contract.
1. 42P01 ALONE DOES NOT MEAN "OUR TABLE IS MISSING"
The deploy-before-migrate tolerance in getUserRouterModels swallowed
any undefined_table error and answered "no saved models". But 42P01
is raised for whatever relation was missing - a policy, view or
trigger reaching for some other dropped table raises it from this
query too. The PGRST205 arm already required the message to name
user_router_models; the 42P01 arm now does the same, so a genuine
schema fault surfaces instead of quietly reading as an empty
selection.
2. ADVISORY LOCKS ARE KEYED WITH hashtextextended IN THIS REPO
replace_user_router_models keyed its per-(user, router) advisory lock
with hashtext(...) - an int4 hash. Every other advisory lock in this
schema uses hashtextextended(..., 0), an int8. Two lock keys collide
when their hashes collide, and collisions in a 32-bit space are not
exotic: unrelated writers would then serialize on each other for no
reason. Changed in BOTH of the places that now carry the hardened
definition - backend/migrations/20260819_01_harden_replace_user_router_
models.sql (the forward-shipping step for existing deployments) and
backend/schema.sql (the fresh install) - kept byte-identical between
them, so the function body diffs clean. The already-merged
20260818_01 is not touched.
3. ENTER THAT DOES NOTHING MUST SAY WHY
The typeahead's "Enter adds the typed id" path returned silently
whenever the text was not id-shaped. From the user's side that is
indistinguishable from a dead key. Enter on non-empty, non-id text
now sets the same error line the save failure uses; an empty box
stays silent, because nothing was asked for.
4. TWO SMALLER EDGES IN THE SAME COMPONENT
- ARIA: the "Press Enter to add this model ID" hint and the "No
matching models" placeholder lived INSIDE role="listbox", which
admits only option/group children. Both move out to the surface
around it; the listbox now holds options only, so the indices a
screen reader announces line up with aria-activedescendant.
- The 200-character model_id CHECK from the migration is mirrored
into normalizeTypedModelId, so an over-long paste fails in the box
with "Model IDs are at most 200 characters." instead of as an
opaque 400 from the profile PATCH.
Also here: the stale-selection effect added earlier in this branch trips
react-hooks/set-state-in-effect, which is an ERROR in this config - the
frontend lint gate was red on this branch. Suppressed with the reason
(it reconciles state against asynchronously arriving data, and the
functional update is a no-op unless the selection is genuinely stale).
Tests: routerModels.test.ts (42P01 naming a different relation must
reject - pre-fix "promise resolved '[]' instead of rejecting");
RouterSettingsSection.test.tsx (the no-op-Enter case becomes a
feedback case, plus an over-long-id case, a hint-outside-the-listbox
case, and a normalizeTypedModelId length boundary at exactly 200). All
five fail pre-fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|