You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(byok): honor compat.openRouterRouting for BYOK custom providers - #1
Implements the config half of MiniMax-AI#288: honor compat.openRouterRouting for BYOK custom_provider models.
The pi transport already supports this end to end — third_party/pi-mono/packages/ai/src/providers/openai-completions.ts sends model.compat.openRouterRouting as the request provider field, and OpenRouterRouting is fully typed in @earendil-works/pi-ai. The only gap was the BYOK config reader: readModelCompat() accepts each compat key at its declared type, and openRouterRouting was not among them, so the field was dropped before it reached the model.
Two files:
contracts.ts — declares openRouterRouting?: OpenRouterRouting and vercelGatewayRouting?: VercelGatewayRouting on LocalModelCompatOverrides, importing both types from pi-ai rather than re-declaring them so the config shape cannot drift from the transport.
openRouterRouting is the one object-valued compat field, so it needs a structural reader rather than the existing boolean/enum handling. It follows the rule already documented on readModelCompat(): a value of the wrong type is dropped rather than forwarded, because pi treats any present field as an explicit override and a truthy "false" would invert the intended behavior.
The three shapes are handled differently, deliberately:
shape
bad member means
behavior
scalar (zdr, data_collection, …)
the key is not the declared type
drop that key
list (only/order/ignore/quantizations)
the provider set changes — a different routing decision
reject the whole list
sort object
the sort strategy changes
reject the whole object
record (max_price, percentiles)
one optional constraint stops applying; the rest apply as written
drop the unknown/bad key, keep the valid ones
A list is one declared value, so a non-string element rejects it — only: ["DeepSeek", 42] is not silently shrunk to ["DeepSeek"], which would pin a different provider set than the one written.
Records iterate their known keys, so an undeclared key is never forwarded: max_price: {prompt, surge} does not send surge, and preferred_min_throughput: {p50, p95} does not send p95 (not in p50/p75/p90/p99).
max_price.* takes a number or a decimal string. A string that does not parse as one ("free", "$5") is dropped instead of being forwarded as a ceiling. Non-finite values are dropped in both forms, including the exponent shape ("1e999" parses to infinity).
vercelGatewayRouting is produced only when the provider base URL parses to exactly ai-gateway.vercel.sh, with the host folded to lower case first so the URL the transport checks is the one the decision was made about. The transport's own check is baseUrl.includes(...), a substring test a lookalike host would pass, so the enforced guarantee lives here.
sort, sort.by and sort.partition accept any non-empty string, because that is what the contract declares; the documented strategies are examples. Incidental surrounding whitespace is trimmed, the one place the reader rewrites rather than forwards a value. data_collection keeps its declared two-value union.
A list entry must be a non-empty string: an empty or whitespace-only slug can never match a provider, so it rejects the whole list like any other malformed element.
sort object form is atomic: a present member that fails validation drops the whole object, so a partial sort is never sent. Unknown sort keys are ignored.
An object left with no usable keys is discarded entirely, so an empty provider: {} is never sent.
This is config-only and adds no TUI surface, so the model picker is untouched.
Also present in this branch, independent of this change and separable:
6d27a14 — a preset save persists only the chosen model. Before, it wrote the whole models.dev catalog into ~/.minimax/config.yaml (~300 OpenRouter models, ~130 KiB). Reported on #289.
The branch is rebased onto v0.6.5 (2026-10-09); commit hashes changed with that rebase.
Checks run
PASS: pnpm verify, full profile on Linux, on 6d27a14 after the v0.6.5 rebase — all 14 gates.
PASS: pnpm lint on the changed files — 0 errors, 0 warnings. (Targeted lint only; a broad eslint --fix picks up unrelated formatting churn in sibling files, which is not included here.)
PASS: vitest run packages/local-runtime-v2/src/service/model-system/ — 15 files, 545 tests.
PASS: pnpm test:byok — 3/3.
The routing change adds 25 reader cases (51 in that file, up from 25), plus 2 wire-level cases in local-model-resolver.test.ts, covering: pinned provider (only), ordered preference (order) with ignore, bare and object sort, atomic sort rejection, privacy/cost keys (zdr, data_collection, max_price), both shapes of the throughput/latency preferences, non-numeric price strings, unknown nested keys in both record shapes, known-keys-only filtering, all-malformed and all-unknown drops, mixed-list rejection, wrong-type sibling isolation, non-record values, unknown top-level key stripping, and the absent-field no-op.
Not run
NOT RUN: live OpenRouter calls. Routing behavior is asserted at the config-reader boundary; no request was issued against a real endpoint or API key.
NOT RUN: the macOS platform profile and the Windows / Node-compatibility matrices. Local runs were Linux-only.
NOT RUN: TUI acceptance. There is no TUI change, so no interactive flow was exercised.
Publication and contribution checks
Provenance: my own work, plus one commit re-applying a fix first authored by copilot-swe-agent (4abfca2) after a force-push here overwrote it. That authorship is preserved with a Co-authored-by trailer.
Offered under the repository's existing applicable licenses (MIT).
No credentials, account data, real user content, or internal review material included.
Existing copyright notices and third-party license text untouched.
No new test registration or inventory regeneration needed; no documentation impact.
Review history
Fifteen rounds on this fork. Rounds 1-6 and 8-14 were real and are fixed; rounds 7 and 15 were false positives, both rejected with measurements. findings in rounds 1-6 were all real. Round 3 is the one where my rebuttal was wrong and I said so — see the correction.
Summary of what review caught, none of which the original implementation had:
round
finding
1
mixed provider lists silently filtered instead of rejected
2
any non-empty string accepted as a price; sort object not atomic
"1e999" → infinity bypassed the finite check; sort narrowed below the declared string contract; tests never reached the request layer
5
contract comment claimed the field is ignored by non-OpenRouter providers; the transport attaches it unconditionally
6
provider lists accepted an empty slug; readStrategy trimmed without documenting it
7
(false positive) claimed the bundled catalog has no openrouter provider
8
Vercel routing reachable from a lookalike host, because the transport's endpoint check is a substring test; two comments overclaiming guarantees
9
the new hostname gate dropped a trailing-dot FQDN (same host, silently discarding routing); a stale comment framed the gate as redundant
10
a case-sensitivity mismatch meant the reader approved a host the transport then ignored — and the round-9 uppercase test asserted nothing
11
docstring still called openRouterRouting "the one object-valued field" after a second one was added
12
every() skips holes, so the string[] predicate accepted a sparse list (soundness; not JSON-reachable)
13
negative routing assertions passed vacuously — an uncaptured payload reads the same as an absent field
14
a padded list entry was forwarded verbatim, so the routing silently matched no provider (reachable)
15
(false positive) claimed unset routing emits provider: {}; measured: no provider key at all
The last one is worth flagging for review: the reader tests originally stopped at the resolved compat, so they would all have passed with routing still absent from the request. There is now a wire-level assertion through the captured openai-completions payload, verified to fail when the field is dropped from the reader.
Maintainer note
I am not a collaborator, so I understand a PR cannot be opened here. Following the process in CONTRIBUTING.md and the precedent in MiniMax-AI#178, this is offered for maintainer review and adoption. The commits are public in the fork at SvenMeyer/minimax-code, branch openrouter-preset.
Addressed a nested validation gap in openRouterRouting: unknown keys inside max_price and percentile objects are now stripped instead of being forwarded. Fixed in 4abfca2.
The object-form validation is not atomic as documented: when one member is malformed, the other valid member is still forwarded (for example {by: "price", partition: 42} becomes {by: "price"}). This contradicts the stated requirement to drop the whole sort object when either by or partition is invalid, and can leave OpenRouter with a partial routing override. Return undefined as soon as a provided member fails validation.
Round 3 surfaced two store_comment findings (moderate, body: null) at model-resolver-byok.ts:269-271 and :282-284 — the .filter(...) in readMaxPrice and readPercentiles. Round 1 raised the same point and I replied on the thread then; rather than change the code again, here is the fuller analysis for the record.
These are not silent value changes, and rejecting the whole object would be worse.
The two shapes behave differently, and the difference is what round 1 got right and round 2 corrected:
shape
dropping a bad member means
verdict
list (only/order/ignore)
the set of providers pinned, ordered or excluded changes — a different routing decision
reject whole value
sort object
the sort strategy changes ({by, partition} → {by} is a different, still-valid sort)
reject whole value
record (max_price, percentiles)
one optional constraint stops applying; every other constraint applies exactly as written
keep the valid keys
max_price is the case worth spelling out, because "silently dropping a price cap" sounds bad. It is still the lesser evil. For max_price: {prompt: 0.001, completion: "$5"}:
current behavior → {prompt: 0.001}: the prompt cap holds, the malformed completion cap is ignored.
rejecting the whole object → no caps at all, so the validprompt ceiling is lost too.
Either way the malformed cap does not apply. Keeping the valid keys preserves strictly more of the user's intended limits, so the whole-object rejection loses safety rather than gaining it. The same holds for the percentile maps.
This also matches what the surrounding reader already does for scalar fields: readModelCompat has always ignored unknown keys, and the existing ignores unknown keys test asserts it for compat.unknownFlag. A record key of the wrong type is just an unknown key from that key's point of view.
I also checked the other thing these two lines could plausibly be flagged for — prototype pollution via spreading untrusted JSON. Object.fromEntries uses CreateDataPropertyOrThrow, so a JSON-parsed __proto__ key becomes a plain own property rather than mutating the prototype, and it is not re-emitted downstream. Verified directly: Object.prototype stays clean and the key does not survive serialization.
No code change this round. Leaving the behavior as is unless someone can point at a concrete configuration where per-key filtering produces a wrong routing or pricing decision.
My previous comment rejected the findings at readMaxPrice and readPercentiles. One of its claims was wrong, and the review was right about the underlying bug. Retracting that part.
I wrote: "Unknown keys are never forwarded to OpenRouter."
That is true for the top-level reader, which iterates a known-key list. It was false for the nested records, which iterated the object's own entries and kept every one that parsed:
My framing was also a false binary. I argued "keep the valid keys vs. reject the whole record" and never considered the actual answer: keep the valid known keys and drop the unknown ones. That is what the fix does, and it is consistent with the part of my reasoning that still holds — a declared key with a bad value is dropped individually, while an undeclared key is removed entirely.
This was already found, and I destroyed the fix
A copilot-swe-agent commit on this branch had already fixed exactly this — 4abfca2 "fix(byok): strip unknown nested OpenRouter routing keys", with a test asserting both cases above.
My next force-push overwrote it. --force-with-lease could not catch it: the value it leases against had been fetched moments earlier in the same run, so the lease always matched and the remote commit was silently discarded.
Both are fixed:
Code — 7a62cd8 re-applies the fix, iterating the known keys in both record readers. Authorship preserved via Co-authored-by: copilot-swe-agent[bot], per CONTRIBUTING.md ("Preserve real author identities").
Process — the update script now records what it last pushed (refs/minimax-fork/pushed/<branch>) and refuses to force-push when the remote has moved independently, naming the commits and printing the explicit override. Only a divergence whose remote tip matches our own last push is force-pushed. Verified in a sandbox: a foreign commit is now preserved with exit 2, while a legitimate rebase still force-pushes.
Gates after the change: 539/539 model-system tests, pnpm typecheck clean.
Round 4 raised three findings. All three were real and are fixed in 40e82c1.
1. max_price exponent form bypassed the finite check.readNumberish accepted "1e999" — it matches the numeric-string shape but parses to Infinity, which is the same non-finite value the numeric branch rejects. The string form now has to parse to a finite number. "1e300" (finite) still passes.
2. sort was narrower than the contract it mirrors. pi types sort, sort.by and sort.partition as plain string and documents price/throughput/latency and model/none as examples ("e.g."), not as an exclusive union. My allowlist silently dropped any other strategy, which is drift from the transport this reader is supposed to mirror. All three now accept any non-empty string; the empty string and non-strings still reject the whole object, so atomicity is preserved. data_collection keeps its two-value union, because "deny" | "allow" is what the contract actually declares.
3. The tests never left the config reader. Every assertion stopped at planCustomProviderResolution().modelCompat, so a regression downstream of readModelCompat() would have left all of them green while BYOK routing was still omitted from the request. Added a wire-level test using the existing offline onPayload harness (local-model-resolver.test.ts), asserting the captured openai-completions payload carries provider: {only, allow_fallbacks} — and that no provider field is sent when routing is absent.
I confirmed the new test has teeth rather than trusting it: disabling the field in readModelCompat() makes it fail (Tests 1 failed | 105 passed), and it passes again once restored.
This one is on me twice over. I had called the loop at round 3 and framed it as re-raised noise — round 4 then found three issues that were not duplicates of anything before, including the test-depth gap that mattered most. The brake was applied too early; a re-raised point does not mean the next round is empty.
Gates: 543/543 model-system tests, pnpm typecheck clean, pnpm lint clean on the changed files. One housekeeping note: an earlier revision of this commit also picked up two unrelated formatting-only files from a broad eslint --fix; those were reverted so the patch stays limited to the routing change.
The openRouterRouting contract comment claimed the field is "ignored by other providers". That is false.openai-completions.ts attaches it whenever present, with no endpoint check:
The Vercel routing immediately below it gates on the base URL; OpenRouter's does not. So the comment was describing a guarantee the transport does not make. It now states what actually happens and warns that the field belongs only on models served by OpenRouter, since other OpenAI-compatible gateways will receive the extra provider field in the request body.
Also: the overview lists one open item, Test openRouterRouting propagation to request provider, which is the round-4 thread. That was fixed in 40e82c1 and I have replied on the thread — it is unresolved in the GitHub UI, not unaddressed in the code.
Gates: pnpm typecheck clean, pnpm lint clean, 216/216 in the resolver suite. Docs-only change, no behavior.
Round 6: two session-stored findings, fixed in dca3d83.
A caveat on this round. Both stored records had body: null — the ensemble log carried only type/severity/location, and the prose was not retrievable from the action logs either. So unlike previous rounds I could not read the reviewer's reasoning and had to assess the two locations directly. Here is what I found and why I changed it; if the intent was something else, say so and I will revisit.
1. readStringList accepted an empty provider slug (model-resolver-byok.ts:240). isStringArray only checked typeof entry === 'string', so only: [""] or [" "] passed and was forwarded as a provider list that cannot match anything. readStrategy already rejects the empty/whitespace-only string in the scalar case, so the list was laxer than a plain value. isProviderSlugList now requires every entry to be a non-empty string; a padded list rejects the whole key like any other malformed list, and its valid siblings survive.
2. readStrategy trims but the comment did not say so (:330-331). The comment claimed it "checks the type and rejects only the empty string", while the code also normalises surrounding whitespace. The behaviour is deliberate and is now documented rather than changed: a padded value cannot match anything OpenRouter knows, so forwarding it verbatim would fail the request, and dropping the key would silently discard routing the user did configure. Rewriting is the safer of the three options — which is precisely why this is the one place the reader does not forward verbatim, and the comment now says that.
I also resolved the round-4 thread now that its fix has landed, so the overview is not reporting fixed work as open.
One note for the record: the two remaining lint warnings in this directory are pre-existing upstream formatting in model-ref.ts / model-ref.test.ts, which this patch does not touch.
The reason will be displayed to describe this comment to others. Learn more.
I tested this rather than reasoning about it, and I believe the premise is mistaken: an unset routing config produces no provider key at all, not provider: {}, and the expectations here pass.
Captured from the real request payload through the offline onPayload harness, with no routing configured:
hasOwn=false is the important part — the key is absent, so this is not an undefined field being mistaken for an omitted one.
I think the conflation is between two different compat objects:
getCompat() does default openRouterRouting to {} — but that default lands on the resolved compat (ResolvedOpenAICompletionsCompat) it returns. buildParams receives that as its compat parameter (passed at line 147) and uses it for convertMessages, getCompatCacheControl, and the other resolved fields.
The provider emission does not read that parameter. openai-completions.ts:619 reads the raw model compat:
model.compat is the model's own compat, which the resolved default never writes back into. With no routing configured it is undefined, the guard is false, and nothing is assigned — hence hasOwn=false.
So the expectations do not contradict the transport, and they are not the "same assumption repeated": all three sites assert the absence of a field that is genuinely absent. They would fail loudly if that ever changed — if line 619 were made to read the resolved compat, provider: {} would appear and these assertions are exactly what should catch it, which is the job they exist to do.
Round 13: three stored bug findings, all real and all the same root cause. Fixed in 6b83fd7. Published surface clean again.
All three were my negative routing assertions — expect(payload.provider).toBeUndefined() and friends — reading from a payload that started as {}. An absent field and an uncaptured payload are the same value, so those assertions passed for the wrong reason: if onPayload stopped assigning, they would still report green.
I demonstrated it rather than assuming it. With the capture disabled:
× sends OpenRouter routing preferences as the request provider field
× sends Vercel gateway routing as providerOptions.gateway
× sends Vercel gateway options for a mixed-case host
Tests 3 failed | 107 passed
Three positives failed and every negative passed — so three of the six routing tests could not detect the harness breaking underneath them. routingPayloadFor now throws when nothing was captured, so an absence assertion can only pass when a payload actually arrived. Re-running the same experiment now fails all six.
No production code changed; this is the test-side half of the same defect round 10 found on the reader side.
This is the fourth time in this loop that "verify the test can fail" was the thing that found the bug — and the second time the defect was an assertion that could not fail. That check is now applied to every test whose purpose is "this value reaches the other side", and it has yet to come back empty when I had written the test myself.
Round 14: one finding, real, fixed in 24b2d74. Published surface clean for the fourth round running.
readStringList rejected an all-whitespace entry but passed a non-empty padded one through unchanged, so only: [" DeepSeek "] went out as written. Verified — provider slugs are matched exactly, so a padded entry matches nothing:
The routing is then silently ineffective, which is the failure mode this reader exists to prevent, committed by the guard doing the preventing.
readStrategy has trimmed scalars since round 6 for exactly this reason, so the list was the odd one out. Entries are now trimmed, which keeps the configured provider set intact: rejecting the list would silently discard routing the user did configure, and forwarding verbatim would silently not apply it. The guard is folded into readStringList and returns a new array, so the parsed config is never mutated. Added the list coverage the finding asked for, confirmed it fails without the trim (2 failed | 60 passed).
The inconsistency was the tell. I applied "a padded value must not be forwarded verbatim" to the scalar path in round 6, wrote a paragraph explaining why, and never carried it across to the list path three lines away — where the same reasoning holds unchanged.
Round 15: one published inline comment, rejected with evidence — no code change. The overview itself says "Approval recommended / No unresolved issues were identified that would block approval" while listing this one item as Open and high severity, so the two halves of the same review disagree.
The claim: getCompat() initialises openRouterRouting to {} and buildParams() assigns any truthy value, so an unset config yields provider: {} rather than undefined, and the expectations "fail".
I captured the actual payload instead of arguing from the source. With no routing configured, on both hosts:
hasOwn=false — the key is absent, not an undefined value. The tests pass.
The conflation is between two compat objects. getCompat()'s {} default lands on the resolved compat, which buildParams receives as its compat parameter and uses for convertMessages and the other resolved fields. The provider emission does not read that parameter — openai-completions.ts:619 reads the rawmodel.compat, which the resolved default never writes back into. Unset ⇒ undefined ⇒ guard false ⇒ nothing assigned.
Worth keeping in mind for the loop's own accounting: the three expectations named here are precisely the absence assertions that round 13 found were passing vacuously. They are no longer vacuous — the harness throws if nothing was captured, so hasOwn=false is now a real result rather than a default. That fix is what makes this rebuttal provable rather than a matter of reading.
This is the second false positive in fifteen rounds (after round 7) and, like round 7, it is rejected on measurement rather than opinion. No code changed, so the skill's second terminal condition applies and further rounds would see an identical diff.
models.dev tags OpenRouter with @openrouter/ai-sdk-provider, so the preset
picker dropped it. Map provider id openrouter to openai-completions and
leave other users of that package excluded.
The pi transport already forwards model.compat.openRouterRouting as the
request `provider` field, and OpenRouterRouting is fully typed in pi-ai,
but readModelCompat() whitelists compat keys and did not include it, so
the BYOK config reader dropped the field before it reached the model.
Declare it on LocalModelCompatOverrides and read it structurally: it is
the one object-valued compat field, so it needs a real per-key reader
rather than the existing boolean/enum handling. Each key is accepted only
at its declared type, a malformed key is dropped without invalidating its
valid siblings, and an object left with no usable keys is discarded so an
empty `provider` object is never sent. `sort` is validated in both the
bare-string and {by, partition} forms, and the percentile fields keep
their numeric-only object form.
Config-only: no TUI surface, so the already-crowded model picker is
untouched. Closes the config half of MiniMax-AI#288.
Copilot review on the routing patch: readStringList filtered a mixed array
down to its string entries, so `only: ["DeepSeek", 42]` was forwarded as
`["DeepSeek"]` — silently pinning a different provider set than the one
written. That contradicts the rule readModelCompat() documents, where a
wrong-typed value is dropped rather than forwarded.
A list is one declared value, so a non-string element invalidates the
whole key. Guard with an explicit isStringArray predicate and reject
empty lists too. Records (max_price, percentile maps) keep per-key
dropping: their declared type is a set of independently-typed optional
keys, so an unknown key is just an unknown key, matching the existing
`ignores unknown keys` behavior for scalar compat fields.
Two findings from the second Copilot review round on the routing patch.
readNumberish accepted any non-empty string as a price, so
max_price: {prompt: "free"} or "$5" was forwarded verbatim to OpenRouter
as a price ceiling. The declared union is number-or-numeric-string, so
the string form now has to parse as a decimal before it is kept.
readRoutingSort was not atomic despite documenting that it was:
{by: "price", partition: 42} collapsed to {by: "price"} and was sent as a
partial routing override. A key that is present but invalid now rejects
the whole sort object. Unknown sort keys are still ignored, matching the
rest of the reader.
readMaxPrice and readPercentiles iterated the object's own entries and
kept every one that parsed, so an undeclared nested key rode along into
the request: max_price {prompt, surge} forwarded surge, and
preferred_min_throughput {p50, p95} forwarded p95, which is not in the
declared p50/p75/p90/p99 set. The top-level reader was already
known-keys-only; the nested records were not.
Iterate the known keys instead, matching how the scalar reader treats
compat fields. A record left with no known keys is dropped.
This re-applies a fix first authored by the Copilot code-review agent
(copilot-swe-agent) in 4abfca2, which was lost when a later force-push
overwrote the branch. Authorship is preserved via the trailer below.
Co-authored-by: copilot-swe-agent[bot] <198982749+copilot-swe-agent[bot]@users.noreply.github.com>
…ract
Three findings from the fourth review round.
readNumberish accepted the exponent form without checking magnitude, so
max_price: {prompt: "1e999"} was forwarded as a price ceiling even though
it parses to Infinity — the same non-finite value the numeric branch
already rejects. The string form now has to parse to a finite number.
sort, sort.by and sort.partition were narrowed to the documented examples
(price/throughput/latency, model/none), but pi types all three as plain
string and documents those as examples, not an exclusive union. A strategy
the shared contract permits was therefore dropped. They now accept any
non-empty string; the empty string and non-strings still reject the whole
object, preserving atomicity. data_collection keeps its two-value union,
since that is what the contract actually declares.
The reader tests stopped at planCustomProviderResolution().modelCompat and
never proved the value reaches the request, so a regression downstream of
readModelCompat() would have left them all green. Added a wire-level
assertion through LocalModelResolver and the captured openai-completions
payload, matching the existing offline harness. Verified it fails when the
field is dropped from the reader.
The comment claimed the field is "ignored by other providers". It is not:
openai-completions.ts attaches compat.openRouterRouting to the request
whenever it is present, with no endpoint check — unlike the Vercel gateway
routing three lines below it, which gates on the base URL.
State what the transport actually does and warn that the field belongs
only on models served by OpenRouter, so the comment cannot be read as a
promise the transport does not make.
Two findings from the sixth review round, both at points where a list or a
string was laxer than its declared contract.
readStringList accepted an entry that is empty or whitespace-only. An empty
slug cannot match a provider, so it is as invalid as a non-string, and
readStrategy already rejects the empty string in the scalar case; the list
was laxer than a plain value. isProviderSlugList now requires every entry to
be a non-empty string, so a padded list rejects the whole key like any other
malformed list, and its siblings survive.
readStrategy trims incidental whitespace but the comment claimed it only
rejects the empty string. The behaviour is deliberate and now documented:
a padded value cannot match anything OpenRouter knows, so forwarding it
verbatim would fail the request while dropping the key would discard routing
the user did configure. Rewriting is the safer of the three options here,
which is why it is the one place the reader does not forward verbatim.
…uting
Completes the maintainer's implementer brief on MiniMax-AI#288, which asks for
openRouterRouting "and ideally vercelGatewayRouting". The previous commit
delivered only the first, so the brief was half-covered.
VercelGatewayRouting declares exactly `only` and `order`, both string[],
and the transport reads only those two before building
providerOptions.gateway, so the reader keeps to the declared keys and
reuses readStringList — a mixed or padded list is rejected rather than
silently shrunk, and a valid sibling survives.
Unlike openRouterRouting there is no endpoint concern here: the transport
already gates this on `model.baseUrl.includes("ai-gateway.vercel.sh")`, so
the same config set on a non-Vercel provider is inert rather than leaked.
That asymmetry is why the OpenRouter field is flagged for a maintainer
decision while this one is not.
Wire-level tests cover the providerOptions.gateway payload, the off-host
case (asserting the base-URL gate holds), and the absent case. Verified to
fail when the field is dropped from the reader.
Review finding: accepting this field for every custom provider made the
Vercel-only body fields reachable from any base URL that merely contains
"ai-gateway.vercel.sh", because the transport tests
`model.baseUrl.includes(...)` — a substring check, not a hostname check.
Verified: ai-gateway.vercel.sh.example.com, ai-gateway.vercel.sh.evil.com,
a subdomain, and even a URL with the string in its path all satisfy it.
The Vercel branch attaches gateway-specific body fields, so an endpoint
that only looks Vercel-ish should not be able to attract them.
The transport is vendored third-party code this patch does not modify, so
the reader gates instead: vercelGatewayRouting is produced only when the
configured base URL parses to exactly that hostname. Every other compat
key passes through untouched, so a lookalike endpoint loses only the field
that was never valid for it.
hostnameOf tolerates a missing scheme and any port. Tests cover four
lookalike shapes, the scheme-less and port forms, sibling survival, and
that openRouterRouting stays ungated (the transport applies that one for
any endpoint, so dropping it there would be silent config loss). Verified
the gate tests fail when the gate is removed.
Also corrects two comments the same review flagged as overclaiming:
readOpenRouterRouting is no longer described as "forwarded verbatim to
OpenRouter" (it is filtered, and the transport does not check the
endpoint), and the contract note no longer claims the field "cannot reach
another gateway".
…mment
Three findings from the ninth review round.
The endpoint gate rejected `https://ai-gateway.vercel.sh./v1`. A single
trailing dot is the DNS root label, so that names the same host, and the
transport's substring check accepts it — refusing there silently discarded
routing the user did configure, which is the failure mode this gate exists
to avoid, not one to introduce. hostnameOf now strips one trailing dot
before comparing. Uppercase hosts were already handled by lowercasing.
The readVercelGatewayRouting comment still said "no endpoint concern
applies: the transport already gates this", which was true before the gate
was added and is now the opposite of the truth — it framed the gate as
redundant and invited its removal. It now says the gate is load-bearing and
explains why, so a future reader does not delete the check that stops
Vercel-only body fields reaching a lookalike host.
The two wire-level routing harnesses had grown into near-identical copies
of the same resolver setup. They now share routingPayloadFor, with thin
wrappers picking the field each case asserts on.
The hostname gate lowercased the host before comparing, so it approved
https://AI-GATEWAY.VERCEL.SH/v1 — but the pi transport recognises the
Vercel gateway with a case-sensitive substring test and the URL normalizer
only strips path suffixes, so that URL reached the wire unconverted and the
request carried no providerOptions.gateway at all.
The reader and the transport therefore disagreed about the same URL, and
the uppercase reader test added last round asserted nothing end to end: it
passed while the routing it claimed to preserve was silently dropped.
planCustomProviderResolution now folds the host (and port) to lower case
before both the gate and the plan, leaving scheme, userinfo and path
untouched, so the URL the transport sees is the one the decision was made
about. Hostnames are case-insensitive, so this changes no endpoint. A
wire-level assertion covers the mixed-case host and fails without the fold.
The docstring still said openRouterRouting "is the one object-valued
field". Adding vercelGatewayRouting made that false, and a reader relying
on it would not look for the second structural reader. Names both, and
drops the {@link} to a now-unquoted function so the sentence holds if
either reader is renamed.
`value.every(...)` skips holes in a sparse array, so `["a", , "b"]` satisfied
the `value is string[]` predicate while not being a `string[]` at all. A hole
serialises to `null`, so such a list would have reached the wire as
`only: ["a", null, "b"]` — the malformed provider list this reader exists to
reject, waved through by its own guard.
Array.from first, which materialises holes as undefined so the predicate sees
them. Persisted config is JSON-parsed and `JSON.parse` cannot produce a hole,
so this closes a soundness gap rather than a live path — confirmed both ways,
including that the repo's own `no-sparse-arrays` lint rule forbids writing the
construct as a literal, which is why the test allocates and leaves an index
unset instead.
The negative routing assertions — "no provider field", "no gateway
options", "off the Vercel host" — all read `expect(payload.X).toBeUndefined()`
from a payload that started as `{}`. If onPayload never assigned, those
assertions passed for the wrong reason: an absent field and an uncaptured
payload are the same value.
Demonstrated rather than assumed. With the capture disabled, the three
positive tests failed and every negative one still passed, so three of the
six routing tests could not detect the harness breaking underneath them.
routingPayloadFor now throws when nothing was captured, so an absence
assertion can only pass when a payload actually arrived. Re-running the
same experiment fails all six. No production code changes.
readStringList rejected an all-whitespace entry but passed a non-empty
padded one through unchanged, so `only: [" DeepSeek "]` was forwarded as
written. Provider slugs are matched exactly, so that entry matches nothing
and the requested routing is silently ineffective — the failure mode this
reader exists to prevent, in the guard doing the preventing.
readStrategy already trims scalars for precisely this reason, so the list
was the odd one out. Entries are now trimmed, which keeps the configured
provider set intact: rejecting the list would silently discard routing the
user did configure, and forwarding verbatim would silently not apply it.
The guard is folded into readStringList and returns a new array, so the
parsed config is never mutated.
Saving a models.dev preset sent the whole catalog to the runtime, which
wrote every entry into config.yaml. For OpenRouter that is ~300 models
and ~130 KiB, although only one model was chosen. Persist the chosen
model; re-running the preset on an existing connection appends just the
newly chosen one. Model import from /models is unchanged: its screen
states that it imports every discovered model.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep the fork's working notes (carried patches, upstream issue status,
review history, update-script pitfalls) on the patch branch instead of
outside the clone, and record the file in the source inventory.
Co-Authored-By: Claude Opus 5.5 <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
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.
Change
Implements the config half of MiniMax-AI#288: honor
compat.openRouterRoutingfor BYOKcustom_providermodels.The pi transport already supports this end to end —
third_party/pi-mono/packages/ai/src/providers/openai-completions.tssendsmodel.compat.openRouterRoutingas the requestproviderfield, andOpenRouterRoutingis fully typed in@earendil-works/pi-ai. The only gap was the BYOK config reader:readModelCompat()accepts eachcompatkey at its declared type, andopenRouterRoutingwas not among them, so the field was dropped before it reached the model.Two files:
contracts.ts— declaresopenRouterRouting?: OpenRouterRoutingandvercelGatewayRouting?: VercelGatewayRoutingonLocalModelCompatOverrides, importing both types from pi-ai rather than re-declaring them so the config shape cannot drift from the transport.model-resolver-byok.ts— addsreadOpenRouterRouting()andreadVercelGatewayRouting(), both called fromreadModelCompat(). The Vercel field is the one the maintainer brief on [Feature]: Honor compat.openRouterRouting in BYOK custom providers (OpenRouter provider routing: sort/order/only) MiniMax-AI/minimax-code#288 asked for "ideally"; it completes the brief.Validation posture
openRouterRoutingis the one object-valuedcompatfield, so it needs a structural reader rather than the existing boolean/enum handling. It follows the rule already documented onreadModelCompat(): a value of the wrong type is dropped rather than forwarded, because pi treats any present field as an explicit override and a truthy"false"would invert the intended behavior.The three shapes are handled differently, deliberately:
zdr,data_collection, …)only/order/ignore/quantizations)sortobjectmax_price, percentiles)only: ["DeepSeek", 42]is not silently shrunk to["DeepSeek"], which would pin a different provider set than the one written.max_price: {prompt, surge}does not sendsurge, andpreferred_min_throughput: {p50, p95}does not sendp95(not inp50/p75/p90/p99).max_price.*takes a number or a decimal string. A string that does not parse as one ("free","$5") is dropped instead of being forwarded as a ceiling. Non-finite values are dropped in both forms, including the exponent shape ("1e999"parses to infinity).vercelGatewayRoutingis produced only when the provider base URL parses to exactlyai-gateway.vercel.sh, with the host folded to lower case first so the URL the transport checks is the one the decision was made about. The transport's own check isbaseUrl.includes(...), a substring test a lookalike host would pass, so the enforced guarantee lives here.sort,sort.byandsort.partitionaccept any non-empty string, because that is what the contract declares; the documented strategies are examples. Incidental surrounding whitespace is trimmed, the one place the reader rewrites rather than forwards a value.data_collectionkeeps its declared two-value union.sortobject form is atomic: a present member that fails validation drops the whole object, so a partial sort is never sent. Unknownsortkeys are ignored.provider: {}is never sent.This is config-only and adds no TUI surface, so the model picker is untouched.
Also present in this branch, independent of this change and separable:
a9529cf— the already-reported fix for [Feature]: Provider onboarding: allow editing/adding model IDs, reachable custom mode, and persist a config.yaml template when the test fails MiniMax-AI/minimax-code#289 item 1, which makes the OpenRouter models.dev preset selectable.6d27a14— a preset save persists only the chosen model. Before, it wrote the whole models.dev catalog into~/.minimax/config.yaml(~300 OpenRouter models, ~130 KiB). Reported on #289.The branch is rebased onto
v0.6.5(2026-10-09); commit hashes changed with that rebase.Checks run
pnpm verify, full profile on Linux, on6d27a14after thev0.6.5rebase — all 14 gates.Earlier, per-gate runs on
v0.5.8:pnpm typecheck(tsc -p tsconfig.standalone.json).pnpm linton the changed files — 0 errors, 0 warnings. (Targeted lint only; a broadeslint --fixpicks up unrelated formatting churn in sibling files, which is not included here.)vitest run packages/local-runtime-v2/src/service/model-system/— 15 files, 545 tests.pnpm test:byok— 3/3.The routing change adds 25 reader cases (51 in that file, up from 25), plus 2 wire-level cases in
local-model-resolver.test.ts, covering: pinned provider (only), ordered preference (order) withignore, bare and objectsort, atomicsortrejection, privacy/cost keys (zdr,data_collection,max_price), both shapes of the throughput/latency preferences, non-numeric price strings, unknown nested keys in both record shapes, known-keys-only filtering, all-malformed and all-unknown drops, mixed-list rejection, wrong-type sibling isolation, non-record values, unknown top-level key stripping, and the absent-field no-op.Not run
platformprofile and the Windows / Node-compatibility matrices. Local runs were Linux-only.Publication and contribution checks
copilot-swe-agent(4abfca2) after a force-push here overwrote it. That authorship is preserved with aCo-authored-bytrailer.Review history
Fifteen rounds on this fork. Rounds 1-6 and 8-14 were real and are fixed; rounds 7 and 15 were false positives, both rejected with measurements. findings in rounds 1-6 were all real. Round 3 is the one where my rebuttal was wrong and I said so — see the correction.
Summary of what review caught, none of which the original implementation had:
sortobject not atomicmax_price.surge,p50/p95)"1e999"→ infinity bypassed the finite check;sortnarrowed below the declaredstringcontract; tests never reached the request layerreadStrategytrimmed without documenting itopenrouterprovideropenRouterRouting"the one object-valued field" after a second one was addedevery()skips holes, so thestring[]predicate accepted a sparse list (soundness; not JSON-reachable)provider: {}; measured: noproviderkey at allThe last one is worth flagging for review: the reader tests originally stopped at the resolved compat, so they would all have passed with routing still absent from the request. There is now a wire-level assertion through the captured
openai-completionspayload, verified to fail when the field is dropped from the reader.Maintainer note
I am not a collaborator, so I understand a PR cannot be opened here. Following the process in CONTRIBUTING.md and the precedent in MiniMax-AI#178, this is offered for maintainer review and adoption. The commits are public in the fork at
SvenMeyer/minimax-code, branchopenrouter-preset.