diff --git a/README-fork-openrouter.md b/README-fork-openrouter.md new file mode 100644 index 000000000..58ea9b0bb --- /dev/null +++ b/README-fork-openrouter.md @@ -0,0 +1,612 @@ +# MiniMax Code fork — OpenRouter patches + +Local fork of [`MiniMax-AI/minimax-code`](https://github.com/MiniMax-AI/minimax-code) carrying +OpenRouter support that upstream has not accepted. This file is the working note for that fork: +what is patched, what is still upstream-only, and how to rebase it. + +Upstream-facing documentation lives elsewhere and is not duplicated here: + +| Audience | Where | +| --- | --- | +| Maintainers (technical + validation) | Fork PR body — | +| Maintainers (proposal) | Comment on [#288](https://github.com/MiniMax-AI/minimax-code/issues/288) | +| Fork workflow + history | This file, `README-fork-openrouter.md` on branch `openrouter-preset` | + +## Layout + +- `~/src/minimax-code/` — the clone. `upstream` → MiniMax-AI, `origin` → SvenMeyer. This file is + committed on `openrouter-preset` only; `main` stays a pure upstream mirror. +- `~/src/update-minimax-fork.sh` — outside the clone. Rebases the patch branch onto a release, + pushes, rebuilds `dist/`. + +Branch layout: + +- `main` — a pure mirror of upstream. No local commits, ever. The update script resets it. +- `openrouter-preset` — upstream + the carried patches. This is the branch to run from. + +## Running it + +```bash +cd ~/src +./update-minimax-fork.sh # latest stable vX.Y.Z tag +./update-minimax-fork.sh --main # upstream main, ahead of the last tag +./update-minimax-fork.sh v0.6.5 # pin a specific release +node ~/src/minimax-code/dist/cli.js +``` + +The script stops (exit 2) only when a rebase conflicts, a carried commit would be lost, or the +remote patch branch moved independently (see [the force-push hazard](#the-force-push-that-destroyed-someone-elses-commit)). +Everything else — including a diverged `main` — is handled and reported. + +## Current state + +v0.6.5, branch `openrouter-preset`, seventeen code commits above upstream (plus the commit adding +this file). Rebased 2026-10-09; +the only conflicts were the import lines in `model-resolver-byok.ts` (upstream renamed +`MINIMAX_API_MODEL_CATALOG` to `minimaxApiModels`). Hashes before that rebase are in the issue comments. + +| commit | what | +| --- | --- | +| `a9529cf` | OpenRouter preset appears in the picker (#289 item 1) | +| `a7fe150` | `compat.openRouterRouting` reaches the transport (#288) | +| `5102dbf` | reject mixed provider lists instead of filtering them | +| `9cb385c` | validate price strings; make `sort` atomic | +| `af2d6ad` | strip unknown nested routing keys (re-applies `4abfca2`) | +| `db6cc73` | finite-check exponent prices; `sort` honors the declared `string` contract; wire-level test added | +| `fd018ef` | correct the `openRouterRouting` contract comment | +| `f1a9dee` | reject empty provider slugs; document strategy trimming | +| `a2bbe15` | implement `compat.vercelGatewayRouting` (completes the maintainer brief) | +| `6eef167` | gate `vercelGatewayRouting` on an exact hostname; correct two overclaiming comments | +| `807ca26` | accept a trailing-dot Vercel FQDN; fix a stale gate comment; share the wire harness | +| `5bba6ab` | fold the base URL host to lower case before the endpoint gate | +| `eb1355b` | `readModelCompat` docstring: there are now two object-valued fields | +| `cf05e5f` | densify provider lists before the `string[]` predicate (sparse-array soundness) | +| `5851cef` | wire harness fails loudly when it captures no payload (test-only) | +| `8b07198` | trim provider list entries, matching the scalar rule | +| `6d27a14` | preset save persists only the chosen model, not the whole catalog (config.yaml was ~130 KiB with ~300 OpenRouter models) | + +### Outstanding + +- **Upstream comments posted, nothing further planned.** + - **#288** — [the offer](https://github.com/MiniMax-AI/minimax-code/issues/288#issuecomment-5870900552), + [the claim correction](https://github.com/MiniMax-AI/minimax-code/issues/288#issuecomment-5870917324), + [the `vercelGatewayRouting` completion](https://github.com/MiniMax-AI/minimax-code/issues/288#issuecomment-5871123931). + @Ralle1976 has claimed the work; awaiting maintainers. **No further action** — see below. + - **#289** — [item 1 raised separately](https://github.com/MiniMax-AI/minimax-code/issues/289#issuecomment-5871122391), + offering `2653f44` against the triage's own invitation ("PRs welcome, especially for the transport + mapping"). This is the standalone raise; it was never only a mention inside the #288 comment. + - **#289 items 2 and 3** are deliberately not implemented. Item 2 is pointed at as a lead, not a + finding (`saveInput()` merges additively for an existing connection). Item 3 needs the maintainers' + call on persisting an unverified provider before anyone writes it. + - **2026-10-09, after 0.6.5** — [status + config-size finding + "how do we get this into main?"](https://github.com/MiniMax-AI/minimax-code/issues/289#issuecomment-6074335370). + Awaiting the maintainers' answer on PR access vs. internal pickup. + +### config.yaml size + +Saving a models.dev preset used to copy the whole catalog into `custom_provider..models`. `6d27a14` +saves only the chosen model; add more by running the preset again on the same connection. The +existing `~/.minimax/config.yaml` was pruned to the five hand-added OpenRouter entries (backup: +`config.yaml.bak-before-prune-1791520404`). Pi does it differently: it keeps the catalog in code +(`models.generated.ts`) and the user file holds only overrides plus an `enabledModels` filter. A +catalog-backed custom provider would be the cleaner fix, but it touches the resolver, availability +and list paths, so it is too large to carry in a fork. + +### One issue per comment, even from one branch + +The preset fix (#289 item 1) and the routing work (#288) live in the same branch, which is fine, but +each was offered on **its own issue**, not bundled into one post. That is the convention the tracker +expects: a maintainer landing the preset mapping should not have to take the routing change with it, +and #289 would otherwise look unresolved while a fix sat in a fork. #289's triage had explicitly +invited "PRs, especially for the transport mapping", so the offer belonged there. + +Item 3 on #289 was deliberately left out of both the patch and the offer, with the reasoning stated +publicly: persisting a provider that failed its connectivity test means the next session silently +loads an unverified provider. Printing the YAML that *would* have been written gets recoverability +without that risk — the maintainer's call, not mine to make by default. + +### Read the whole thread before posting to it + +**Status: no further action planned on #288** (decision 2026-09-29). The offer, the correction and the +`vercelGatewayRouting` completion are all posted; @Ralle1976 has claimed the work and the maintainers +have not responded. Nothing more is to be added unless they ask — do not re-post, do not re-offer, and +do not chase. This note exists so the reasoning survives if the thread is revisited later. + +The first upstream comment was written, and posted, **without reading the comments between ours and +the present day**. That was a mistake, and only after posting did the full thread show: + +- **`hetaoBackend` (COLLABORATOR) had already triaged it** — verified the producer/consumer + asymmetry line-for-line, classified it `enhancement`, marked it ready for an implementer, and + wrote a **six-point implementer spec** (2026-09-25). +- **`Ralle1976` (CONTRIBUTOR) had already claimed the work** — announcing a fork branch + `fix/open-router-routing-compat` to be prepared along that outline (2026-09-26), two days before + our comment. + +So the post landed as a competing offer of already-claimed work. A follow-up was posted +acknowledging the claim and deferring to the maintainers, because leaving it as-is would have read +as ignoring a contributor who has standing in that tracker. + +Two traps this exposed, both cheap to avoid: + +- **Issue state is not issue context.** Checking `state`, labels and title tells you nothing about + triage or claims. Read every comment, oldest to newest, before adding one. +- **Our own earlier comments are not the thread.** Both of ours were from 2026-09-22; everything + decisive happened after. Re-reading only the parts we wrote recreates the same blind spot. + +Useful outcome despite the misstep: the maintainer's spec independently converges on this patch's +design (strict per-key validation, drop wrong types, no blind passthrough), and it names one gap — +**the brief asks for `vercelGatewayRouting` too, which this patch does not implement.** That is the +one piece of scope the collaborator asked for that we have not delivered. +- **PR #1 is open for review, not for merging.** `main` is a pure upstream mirror, so merging + there is pointless — see [Review workflow](#review-workflow). + +### `2653f44` — OpenRouter preset in the picker + +Fixes #289 item 1. `models.dev` tags OpenRouter as `@openrouter/ai-sdk-provider`, which +`resolveTransport()` did not recognise, so the preset was silently dropped from the picker. +Maps the provider **id** `openrouter` to `openai-completions` rather than matching on the npm +package, because other catalogs reuse that package. + +### `cca57f1` — `compat.openRouterRouting` for BYOK providers + +Fixes the config half of #288. The transport was already complete: `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 config reader — `readModelCompat()` +accepts each `compat` key at its declared type, and this one was not among them, so the value was +dropped before reaching the model. + +It is the one **object-valued** `compat` field, so it needs a structural reader rather than the +existing boolean/enum handling. The rule it follows is the one already documented on +`readModelCompat()`: drop a wrong-typed value rather than forward it, because pi treats any present +field as an explicit override and a truthy `"false"` would invert the intended behavior. + +The shapes are handled differently on purpose — conflating them caused three review rounds: + +| shape | a 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 unknown/bad keys, keep 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 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 the declared `p50`/`p75`/`p90`/`p99`). +- A malformed key is dropped **without** invalidating its valid siblings. +- `sort` validates in both forms — bare string, and `{by, partition}`. The 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. +- `max_price.*` takes a number or a **decimal** string; a string that does not parse as one + (`"free"`, `"$5"`) is dropped rather than forwarded as a ceiling. Non-finite numbers are dropped. +- An object left with no usable keys is discarded entirely, so an empty `provider: {}` is never + sent. + +Now works as documented in the issue: + +```yaml +custom_provider: + openrouter: + api: openai-completions + options: + apiKey: sk-or-v1-... + baseURL: https://openrouter.ai/api/v1 + models: + deepseek/deepseek-v4.1-flash: + compat: + openRouterRouting: + only: ["DeepSeek"] + allow_fallbacks: false +``` + +## Upstream issue status + +| Issue | State | Covered here | +| --- | --- | --- | +| [#215](https://github.com/MiniMax-AI/minimax-code/issues/215) generic OpenRouter support | open | not ours (cktang88) | +| [#288](https://github.com/MiniMax-AI/minimax-code/issues/288) `compat.openRouterRouting` | open | **yes** — `cca57f1` + `69e6b0c`, full brief | +| [#289](https://github.com/MiniMax-AI/minimax-code/issues/289) provider onboarding | open | **partly** — item 1 (`2653f44`); item 3 deliberately not done | +| [#258](https://github.com/MiniMax-AI/minimax-code/issues/258) Z.AI token counter | closed | landed upstream | +| [#259](https://github.com/MiniMax-AI/minimax-code/issues/259) Z.AI coding-plan preset | closed | landed upstream | + +### Not done, on purpose + +**#289 item 3 — persist the provider when the connectivity test fails.** Deliberately skipped. +`onboarding.ts` `save()` discards the draft on failure, so a wrong model ID means hand-editing +`config.yaml`. Writing the entry anyway would mean the *next* session silently loads a provider +that was never verified. That is a UX call worth making explicitly rather than by default. + +Note that **#289 item 2 is already upstream** — the picker now reads "select or add models" and +`saveInput()` calls `additiveProviderModels()`. Only item 3 genuinely remains. + +**A TUI picker for routing schemes.** Not implemented, and it should not be bolted on. The model +picker already stacks provider, model and think-level; adding routing on top makes it worse. If it +is ever wanted, it belongs to a model-picker redesign. + +## Submitting upstream + +The repo is `pull_request_creation_policy: collaborators_only`, so no upstream PR is possible, and +CONTRIBUTING.md asks non-collaborators to open an issue instead. The working precedent is upstream +PR #178, where a maintainer ported an external contributor's commit that had been "offered for +maintainer adoption in the issue", linking a commit in that contributor's fork. + +So the flow is: keep the patch public in this fork, get it reviewed here, and reference the commit +from the issue. Preserve authorship and state provenance in the comment — the maintainers route on +that. + +## Review workflow + +`pr-review-loop` drives the fork PR. Two things about this repo are unusual: + +**Never merge PR #1 into `main`.** `main` is a pure upstream mirror that the update script +force-resets, so a merge there is pointless and would be undone on the next run. The PR exists +only to collect review findings and to give the maintainer a browsable diff. + +**`@copilot review` in a comment does not work here** — it reaches `chatgpt-codex-connector`, +which answers with a Codex usage-limit error, and is *not* the PR reviewer. Request the reviewer +through the REST endpoint the skill uses, then let the wait script collect findings from every +surface: + +```bash +gh api -X POST "repos/SvenMeyer/minimax-code/pulls/1/requested_reviewers" \ + -f 'reviewers[]=copilot-pull-request-reviewer[bot]' +python ~/.claude/skills/pr-review-loop/scripts/wait_copilot_feedback.py \ + --repo SvenMeyer/minimax-code --pr 1 --request +``` + +The script also reads session-stored findings, which the REST comments API misses. A +`Findings: None` overview does not mean clean. + +Review history — six rounds so far, and the reviewer was right more often than I was: + +- **Round 1** (published thread): mixed provider lists were filtered rather than rejected + (`only: ["DeepSeek", 42]` → `["DeepSeek"]`), contradicting the reader's own rule. Fixed in + `ef3f70a`. +- **Round 2** (`store_comment` ×2 + `previously_missed`): both real. `readNumberish` forwarded any + non-empty string as a price (`"free"`, `"$5"`), and `readRoutingSort` was not atomic despite + documenting that it was. Fixed in `739e637`. +- **Round 3** (`store_comment` ×2, `body: null`): re-raised the record readers. I rejected it + with the claim "unknown keys are never forwarded to OpenRouter" — **that was wrong**. True at + top level, false for the nested records, which iterated all entries: `max_price {prompt, surge}` + forwarded `surge`. My framing was also a false binary (keep-valid vs reject-all); the actual + answer was "keep valid *known* keys, drop unknown ones". +- **Round 4** (`store_comment` ×3): all three real, all fixed in `40e82c1`. + 1. `"1e999"` matched the numeric-string shape but parses to `Infinity`, bypassing the documented + finite-number rule. + 2. `sort`/`by`/`partition` were narrowed to the documented examples, but the contract types them + as plain `string` — drift from the transport this reader mirrors. + 3. The tests never left the config reader, so a downstream regression would have stayed green + while routing was absent from the request. A wire-level assertion through the captured + `openai-completions` payload now covers it, verified to fail when the field is dropped. +- **Round 5** (`store_comment` ×1): real, fixed in `dcab04f`. The contract comment claimed the + field is "ignored by other providers". It is not — `openai-completions.ts` attaches it whenever + present, with **no endpoint check**, unlike the Vercel gateway routing three lines below it which + gates on the base URL. The comment now states what the transport actually does. +- **Round 6** (`store_comment` ×2, `body: null`): fixed in `dca3d83`. Both bodies were null and the + prose was not in the action logs either, so the two locations had to be assessed directly. + 1. `readStringList` accepted an empty/whitespace-only provider slug (`only: [""]`), forwarding a + list that cannot match anything — laxer than `readStrategy`, which rejects the empty string. + 2. `readStrategy` trimmed but its comment claimed it only rejected the empty string. Behaviour + kept, comment corrected. +- **Round 7** (published inline, ×1 **+** a `store_comment` at the same site): **rejected with + evidence, no code change.** The claim that the bundled catalog has no `openrouter` provider is + false — see below. The only thing it legitimately surfaced is that this patch is a mapper, not an + injector. +- **Round 15** (published inline ×1, high severity): **rejected with evidence, no code change.** Claimed + an unset routing config emits `provider: {}` rather than `undefined`, so the absence assertions "fail". + Measured instead: the payload has no `provider` key at all (`hasOwn=false`) on both hosts, and the + tests pass. The claim conflated `getCompat()`'s resolved `{}` default with the **raw** `model.compat` + that `openai-completions.ts:619` actually reads. The review's own overview said "Approval + recommended … no unresolved issues" while listing this item as Open — the two halves disagree. +- **Round 14** (`store_comment` ×1, zero published inline comments): real and **reachable**, fixed in + `24b2d74`. A non-empty padded list entry (`only: [" DeepSeek "]`) passed the guard unchanged and + matches no provider exactly, so the routing was silently ineffective — committed by the guard that + exists to prevent it. `readStrategy` had trimmed scalars since round 6 for the same reason; the list + path was the odd one out three lines away. +- **Round 13** (`store_comment` ×3, zero published inline comments): real, fixed in `6b83fd7`. All + three were negative routing assertions reading from a payload initialised to `{}` — an absent field + and an uncaptured payload are the same value, so they passed for the wrong reason. Demonstrated: + with capture disabled, three positives failed and **every negative passed**. The harness now throws + when nothing was captured, and the same experiment fails all six. +- **Round 12** (`store_comment` ×1, zero published inline comments): real, fixed in `3297b15`. + `value.every()` skips holes, so the `value is string[]` predicate was unsound — a sparse list passed + the guard and would have serialised a hole as `null` on the wire. Soundness, not a live path: JSON + cannot carry a hole. The repo's own `no-sparse-arrays` lint rule errors on the literal, so the test + allocates and leaves an index unset. +- **Round 11** (`store_comment` ×1, zero published inline comments): real, fixed in `290cb0e`. + The `readModelCompat` docstring still called `openRouterRouting` "the one object-valued field" — + false since `vercelGatewayRouting` was added, and misleading to anyone looking for structural + readers. +- **Round 10** (published inline ×1 + `store_comment` ×2): real, fixed in `b94d74c`. The reader + lowercased the host before comparing but the transport's check is case-sensitive, so + `https://AI-GATEWAY.VERCEL.SH/v1` was approved by the reader and then silently dropped on the wire. + The uppercase reader test added in round 9 therefore asserted nothing — a false green. +- **Round 9** (`store_comment` ×3): all real, fixed in `2ad9e83`. The hostname gate had a false + negative — a trailing-dot FQDN is the same host and was being dropped, silently discarding routing + the user did configure. The `readVercelGatewayRouting` comment had gone stale in the worst + direction, framing the gate as redundant. And the two wire harnesses were near-duplicates. +- **Round 8** (published inline ×1 + `store_comment` ×2): all real, fixed in `6bbec38`. The + transport's Vercel endpoint check is `baseUrl.includes("ai-gateway.vercel.sh")` — a substring + test, so `ai-gateway.vercel.sh.evil.com` and even a path containing the string match. The reader + now gates on an exact parsed hostname, so the Vercel-only body fields can only be produced for a + real Vercel endpoint. Two comments of mine were also overclaiming and are corrected. + +### When a stored finding has no body + +`body: null` means the ensemble log kept only type/severity/location. There is no API surface that +returns the prose — the action logs do not carry it, and the session page is a UI transcript. +Assess the location yourself and say in the PR comment that you did so and why; do not present a +guess as if it were the reviewer's words. If the intent turns out to be different, revisit. + +### A store comment with `body: null` is not the whole story + +Round 7 also produced a **published inline comment**, and it was flatly wrong: it claimed the +bundled `models-dev-catalog.json.gz` has no `openrouter` provider and that offline/first-run users +therefore cannot select OpenRouter, asking for the snapshot to be regenerated or the patch split. + +The premise is false, and verifiably so without leaving the repo: + +``` +top-level keys : ['version', 'source', 'updatedAt', 'etag', 'catalog'] +'openrouter' in d : False <- what a top-level membership check sees +in d['catalog'] : True <- 356 models, api https://openrouter.ai/api/v1 +``` + +The gzip is wrapped; providers sit under `catalog`. A top-level `'openrouter' in data` check +reproduces the reported conclusion exactly — I made the same mistake on my first pass, which is how +I recognised it. The file is tracked, unmodified, and byte-identical to `upstream/main` at `v0.5.8`. + +Rejected on the thread with that reproduction, no code change. The skill's rule applies: verify +empirically, then reject **with the evidence on the record**. + +What the finding did surface legitimately is scope: `resolveTransport()` maps an entry only when it +is present, so this patch is a mapper and will not conjure OpenRouter into a catalog lacking it. +That is intentional — the catalog is upstream data — and it is the reason to fix the mapping rather +than bundle an entry. + +### A vacuity fix is what makes a rebuttal provable + +Round 13 found the absence assertions were passing vacuously. Round 15 then accused those same three +assertions of failing, and the answer could only be settled **because** round 13 had already made them +meaningful: the harness now throws when nothing is captured, so `hasOwn=false` is a measured result +rather than an initialiser default. Without that fix the rebuttal would have been an argument about +reading comprehension; with it, it is a measurement. + +Fixing an unfalsifiable test is not only about catching future bugs — it is what lets you defend a +correct one. Two of the fifteen rounds were false positives, and both were rejected on evidence rather +than opinion. + +### Do not read convergence off severity + +Twice now the call was made that severity had decayed and the loop could stop — after round 7 and +after round 13 — and twice the next round found a **reachable** defect (the case-sensitivity +mismatch at round 10, and the untrimmed list entry at round 14). A run of low-severity rounds is not +evidence that the next one will be low-severity, because each fix creates the surface the next +finding lands on. + +Judge convergence on a round that comes back *empty*, and on nothing else. "The last few were only +docs" is a feeling, not a signal. + +### Apply a rule you just established to the code next to it + +Round 6 established, with a paragraph of reasoning, that a padded scalar must be trimmed rather than +forwarded verbatim or rejected. Round 14 found the list path three lines away still forwarding padded +values verbatim. The reasoning transferred unchanged and simply was not applied. + +When a fix encodes a rule, grep for the sibling call sites before moving on. The inconsistency is the +tell: two adjacent paths handling the same class of input differently, with no stated reason. + +### Absence assertions need a positive control + +Round 13's finding is the test-side half of round 10's: `expect(x).toBeUndefined()` cannot tell "the +value is correctly absent" from "nothing was captured at all". A negative assertion is only +meaningful if the harness proves something *did* happen. + +The rule that closed it: **every absence assertion needs a positive control** — a captured payload, +a fired callback, a non-empty result. `routingPayloadFor` now throws if `onPayload` never assigned, +so the check is structural rather than something a future test author has to remember. + +Across the loop, "verify the test can fail" has found the bug four times, twice for assertions that +could not fail, and it has never come back empty when the test was mine. + +### Soundness fixes still count, but say so + +Round 12 was a genuine defect in a guard (`every` skips holes, so the predicate lied about its input) +that is **not reachable** from persisted config, because JSON cannot express a hole. It was right to +fix — a wrong predicate is wrong — but the PR comment says plainly that it is soundness rather than a +live path, and how that was established. Overstating an unreachable defect as an exploit is its own +kind of comment rot, the same failure this patch keeps correcting. + +### Superlatives in comments rot fastest + +Four rounds (5, 8, 9, 11) caught a comment of mine that had outlived the code it described, and in +three of those my own later commit caused the drift. The recurring shape is a **superlative**: "the +one object-valued field", "cannot reach another gateway", "ignored by other providers". Each was +true when written and quietly false after the next change. + +Treat a superlative in a contract comment as a tripwire: whenever the surrounding field set or an +adjacent layer changes, re-read every "the one", "the only", "cannot", and "ignored by". They are +invisible to the compiler and to the tests, and a maintainer reads them as the contract. + +### The newest code is where the defects are + +Rounds 8, 9 and 10 each found a defect in the code added at the previous step — a loose check +inherited from the transport, an edge case the new gate missed, and then a test that could not fail. +Nothing found after round 7 touched the older parts of the patch, which had been stable for several +rounds. That is the expected shape of a review loop on a layered fix, and it is why the loop should +be run to a *converged* state rather than stopped when the diff starts to look familiar. + +**A false-green assertion is worse than no assertion.** The mixed-case test passed `2ad9e83` while +the routing it claimed to preserve was silently dropped, because it stopped at the reader and the +reader was the half that was wrong. Every test whose whole purpose is "this value reaches the other +side" must be verified by breaking the path and watching it fail — the practice used throughout this +patch (`disable the field, confirm red, restore`). + +### A gate written defensively inherits its own edge cases + +Round 9 caught a defect that round 8's fix *introduced*, which is a different shape from the earlier +rounds: rejecting a loose substring check by adding a stricter check in another layer means the +stricter check now carries the burden — including the hostname semantics the original author never +had to think about. + +The specific miss: `https://ai-gateway.vercel.sh./v1`. One trailing dot is the DNS root label and +names the same host, so it is not a lookalike. The gate dropped it, silently discarding the user's +routing — the exact failure this whole patch series has been eliminating, caused by the code meant to +prevent it. + +When replacing a permissive check with a strict one, enumerate what the *permissive* version +accepted that was legitimate, not only what it accepted that was wrong. The strict version has to +keep the first set. + +### Comments get written more confidently than the code warrants + +Rounds 5 and 8 both caught the same personal failure mode, and it is worth naming: a comment +asserting a *guarantee* the code does not provide. + +- Round 5: "Ignored by other providers" — the transport attaches the field for any endpoint. +- Round 8: "cannot reach another gateway" — the endpoint check is a substring test. +- Round 8 also: "forwarded verbatim to OpenRouter" — the value is filtered, and not endpoint-checked. + +A maintainer reads those comments as the contract, so an overclaim is worse than no comment. Describe +what the code does, and where a guarantee genuinely exists say which layer enforces it. The Vercel +entry is now correct *because* the reader enforces the hostname — that is a true statement about a +real check, not a hopeful one. + +**The general lesson: do not assume a finding is right because the previous six were.** Rounds 1–6 +were all real; this one was not, and accepting it would have meant regenerating a correct asset and +splitting a working patch for no reason. Verify each claim, in both directions. + +### Loop terminal state + +**Reached at round 15, on the skill's second condition.** The latest review is dated to the current +HEAD (`24b2d74`), its single finding was rejected with a documented reply, and **zero code changes** +followed — so the next review would see an identical diff and further rounds are noise. + +The condition first held at round 7 and was then **deliberately given up**, which is worth +understanding rather than treating as a contradiction: the request to add `vercelGatewayRouting` was +planned scope, not a response to review, and it changed code — so a fresh review became required and +the loop restarted at round 8. A terminal state only holds while the diff is frozen; completing +agreed scope always reopens it. + +Two false positives were rejected along the way (rounds 7 and 15), both on measurement rather than +opinion. Both threads are left **unresolved on purpose**, unlike round 4's. Round 4 was resolved +because it was fixed; resolving a thread we *disagree* with would hide the evidence and pre-empt the +maintainer's arbitration. Reply-on-the-record is the required action; resolving is not. + +Nothing is merged: PR #1 exists to collect review and show a diff, and merging into the fork's +`main` would be undone by the next update run. + +### An OpenRouter-only field goes to every provider + +Worth deciding on, not yet changed. Because the transport does not gate +`openRouterRouting` by endpoint, setting it on a `custom_provider` that points at a non-OpenRouter +gateway puts a `provider: {...}` object into that gateway's request body. Most OpenAI-compatible +endpoints ignore unknown fields; a strict one could reject the request. + +Our reader could gate it, mirroring the Vercel precedent: + +- **by base URL** (`openrouter.ai`) — robust to what the provider is named, but a heuristic, and a + proxy on another host would stop receiving routing; +- **by provider id** (`openrouter`) — exact, and matches how the preset is keyed, but silently drops + routing for a differently-named `custom_provider` pointed at OpenRouter. + +Left alone for now: it is a behaviour change with a real tradeoff, the field is documented as +OpenRouter-specific, and the round-5 finding was about the comment being untrue rather than the +behaviour being wrong. Decide deliberately rather than by reflex. + +### Don't brake the review loop early + +At round 3 I applied the skill's pathological-loop brake and stopped. That was **premature**: +round 4 found three genuinely new issues, including the test-depth gap that mattered most — none +were duplicates of earlier rounds. A re-raised point does not imply the next round will be empty. +The brake is for oscillation on the same code, and it should be a conclusion drawn from a *clean* +round, not from annoyance at a repeat. + +Two related traps now on the record: + +- **`eslint --fix` on a whole directory drags in unrelated formatting churn.** One revision of + `40e82c1` picked up `model-ref.ts` / `model-ref.test.ts` line rewrapping. Lint only the files you + actually changed, and check `git diff --cached` before committing. +- **A test that passes is not a test that works.** The wire-level assertion was verified by + disabling the field in `readModelCompat()` and watching it fail, then restoring. Do that for any + test whose whole purpose is "this value reaches the other side". + +### The force-push that destroyed someone else's commit + +A `copilot-swe-agent` commit `4abfca2` had **already fixed** the round-3 bug, with a test. The +next force-push overwrote it and the commit became an orphan in `.git`. + +`--force-with-lease` cannot prevent this. The script fetches immediately before pushing, so the +value it leases against is always the current remote tip and the lease always matches. It guards +against *concurrent* updates, not against a remote commit fetched moments earlier in the same run. + +Fixed by recording what we last pushed in `refs/minimax-fork/pushed/` and only +force-pushing when the remote tip equals that marker — i.e. only when rewriting our own history. +Otherwise the script refuses, names the foreign commits, and prints the explicit override. Verified +in a sandbox: a foreign commit is preserved (exit 2); a legitimate rebase still force-pushes. + +The fix was re-applied as `7a62cd8` with the agent's authorship preserved via `Co-authored-by`. + +Copilot stores findings in the session without publishing them, so `Findings: None` on the +overview is not a clean result — the wait script is the source of truth, because it reads the +`CCR Agent: Comment stored` rows the REST comments API misses. Where a stored record has +`body: null`, the prose may exist only in the session UI; the location and severity are still +enough to go read that code. + +## Notes on the update script + +Four bugs, all found the hard way. Keep them in mind before "simplifying" any away. + +**Never force-push the patch branch someone else may have touched.** The script fetched, then +force-pushed with `--force-with-lease=:`. Because the lease value was +fetched moments earlier in the same run, it always matched — so the push silently discarded a +`copilot-swe-agent` commit (`4abfca2`) that had already fixed a bug I was about to be told about. +A lease guards against *concurrent* updates, never against one you just fetched yourself. + +It now records its own last push in `refs/minimax-fork/pushed/` and force-pushes only when +the remote tip equals that marker. A fast-forward needs no force at all. Anything else stops with +the foreign commits listed and the override printed. Both paths are sandbox-tested. + +**A diverged `main` is not necessarily real work.** Upstream force-pushed a rewritten 0.5.4 +release commit, so the fork's `main` held `9639594 chore: bump version to 0.5.4` where upstream had +`135584a chore: bump version to 0.5.4 (#354)` — the same two-line bump, different SHA. +`git merge-base --is-ancestor` failed, the script hit its bail-out, and the run died *before* the +branch push and the build. `dist/` was left stamped at 0.5.4 while the source was a month newer. + +The script now distinguishes the two cases with `git cherry`: a leading `-` means an equivalent +patch already exists upstream, which is a rewritten commit, not work. Only genuine divergence stops +the run, and it warns instead of dying. A failed `main` sync is also non-fatal — an unrelated +branch must not gate the patch delivery the script exists to perform. + +**A missing build stamp means "unknown", not "current".** `scripts/build.mjs` does +`rmSync(dist, { recursive: true })` and wipes `.source-commit` along with everything else. The +stamp is the only record of which commit produced `dist/`, and a release often keeps the same +version string, so version alone cannot detect a stale binary. Treating a missing stamp as +up-to-date silently blessed unverified builds — the exact failure above. It now rebuilds. + +**`die` must be defined before the argument loop.** It was declared after the loop that calls it, +so a bad argument printed `die: command not found` (exit 127) instead of the helpful usage message. + +## Housekeeping + +- `refs/heads/pr/openrouter-preset` (at `71f9082`) is a stale duplicate of the preset patch, left + over from the PR-prep workflow. Nothing references it; delete it when convenient. +- `refs/minimax-fork/pushed/openrouter-preset` is the force-push marker. Deleting it makes the next + divergent push stop for manual confirmation, which is the safe failure direction. + +## Verification + +Run before pushing: + +```bash +pnpm install --frozen-lockfile +pnpm typecheck +pnpm lint +npx vitest run --config vitest.oss.config.mjs packages/local-runtime-v2/src/service/model-system/ +pnpm test:byok +pnpm build && node dist/cli.js --version +``` + +A bare `npx vitest run ` fails with `Failed to resolve entry for package "@mavis/shared"` — +that is a missing install or a missing build, not a code problem. The repo's `verify` profiles +(`pnpm verify --list`) are the upstream CI equivalent. diff --git a/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.test.ts b/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.test.ts index 9617b2252..62e1aa766 100644 --- a/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.test.ts @@ -241,6 +241,49 @@ describe('models.dev Provider Presets', () => { expect(presets.map((preset) => preset.providerId)).toEqual(['compatible']); }); + it('maps OpenRouter onto Chat Completions without admitting other OpenRouter SDK providers', async () => { + const presets = await parsePresetsForTest({ + openrouter: { + name: 'OpenRouter', + npm: '@openrouter/ai-sdk-provider', + api: 'https://openrouter.ai/api/v1', + models: { + 'xiaomi/mimo-v2.6-pro': { + name: 'MiMo-V2.6-Pro', + tool_call: true, + reasoning: true, + }, + 'text-only': { name: 'Text only', tool_call: false }, + }, + }, + standardcompute: { + name: 'Standard Compute', + npm: '@openrouter/ai-sdk-provider', + api: 'https://api.stdcmpt.com/v1', + models: { model: { name: 'Model', tool_call: true } }, + }, + }); + + expect( + presets.map(({ providerId, baseUrl, apiFormat, models }) => ({ + providerId, + baseUrl, + apiFormat, + modelIds: models.map((model) => model.modelId), + })), + ).toEqual([ + { + providerId: 'openrouter', + baseUrl: 'https://openrouter.ai/api/v1', + apiFormat: 'openai-completions', + modelIds: ['xiaomi/mimo-v2.6-pro'], + }, + ]); + expect(providerCompletionUrl('openai-completions', presets[0]!.baseUrl)).toBe( + 'https://openrouter.ai/api/v1/chat/completions', + ); + }); + it('maps the three supported transports and normalizes their request bases', async () => { const messagesProviderId = 'anthropic'; const presets = await parsePresetsForTest({ diff --git a/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.ts b/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.ts index bda1f9760..5506fa4fd 100644 --- a/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.ts +++ b/packages/local-runtime-v2/src/service/model-system/catalog/provider-presets/provider-presets.service.ts @@ -164,6 +164,12 @@ function resolveTransport( providerId === 'anthropic' && provider.api === undefined ? MESSAGES_API_BASE_URL : stringValue(provider.api); + } else if (providerId === 'openrouter') { + // models.dev publishes OpenRouter as @openrouter/ai-sdk-provider. Its HTTP + // API is OpenAI-compatible Chat Completions, which this runtime already speaks. + // Match the provider id, not the npm package: other catalogs reuse that package. + apiFormat = 'openai-completions'; + baseUrl = stringValue(provider.api); } else { return undefined; } diff --git a/packages/local-runtime-v2/src/service/model-system/contracts.ts b/packages/local-runtime-v2/src/service/model-system/contracts.ts index 7505cf2d1..3283e9de7 100644 --- a/packages/local-runtime-v2/src/service/model-system/contracts.ts +++ b/packages/local-runtime-v2/src/service/model-system/contracts.ts @@ -1,4 +1,4 @@ -import type { Api } from '@earendil-works/pi-ai'; +import type { Api, OpenRouterRouting, VercelGatewayRouting } from '@earendil-works/pi-ai'; import type { StreamFn } from '@earendil-works/pi-agent-core'; import type { LLMModelConfig } from '@mavis/agent-core/pi-turn-runner'; import type { @@ -60,6 +60,30 @@ export interface LocalModelCompatOverrides { | 'string-thinking' | 'ant-ling'; cacheControlFormat?: 'anthropic'; + /** + * OpenRouter provider routing preferences, sent as the request `provider` + * field. + * + * The pi openai-completions transport attaches this whenever it is present, + * for any endpoint on that transport — it does not check the base URL, unlike + * the Vercel gateway routing beside it. Set it only on models served by + * OpenRouter: other OpenAI-compatible gateways receive the extra `provider` + * field in the request body. + * @see https://openrouter.ai/docs/guides/routing/provider-selection + */ + openRouterRouting?: OpenRouterRouting; + /** + * Vercel AI Gateway provider routing preferences, sent as + * `providerOptions.gateway` on the request. + * + * Only `only` and `order` are declared, and the pi transport reads only those + * two. The transport's own endpoint check is a substring test on the base URL + * (`includes("ai-gateway.vercel.sh")`), which a lookalike host would pass, so + * the BYOK reader additionally gates this on an exact hostname — the field is + * only produced for a real Vercel AI Gateway endpoint. + * @see https://vercel.com/docs/ai-gateway/models-and-providers#provider-routing + */ + vercelGatewayRouting?: VercelGatewayRouting; } export interface LocalModelConfig extends Omit< @@ -217,8 +241,7 @@ export interface LocalModelResolverOptions { loginEpoch?: string, ) => void | Promise; readonly routingContextGetter?: () => - | import('@mavis/agent-tools/desktop').ManagedBackendRoutingContext - | undefined; + import('@mavis/agent-tools/desktop').ManagedBackendRoutingContext | undefined; readonly providerAuthGetter?: ( provider: string, ) => Promise | string | undefined; @@ -365,8 +388,7 @@ export interface DiscoveredModel { } export type ModelDiscoveryResult = - | { ok: true; models: DiscoveredModel[] } - | { ok: false; errorCode: string; errorMessage: string }; + { ok: true; models: DiscoveredModel[] } | { ok: false; errorCode: string; errorMessage: string }; export interface ModelDiscoveryClientLike { discover(target: ModelDiscoveryTarget): Promise; diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts index 50e6c42db..6271cc21d 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.test.ts @@ -1505,20 +1505,12 @@ describe('LocalModelResolver custom provider compat overrides', () => { 'https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1', 'https://example-workspace.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1', ])('honors an explicit developer-role override on %s', async (baseURL) => { - expect( - await systemPromptRoleFor( - { supportsDeveloperRole: true }, - baseURL, - ), - ).toBe('developer'); + expect(await systemPromptRoleFor({ supportsDeveloperRole: true }, baseURL)).toBe('developer'); }); it('honors an explicit developer-role override on a Kimi Coding endpoint', async () => { expect( - await systemPromptRoleFor( - { supportsDeveloperRole: true }, - 'https://api.kimi.com/coding/v1', - ), + await systemPromptRoleFor({ supportsDeveloperRole: true }, 'https://api.kimi.com/coding/v1'), ).toBe('developer'); }); }); @@ -1539,10 +1531,7 @@ describe('LocalModelResolver custom provider session affinity', () => { ); }; - const requestFor = async ( - compat: LocalModelConfig['compat'], - cacheRetention?: 'none', - ) => { + const requestFor = async (compat: LocalModelConfig['compat'], cacheRetention?: 'none') => { const modelConfig: LocalModelConfig = compat ? { compat } : {}; const resolver = new LocalModelResolver({ byokConfigGetter: () => ({ @@ -1609,6 +1598,140 @@ describe('LocalModelResolver custom provider session affinity', () => { expect(headers['x-session-affinity']).toBeUndefined(); }); + + // The routing readers exist to get a value onto the wire, so these pin the + // request pi would actually send rather than the resolved compat in between. + // Without them a regression after readModelCompat() would leave every reader + // test green while BYOK routing is still omitted from the request. + const routingPayloadFor = async (input: { + providerKey: string; + baseURL: string; + modelId: string; + compat: LocalModelConfig['compat']; + }) => { + const modelConfig: LocalModelConfig = input.compat ? { compat: input.compat } : {}; + const resolver = new LocalModelResolver({ + byokConfigGetter: () => ({ + custom_provider: { + [input.providerKey]: { + api: 'openai-completions', + options: { apiKey: 'route-key', baseURL: input.baseURL }, + models: { [input.modelId]: modelConfig }, + }, + }, + }), + }); + const resolved = await resolver.resolveModel({ + sessionId: 'session-routing-wire', + turnId: 'turn-routing-wire', + agentConfig: { + ...AGENT_CONFIG, + model: modelRefForModel(`custom_provider:${input.providerKey}`, input.modelId, modelConfig), + }, + }); + + let payload: { provider?: unknown; providerOptions?: unknown } | undefined; + await streamSimple( + resolved.model, + { + systemPrompt: 'Follow instructions.', + messages: [{ role: 'user', content: 'Hi', timestamp: Date.now() }], + }, + { + apiKey: 'route-key', + onPayload: (params: unknown) => { + payload = params as { provider?: unknown; providerOptions?: unknown }; + }, + // The payload is captured before transport, so the request never leaves the test. + fetch: (() => Promise.reject(new Error('offline'))) as typeof globalThis.fetch, + }, + ).result(); + // Absence assertions below are only meaningful if a payload was actually + // captured: with an uncaptured payload every `toBeUndefined()` on it would + // pass for the wrong reason, so fail loudly rather than report green. + if (!payload) { + throw new Error('onPayload captured nothing; an absence assertion would be vacuous.'); + } + return payload; + }; + + const providerRoutingFor = async (compat: LocalModelConfig['compat']) => + ( + await routingPayloadFor({ + providerKey: 'openrouter', + baseURL: 'https://openrouter.ai/api/v1', + modelId: 'deepseek/deepseek-v4.1-flash', + compat, + }) + ).provider; + + const vercelRoutingFor = async ( + compat: LocalModelConfig['compat'], + baseURL = 'https://ai-gateway.vercel.sh/v1', + ) => + routingPayloadFor({ + providerKey: 'vercel', + baseURL, + modelId: 'anthropic/claude', + compat, + }); + + it('sends OpenRouter routing preferences as the request provider field', async () => { + expect( + await providerRoutingFor({ + openRouterRouting: { only: ['DeepSeek'], allow_fallbacks: false }, + }), + ).toEqual({ only: ['DeepSeek'], allow_fallbacks: false }); + }); + + it('omits the provider field when no OpenRouter routing is configured', async () => { + expect(await providerRoutingFor(undefined)).toBeUndefined(); + expect(await providerRoutingFor({ supportsStrictMode: false })).toBeUndefined(); + }); + + // The Vercel keys take a different route: the transport gates them on the base + // URL and builds `providerOptions.gateway`, so this pins that separate path. + it('sends Vercel gateway routing as providerOptions.gateway', async () => { + const payload = await vercelRoutingFor({ + vercelGatewayRouting: { only: ['bedrock', 'anthropic'], order: ['anthropic'] }, + }); + + expect(payload.providerOptions).toEqual({ + gateway: { only: ['bedrock', 'anthropic'], order: ['anthropic'] }, + }); + // The OpenRouter field is not a Vercel field, and must stay out of it. + expect(payload.provider).toBeUndefined(); + }); + + it('does not send Vercel gateway options off the Vercel host', async () => { + // The transport gates on the base URL, so the same config is inert elsewhere. + const payload = await vercelRoutingFor( + { vercelGatewayRouting: { only: ['bedrock'] } }, + 'https://gateway.example/v1', + ); + + expect(payload.providerOptions).toBeUndefined(); + }); + + it('omits providerOptions.gateway when no Vercel routing is configured', async () => { + const payload = await vercelRoutingFor(undefined); + + expect(payload.providerOptions).toBeUndefined(); + }); + + it('sends Vercel gateway options for a mixed-case host', async () => { + // The transport's endpoint check is case-sensitive, so a host the reader + // approved must actually reach the wire in the form the transport matches. + // Without host folding this asserted nothing: the reader said yes and the + // request silently carried no gateway options. + const payload = await vercelRoutingFor( + { vercelGatewayRouting: { only: ['bedrock'] } }, + 'https://AI-GATEWAY.VERCEL.SH/v1', + ); + + expect(payload.providerOptions).toEqual({ gateway: { only: ['bedrock'] } }); + expect(payload.provider).toBeUndefined(); + }); }); describe('LocalModelResolver request image ceiling (#425)', () => { diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts index 1e1cafad5..6d1459304 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts @@ -238,7 +238,7 @@ describe('BYOK config helpers', () => { describe('custom BYOK compat overrides', () => { // Provider config is restored from on-disk JSON, so compat reaches planning untyped. - const planWithCompat = (rawConfig: string) => + const planWithCompatAt = (baseURL: string, rawConfig: string) => planCustomProviderResolution({ provider: 'custom_provider:gateway', providerKey: 'gateway', @@ -247,12 +247,15 @@ describe('custom BYOK compat overrides', () => { custom_provider: { gateway: { api: 'openai-completions', - options: { apiKey: 'gateway-key', baseURL: 'https://gateway.example/v1' }, + options: { apiKey: 'gateway-key', baseURL }, models: { 'kimi-k2-thinking': JSON.parse(rawConfig) }, }, }, }, })?.modelCompat; + const planWithCompat = (rawConfig: string) => + planWithCompatAt('https://gateway.example/v1', rawConfig); + const VERCEL_HOST = 'https://ai-gateway.vercel.sh/v1'; it.each(['null', '"compat"', '7', '[]', '[{"supportsDeveloperRole":false}]'])( 'ignores non-record compat value %s', @@ -313,4 +316,391 @@ describe('custom BYOK compat overrides', () => { planWithCompat('{"compat":{"__proto__":{"supportsDeveloperRole":false}}}'), ).toBeUndefined(); }); + + it('keeps a pinned OpenRouter provider and disables fallbacks', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"only":["DeepSeek"],"allow_fallbacks":false}}}', + ), + ).toEqual({ openRouterRouting: { only: ['DeepSeek'], allow_fallbacks: false } }); + }); + + it('keeps an ordered OpenRouter provider preference list', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"order":["DeepInfra","Fireworks"],"ignore":["Morph"]}}}', + ), + ).toEqual({ openRouterRouting: { order: ['DeepInfra', 'Fireworks'], ignore: ['Morph'] } }); + }); + + it('keeps a bare sort strategy and a sort object', () => { + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":"throughput"}}}')).toEqual({ + openRouterRouting: { sort: 'throughput' }, + }); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":"price","partition":"none"}}}}'), + ).toEqual({ openRouterRouting: { sort: { by: 'price', partition: 'none' } } }); + }); + + it('keeps privacy and cost routing keys', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"zdr":true,"data_collection":"deny","max_price":{"prompt":"0.000002","completion":1}}}}', + ), + ).toEqual({ + openRouterRouting: { + zdr: true, + data_collection: 'deny', + max_price: { prompt: '0.000002', completion: 1 }, + }, + }); + }); + + it('keeps throughput and latency preferences in both accepted shapes', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"preferred_min_throughput":{"p50":100},"preferred_max_latency":2.5}}}', + ), + ).toEqual({ + openRouterRouting: { preferred_min_throughput: { p50: 100 }, preferred_max_latency: 2.5 }, + }); + }); + + it('drops a routing object when every key is malformed', () => { + expect( + planWithCompat('{"compat":{"openRouterRouting":{"only":"DeepSeek","zdr":"yes"}}}'), + ).toBeUndefined(); + }); + + it('drops a routing key carrying the wrong type but keeps its valid siblings', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"only":"DeepSeek","allow_fallbacks":"false","zdr":true}}}', + ), + ).toEqual({ openRouterRouting: { zdr: true } }); + }); + + it('rejects a mixed provider list instead of filtering it down', () => { + // A shrunk list would silently pin/order/exclude a different provider set + // than the one written, so the whole key is dropped. + expect( + planWithCompat('{"compat":{"openRouterRouting":{"only":["DeepSeek",42]}}}'), + ).toBeUndefined(); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"order":["DeepInfra",null],"zdr":true}}}'), + ).toEqual({ openRouterRouting: { zdr: true } }); + }); + + it('rejects a sparse provider list', () => { + // JSON cannot carry a hole, so the config is built directly rather than + // parsed. `every` skips holes: without densifying first, this array would + // satisfy the `string[]` predicate and serialise its hole as `null` on the + // wire, which is the malformed list the reader exists to reject. + // The repo's linter forbids a sparse array literal, so the hole is made by + // allocating then leaving an index unset. + const sparse = new Array(3); + sparse[0] = 'DeepSeek'; + sparse[2] = 'DeepInfra'; + expect(1 in sparse).toBe(false); // index 1 really is a hole + expect(sparse.every(() => true)).toBe(true); // every() is blind to it + expect( + planCustomProviderResolution({ + provider: 'custom_provider:gateway', + providerKey: 'gateway', + modelId: 'kimi-k2-thinking', + byok: { + custom_provider: { + gateway: { + api: 'openai-completions', + options: { apiKey: 'gateway-key', baseURL: 'https://gateway.example/v1' }, + models: { + 'kimi-k2-thinking': { + compat: { openRouterRouting: { only: sparse } } as never, + }, + }, + }, + }, + }, + })?.modelCompat, + ).toBeUndefined(); + }); + + it('rejects a provider list padded with an empty entry', () => { + // An empty slug can never match a provider, so it is as invalid as a + // non-string. readStrategy rejects the empty string in the scalar case, so + // the list must not be laxer. + expect(planWithCompat('{"compat":{"openRouterRouting":{"only":[""]}}}')).toBeUndefined(); + expect(planWithCompat('{"compat":{"openRouterRouting":{"only":[" "]}}}')).toBeUndefined(); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"order":["DeepInfra",""],"zdr":true}}}'), + ).toEqual({ openRouterRouting: { zdr: true } }); + }); + + it('trims incidental whitespace from list entries', () => { + // A padded slug matches no provider, so forwarding it verbatim would make + // the routing silently ineffective while rejecting would discard routing the + // user did configure. readStrategy trims scalars for the same reason. + expect(planWithCompat('{"compat":{"openRouterRouting":{"only":[" DeepSeek "]}}}')).toEqual({ + openRouterRouting: { only: ['DeepSeek'] }, + }); + expect( + planWithCompatAt( + VERCEL_HOST, + '{"compat":{"vercelGatewayRouting":{"order":[" anthropic ","bedrock"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { order: ['anthropic', 'bedrock'] } }); + }); + + it('trims incidental whitespace from a strategy string', () => { + // A padded value cannot match anything OpenRouter knows; forwarding it + // verbatim would fail, and dropping the key would discard routing silently. + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":" latency "}}}')).toEqual({ + openRouterRouting: { sort: 'latency' }, + }); + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":" price "}}}}')).toEqual({ + openRouterRouting: { sort: { by: 'price' } }, + }); + }); + + it('rejects an empty provider list', () => { + expect(planWithCompat('{"compat":{"openRouterRouting":{"only":[]}}}')).toBeUndefined(); + }); + + it('drops a price that is a non-numeric string rather than forwarding it', () => { + // "free" / "$5" are strings, but not prices; forwarding them as a ceiling + // would send a malformed provider override to OpenRouter. + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"max_price":{"prompt":"free","completion":1}}}}', + ), + ).toEqual({ openRouterRouting: { max_price: { completion: 1 } } }); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"max_price":{"prompt":"$5"}}}}'), + ).toBeUndefined(); + }); + + it('keeps decimal and exponent price strings', () => { + expect( + planWithCompat('{"compat":{"openRouterRouting":{"max_price":{"prompt":"0.000002"}}}}'), + ).toEqual({ openRouterRouting: { max_price: { prompt: '0.000002' } } }); + }); + + it('drops the whole sort object when a present member is not a usable strategy', () => { + // Atomic: {by: "price", partition: 42} must not become {by: "price"}. + expect( + planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":"price","partition":42}}}}'), + ).toBeUndefined(); + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":42}}}}')).toBeUndefined(); + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":""}}}}')).toBeUndefined(); + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":""}}}')).toBeUndefined(); + }); + + it('accepts any non-empty sort strategy, not just the documented examples', () => { + // pi types `sort`/`by`/`partition` as plain string and documents + // price/throughput/latency as examples, so narrowing to those would drop a + // strategy the contract permits and the transport forwards. + expect(planWithCompat('{"compat":{"openRouterRouting":{"sort":"custom-strategy"}}}')).toEqual({ + openRouterRouting: { sort: 'custom-strategy' }, + }); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":"custom-strategy"}}}}'), + ).toEqual({ openRouterRouting: { sort: { by: 'custom-strategy' } } }); + }); + + it('drops a price whose exponent form is not finite', () => { + // "1e999" matches the numeric string shape but parses to Infinity, the same + // non-finite value the numeric branch rejects. + expect( + planWithCompat('{"compat":{"openRouterRouting":{"max_price":{"prompt":"1e999"}}}}'), + ).toBeUndefined(); + expect( + planWithCompat('{"compat":{"openRouterRouting":{"max_price":{"prompt":"1e300"}}}}'), + ).toEqual({ openRouterRouting: { max_price: { prompt: '1e300' } } }); + }); + + it('strips unknown nested routing keys from structured objects', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"max_price":{"prompt":"0.000002","surge":1},"preferred_min_throughput":{"p50":100,"p95":200}}}}', + ), + ).toEqual({ + openRouterRouting: { + max_price: { prompt: '0.000002' }, + preferred_min_throughput: { p50: 100 }, + }, + }); + }); + + it('keeps only known price keys when every known key is valid', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"max_price":{"prompt":1,"completion":2,"image":3,"audio":4,"request":5,"bogus":6}}}}', + ), + ).toEqual({ + openRouterRouting: { + max_price: { prompt: 1, completion: 2, image: 3, audio: 4, request: 5 }, + }, + }); + }); + + it('keeps only known percentile keys', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"preferred_max_latency":{"p50":1,"p75":2,"p90":3,"p99":4,"p100":5}}}}', + ), + ).toEqual({ + openRouterRouting: { preferred_max_latency: { p50: 1, p75: 2, p90: 3, p99: 4 } }, + }); + }); + + it('drops a record whose only keys are unknown', () => { + expect( + planWithCompat('{"compat":{"openRouterRouting":{"max_price":{"surge":1}}}}'), + ).toBeUndefined(); + }); + + it('keeps a sort object whose present members all validate', () => { + expect( + planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":"price","partition":null}}}}'), + ).toEqual({ openRouterRouting: { sort: { by: 'price', partition: null } } }); + }); + + it('ignores unknown sort keys instead of invalidating it', () => { + expect( + planWithCompat('{"compat":{"openRouterRouting":{"sort":{"by":"latency","bogus":true}}}}'), + ).toEqual({ openRouterRouting: { sort: { by: 'latency' } } }); + }); + + it('drops non-record routing values', () => { + expect(planWithCompat('{"compat":{"openRouterRouting":[]}}')).toBeUndefined(); + expect(planWithCompat('{"compat":{"openRouterRouting":"only"}}')).toBeUndefined(); + }); + + it('does not forward unknown routing keys to OpenRouter', () => { + expect( + planWithCompat( + '{"compat":{"openRouterRouting":{"only":["DeepSeek"],"require_attestation":true}}}', + ), + ).toEqual({ openRouterRouting: { only: ['DeepSeek'] } }); + }); + + it('leaves routing absent so no provider field is sent by default', () => { + expect(planWithCompat('{"compat":{"supportsStore":false}}')).toEqual({ supportsStore: false }); + }); + + it('keeps Vercel gateway routing and nothing else', () => { + expect( + planWithCompatAt( + VERCEL_HOST, + '{"compat":{"vercelGatewayRouting":{"only":["bedrock","anthropic"],"order":["anthropic"],"bogus":["x"]}}}', + ), + ).toEqual({ + vercelGatewayRouting: { only: ['bedrock', 'anthropic'], order: ['anthropic'] }, + }); + }); + + it('applies the list rules to Vercel routing', () => { + // Same list semantics as the OpenRouter keys: a mixed or padded list is not + // silently shrunk, and a sibling left valid survives. + expect( + planWithCompatAt(VERCEL_HOST, '{"compat":{"vercelGatewayRouting":{"only":["bedrock",42]}}}'), + ).toBeUndefined(); + expect( + planWithCompatAt( + VERCEL_HOST, + '{"compat":{"vercelGatewayRouting":{"only":[""],"order":["anthropic"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { order: ['anthropic'] } }); + expect( + planWithCompatAt(VERCEL_HOST, '{"compat":{"vercelGatewayRouting":{"only":[]}}}'), + ).toBeUndefined(); + }); + + it('drops Vercel routing when it is not a record of usable keys', () => { + expect(planWithCompatAt(VERCEL_HOST, '{"compat":{"vercelGatewayRouting":[]}}')).toBeUndefined(); + expect( + planWithCompatAt(VERCEL_HOST, '{"compat":{"vercelGatewayRouting":"only"}}'), + ).toBeUndefined(); + expect( + planWithCompatAt(VERCEL_HOST, '{"compat":{"vercelGatewayRouting":{"bogus":["x"]}}}'), + ).toBeUndefined(); + }); + + it('drops Vercel routing on an endpoint that merely looks like the Vercel host', () => { + // The transport tests `baseUrl.includes("ai-gateway.vercel.sh")`, which all + // of these satisfy, so the reader has to be the one to refuse them. + const lookalikes = [ + 'https://ai-gateway.vercel.sh.example.com/v1', + 'https://proxy.ai-gateway.vercel.sh/v1', + 'https://notvercel.example/ai-gateway.vercel.sh/v1', + 'https://ai-gateway.vercel.sh.evil.com/v1', + ]; + for (const baseURL of lookalikes) { + expect( + planWithCompatAt(baseURL, '{"compat":{"vercelGatewayRouting":{"only":["bedrock"]}}}'), + ).toBeUndefined(); + } + }); + + it('keeps other compat keys when Vercel routing is dropped for the endpoint', () => { + expect( + planWithCompatAt( + 'https://ai-gateway.vercel.sh.example.com/v1', + '{"compat":{"supportsStore":false,"vercelGatewayRouting":{"only":["bedrock"]}}}', + ), + ).toEqual({ supportsStore: false }); + }); + + it('accepts the Vercel host without a scheme or with a port', () => { + expect( + planWithCompatAt( + 'ai-gateway.vercel.sh/v1', + '{"compat":{"vercelGatewayRouting":{"only":["bedrock"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { only: ['bedrock'] } }); + expect( + planWithCompatAt( + 'https://ai-gateway.vercel.sh:443/v1', + '{"compat":{"vercelGatewayRouting":{"only":["bedrock"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { only: ['bedrock'] } }); + }); + + it('treats a trailing-dot FQDN as the same host', () => { + // `ai-gateway.vercel.sh.` is the DNS root-label form of the same name, so it + // must not be dropped as if it were a lookalike — the transport's substring + // check accepts it, and refusing here would silently discard routing. + expect( + planWithCompatAt( + 'https://ai-gateway.vercel.sh./v1', + '{"compat":{"vercelGatewayRouting":{"only":["bedrock"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { only: ['bedrock'] } }); + expect( + planWithCompatAt( + 'AI-GATEWAY.VERCEL.SH/v1', + '{"compat":{"vercelGatewayRouting":{"only":["bedrock"]}}}', + ), + ).toEqual({ vercelGatewayRouting: { only: ['bedrock'] } }); + }); + + it('leaves OpenRouter routing ungated by the endpoint', () => { + // Unlike the Vercel field, the transport applies this one for any endpoint, + // so the reader must not quietly drop it on a non-OpenRouter host. + expect(planWithCompat('{"compat":{"openRouterRouting":{"only":["DeepSeek"]}}}')).toEqual({ + openRouterRouting: { only: ['DeepSeek'] }, + }); + }); + + it('carries both routing dialects at once without cross-talk', () => { + expect( + planWithCompatAt( + VERCEL_HOST, + '{"compat":{"openRouterRouting":{"only":["DeepSeek"]},"vercelGatewayRouting":{"order":["anthropic"]}}}', + ), + ).toEqual({ + openRouterRouting: { only: ['DeepSeek'] }, + vercelGatewayRouting: { order: ['anthropic'] }, + }); + }); }); diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts index 32751984e..1534726b9 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts @@ -1,4 +1,4 @@ -import type { Api } from '@earendil-works/pi-ai'; +import type { Api, OpenRouterRouting, VercelGatewayRouting } from '@earendil-works/pi-ai'; import { minimaxApiModels, getRuntimeRegion } from '@mavis/config'; import type { @@ -84,11 +84,15 @@ export function planCustomProviderResolution(input: { readStringRecord(config.options?.headers), readStringRecord(modelConfig.headers), ); - const modelCompat = readModelCompat(modelConfig.compat); + // The host is folded to lower case before the gate reads it, so the URL the + // transport will see is the same one this decision is made about. + const baseUrl = lowercaseBaseUrlHost(credentials.baseUrl); + const modelCompat = restrictRoutingToEndpoint(readModelCompat(modelConfig.compat), baseUrl); return { provider: input.provider, api: resolveCustomProviderApi(config.api), ...credentials, + baseUrl, ...customProviderLimits(modelConfig), ...(configHeaders ? { configHeaders } : {}), ...(modelCompat ? { modelCompat } : {}), @@ -220,6 +224,291 @@ function readCompatEnum(value: unknown, allowed: readonly T[]) return typeof value === 'string' ? allowed.find((option) => option === value) : undefined; } +/** + * Reads a list-shaped routing key (`only`, `order`, `ignore`, `quantizations`). + * + * A mixed array is not a `string[]`, so the whole list is rejected rather than + * filtered down. Silently shrinking one would change the routing decision + * without saying so — `only: ["DeepSeek", 42]` would otherwise pin to a + * different provider set than the one that was written. A list padded with an + * empty entry is rejected for the same reason: an empty slug can never match. + * + * Entries are trimmed, matching {@link readStrategy} and for the same reason: a + * padded slug matches no provider, so forwarding it verbatim makes the routing + * silently ineffective, while dropping the list would silently discard routing + * the user did configure. The result is always a new array, so the parsed config + * is never mutated. + * + * `Array.from` densifies first because `every` skips holes: `["a", , "b"]` would + * otherwise satisfy the string check while not being a `string[]` at all, and a + * hole serialises to `null` on the wire. Persisted config is JSON-parsed and + * cannot contain a hole, so that part is soundness rather than a live path. + */ +function readStringList(value: unknown): string[] | undefined { + if (!Array.isArray(value) || value.length === 0) return undefined; + const entries = Array.from(value); + if (!entries.every((entry): entry is string => typeof entry === 'string')) return undefined; + const trimmed = entries.map((entry) => entry.trim()); + return trimmed.includes('') ? undefined : trimmed; +} + +/** Reads a finite number, dropping NaN/Infinity that JSON cannot legitimately carry. */ +function readNumber(value: unknown): number | undefined { + return typeof value === 'number' && Number.isFinite(value) ? value : undefined; +} + +/** A decimal number, optionally signed and optionally in exponent form. */ +const NUMERIC_STRING = /^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$/; + +/** + * Reads a numeric-or-string money field (`max_price.*`). OpenRouter accepts both + * a number and a decimal string here, so the declared type is a union. + * + * The string form must still be a usable number. Accepting any non-empty string + * would forward values like `"free"` or `"$5"` as a price ceiling, and accepting + * the exponent form unchecked would forward `"1e999"`, which parses to + * `Infinity` — the same non-finite value the numeric branch rejects. + */ +function readNumberish(value: unknown): number | string | undefined { + if (typeof value === 'number') return readNumber(value); + if (typeof value !== 'string') return undefined; + const trimmed = value.trim(); + if (!NUMERIC_STRING.test(trimmed) || !Number.isFinite(Number(trimmed))) return undefined; + return trimmed; +} + +const MAX_PRICE_KEYS = ['prompt', 'completion', 'image', 'audio', 'request'] as const; +const PERCENTILE_KEYS = ['p50', 'p75', 'p90', 'p99'] as const; + +/** + * Reads `{ prompt?, completion?, image?, audio?, request? }` price ceilings. + * + * Iterates the known keys rather than the object's own entries: an unrecognised + * key is not a declared field, so it must not ride along into the request. This + * is the same rule the scalar reader applies, and it is what keeps a + * non-OpenRouter shape from being forwarded as a routing override. + */ +function readMaxPrice(value: unknown): Record | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const source = value as Record; + const maxPrice: Record = {}; + for (const key of MAX_PRICE_KEYS) { + const candidate = readNumberish(source[key]); + if (candidate !== undefined) maxPrice[key] = candidate; + } + return Object.keys(maxPrice).length > 0 ? maxPrice : undefined; +} + +/** + * Reads a percentile cutoff map (`p50`, `p75`, `p90`, `p99`), used by the + * `preferred_min_throughput` / `preferred_max_latency` fields. These accept a + * bare number as well, but the object form is numeric-only. + * + * Known-keys-only, for the same reason as {@link readMaxPrice}: a percentile + * outside the declared set is not a field OpenRouter accepts. + */ +function readPercentiles(value: unknown): Record | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const source = value as Record; + const percentiles: Record = {}; + for (const key of PERCENTILE_KEYS) { + const candidate = readNumber(source[key]); + if (candidate !== undefined) percentiles[key] = candidate; + } + return Object.keys(percentiles).length > 0 ? percentiles : undefined; +} + +const ROUTING_BOOLEAN_KEYS = [ + 'allow_fallbacks', + 'require_parameters', + 'zdr', + 'enforce_distillable_text', +] as const; +const ROUTING_LIST_KEYS = ['order', 'only', 'ignore', 'quantizations'] as const; +const ROUTING_PERCENTILE_KEYS = ['preferred_min_throughput', 'preferred_max_latency'] as const; +const DATA_COLLECTIONS = ['allow', 'deny'] as const; +const VERCEL_ROUTING_LIST_KEYS = ['only', 'order'] as const; +const VERCEL_GATEWAY_HOST = 'ai-gateway.vercel.sh'; + +/** Hostname of a base URL, tolerating a missing scheme and a trailing-dot FQDN. */ +function hostnameOf(value: string | undefined): string | undefined { + if (!value) return undefined; + const withScheme = /^[a-z][a-z0-9+.-]*:\/\//iu.test(value) ? value : `https://${value}`; + try { + // A single trailing dot is the DNS root label and names the same host, so + // `ai-gateway.vercel.sh.` must not be treated as a different endpoint. + return new URL(withScheme).hostname.toLowerCase().replace(/\.$/u, ''); + } catch { + return undefined; + } +} + +/** + * Lowercases the host (and port) of a base URL, leaving scheme and path alone. + * + * Hostnames are case-insensitive, but the pi transport recognises the Vercel + * gateway with a case-sensitive substring test, and the URL normalizer only + * strips path suffixes. `https://AI-GATEWAY.VERCEL.SH/v1` would therefore be + * approved by the reader below and then silently ignored on the wire — the + * reader and transport disagreeing about the same URL. Folding the host here + * keeps them in agreement, and the routing the user configured is applied. + */ +function lowercaseBaseUrlHost(baseUrl: string): string { + const schemeEnd = baseUrl.indexOf('://'); + const scheme = schemeEnd === -1 ? '' : baseUrl.slice(0, schemeEnd + 3); + const remainder = baseUrl.slice(scheme.length); + const authorityEnd = remainder.search(/[/?#]/u); + const authority = authorityEnd === -1 ? remainder : remainder.slice(0, authorityEnd); + const tail = authorityEnd === -1 ? '' : remainder.slice(authorityEnd); + const credentialsEnd = authority.lastIndexOf('@'); + const userinfo = authority.slice(0, credentialsEnd + 1); + const hostAndPort = authority.slice(credentialsEnd + 1); + return `${scheme}${userinfo}${hostAndPort.toLowerCase()}${tail}`; +} + +/** + * Drops `vercelGatewayRouting` unless the endpoint really is a Vercel AI + * Gateway host. + * + * The transport decides with `model.baseUrl.includes("ai-gateway.vercel.sh")` — + * a substring test, not a hostname test. `ai-gateway.vercel.sh.example.com` + * matches it, and so does any URL carrying that string in a path. Since the + * Vercel branch attaches gateway-only body fields, an endpoint that merely looks + * Vercel-ish should not be able to attract them, and the transport is vendored + * third-party code that this patch does not modify. + * + * Gating here means the field is only ever produced for an exact hostname, so + * the loose check downstream cannot be reached with a lookalike. Every other + * compat key passes through untouched. + */ +function restrictRoutingToEndpoint( + compat: LocalModelCompatOverrides | undefined, + baseUrl: string | undefined, +): LocalModelCompatOverrides | undefined { + if (!compat?.vercelGatewayRouting) return compat; + if (hostnameOf(baseUrl) === VERCEL_GATEWAY_HOST) return compat; + const rest: LocalModelCompatOverrides = { ...compat }; + delete rest.vercelGatewayRouting; + return Object.keys(rest).length > 0 ? rest : undefined; +} + +/** + * Reads `compat.vercelGatewayRouting` out of untrusted provider config. + * + * `VercelGatewayRouting` declares exactly `only` and `order`, both `string[]`, + * and the transport reads only those two before building + * `providerOptions.gateway` — so keeping to the declared keys is both the + * contract and the whole of what can have an effect. + * + * This function does not decide the endpoint. {@link restrictRoutingToEndpoint} + * does, and it is load-bearing rather than defensive: the transport's own check + * is a substring test (`baseUrl.includes("ai-gateway.vercel.sh")`) that a + * lookalike host passes, so removing that gate would let Vercel-only body fields + * reach a host that merely looks Vercel. Do not drop it as redundant. + */ +function readVercelGatewayRouting(value: unknown): VercelGatewayRouting | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const source = value as Record; + const routing: VercelGatewayRouting = {}; + for (const key of VERCEL_ROUTING_LIST_KEYS) { + const list = readStringList(source[key]); + if (list) routing[key] = list; + } + return Object.keys(routing).length > 0 ? routing : undefined; +} + +/** + * A non-empty strategy string, for `sort` / `sort.by` / `sort.partition`. + * + * The declared contract is plain `string` — pi documents `price`, `throughput` + * and `latency` as examples ("e.g."), not as an exhaustive union — so this + * checks the type and rejects only the empty/whitespace-only string. Narrowing + * to the examples would silently drop a strategy the shared type permits and pi + * forwards. + * + * Incidental surrounding whitespace is trimmed rather than rejected: a padded + * value cannot match anything OpenRouter knows, so forwarding it verbatim would + * fail the request, and dropping the whole key would silently discard routing + * the user did configure. Trimming is the one case where rewriting the value is + * safer than either alternative. + */ +function readStrategy(value: unknown): string | undefined { + if (typeof value !== 'string') return undefined; + const trimmed = value.trim(); + return trimmed === '' ? undefined : trimmed; +} + +/** + * Reads `sort`, which OpenRouter accepts either as a bare strategy name or as + * `{ by, partition }`. + * + * The object form is validated atomically: if a key that is *present* fails + * validation the whole object is dropped, so a partial sort is never forwarded + * as a routing override. Unknown keys are ignored, as everywhere else. + */ +function readRoutingSort( + value: unknown, +): string | { by?: string; partition?: string | null } | undefined { + const bare = readStrategy(value); + if (bare) return bare; + if (typeof value === 'string') return undefined; + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const source = value as Record; + const sort: { by?: string; partition?: string | null } = {}; + if (source.by !== undefined) { + const by = readStrategy(source.by); + if (!by) return undefined; + sort.by = by; + } + if (source.partition !== undefined) { + if (source.partition === null) { + sort.partition = null; + } else { + const partition = readStrategy(source.partition); + if (!partition) return undefined; + sort.partition = partition; + } + } + return sort.by || sort.partition !== undefined ? sort : undefined; +} + +/** + * Read `compat.openRouterRouting` out of untrusted provider config. + * + * The result is attached to the request as the `provider` field by the + * openai-completions transport, which does not check the endpoint — so this is + * not restricted to OpenRouter and it is not forwarded verbatim: every key is + * validated at its declared type and anything unknown or malformed is dropped. + * A rejected key never invalidates its valid siblings, and an object left with + * no usable keys is dropped entirely so an empty `provider` object is never + * sent. + */ +function readOpenRouterRouting(value: unknown): OpenRouterRouting | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const source: Record = { ...value }; + const routing: OpenRouterRouting = {}; + for (const key of ROUTING_BOOLEAN_KEYS) { + const candidate = source[key]; + if (typeof candidate === 'boolean') routing[key] = candidate; + } + const dataCollection = readCompatEnum(source.data_collection, DATA_COLLECTIONS); + if (dataCollection) routing.data_collection = dataCollection; + for (const key of ROUTING_LIST_KEYS) { + const list = readStringList(source[key]); + if (list) routing[key] = list; + } + const sort = readRoutingSort(source.sort); + if (sort) routing.sort = sort; + const maxPrice = readMaxPrice(source.max_price); + if (maxPrice) routing.max_price = maxPrice; + for (const key of ROUTING_PERCENTILE_KEYS) { + // A bare number is valid on its own; the object form needs the map reader. + const percentiles = readPercentiles(source[key]) ?? readNumber(source[key]); + if (percentiles !== undefined) routing[key] = percentiles; + } + return Object.keys(routing).length > 0 ? routing : undefined; +} + /** * Read model-level compatibility overrides out of untrusted provider config. * @@ -227,6 +516,9 @@ function readCompatEnum(value: unknown, allowed: readonly T[]) * accepted only at its declared type. A value of the wrong type is dropped rather than * forwarded, because pi treats any present field as an explicit override and a truthy * string such as `"false"` would otherwise invert the intended behavior. + * + * The object-valued fields, `openRouterRouting` and `vercelGatewayRouting`, are + * validated structurally by their own readers rather than key by key here. */ function readModelCompat(value: unknown): LocalModelCompatOverrides | undefined { if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; @@ -242,6 +534,10 @@ function readModelCompat(value: unknown): LocalModelCompatOverrides | undefined if (thinkingFormat) compat.thinkingFormat = thinkingFormat; const cacheControlFormat = readCompatEnum(source.cacheControlFormat, CACHE_CONTROL_FORMATS); if (cacheControlFormat) compat.cacheControlFormat = cacheControlFormat; + const openRouterRouting = readOpenRouterRouting(source.openRouterRouting); + if (openRouterRouting) compat.openRouterRouting = openRouterRouting; + const vercelGatewayRouting = readVercelGatewayRouting(source.vercelGatewayRouting); + if (vercelGatewayRouting) compat.vercelGatewayRouting = vercelGatewayRouting; return Object.keys(compat).length > 0 ? compat : undefined; } diff --git a/packages/tui/src/tui/features/provider/onboarding.ts b/packages/tui/src/tui/features/provider/onboarding.ts index a3bed0bb3..68dd02053 100644 --- a/packages/tui/src/tui/features/provider/onboarding.ts +++ b/packages/tui/src/tui/features/provider/onboarding.ts @@ -681,6 +681,10 @@ export class TuiProviderOnboarding implements Component, Focusable { private saveInput(apiKey: string): McodeSaveProviderCandidateInput | undefined { if (this.template) { if (!this.selectedModelId) return undefined; + // Persist only the chosen model; a catalog such as OpenRouter's would + // otherwise copy hundreds of entries into config.yaml. + const selected = this.selectedModel(); + const models = selected ? [selected] : []; return { ...(this.connection ? { @@ -692,9 +696,7 @@ export class TuiProviderOnboarding implements Component, Focusable { baseUrl: this.presetBaseUrl || this.connection?.baseUrl || this.template.baseUrl, ...(apiKey ? { apiKey } : {}), apiFormat: this.template.apiFormat, - models: this.connection - ? additiveProviderModels(this.connection, this.template.models) - : this.template.models, + models: this.connection ? additiveProviderModels(this.connection, models) : models, modelId: this.selectedModelId, saveAndUse: true, }; diff --git a/packages/tui/test/unit/tui-provider-onboarding.test.ts b/packages/tui/test/unit/tui-provider-onboarding.test.ts index 37df0e843..74c1658e9 100644 --- a/packages/tui/test/unit/tui-provider-onboarding.test.ts +++ b/packages/tui/test/unit/tui-provider-onboarding.test.ts @@ -143,7 +143,7 @@ describe("TuiProviderOnboarding", () => { baseUrl: "https://api.deepseek.com/v1", apiKey: "sk-super-secret", apiFormat: "openai-completions", - models: knownTemplate.models, + models: [knownTemplate.models[0]], modelId: "deepseek-chat", saveAndUse: true, }); @@ -415,17 +415,37 @@ describe("existing connection onboarding", () => { apiFormat: knownTemplate.apiFormat, saveAndUse: true, modelId: "deepseek-chat", - models: [ - { modelId: "deepseek-chat" }, - { modelId: "private-model" }, - knownTemplate.models[1], - ], + models: [{ modelId: "deepseek-chat" }, { modelId: "private-model" }], }); expect(onComplete).toHaveBeenCalledWith( expect.objectContaining({ reused: true }), ); }); + it("adds only the newly chosen catalog model to an existing connection", async () => { + const onSave = vi.fn(async () => ({ success: true })); + const onboarding = new TuiProviderOnboarding({ + templates: [knownTemplate], + providers: [{ ...savedConnection, models: [{ modelId: "private-model" }] }], + onSave, + onComplete: vi.fn(), + onCancel: vi.fn(), + requestRender: vi.fn(), + }); + onboarding.handleInput("\r"); + onboarding.handleInput("\r"); + onboarding.handleInput("\u001b[B"); + onboarding.handleInput("\r"); + onboarding.handleInput("\r"); + await vi.waitFor(() => expect(onSave).toHaveBeenCalledOnce()); + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ + modelId: "deepseek-reasoner", + models: [{ modelId: "private-model" }, knownTemplate.models[1]], + }), + ); + }); + it("creates a separate account only after an explicit choice and alias", async () => { const onSave = vi.fn(async () => ({ success: true })); const onboarding = new TuiProviderOnboarding({ diff --git a/packages/tui/test/unit/tui/controller/product/feature-flow.test.ts b/packages/tui/test/unit/tui/controller/product/feature-flow.test.ts index 0e6d77be3..1ff9b2245 100644 --- a/packages/tui/test/unit/tui/controller/product/feature-flow.test.ts +++ b/packages/tui/test/unit/tui/controller/product/feature-flow.test.ts @@ -872,7 +872,7 @@ describe("TuiFeatureFlow", () => { baseUrl: "https://api.deepseek.com/v1", apiKey: "sk-secret", apiFormat: "openai-completions", - models: template.models, + models: [template.models[0]], modelId: "deepseek-chat", saveAndUse: true, }, diff --git a/release/public-source.json b/release/public-source.json index 28bc3882f..878559ef2 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -34,6 +34,7 @@ "LICENSE", "LICENSE-STATUS.md", "NOTICE", + "README-fork-openrouter.md", "README.md", "README_ZH.md", "SECURITY.md",