feat: OpenRouter provider routing and reasoning control#1958
Conversation
|
Automated Test Results: PASSED Cloud test-engine rerun against
Manual code review is still required before merge. |
|
Conflict status update: I verified locally that Current conclusion: automated tests passed, but the PR cannot be merged while GitHub still reports it as conflicting. The branch needs to be updated by the author/maintainer, or a replacement branch/PR needs to be created from the locally clean merge commit and then retested. |
shinetata
left a comment
There was a problem hiding this comment.
Thanks for this PR, Erick — OpenRouter provider routing is a useful gap to close, and the test coverage (streaming / non-streaming / embedding, non-OpenRouter passthrough, config defaults, bootstrap forwarding) is solid. The Cloud test-engine rerun also passed (100/100) on dev-v2.0.22.
Before we can merge, a few items need attention. Marking as changes_requested to track them.
Blocking — merge conflict
GitHub currently reports this PR as CONFLICTING / DIRTY against main. The head last merged main on 2026-07-04; main has since advanced to 13fbd437 (2026-07-09). The earlier bot note referenced dev-v2.0.22, but that branch no longer exists, so a fresh rebase onto the current main is needed.
Could you rebase feat/openrouter-routing onto main and force-push? If you'd prefer a maintainer to take it over, just say so and we'll open a replacement PR (we've done this for a couple of other stale external PRs).
Code — three improvements
1. DRY: the routing helper is duplicated 4×
applyOpenRouterProviderRouting is implemented nearly verbatim in:
apps/memos-local-plugin/core/llm/providers/openai.tsapps/memos-local-plugin/core/embedding/providers/openai.tsapps/memos-local-openclaw/src/shared/llm-call.tsapps/memos-local-openclaw/src/ingest/providers/openai.ts(here it's still inlined inbuildRequestBody, not even extracted)
Please factor it into a single shared helper per app (the two apps/ don't share code, so one helper under each app's shared layer is fine) and have all call sites use it. The inlined version in ingest/providers/openai.ts should call the helper for consistency.
2. OpenRouter detection is brittle
endpoint.includes("openrouter.ai") is case-sensitive and substring-based:
OpenRouter.AI/OPENROUTER.aiwon't match.- A self-hosted proxy whose URL happens to contain
openrouter.aias a substring (path / query / hostname fragment) would be misclassified. - Conversely, users fronting OpenRouter via their own domain (reverse proxy / CNAME) get no routing even though the upstream is OpenRouter.
Suggest normalizing with new URL(endpoint).hostname lowercased and comparing against a known host list, and/or adding an explicit openrouter: true opt-in flag on the LLM / embedding config so routing isn't inferred from the URL at all.
3. reasoning is forwarded unconditionally
In core/llm/providers/openai.ts:
if (config.reasoning) body.reasoning = config.reasoning;This sends reasoning to every endpoint, not just OpenRouter. Non-OpenRouter providers may reject the unknown field or silently ignore it. Either gate it behind the OpenRouter check, or document explicitly that it's a passthrough intended for OpenAI-compatible reasoning models and accept that behavior.
Deadline
Could you push an update by 2026-07-23? If there's no activity by then, to keep main moving we'll close this PR and open a maintainer-side replacement with the above items addressed (credit will be preserved in the replacement PR description). Happy to help with the rebase or the refactor if useful — just ping here.
40af2fd to
2c8178d
Compare
🤖 Open Code ReviewTarget: PR #1958 🔍 OpenCodeReview found 13 issue(s) in this PR. 1.
|
|
Rebased onto current main and force-pushed 2c8178d. Consolidated OpenRouter routing into one helper per app, replaced all four call sites, added robust hostname detection plus explicit openRouter proxy opt-in, and gated/normalized reasoning fields. Also fixed dedicated L3 config propagation. Validation: 66 focused plugin tests, 3 OpenClaw call tests, plugin type-check, and make format. |
✅ Automated Test Results: PASSEDAll tests passed (134/134 executed). memos_local_openclaw/unit: 33/33, memos_local_plugin/unit: 101/101. Duration: 17s [advisory, non-gating] AI-generated tests on branch test/auto-gen-650698fd619b8f1a-20260717122622: 110/112 passed, 2 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
|
Thanks for the update — the earlier concerns around OpenRouter routing detection, reasoning passthrough for non-OpenRouter requests, and helper reuse appear to be addressed well, and the test coverage looks solid. Before merging, could we please clarify two small edge cases?
Also, when convenient, could you please confirm that the latest branch now merges cleanly with current |
|
@whipser030 Addressed in aa75c36:
Added focused regressions for both paths. The branch contains current main and GitHub reports the PR mergeable. |
✅ Automated Test Results: PASSEDAll tests passed (137/137 executed). memos_local_openclaw/unit: 33/33, memos_local_plugin/unit: 104/104. Duration: 17s [advisory, non-gating] AI-generated tests on branch test/auto-gen-b5be9075636df774-20260717203542: 126/126 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Problem
When using OpenRouter as the LLM provider, operators have no way to:
provider.ignore)provider.order)reasoning)All three are standard OpenRouter-specific request fields that MemOS currently passes through to the HTTP body without any config surface.
Changes
Config schema — adds
providerIgnore,providerOrder, andreasoningtoLlmSchemaandSkillEvolverSchema:Provider layer —
openai.tsdetects OpenRouter endpoints and injects theproviderrouting object;reasoningis forwarded unconditionally when set. Non-OpenRouter endpoints are unaffected.Embedding —
providerIgnore/providerOrderadded to the embedding config schema;openai.tsembedding provider applies the same routing logic.Pipeline —
bootstrapMemoryCoreforwards the new fields tocreateLlmClientfor theskillEvolverslot.Tests
providers.test.ts— reasoning forwarding, routing injection for non/streaming calls, non-OpenRouter passthroughbootstrap-llm-config.test.ts— verifies routing fields propagate to dedicated LLM clientsload.test.ts— defaults to empty arrays, accepts explicit values