feat: route memory LLM completions through the host model runtime (opt-in transport) - #947
Conversation
571abcd to
d5dca72
Compare
08b9083 to
a31460f
Compare
…m.transport knob, credential hygiene, 30c-composed llm-client)
f0f9b28 to
90fdf39
Compare
|
Recomposed against current master (post-#988) as a single commit. The llm.transport schema entry now sits under the llm config schema (it was misplaced in an earlier composition), and the transport-aware thinkLevel description rides along. Full suite green locally. Ready for review. |
rwmjhb
left a comment
There was a problem hiding this comment.
The host-runtime direction is valuable, and the new client-level tests plus current CI are green. On head 90fdf39, however, three production composition/security blockers remain:
-
Admission strips the catalog provider prefix.
resolveAdmissionModel()normalizes the selected model, andadmissionClientFor()normalizes it again before constructing the client. Withopenrouter/anthropic/claude-opus-4-8, extraction keeps the full host catalog reference while admission receivesanthropic/claude-opus-4-8. That can bypass the selected catalog provider or fail its allowlist. Please make model normalization transport-aware and add anindex.tscomposition regression; the current admission test constructs the host client directly and misses this path. -
Every host request is an unauthorized model override under the default host policy.
createHostClient()always sendsmodel: config.model, and the plugin always populates that field. OpenClaw's runtime checks any supplied model against plugin model-override authority and rejects it unlessallowModelOverride/the allowed-model policy permits it. Please omit the model when the operator did not explicitly select one, and document/preflight the policy for explicit catalog models with a policy-enforcing integration test. -
Fallback can send an unrelated credential to OpenRouter. When the host surface is unavailable, an absent
llm.baseURLis replaced withhttps://openrouter.ai/api/v1, then combined with any explicitllm.apiKey. An existing OpenAI-compatible provider key can therefore cross into an unrelated endpoint. Please fail closed unless the fallback endpoint is explicit/provider-bound; do not infer a third-party destination for an arbitrary credential.
Also, the advertised OAuth fallback currently checks for apiKey before branching on auth === "oauth", so host-unavailable OAuth configurations cannot reach createOauthClient(). Please cover that while revising fallback behavior.
The full local suite and GitHub checks pass, but their permissive mocks do not exercise these production composition and host-policy cases.
…il-closed fallback Review round: host transport keeps full catalog model references end to end (admission resolution + index wiring, composition-regression tested); host requests omit the model unless operator-configured; the host->direct fallback refuses to infer a third-party endpoint for llm.apiKey and reaches the OAuth client without one.
|
All four findings addressed in bb7f663, thanks for the sharp round:
Full local suite green including the new coverage. |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head bb7f663. All four blockers from the previous review are resolved:
- admission/extraction preserve full host catalog references through the real
index.tscomposition path; - unconfigured models are omitted so default host policy does not see an override;
- host-to-direct fallback now fails closed without an explicit
llm.baseURL, so arbitrary credentials are not sent to an inferred third-party endpoint; - OAuth fallback branches before the API-key requirement and is reachable without an API key.
The focused host/admission/fallback suites pass 37/37, the full suite and required GitHub checks are green, npm run build succeeds, and the generated tree remains clean. Approved.
Non-blocking follow-up: the host timeout currently stops waiting but does not pass an AbortSignal, so the underlying completion may continue consuming quota after timeout.
Summary
Adds a host-managed completion transport alongside the existing direct OpenAI-compatible client, so the plugin's five completion lanes (extraction, admission-utility, dedup, merge, memory-upgrader) can route through OpenClaw's host-managed runtime LLM catalog instead of always talking to
llm.baseURLdirectly. This is the structural fix requested in issue #901: today the plugin always uses the direct client, which means no provider routing, no app attribution, and ad hoc model-string handling.What changed
Transport.
llm.transport:"direct" | "host", default"direct"(zero behavior change for existing installs), new knob inopenclaw.plugin.json's config schema.createHostClient()insrc/llm-client.tssits alongside the existingcreateApiKeyClient()/createOauthClient(), all three implementing the sameLlmClientinterface.createLlmClient()picks"host"only when aruntimeLlmCompletefunction is supplied (feature-detected viaapi.runtime?.llm?.complete); otherwise it falls back to the direct/oauth transport with one deduped warning per process, not one per client constructed, so a misconfigured or older host never crashes the lane and never spams the log. Model strings pass to the host transport unmodified; the direct client keeps its ownopenrouter/prefix-strip normalization, now transport-aware so it never mangles a core-style catalog reference the host transport needs intact.runtime.llm.completehas noAbortSignalparameter, sotimeoutMsis enforced with an application-level race instead of true cancellation, and it has no custom-header support, so the existingx-memory-call-labelbecomes amemory-lancedb-pro:<label>purposestring for equivalent call-site attribution. The host client reuses the same JSON-extraction-and-repair logic as the direct client, so malformed or fenced responses behave identically across transports.Credential hygiene on fallback. When
transport: "host"is configured but the runtime surface turns out to be unavailable, the fallback to the direct client no longer inheritsembedding.apiKey/embedding.baseURLas a silent default. On a split-provider setup (embedding via one provider, the LLM lane via host-managed routing) this previously meant the fallback client silently talked to the embedding provider's endpoint with the embedding key instead of failing loudly. It now throws a clear error naming the missingllm.apiKey, caught by the existing surrounding try/catch so it degrades to regex extraction with a warning rather than crashing, and defaultsbaseURLto OpenRouter's API whenllm.apiKeyis set butllm.baseURLisn't. The fallback also normalizes a catalog-style model string to the bare provider/model form the direct client needs; an explicitly configured direct transport keeps sending whatever model string it was given, unchanged.Reasoning effort. Both the host and direct transports now send an explicit reasoning effort on requests: the host transport as a
reasoningparameter, the direct transport asreasoning: {effort: ...}. Renamed fromllm.reasoningEfforttollm.thinkLevelfor consistency withmemoryReflection.thinkLevel;llm.reasoningEffortkeeps working as a deprecated alias so existing configs don't break.resolveThinkLevel()centralizes the precedence:thinkLevelwins when both are configured (one deduped warning per process),reasoningEffort-only usage keeps working with its own deduped deprecation warning, and blank/whitespace-only values are treated as unset for both keys.No schema defaults on the effort keys. Neither
llm.thinkLevelnor the deprecatedllm.reasoningEffortdeclares a JSON-schemadefaultanymore. A live check found a config that set onlyllm.reasoningEffort: "low"on disk, but a schema default of"medium"on both keys got materialized into the config object upstream of this plugin on at least one host config-loading path, and once a default is materialized that way it is indistinguishable from a genuine user value by the time it reachesresolveThinkLevel, silently overriding the operator's explicit setting. The code-level"medium"default for the host transport (applied only when neither key is configured) is untouched and still the correct place for that default to live.Composition with the rest of this wave
Built on plain
masterbefore several sibling branches merged, which needed reconciliation once everything came together in the deploy assembly:fix/prompt-architecture's{system, user}prompt split and three-argumentcompleteJson()(the host transport was written with that signature from the start);feat/admission-batch-utility's batch scoring call, which still concatenated system and user into one string for the old two-argumentcompleteJson();feat/admission-lane-model-affinity's model resolver, which needed a transport-aware skip of the direct-only prefix normalization; and a grounding-v3 interaction where the constructed-grounding short-circuit needed to run before utility scoring on both the single-candidate and batch paths to keep its zero-call guarantee.A related, pre-existing auto-capture watermark defect (a valid-empty extraction skips the watermark reset a successful extraction applies) was found via the same live probe that surfaced these issues, but is being fixed on its own branch, #948, since it doesn't touch the transport work.
Tests
Red-first throughout.
test/llm-host-transport.test.mjs: transport selection, mockedruntimeLlmCompletecapturing model and messages, fallback-to-direct with a deduped warning, malformed/throwing host calls, timeout bounding, catalog-model normalization on fallback, and reasoning-effort forwarding (default, explicit override, blank-value fallback).test/admission-control-host-transport.test.mjsandtest/batch-admission-host-transport.test.mjs:AdmissionControllerdegrades identically over both transports, including in batch mode.test/admission-model-resolution.test.mjs: transport-awareopenrouter/prefix handling.test/llm-thinklevel-alias.test.mjs: alias precedence and both deduped warnings.test/llm-transport-credential-hygiene.test.mjs: the fallback client never inherits embedding credentials on a split-provider config.test/plugin-manifest-regression.mjs: neitherllm.thinkLevelnorllm.reasoningEffortdeclares a schema default. Existingllm-api-key-client/llm-oauth-clientsuites pass unchanged, confirming the direct path is unmodified by this PR.Notes
authProfileId,agentId, andreasoningfrom the reference runtime-LLM contract aren't all wired through yet; there's no existing config concept in this plugin for auth-profile scoping today, and forwardingreasoningfrom the plugin is necessary but not sufficient until the host's own runtime-LLM layer also reads and forwards it, since at least one installed core version drops the field before any reasoning-effort default would apply. Can be extended in a follow-up once a host deployment needs it.Update: remove the llm.reasoningEffort alias entirely (2026-07-17)
llm.reasoningEffortnever shipped upstream (zero hits on synced master at the time this was checked), so there is no deprecation constituency to preserve for it. Removed it entirely rather than continuing to carry it as a deprecated alias:resolveThinkLevelno longer readsreasoningEffortor emits either deprecation warning; it is now a plain presence check onllm.thinkLevel.LlmClientConfigdrops thereasoningEffortfield.llm.reasoningEffortschema key.llm.thinkLevelis simply the knob's name now.Test changes:
test/llm-thinklevel-alias.test.mjsis renamed totest/llm-thinklevel.test.mjsand stripped down to the single-key behavior (configured / unconfigured / blank).test/llm-host-transport.test.mjs's reasoning-effort tests now configurethinkLevelinstead ofreasoningEffort.test/plugin-manifest-regression.mjs's assertion that the deprecated key must stay in the schema is inverted to assert it is absent.Update (2026-07-18)
Design change after operator review:
llm.reasoningEffortnever shipped in any release, so the deprecation alias is gone entirely. The knob isllm.thinkLevelonly; the schema, client, and tests carry no legacy key. (The schema-default override fix from the previous revision is retained.)