Repository navigation
feat(providers): add Ollama & LM Studio local providers and explicit model attribution - #204
Conversation
There was a problem hiding this comment.
Thanks for this. Native Ollama / LM Studio support with honest ollama: / lmstudio: labels is something we want. This review was cross-checked by a multi-model panel (GPT, Gemini, Grok, DeepSeek, Kimi) over 6 rounds; every item below was verified against the diff. Requesting changes before merge.
Naming rule the items below assume: the Gemini built-in keeps the name gemini; every model record is named <provider>:<alias> (the unique config id), never by model slug.
Blocking
B13 (security): OPENROUTER_API_KEY is sent to non-OpenRouter endpoints, on two paths.
- (a) The standalone bridge
server/openrouter/index.jsresolves any record from the sharedmodelslist, which now includes ollama / lmstudio / google records, and always sendsprocess.env[or.apiKeyEnv]todelegate.apiBase || or.apiBase. A local record with its ownapiBasereceives the OpenRouter key; one withoutapiBasesends a local slug to OpenRouter. - (b) Unified server: on master
pinAliasdid not forward a per-recordapiBase. This PR forwards it, andopenai-compatible.jsfalls back to the providerapiKeyEnv, so any record (includingprovider: "openrouter") with a customapiBaseand noapiKeyEnvnow receivesOPENROUTER_API_KEY. - Please require an explicit credential choice for any non-default endpoint, and keep non-openrouter records out of the standalone bridge.
B14: Same root cause. openrouter-list and the openrouter tool list and serve local and google records as OpenRouter delegates.
B15: isModelEnabled in validateConfig checks providersRaw[m.provider]. For google records that is providers.google, which does not exist, so providers.gemini.enabled: false does not suppress them.
B16: formatDelegateName uses the model slug, not the alias. Two records with the same slug (for example two Ollama endpoints) get the same provider name, so the dedup cache, circuit-breaker streaks and arbiter exclusion treat them as one.
B1: startStdio names the Gemini built-in google:${geminiCfg.model}. config.default.json ships model: "auto-gemini-3", so every existing install's gemini voice is renamed to google:auto-gemini-3. Lookups still work through the new alias, but opinion source, debug-log / analyze rows, session history and the command docs all change, and history splits across the upgrade. Keep the built-in name gemini and report the model in the model field.
B2: resolveModels dropped the if (!isObject(m)) guard. models: { x: null } now throws a TypeError at load instead of landing in invalidModels. Please restore it and add a test.
B3: antigravity.js now honors req.model, so google records pass arbitrary ids to agy. agy rejects non-shipped pins with model-not-allowed (see CLAUDE.md design decision #9). Either drop google records or show with a dispatch test that per-call ids work.
B4: The object form of resolveArbiter sends every record through pinAlias(orProvider, ...), so a google arbiter is dispatched to OpenRouter.
B5: Local records ride the openrouter transport object. It is always constructed, so dispatch still works with OpenRouter disabled, but invalidModels is reported only when OpenRouter is enabled, so a bad local record is dropped without a word. Please report invalid local records regardless of OpenRouter state. A dedicated local transport would also close B13(b).
B6: On master, ask-one only dispatches from the selectForAskAll set. The new matchModel fallback makes askAll: false, over-cap and expert-ineligible records callable, while the error text still says "not in the active panel". If this is intended, document it and honor the enabled flags; otherwise remove the fallback.
B7: Local health() always returns { ok: true }, and resolveArbiter now auto-prefers ollama: / lmstudio:, so a stopped local daemon can be picked as the arbiter. Please add a reachability probe that fails closed, or leave local names out of the auto-arbiter preference.
B8: In core/analyze.js, leverFor labels ollama: / lmstudio: rows as kind openrouter with the slug as alias, so analyze can recommend nonexistent models.<slug>.* settings. configLevers also does byAlias.set(m.model), which lets a slug overwrite another record's alias. Remove the slug keys, resolve rows by alias, and add a regression test (fixing B16 alone does not fix this).
B9: providers.ollama.timeout / providers.lmstudio.timeout are documented and resolved but never applied: pinAlias reads only delegate.timeout, so local calls inherit the OpenRouter provider ceiling. Wire it through or remove it from the schema and docs.
B12 (tests): The new tests cover validation and naming only. Please add tests for:
- no
OPENROUTER_API_KEYreaching a custom or localapiBase, on both paths - local
apiBaserouting, and a keyless call sending noAuthorizationheader (callOpenRouteralready omits it for an empty key, so this is a regression guard) - an invalid local record with OpenRouter disabled (B5)
- B15, B16 (duplicate slugs), B1 naming, B2 null guard, B4 / B6 routing, B8, B9
Follow-ups (not blocking this PR)
- Temporal grounding: merged in #205 (now on
master), please drop it from this PR and rebase. We agree with the problem (delegates rejecting newer model or tool names as "hallucinated") and fixed it differently. Grok and OpenRouter run with no tools, so adate -uplus live-search rule cannot reach them. #205 instead stamps today's UTC date and a short "unrecognized means[unverified], never non-existent" rule into every delegate prompt in code. Looking up the facts stays with the host (a "Time-sensitive questions" note inAGENTS.md). Please removeplugins/deliberation/skills/temporal-grounding/, the "Mandatory Temporal Grounding" blocks inAGENTS.md/POWER.md/plugins/deliberation/rules/AGENTS.md, and the AWS / Terraform tool names. - Split the PR. The new
ask-all/consensushost skills are not assessed in this review. Please open them as a separate PR. - Drop the
llmstudioalias. It is new in this PR, so nothing depends on it yet. config/config.default.jsonneeds entries for the new provider keys (the repo's doc-upkeep rule), even though the schema andpinAliasalready carry runtime defaults.- Hardening: MCP tool arguments never reach
req.apiBase/req.apiKeyEnvtoday. Prefer resolving them from config inside the provider over wideningDelegationRequest.
A narrower PR that adds a dedicated local provider keyed by record alias would very likely be accepted.
…gate prompt (#205) * feat: stamp the current UTC date and a no-denial rule into every delegate prompt Delegates trained months ago call newer model or tool names hallucinated and file false critical issues. core/grounding.js adds today's UTC date plus a one-sentence rule (unrecognized means [unverified], never non-existent) in all four prompt builders, so every path gets it, standalone bridges included. Looking facts up stays with the host: AGENTS.md and the ask-all/consensus commands tell it to verify and inline them with an as-of date and source. Replaces the temporal-grounding skill proposed in #204. * fix: say "tools you actually have" so the grounding note does not contradict tool-less delegates * docs: add the time-sensitive facts step to every /ask-* command and the delegation template
223b09b to
c8bd45b
Compare
|
Thanks for the thorough review and clear architectural feedback, @antonbabenko! I have addressed all the blocking points (B1–B16) and follow-up items, rebased onto Resolution Summary
|
|
As requested in the follow-up review, the native host skills have been opened as a separate PR: #211 |
antonbabenko
left a comment
There was a problem hiding this comment.
Thanks for the rebase and for working through B1-B16. The temporal grounding is gone, the skills moved out, llmstudio is dropped, and the default config has the new provider blocks. On the PR branch, npm test passes 894/894, and tsc and sync-hosts --check are clean.
We ran the branch through a multi-round cross-model review (GPT, Gemini, Grok, and two OpenRouter models). Every reviewer agreed on the blockers below. Most of them are about which endpoint an API key is sent to.
"Provider key" below means process.env[or.apiKeyEnv] (OPENROUTER_API_KEY by default).
Blockers
1. Per-record apiBase / apiKeyEnv are only half wired.
The schema now advertises models.<id>.apiBase and models.<id>.apiKeyEnv ("for this record only"). The unified server's pinAlias (core/registry.js) forwards neither of them. It does forward delegate.apiKey, but resolveModels never sets that field, so that path is dead. The PR also deletes the NOTE that said per-model apiBase only applies to standalone /ask-openrouter.
Smallest fix: keep both record fields standalone-bridge-only.
- Say so in the schema descriptions and the docs.
- Have the unified server emit a config warning at load for any record that sets them.
- Remove the dead
apiKeyforwarding and restore the NOTE. - Validate
models.<id>.apiKeyEnvas a non-empty string inresolveModels. A bad value becomes aninvalidModelsentry.
Wiring these fields into the unified server can come in a later PR.
2. The provider lookup must fail closed.
pinDelegates and resolveArbiter use byName.get(prov) || byName.get("openrouter"). If a provider is ever not registered, its model slug and prompt go to OpenRouter with the provider key. Validation prevents this today, but the fallback is the wrong default.
- Never dispatch through another provider's transport.
- If a delegate's provider is not registered, do not dispatch it; report it in
unavailable. - If an explicit arbiter
{ model }has an unregistered provider, do not dispatch it. Warn and keep master's auto selection from the registered pool. Return an error without dispatch only when that pool is empty.
3. Standalone bridge key rule: good direction, needs to be complete and documented.
The bridge only serves provider: "openrouter" records, which the PR already enforces with model-not-allowed. Keep that. Evaluate the key in this order:
apiKeyEnvis set on the record: use that env var. If the var is unset or empty, send no key. Never fall through to the provider key.- The record has no
apiBase, or itsapiBaseequalsor.apiBaseafter stripping trailing slashes: use the provider key. - Otherwise, send no key.
This changes behavior for existing users: a record with its own apiBase used to get OPENROUTER_API_KEY. Please add a migration note to SETUP.md and TECHNICAL.md. Authenticated proxies must set models.<id>.apiKeyEnv; keyless proxies leave it unset.
4. The standalone reply path sends proxy sessions to OpenRouter.
openrouter-reply rebuilds the delegate as { model: priorSession.model }. A reply to a proxy session therefore goes to or.apiBase with the provider key. Fix:
- When a session starts, store the alias, the effective
apiBase, and the NAME of the env var chosen by rule 3. Never store the secret. - Replies reuse that stored copy and read the env value at call time. Editing the config mid-session then does not change where replies go.
- A reply with no alias/model, or with the session's own alias/model, keeps the session.
- A reply that names a different alias (compared by alias, not model slug) or a different raw model is resolved again from the config. Raw models use
or.apiBaseand the provider key.
5. Tests for these paths.
The 10 new tests do not reach the defects above. Please add tests for:
- An unknown-provider delegate is not dispatched and appears in
unavailable. - An unknown-provider explicit arbiter is not dispatched, warns, auto-selects from the pool, and errors when the pool is empty.
- The unified-server warning for ignored record fields, and
apiKeyEnvvalidation. - Rules 1-3, including an unset named var, a trailing slash, and a non-default
or.apiKeyEnv. - A reply on a session that uses the provider key sends that key to
or.apiBase. - A reply on a proxy session reaches the proxy with its own key or no key, never the provider key.
- A reply after a config edit keeps the stored values.
- A reply naming a different alias or raw model is resolved again.
- OpenRouter delegates are still named
openrouter:<alias>.
Small, same PR
- Drop the
"NONE"sentinel from theopenai-compatible.jshealth check and from TECHNICAL.md. An empty or unsetapiKeyEnvfield already means keyless. config.schema.json:ollamaProvider.apiKeyEnvdefaults to"OLLAMA_API_KEY", but the code default is keyless. Make it keyless, likelmstudioProvider.- One doc line:
providers.ollama/providers.lmstudioapiBaseis read at server start, so changing it needs a restart.providers.openrouterworks the same way today.
Follow-ups (not blocking, we will track them)
- Hot reload of
apiBasefor all HTTP providers. - A cheap daemon check for local providers (e.g.
GET /v1/models) so a stopped Ollama or LM Studio shows up as unavailable. Today the retry and the circuit breaker limit the cost. core/analyze.js: derive theollama:/lmstudio:prefixes from one list.- Optional per-record
apiBase/apiKeyEnvin the unified server.
Related PRs
…model attribution - Add first-class Ollama and LM Studio local providers backed by OpenAI-compatible HTTP bridge - Support explicit model naming/attribution across fan-out, arbitration, analysis, and dashboard - Fail-closed provider dispatch: reject cross-transport fallback, mark unregistered delegates unavailable - Safe auth resolution on standalone OpenRouter bridge with keyless defaults and session isolation - Validate models.<id>.apiKeyEnv as non-empty string and warn on standalone-only keys in unified server - Add comprehensive regression tests in test/local-providers.test.js covering all review concerns - Update schemas, documentation (SETUP.md, TECHNICAL.md, README.md, tool-guide.md), and host skills
c8bd45b to
a37b4f0
Compare
|
Thanks for the thorough review, @antonbabenko! All feedback items have been addressed and rebased cleanly on top of master ( 1. Per-record
|
…st to openrouter - core/settings.js resolves any <provider>:<alias> name, so ollama/lmstudio delegates keep models.<id>.timeout and providers.<p>.timeout at dispatch instead of the 180s default, and report their model in provenance - standalone openrouter-list returns only openrouter records; the openrouter tool rejects local aliases - drop duplicate unified-server warning loops (validateConfig already emits it) - revert the unrelated codex configKey in analyze high-error recommendations - TECHNICAL.md: list scope, invalidModels with a disabled provider, provider rule - tests: LP-R9 (timeout through the settings overlay), LP-R10 (list scope)
|
Thanks @ivankorn, this round addressed every item from the last review. I checked each one against the code: per-record I pushed two small commits on top so this can merge:
|
|
This PR is included in v3.31.0 🎉 Claude Code If the update still shows the old version, the marketplace re-pin is a few minutes behind; run it again shortly. Codex, Kiro, Cursor, OpenCode plugins: update deliberation from your host's plugin manager. MCP server (any host): |
Summary
This PR adds first-class support for local inference runtimes (Ollama and LM Studio) alongside OpenRouter and built-in providers, and introduces explicit, collision-free delegate attribution (
<provider>:<alias>).Following reviewer feedback on PR #204, this PR has been rebased onto
master(v3.30.0), temporal grounding has been dropped (addressed in #205), host skills have been split to a separate PR, and the blocking items from both review rounds (B1-B16, then section 4 below) have been resolved.Key Changes
1. Dedicated Local Transports & Model Record Providers
ollama,lmstudio):modelsrecords accept"provider": "openrouter" | "ollama" | "lmstudio".ollama(http://localhost:11434/v1) andlmstudio(http://localhost:1234/v1)OpenAICompatibleProviderinstances inserver/mcp/index.js.ollamaandlmstudioblocks with default endpoints toconfig/config.default.jsonand updatedconfig/config.schema.json.llmstudioalias in favor of canonicallmstudio.apiKeyEnv: ""), sending noAuthorizationheader and ensuringOPENROUTER_API_KEYis never leaked to local or custom endpoints.provider === "openrouter"only and rejects non-OpenRouter models withmodel-not-allowed.apiBaseendpoints on OpenRouter delegates do not inheritOPENROUTER_API_KEYunless explicitly configured withapiKeyEnv.models.<alias>.modelaccept colons, dots, and slashes (e.g.nemotron-3-ultra:cloud,llama3.3:70b,glm-5.3:cloud), while aliases stay restricted to^[a-z0-9-]+$.2. Collision-Free Delegate Attribution (B1, B16)
<provider>:<alias>Attribution:core/registry.js,formatDelegateName(m)formats configured delegates as<provider>:<alias>(e.g.,ollama:nemotron-local,lmstudio:deepseek-local,openrouter:claude-arb).geminiinstartStdio,panel,ask-all, etc., reporting the active model in themodelresult field.googleProvider frommodels(B3, B4, B15):providers.gemini.model, eliminating the risk of arbitrary model IDs being sent toagy, misrouting arbiters to OpenRouter, or bypassingproviders.gemini.enabled: false.3. Reviewer Feedback & Hardening (B2, B5, B6, B7, B8, B9)
resolveModels(B2): Restoredif (!isObject(m))null guard so entries likemodels: { x: null }land cleanly ininvalidModelsinstead of throwing.invalidModelsinvalidateConfigregardless of OpenRouter enabled state so malformed local records are always reported.matchModelfallback inask-oneto strictly dispatch from active panel members (selectForAskAll).openrouter:*so stopped local daemons are not auto-selected as arbiters. Explicit{ model: "<id>" }arbiters dispatch cleanly to their configured provider.core/analyze.jskeysconfigLeversstrictly by alias, identifies local models askind: "local", and suggestsmodels.<alias>.askAllwithout skewing OpenRouter outlier counts or generating invalid OpenRouter compare links.pinAliasfalls back toproviders.<prov>.timeoutwhendelegate.timeoutis unset.DelegationRequestwith transport details; endpoints and auth are encapsulated in provider instances.4. Standalone Bridge Auth and Sessions (review round 2)
models.<id>.apiBase/apiKeyEnvare honored only by the standalone OpenRouter bridge. The unified server ignores them and emits a warning;apiKeyEnvmust be a non-empty string.unavailable[]and never falls back to OpenRouter. An explicit arbiter on an unregistered provider warns and falls back to auto-selection.resolveEffectiveAuth): recordapiKeyEnvset -> that env var, or keyless if empty, never the provider key; no recordapiBase, or the same base as the provider after trailing slashes are stripped -> provider key; any other base -> keyless. SETUP.md and TECHNICAL.md carry the migration note.openrouter-replyreuses the session's stored endpoint and key env-var name (never the secret), so config edits do not change a running thread; a different alias or model re-resolves from config.Configuration Example
{ "version": 1, "providers": { "gemini": { "enabled": true, "model": "gemini-3.8-flash-high" }, "ollama": { "enabled": true, "apiBase": "http://localhost:11434/v1" }, "lmstudio": { "enabled": true, "apiBase": "http://localhost:1234/v1" }, "openrouter": { "enabled": true, "apiKeyEnv": "OPENROUTER_API_KEY" } }, "models": { "nemotron-local": { "provider": "ollama", "model": "nemotron-3-ultra:cloud", "askAll": true, "consensus": true }, "deepseek-local": { "provider": "lmstudio", "model": "deepseek-r1-distill-qwen-14b", "askAll": true, "consensus": false }, "claude-arb": { "provider": "openrouter", "model": "anthropic/claude-3.7-sonnet", "askAll": true, "consensus": true } } }Dispatched delegates resolve explicitly as:
gemini(runtime model reported inmodel)ollama:nemotron-locallmstudio:deepseek-localopenrouter:claude-arbVerification & Tests
test/local-providers.test.js: 17 targeted regression tests, including:LP1:validateConfigacceptsollamaandlmstudioproviders inmodels.LP-B2:models: { x: null }lands ininvalidModelswithout throwing.LP-B5: Invalid local record reported ininvalidModelswhen OpenRouter is disabled.LP-B1-B16: Gemini keeps namegemini, andformatDelegateNameuses<provider>:<alias>.LP-B13a-B14: Standalone OpenRouter bridge rejects non-OpenRouter models and guardsapiKey.LP-B13b: Keyless local endpoints send noAuthorizationheader and do not leakOPENROUTER_API_KEY.LP-B4-B6:pinAliaspins to target provider andask-onedispatches from active panel.LP-B8:configLeversmaps by alias only (not slug), andleverForidentifies local models.LP-B9:providers.ollama.timeoutapplies whendelegate.timeoutis unset.LP3:validateConfigaccepts colons, dots, and slashes in model slugs.LP-R1-LP-R8: fail-closed provider lookup, arbiter fallback, unified-server warning, standalone auth rules 1-3, reply session reuse.LP-R9: local delegate timeout and model survive the dispatch settings overlay.LP-R10: standaloneopenrouter-listlists onlyopenrouterrecords.npm run check(typecheck + every suite, 1207 tests),npm run sync:checkandnpm run test:contracts(19) pass on the head commit.Maintainer follow-up (4bed433, b587c51)
Fixes found in the final review, pushed to this branch:
core/settings.jsresolves any<provider>:<alias>delegate name. Before, onlyopenrouter:names resolved, soollama:/lmstudio:delegates were dispatched with the 180s default timeout regardless ofmodels.<id>.timeoutorproviders.<p>.timeout, and reported an empty model in provenance (LP-R9).openrouter-listlists onlyopenrouterrecords; theopenroutertool cannot call local aliases (LP-R10).server/mcp/index.js(validateConfigalready emits the warning).core/analyze.jschange that pointed codex high-error rows atproviders.codex.reasoningEffort.invalidModelswith a disabled provider, accepted provider values.config.schema.json: threat model for per-recordapiBase/apiKeyEnv.Cross-model review (
/consensus, code-reviewer, converged in 2 rounds; Gemini, Grok and an OpenRouter delegate approved, Codex timed out). Accepted: schema threat model (fixed in b587c51). Dismissed, with reasons:<provider>:<alias>split ambiguous for aliases with:- record ids must match^[a-z0-9-]+$, so the first-colon split is unambiguous.resolveArbiter({ model })could pin a disabled record - it searches the enabled-filtered model list and checksproviderEnabled.sync:checkpasses.enabled: falseunclear - a missing block counts as enabled, same as other providers.{ model }key unclear - unchanged existing contract: it names the models record id.apiBaseundocumented - documented in TECHNICAL.md and SETUP.md.