Fix #2308: LLM facade drops opts.op before providers — per-op provider switches (e.g. think - #2309
Open
Memtensor-AI wants to merge 1 commit into
Conversation
`LlmClient.buildCallInput()` dropped `opts.op` when constructing the
`ProviderCallInput` object handed to `provider.complete()` / `stream()`.
Callers set `op` for every logical call site (e.g. `capture.summarize`,
`retrieval.filter`, `skill.evolve`) but only the audit / metrics paths
inside the facade saw the tag — providers never did.
As a result any per-op provider behavior (request-body tweaks, routing
overrides, reasoning kill-switches keyed on
`opts.op === "capture.summarize"`) silently could never fire. The
issue-reported symptom: OpenRouter DeepSeek reasoning models burn full
token budget and double latency on `capture.summarize` because the
recommended `thinking: { type: "disabled" }` switch is unreachable.
Fix:
- extend `ProviderCallInput` with optional `op?: string`
- copy `opts?.op` in `buildCallInput()`
Field stays optional so providers must not assume it is set. No public
LlmClient surface change; consumers keep calling `complete` /
`completeJson` / `stream` unchanged.
Added 4 unit tests in `tests/unit/llm/client.test.ts` under an
"op propagation (issue MemTensor#2308)" describe block covering complete /
completeJson / stream forwarding and the "no op supplied" case.
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2309 ✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
✅ Automated Test Results: PASSEDAll tests passed (29/29 executed). memos_local_plugin/unit: 29/29. Duration: 3s [advisory, non-gating] AI-generated tests on branch test/auto-gen-aca74652f3c40919-20260830053736: 17/17 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Memtensor-AI
added
status:ready
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix #2308: the LLM facade in
apps/memos-local-plugin/core/llm/client.tsno longer dropsopts.opwhen it handsProviderCallInputtoprovider.complete()/provider.stream(). Two-line change: extendProviderCallInputincore/llm/types.tswith an optionalop?: string, and copyopts?.opinsidebuildCallInput().opstays optional so providers must not assume it is set; no publicLlmClientsurface change and no new error codes.This unblocks the OpenRouter/DeepSeek reasoning kill-switch reported in the issue (
opts.op === "capture.summarize"inside a provider's request builder can now actually evaluate true). Landing the per-op switch itself — e.g.body.thinking = { type: "disabled" }inproviders/openai.ts— is a routing-policy decision left as a follow-up so operators can review it independently.Tests: added 4 new unit tests under a new "op propagation (issue #2308)" describe block in
tests/unit/llm/client.test.tscoveringcomplete/completeJson/streamforwarding plus the "no op supplied" case. Verification: all 84 LLM unit tests green (5 files: client / providers / json-mode / prompts / fetcher),tsc -p tsconfig.json --noEmitclean. The 46 pre-existing failing test files in the broader unit suite (memory/l3/subscriber etc.) were verified unchanged against the base branch viagit stashand are unrelated to this fix.Confidence: 0.85 — root cause and fix were both explicit in the issue with local verification by the reporter.
Related Issue (Required): Fixes #2308
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist