diff --git a/apps/memos-local-plugin/core/config/defaults.ts b/apps/memos-local-plugin/core/config/defaults.ts index 1cf2d2cf6..ae63896df 100644 --- a/apps/memos-local-plugin/core/config/defaults.ts +++ b/apps/memos-local-plugin/core/config/defaults.ts @@ -53,6 +53,19 @@ export const DEFAULT_CONFIG: ResolvedConfig = { temperature: 0, timeoutMs: 60_000, }, + l3Llm: { + // Empty by default — falls back to the shared `llm` settings. + // Operators set this when they want a stronger model for L3 + // abstraction. L3 runs off the turn-response path, so a slower + // but more reliable model improves world-model quality without + // affecting companion latency. + provider: "", + endpoint: "", + model: "", + apiKey: "", + temperature: 0, + timeoutMs: 60_000, + }, algorithm: { lightweightMemory: { enabled: true, @@ -307,6 +320,7 @@ export const SECRET_FIELD_PATHS: readonly string[] = Object.freeze([ "embedding.apiKey", "llm.apiKey", "skillEvolver.apiKey", + "l3Llm.apiKey", "hub.teamToken", "hub.userToken", ]); diff --git a/apps/memos-local-plugin/core/config/schema.ts b/apps/memos-local-plugin/core/config/schema.ts index 7c9ff193b..8f44f1d1d 100644 --- a/apps/memos-local-plugin/core/config/schema.ts +++ b/apps/memos-local-plugin/core/config/schema.ts @@ -558,6 +558,8 @@ export const ConfigSchema = Type.Object({ embedding: EmbeddingSchema, llm: LlmSchema, skillEvolver: SkillEvolverSchema, + /** Dedicated model slot for L3 abstraction. Same shape as skillEvolver. */ + l3Llm: SkillEvolverSchema, algorithm: AlgorithmSchema, hub: HubSchema, telemetry: TelemetrySchema, diff --git a/apps/memos-local-plugin/core/pipeline/deps.ts b/apps/memos-local-plugin/core/pipeline/deps.ts index fd9c2bbf0..1f7f3823f 100644 --- a/apps/memos-local-plugin/core/pipeline/deps.ts +++ b/apps/memos-local-plugin/core/pipeline/deps.ts @@ -273,7 +273,9 @@ export function buildPipelineSubscribers( repos: deps.repos, l2Bus: buses.l2, l3Bus: buses.l3, - llm: deps.llm, + // Dedicated L3 model when `config.l3Llm.*` is set; otherwise the + // bootstrap aliases `l3Llm` to the main `llm`, so this is a no-op. + llm: deps.l3Llm ?? deps.llm, log: log.child({ channel: "core.memory.l3" }), config: algorithm.l3Abstraction, }); diff --git a/apps/memos-local-plugin/core/pipeline/memory-core.ts b/apps/memos-local-plugin/core/pipeline/memory-core.ts index b4e331c71..89877d850 100644 --- a/apps/memos-local-plugin/core/pipeline/memory-core.ts +++ b/apps/memos-local-plugin/core/pipeline/memory-core.ts @@ -425,6 +425,45 @@ export async function bootstrapMemoryCoreFull( }); } + + // Dedicated LLM for L3 abstraction (mirrors skillEvolver above). + // L3 clustering → world-model abstraction is an infrequent async pass + // that is quality- and shape-sensitive (cheap models over-extract and + // truncate the JSON, producing "'constraints' must be an array"). It runs + // off the turn-response path, so a slower-but-correct model here has no + // impact on companion latency. Blank → falls back to the main `llm`. + let l3Llm: ReturnType | null = null; + try { + const l3c = (config as { l3Llm?: { provider?: string; model?: string; endpoint?: string; apiKey?: string; temperature?: number; timeoutMs?: number } }).l3Llm; + const l3Model = (l3c?.model ?? "").trim(); + const l3Provider = (l3c?.provider ?? "").trim(); + if (l3Model && l3Provider) { + l3Llm = createLlmClient({ + provider: l3Provider, + model: l3Model, + endpoint: l3c?.endpoint ?? "", + apiKey: l3c?.apiKey ?? "", + temperature: l3c?.temperature ?? 0, + timeoutMs: l3c?.timeoutMs ?? 60_000, + maxRetries: 3, + fallbackToHost: true, + onError: (d: { provider: string; model: string; message: string; code?: string; at?: number }) => + log.warn("l3Llm.llm_error", d), + } as never); + log.info("l3Llm.ready", { + provider: l3Provider, + model: l3Model, + source: "l3Llm", + }); + } + } catch (err) { + log.warn("l3Llm.unavailable", { + err: err instanceof Error ? err.message : String(err), + fallback: "main llm", + }); + } + + // 3. Pipeline. const deps: PipelineDeps = { agent: options.agent, @@ -434,6 +473,7 @@ export async function bootstrapMemoryCoreFull( repos, llm, reflectLlm: reflectLlm ?? llm, + l3Llm: l3Llm ?? llm, embedder, log, namespace, diff --git a/apps/memos-local-plugin/core/pipeline/orchestrator.ts b/apps/memos-local-plugin/core/pipeline/orchestrator.ts index 75dc7e244..0c9e43322 100644 --- a/apps/memos-local-plugin/core/pipeline/orchestrator.ts +++ b/apps/memos-local-plugin/core/pipeline/orchestrator.ts @@ -1526,6 +1526,7 @@ export function createPipeline(deps: PipelineDeps): PipelineHandle { repos: deps.repos, llm: deps.llm, reflectLlm: deps.reflectLlm, + l3Llm: deps.l3Llm, embedder: deps.embedder, sessionManager: session.sessionManager, episodeManager: session.episodeManager, diff --git a/apps/memos-local-plugin/core/pipeline/types.ts b/apps/memos-local-plugin/core/pipeline/types.ts index 7969b7d39..b1dceed23 100644 --- a/apps/memos-local-plugin/core/pipeline/types.ts +++ b/apps/memos-local-plugin/core/pipeline/types.ts @@ -138,6 +138,14 @@ export interface PipelineDeps { * absent. Summarization and per-turn lite capture still use `llm`. */ reflectLlm: LlmClient | null; + /** + * Dedicated LLM for L3 abstraction (clustering → world-model facts). + * Built from `config.l3Llm.*` when configured; falls back to `llm` + * when absent. L3 runs off the turn-response path, so a stronger + * (slower) model here improves abstraction quality without affecting + * companion latency. + */ + l3Llm: LlmClient | null; embedder: Embedder | null; log: Logger; namespace: RuntimeNamespace; @@ -169,6 +177,13 @@ export interface PipelineHandle { * status instead of falling back to the summary LLM. */ readonly reflectLlm: LlmClient | null; + /** + * Dedicated client for L3 abstraction. When `l3Llm.*` is blank this is the + * same instance as `llm`; when configured it carries its own model so the + * clustering → world-model pass can run on a stronger LLM than the cheap + * high-frequency main model. + */ + readonly l3Llm: LlmClient | null; readonly embedder: Embedder | null; // Subscribers / runners. diff --git a/apps/memos-local-plugin/tests/unit/pipeline/orchestrator.test.ts b/apps/memos-local-plugin/tests/unit/pipeline/orchestrator.test.ts index f4826a6f3..ec0fb3693 100644 --- a/apps/memos-local-plugin/tests/unit/pipeline/orchestrator.test.ts +++ b/apps/memos-local-plugin/tests/unit/pipeline/orchestrator.test.ts @@ -49,6 +49,7 @@ function buildDeps( repos: h.repos, llm: null, reflectLlm: null, + l3Llm: null, embedder, log: rootLogger.child({ channel: "test.pipeline" }), namespace: { agentKind: "openclaw", profileId: "main" }, @@ -75,6 +76,18 @@ afterEach(async () => { }); describe("pipeline/orchestrator", () => { + it("threads a dedicated l3Llm through to the handle", () => { + const l3Llm = fakeLlm({ completeJson: {} }); + pipeline = createPipeline({ ...buildDeps(dbHandle!), l3Llm }); + expect(pipeline.l3Llm).toBe(l3Llm); + }); + + it("leaves l3Llm null on the handle when not configured", () => { + pipeline = createPipeline(buildDeps(dbHandle!)); + expect(pipeline.l3Llm).toBeNull(); + }); + + it("wires session → episode → turn end cleanly", async () => { pipeline = createPipeline(buildDeps(dbHandle!)); const turn: TurnInputDTO = { diff --git a/src/memos/mem_os/core.py b/src/memos/mem_os/core.py index 54f8f01e0..3ede965d3 100644 --- a/src/memos/mem_os/core.py +++ b/src/memos/mem_os/core.py @@ -1170,7 +1170,7 @@ def share_cube_with_user(self, cube_id: str, target_user_id: str) -> bool: bool: True if successful, False otherwise. """ # Validate current user has access to this cube - self._validate_cube_access(cube_id, target_user_id) + self._validate_cube_access(self.user_id, cube_id) # Validate target user exists if not self.user_manager.validate_user(target_user_id): diff --git a/tests/mem_os/test_format_utils.py b/tests/mem_os/test_format_utils.py new file mode 100644 index 000000000..b97178784 --- /dev/null +++ b/tests/mem_os/test_format_utils.py @@ -0,0 +1,75 @@ +""" +Test suite for src/memos/mem_os/utils/format_utils.py + +Focus: clean_json_response function defensive behavior +Related issue: #1525 +""" + +import pytest + +from memos.mem_os.utils.format_utils import clean_json_response + + +class TestCleanJsonResponse: + """Test clean_json_response function with various inputs.""" + + def test_clean_json_response_with_none_raises_value_error(self): + """Test that passing None raises ValueError with diagnostic message.""" + with pytest.raises(ValueError) as exc_info: + clean_json_response(None) + + error_message = str(exc_info.value) + assert "clean_json_response received None" in error_message + assert "upstream LLM call" in error_message + assert "timed_with_status" in error_message or "generate()" in error_message + + def test_clean_json_response_removes_json_code_block(self): + """Test removal of ```json markers.""" + input_str = '```json\n{"key": "value"}\n```' + expected = '{"key": "value"}' + assert clean_json_response(input_str) == expected + + def test_clean_json_response_removes_plain_code_block(self): + """Test removal of ``` markers without json keyword.""" + input_str = '```\n{"key": "value"}\n```' + expected = '{"key": "value"}' + assert clean_json_response(input_str) == expected + + def test_clean_json_response_strips_whitespace(self): + """Test that leading/trailing whitespace is stripped.""" + input_str = ' \n {"key": "value"} \n ' + expected = '{"key": "value"}' + assert clean_json_response(input_str) == expected + + def test_clean_json_response_handles_plain_json(self): + """Test that plain JSON without markdown is unchanged (except strip).""" + input_str = '{"key": "value"}' + expected = '{"key": "value"}' + assert clean_json_response(input_str) == expected + + def test_clean_json_response_handles_empty_string(self): + """Test that empty string is handled correctly.""" + assert clean_json_response("") == "" + + def test_clean_json_response_with_complex_json(self): + """Test with realistic LLM response containing nested JSON.""" + input_str = """```json +{ + "queries": [ + {"query": "test", "weight": 1.0}, + {"query": "example", "weight": 0.5} + ] +} +```""" + result = clean_json_response(input_str) + assert "```json" not in result + assert "```" not in result + assert '"queries"' in result + assert result.strip() == result # No leading/trailing whitespace + + def test_clean_json_response_preserves_internal_backticks(self): + """Test that backticks inside JSON content are preserved.""" + input_str = '```json\n{"code": "`example`"}\n```' + result = clean_json_response(input_str) + assert "`example`" in result + assert result.count("`") == 2 # Only internal backticks remain diff --git a/tests/mem_os/test_memos_core.py b/tests/mem_os/test_memos_core.py index 6d2408d05..b57b0b254 100644 --- a/tests/mem_os/test_memos_core.py +++ b/tests/mem_os/test_memos_core.py @@ -795,3 +795,146 @@ def test_search_nonexistent_cube( assert result["text_mem"] == [] assert result["act_mem"] == [] assert result["para_mem"] == [] + + +class TestShareCubeWithUser: + """Regression tests for share_cube_with_user (issue #1901). + + The original implementation called ``_validate_cube_access(cube_id, + target_user_id)``, which both (a) swapped the positional arguments and + (b) validated the wrong user. Every well-formed call therefore failed + with ``ValueError: User '' does not exist or is inactive`` even + though the calling user owned the cube. These tests pin down the correct + semantics: validate the *current* user against the cube being shared, + then delegate the share to ``user_manager.add_user_to_cube``. + """ + + def _build_mos( + self, + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ): + mock_llm_factory.from_config.return_value = mock_llm + mock_reader_factory.from_config.return_value = mock_mem_reader + mock_user_manager_class.return_value = mock_user_manager + return MOSCore(MOSConfig(**mock_config)) + + @patch("memos.mem_os.core.UserManager") + @patch("memos.mem_os.core.MemReaderFactory") + @patch("memos.mem_os.core.LLMFactory") + def test_share_cube_validates_current_user_not_target( + self, + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ): + """Cube access must be validated against the *current* user. + + Regression for #1901: previously the cube_id was passed where the + user_id was expected, causing ``_validate_user_exists`` to reject + every call because the cube UUID is obviously not a registered user. + """ + mock_user_manager.validate_user.return_value = True + mock_user_manager.validate_user_cube_access.return_value = True + mock_user_manager.add_user_to_cube.return_value = True + + mos = self._build_mos( + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ) + + cube_id = "cube-uuid-1234" + target_user_id = "target_user" + + result = mos.share_cube_with_user(cube_id=cube_id, target_user_id=target_user_id) + + assert result is True + # The cube-access check must be made against the *current* user, + # not the cube_id and not the target user. + mock_user_manager.validate_user_cube_access.assert_called_once_with(mos.user_id, cube_id) + # And the actual sharing must add the *target* user to the cube. + mock_user_manager.add_user_to_cube.assert_called_once_with(target_user_id, cube_id) + + @patch("memos.mem_os.core.UserManager") + @patch("memos.mem_os.core.MemReaderFactory") + @patch("memos.mem_os.core.LLMFactory") + def test_share_cube_raises_when_current_user_lacks_access( + self, + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ): + """If the current user doesn't have access to the cube, refuse to share. + + The error message must reference the current user, not the cube_id + (which was the misleading symptom in #1901). + """ + mock_user_manager.validate_user.return_value = True + mock_user_manager.validate_user_cube_access.return_value = False + + mos = self._build_mos( + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ) + + with pytest.raises(ValueError, match="test_user"): + mos.share_cube_with_user(cube_id="cube-uuid-1234", target_user_id="target_user") + + mock_user_manager.add_user_to_cube.assert_not_called() + + @patch("memos.mem_os.core.UserManager") + @patch("memos.mem_os.core.MemReaderFactory") + @patch("memos.mem_os.core.LLMFactory") + def test_share_cube_raises_when_target_user_missing( + self, + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ): + """Target user must exist; ``validate_user`` is consulted independently.""" + # validate_user is used twice: once during MOSCore.__init__ for + # ``self.user_id`` (must succeed) and once for the target user (fail). + mock_user_manager.validate_user.side_effect = lambda uid: uid == "test_user" + mock_user_manager.validate_user_cube_access.return_value = True + + mos = self._build_mos( + mock_llm_factory, + mock_reader_factory, + mock_user_manager_class, + mock_config, + mock_llm, + mock_mem_reader, + mock_user_manager, + ) + + with pytest.raises(ValueError, match="Target user 'missing_user'"): + mos.share_cube_with_user(cube_id="cube-uuid-1234", target_user_id="missing_user") + + mock_user_manager.add_user_to_cube.assert_not_called()