From 7fef5c78983123a42286fd530169ed9c8040f7e8 Mon Sep 17 00:00:00 2001 From: ehz0ah Date: Tue, 1 Sep 2026 15:42:16 +0800 Subject: [PATCH] fix(openviking): clarify remember submission status --- plugins/memory/openviking/README.md | 27 ++++++-- plugins/memory/openviking/__init__.py | 66 ++++++++++++++----- tests/openviking_plugin/test_openviking.py | 60 ++++++++++------- .../memory/test_openviking_optional_peer.py | 2 +- 4 files changed, 109 insertions(+), 46 deletions(-) diff --git a/plugins/memory/openviking/README.md b/plugins/memory/openviking/README.md index ebbaf2e741..29fe8d85d7 100644 --- a/plugins/memory/openviking/README.md +++ b/plugins/memory/openviking/README.md @@ -129,15 +129,30 @@ changes future writes, not the location of existing memories. `viking_remember` creates a one-shot `hermes-remember-` OpenViking session, adds the fact as one message, and commits the session with no retained tail. The session remains available in OpenViking for audit. OpenViking then -categorizes, merges, deduplicates, and indexes the result through its normal -memory extraction pipeline. The tool returns the one-shot session ID and the +classifies the source and can add, merge, or skip a memory through its normal +extraction pipeline. The tool returns the one-shot session ID and the extraction task ID when the server provides one. Extraction continues asynchronously after the tool returns. -The optional category is an extraction hint. The fact is stored as a `user` -message so `viking_remember` produces user memory. The one-shot session is -separate from the live Hermes conversation, so an explicit remember does not -commit or rotate the active conversation session. +The tool returns `status: submitted` because extraction can add a memory, merge +the fact into an existing memory, or produce no memory operation. It does not +promise that OpenViking created a distinct memory file. The fact is submitted +as an unchanged `user` message so OpenViking owns the final classification. +The legacy `category` argument is still accepted from existing callers but is +not advertised or used. The one-shot session is separate from the live Hermes +conversation, so an explicit remember does not commit or rotate the active +conversation session. + +If the message request or commit fails, the error includes the canonical +session URI, the failed stage, the observed message status, and an `ov session +commit ` recovery command. Inspect the session first. An archive +means the commit completed. A non-empty live `messages.jsonl` with no archive +means the message was accepted but still needs a commit. An empty live file +without an archive is ambiguous and must not trigger an automatic resubmission. +Use the same OpenViking profile and credentials as Hermes for manual recovery. +OpenViking server auto-commit is disabled by default, so an accepted message +whose explicit commit fails normally remains live and unextracted until it is +manually committed. Hermes built-in `memory` tool additions are mirrored to OpenViking after the local memory operation succeeds: diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 5996367077..3dfeeac522 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -614,21 +614,17 @@ BROWSE_SCHEMA = { REMEMBER_SCHEMA = { "name": "viking_remember", "description": ( - "Explicitly submit a fact or memory to the OpenViking memory pipeline. " - "Use for important information the agent should remember long-term. " - "OpenViking automatically categorizes, merges, and indexes the memory." + "Submit important long-term information to OpenViking through session " + "memory extraction. Success means the source was submitted, not that a " + "distinct memory file was created. OpenViking can add, merge, or skip the " + "final memory. If the message is accepted but commit fails, it normally " + "remains live and unextracted because server auto-commit is disabled by " + "default; follow the returned recovery instructions." ), "parameters": { "type": "object", "properties": { "content": {"type": "string", "description": "The information to remember."}, - "category": { - "type": "string", - "enum": ["preference", "entity", "event", "case", "pattern"], - "description": ( - "Optional extraction hint. OpenViking makes the final classification." - ), - }, }, "required": ["content"], }, @@ -5264,28 +5260,56 @@ class OpenVikingMemoryProvider(MemoryProvider): if not client: return tool_error("OpenViking server not connected") - category = str(args.get("category") or "").strip() - message_content = f"[Remember — {category}] {content}" if category else content session_id = f"hermes-remember-{uuid.uuid4().hex[:12]}" + session_uri = _user_scoped_uri( + self._user_space(client), + f"sessions/{session_id}", + ) + recovery_command = f"ov session commit {session_id}" + recovery_note = ( + "Inspect session_uri before recovery. If history/archive_* exists, do not " + "retry. If messages.jsonl contains the fact and no archive exists, run " + "recovery_command with the same OpenViking profile and credentials as " + "Hermes. Otherwise, do not resubmit automatically; report the uncertain " + "state to the user." + ) message: Dict[str, Any] = { "role": "user", - "parts": [self._text_part(message_content)], + "parts": [self._text_part(content)], } # Use a dedicated session so explicit remember does not commit or # otherwise alter the live Hermes conversation session. try: client.post(f"/api/v1/sessions/{session_id}/messages", message) + except Exception as e: + logger.error("OpenViking remember message failed for %s: %s", session_id, e) + return tool_error( + f"Memory message submission failed for session {session_id}: {e}", + session_id=session_id, + session_uri=session_uri, + failure_stage="message", + message_status="unknown", + recovery_command=recovery_command, + recovery_note=recovery_note, + ) + + try: commit = self._unwrap_result(client.post( f"/api/v1/sessions/{session_id}/commit", {"keep_recent_count": 0}, )) commit = commit if isinstance(commit, dict) else {} result: Dict[str, Any] = { - "status": "stored", + "status": "submitted", "session_id": session_id, + "session_uri": session_uri, + "message_status": "accepted", "extraction_status": str(commit.get("status") or "accepted"), - "message": "Memory stored in an OpenViking session and committed for extraction.", + "message": ( + "Memory source submitted to OpenViking session extraction. " + "OpenViking may add, merge, or skip the final memory." + ), } if commit.get("task_id"): result["task_id"] = commit["task_id"] @@ -5293,8 +5317,16 @@ class OpenVikingMemoryProvider(MemoryProvider): result["trace_id"] = commit["trace_id"] return json.dumps(result) except Exception as e: - logger.error("OpenViking remember session failed for %s: %s", session_id, e) - return tool_error(f"Failed to store memory in session {session_id}: {e}") + logger.error("OpenViking remember commit failed for %s: %s", session_id, e) + return tool_error( + f"Memory message was accepted, but commit failed for session {session_id}: {e}", + session_id=session_id, + session_uri=session_uri, + failure_stage="commit", + message_status="accepted", + recovery_command=recovery_command, + recovery_note=recovery_note, + ) def _tool_forget(self, args: dict) -> str: uri, error = _validate_forget_memory_uri(args.get("uri")) diff --git a/tests/openviking_plugin/test_openviking.py b/tests/openviking_plugin/test_openviking.py index 69dcd36670..b01e7d70bb 100644 --- a/tests/openviking_plugin/test_openviking.py +++ b/tests/openviking_plugin/test_openviking.py @@ -971,8 +971,10 @@ class TestEnsureClientReloadsEnv: {"content": "stable fact"}, )) - assert out["status"] == "stored" + assert out["status"] == "submitted" assert out["session_id"].startswith("hermes-remember-") + assert out["session_uri"] == f"viking://user/default/sessions/{out['session_id']}" + assert out["message_status"] == "accepted" assert out["extraction_status"] == "accepted" assert out["task_id"] == "task-remember" assert out["trace_id"] == "trace-remember" @@ -992,21 +994,7 @@ class TestEnsureClientReloadsEnv: ), ] - @pytest.mark.parametrize( - "category", - [ - "preference", - "entity", - "event", - "case", - "pattern", - ], - ) - def test_remember_uses_category_as_user_memory_hint( - self, - monkeypatch, - category, - ): + def test_remember_accepts_legacy_category_but_submits_raw_user_text(self, monkeypatch): posts = [] class _StubClient: @@ -1023,17 +1011,16 @@ class TestEnsureClientReloadsEnv: out = json.loads(provider._tool_remember({ "content": "stable fact", - "category": category, + "category": "preference", })) session_id = out["session_id"] message_path, message = posts[0] assert message_path == f"/api/v1/sessions/{session_id}/messages" assert message["role"] == "user" - assert message["parts"] == [ - {"type": "text", "text": f"[Remember — {category}] stable fact"} - ] + assert message["parts"] == [{"type": "text", "text": "stable fact"}] assert "peer_id" not in message + assert "category" not in openviking_plugin.REMEMBER_SCHEMA["parameters"]["properties"] assert posts[1] == ( f"/api/v1/sessions/{session_id}/commit", {"keep_recent_count": 0}, @@ -1061,7 +1048,31 @@ class TestEnsureClientReloadsEnv: assert second["session_id"].startswith("hermes-remember-") assert all("/api/v1/content/write" not in path for path, _ in posts) - def test_remember_reports_commit_failure_with_recoverable_session_id(self, monkeypatch): + def test_remember_reports_unknown_message_submission_failure(self, monkeypatch): + posts = [] + + class _StubClient: + def post(self, path, payload=None, **kwargs): + posts.append((path, payload or {})) + raise TimeoutError("message timeout") + + provider = OpenVikingMemoryProvider() + provider._client = _StubClient() + monkeypatch.setattr(provider, "_ensure_client", lambda: provider._client) + + out = json.loads(provider._tool_remember({"content": "stable fact"})) + + assert out["error"].startswith("Memory message submission failed for session ") + assert out["error"].endswith(": message timeout") + assert out["failure_stage"] == "message" + assert out["message_status"] == "unknown" + assert out["session_uri"].endswith(f"/sessions/{out['session_id']}") + assert out["recovery_command"] == f"ov session commit {out['session_id']}" + assert "do not resubmit automatically" in out["recovery_note"] + assert len(posts) == 1 + assert posts[0][0].endswith("/messages") + + def test_remember_reports_commit_failure_with_recovery_command(self, monkeypatch): posts = [] class _StubClient: @@ -1078,9 +1089,14 @@ class TestEnsureClientReloadsEnv: out = json.loads(provider._tool_remember({"content": "stable fact"})) assert out["error"].startswith( - "Failed to store memory in session hermes-remember-" + "Memory message was accepted, but commit failed for session hermes-remember-" ) assert out["error"].endswith(": commit rejected") + assert out["failure_stage"] == "commit" + assert out["message_status"] == "accepted" + assert out["session_uri"].endswith(f"/sessions/{out['session_id']}") + assert out["recovery_command"] == f"ov session commit {out['session_id']}" + assert "same OpenViking profile and credentials as Hermes" in out["recovery_note"] assert len(posts) == 2 assert posts[0][0].endswith("/messages") assert posts[1][0].endswith("/commit") diff --git a/tests/plugins/memory/test_openviking_optional_peer.py b/tests/plugins/memory/test_openviking_optional_peer.py index 691151dd38..6b5f509f24 100644 --- a/tests/plugins/memory/test_openviking_optional_peer.py +++ b/tests/plugins/memory/test_openviking_optional_peer.py @@ -290,7 +290,7 @@ def test_wire_requests_keep_writes_and_session_messages_in_the_selected_scope( result = json.loads( provider.handle_tool_call("viking_remember", {"content": "I like tea"}) ) - assert result["status"] == "stored" + assert result["status"] == "submitted" provider.on_memory_write("add", "user", "I like coffee") provider.sync_turn("hello", "hi", session_id="peer-test") assert provider._drain_writers("peer-test", timeout=5.0)