From d5b3beac98e6b4a1c343777be7e22f82d231604c Mon Sep 17 00:00:00 2001 From: MemOS AutoDev Date: Tue, 15 Sep 2026 05:03:11 +0800 Subject: [PATCH] fix(pipeline): wire attachSkillSubscriber with bgReflectLlm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attachSkillSubscriber` in core/pipeline/deps.ts was wired with `llm: bgLlm` (the main model), never `bgReflectLlm` (the dedicated client built from the `skillEvolver.*` config block). As a result: - Operators configuring a distinct `skillEvolver.provider` / `skillEvolver.model` were silently ignored — skill crystallization / evolution ran on the main model regardless. - `overview.skillEvolver.lastOkAt` was pinned to `null` because the dedicated client was never invoked, making it useless as a liveness signal. Change: `llm: bgReflectLlm ?? bgLlm`. Fallback preserves current behaviour for installs without a distinct `skillEvolver` block. Guarded by a new dedicated regression test (`tests/unit/pipeline/skill-reflect-llm-wiring.test.ts`, 2 cases) mirroring the pattern of `capture-reflect-llm-wiring.test.ts` (the sibling fix #2148, opposite-direction wiring bug in captureRunner). Fixes #2362 --- apps/memos-local-plugin/core/pipeline/deps.ts | 7 +- .../pipeline/skill-reflect-llm-wiring.test.ts | 198 ++++++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 apps/memos-local-plugin/tests/unit/pipeline/skill-reflect-llm-wiring.test.ts diff --git a/apps/memos-local-plugin/core/pipeline/deps.ts b/apps/memos-local-plugin/core/pipeline/deps.ts index 79714b35e..7fe2fd33f 100644 --- a/apps/memos-local-plugin/core/pipeline/deps.ts +++ b/apps/memos-local-plugin/core/pipeline/deps.ts @@ -336,7 +336,12 @@ export function buildPipelineSubscribers( const skillHandle = attachSkillSubscriber({ repos: deps.repos, embedder: bgEmbedder, - llm: bgLlm, + // Skill crystallization / evolution runs on the dedicated + // `skillEvolver.*` client when configured, falling back to the main + // llm when no distinct config is supplied. Without this, operators + // configuring a separate skillEvolver model were silently ignored and + // `skillEvolver.lastOkAt` never advanced. See issue #2362. + llm: bgReflectLlm ?? bgLlm, bus: buses.skill, l2Bus: buses.l2, rewardBus: buses.reward, diff --git a/apps/memos-local-plugin/tests/unit/pipeline/skill-reflect-llm-wiring.test.ts b/apps/memos-local-plugin/tests/unit/pipeline/skill-reflect-llm-wiring.test.ts new file mode 100644 index 000000000..abb59d13f --- /dev/null +++ b/apps/memos-local-plugin/tests/unit/pipeline/skill-reflect-llm-wiring.test.ts @@ -0,0 +1,198 @@ +/** + * Regression test for issue #2362 — + * `attachSkillSubscriber` must receive the dedicated `bgReflectLlm` + * (built from the `skillEvolver.*` config block), NOT the main `bgLlm`. + * + * Background: operators can configure a distinct `skillEvolver.*` model to + * isolate skill-evolution cost/quality from the main model. In `deps.ts` + * the pipeline builds a dedicated `bgReflectLlm` from `deps.reflectLlm`, + * but historically wired the skill subscriber with `llm: bgLlm` — so the + * skillEvolver client was never actually invoked, `skillEvolver.lastOkAt` + * never advanced, and the operator-configured skillEvolver model was + * silently ignored. + * + * Fix: `attachSkillSubscriber({ llm: bgReflectLlm ?? bgLlm, ... })`. The + * fallback preserves current behavior for installations that don't set a + * distinct `skillEvolver` config (bootstrap aliases `reflectLlm` to null + * in that case). + * + * This test locks the wiring at the pipeline layer: no matter what the + * caller sets on `deps.llm` vs `deps.reflectLlm`, `buildPipelineSubscribers` + * must pass the rate-limited `reflectLlm` client (identified by model name) + * to the skill subscriber when one is provided. + * + * This is the mirror of issue #2148 (captureRunner mis-wiring) — same class + * of bug, opposite direction. See `capture-reflect-llm-wiring.test.ts`. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { + LlmCallOptions, + LlmClient, + LlmClientStats, + LlmMessage, + LlmProviderName, +} from "../../../core/llm/types.js"; + +const skillSubscriberCalls: Array<{ + llm: LlmClient | null; +}> = []; + +vi.mock("../../../core/skill/index.js", async () => { + const actual = await vi.importActual< + typeof import("../../../core/skill/index.js") + >("../../../core/skill/index.js"); + return { + ...actual, + attachSkillSubscriber: (deps: { + llm: LlmClient | null; + [k: string]: unknown; + }) => { + skillSubscriberCalls.push({ llm: deps.llm }); + return actual.attachSkillSubscriber( + deps as Parameters[0], + ); + }, + }; +}); + +import { + buildPipelineBuses, + buildPipelineSession, + buildPipelineSubscribers, + extractAlgorithmConfig, + type PipelineDeps, +} from "../../../core/pipeline/index.js"; +import { DEFAULT_CONFIG } from "../../../core/config/defaults.js"; +import { resolveHome } from "../../../core/config/paths.js"; +import { rootLogger } from "../../../core/logger/index.js"; +import { makeTmpDb, type TmpDbHandle } from "../../helpers/tmp-db.js"; +import { fakeEmbedder } from "../../helpers/fake-embedder.js"; + +function fakeLlmClient(name: string): LlmClient { + return { + provider: "local_only" as LlmProviderName, + model: name, + canStream: false, + async complete(_messages: LlmMessage[] | string, _opts?: LlmCallOptions) { + return { + text: "{}", + provider: "local_only" as LlmProviderName, + model: name, + servedBy: "local_only" as LlmProviderName, + durationMs: 0, + }; + }, + async completeJson() { + return { + value: {} as T, + raw: "{}", + provider: "local_only" as LlmProviderName, + model: name, + servedBy: "local_only" as LlmProviderName, + durationMs: 0, + }; + }, + async *stream() { + yield { delta: "", done: true }; + }, + stats(): LlmClientStats { + return { + requests: 0, + hostFallbacks: 0, + failures: 0, + retries: 0, + totalPromptTokens: 0, + totalCompletionTokens: 0, + lastOkAt: null, + lastError: null, + lastStatus: null, + }; + }, + resetStats() {}, + async close() {}, + }; +} + +let dbHandle: TmpDbHandle | null = null; + +function buildDepsWith( + h: TmpDbHandle, + opts: { reflectLlm: LlmClient | null }, +): PipelineDeps { + return { + agent: "openclaw", + home: resolveHome("openclaw", "/tmp/memos-issue-2362-test"), + config: { + ...DEFAULT_CONFIG, + algorithm: { + ...DEFAULT_CONFIG.algorithm, + lightweightMemory: { + ...DEFAULT_CONFIG.algorithm.lightweightMemory, + enabled: false, + }, + }, + }, + db: h.db, + repos: h.repos, + llm: fakeLlmClient("main-llm"), + reflectLlm: opts.reflectLlm, + l3Llm: fakeLlmClient("l3-llm"), + embedder: fakeEmbedder({ dimensions: 384 }), + log: rootLogger.child({ channel: "test.issue-2362" }), + namespace: { agentKind: "openclaw", profileId: "main" }, + now: () => 1_700_000_000_000, + }; +} + +beforeEach(() => { + dbHandle = makeTmpDb(); + skillSubscriberCalls.length = 0; +}); + +afterEach(() => { + dbHandle?.cleanup(); + dbHandle = null; +}); + +describe("pipeline/deps attachSkillSubscriber wiring (issue #2362)", () => { + it("passes bgReflectLlm — not bgLlm — as the skill subscriber's llm when reflectLlm is configured", () => { + const buses = buildPipelineBuses(); + const deps = buildDepsWith(dbHandle!, { + reflectLlm: fakeLlmClient("skill-evolver-llm"), + }); + const algorithm = extractAlgorithmConfig(deps); + const session = buildPipelineSession(deps, buses.session); + buildPipelineSubscribers(deps, buses, algorithm, session); + + expect(skillSubscriberCalls).toHaveLength(1); + const call = skillSubscriberCalls[0]; + + // The dedicated skillEvolver client must actually reach the skill + // subscriber; the reflectLlm client's stats are read as + // `skillEvolver.lastOkAt` in the overview/health endpoint, so it + // must be the client that runs the work. + expect(call.llm?.model).toBe("skill-evolver-llm"); + + // Regression guard: even though `deps.llm` is a distinct main-model + // client, the skill pipeline must ignore it and use the dedicated + // skillEvolver client. See issue #2362. + expect(call.llm?.model).not.toBe("main-llm"); + }); + + it("falls back to bgLlm when reflectLlm is not configured (backwards compatible)", () => { + // Installations that don't configure a distinct `skillEvolver.*` + // block leave `deps.reflectLlm` null — the fix must preserve today's + // behavior for those setups and use the main model instead. + const buses = buildPipelineBuses(); + const deps = buildDepsWith(dbHandle!, { reflectLlm: null }); + const algorithm = extractAlgorithmConfig(deps); + const session = buildPipelineSession(deps, buses.session); + buildPipelineSubscribers(deps, buses, algorithm, session); + + expect(skillSubscriberCalls).toHaveLength(1); + const call = skillSubscriberCalls[0]; + expect(call.llm?.model).toBe("main-llm"); + }); +});