From fcac0f94d4844f904a6eaa8a2b667299408b9f92 Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Fri, 19 Jun 2026 13:53:39 +0530
Subject: [PATCH] fix(openviking): guard empty tool_id in batch skip set; reuse
 env_var_enabled

Two follow-up fixes on top of the cherry-picked structured-sync work:

- _messages_to_openviking_batch only added a recall tool result's id to
  skipped_tool_ids when the id was non-empty. An empty tool_call_id (which
  the canonical transcript can carry; agent_runtime_helpers defaults it to
  "") poisoned the skip set with "", silently dropping any *other* tool
  result that also lacked an id. Move the recall-skip add inside the
  existing `if tool_id:` guard. Adds a regression test (mutation-checked:
  fails on pre-fix code, passes after).

- _sync_trace_enabled() open-coded the canonical truthy-env check; reuse
  utils.env_var_enabled (byte-identical {1,true,yes,on} semantics).
---
 plugins/memory/openviking/__init__.py      |  8 ++--
 tests/openviking_plugin/test_openviking.py | 45 ++++++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py
index 82f1f26a0a0..a57a60e67bd 100644
--- a/plugins/memory/openviking/__init__.py
+++ b/plugins/memory/openviking/__init__.py
@@ -49,7 +49,7 @@ from agent.message_content import flatten_message_text
 from agent.memory_provider import MemoryProvider
 from agent.skill_commands import extract_user_instruction_from_skill_message
 from tools.registry import tool_error
-from utils import atomic_json_write
+from utils import atomic_json_write, env_var_enabled
 
 logger = logging.getLogger(__name__)
 
@@ -160,7 +160,7 @@ def _derive_openviking_user_text(content: Any) -> str:
 
 
 def _sync_trace_enabled() -> bool:
-    return os.environ.get(_SYNC_TRACE_ENV, "").strip().lower() in {"1", "true", "yes", "on"}
+    return env_var_enabled(_SYNC_TRACE_ENV)
 
 
 def _preview(value: Any, limit: int = 160) -> str:
@@ -2461,8 +2461,8 @@ class OpenVikingMemoryProvider(MemoryProvider):
                 tool_id = str(message.get("tool_call_id") or message.get("id") or "")
                 if tool_id:
                     completed_tool_ids.add(tool_id)
-                if cls._is_openviking_recall_tool_name(message.get("name")):
-                    skipped_tool_ids.add(tool_id)
+                    if cls._is_openviking_recall_tool_name(message.get("name")):
+                        skipped_tool_ids.add(tool_id)
                 continue
             if message.get("role") != "assistant":
                 continue
diff --git a/tests/openviking_plugin/test_openviking.py b/tests/openviking_plugin/test_openviking.py
index 3a743287672..171e6abc8ac 100644
--- a/tests/openviking_plugin/test_openviking.py
+++ b/tests/openviking_plugin/test_openviking.py
@@ -539,6 +539,51 @@ class TestOpenVikingTurnConversion:
             assert recall_tool_name not in batch_text
             assert "Old OpenViking memory content" not in batch_text
 
+    def test_messages_to_openviking_batch_empty_tool_id_does_not_drop_other_results(self):
+        # A recall tool result that arrives with an empty tool_call_id must not
+        # poison the skip set with "" and silently drop unrelated tool results
+        # that also lack an id. Empty tool_call_id is reachable in the canonical
+        # transcript (agent_runtime_helpers defaults it to "").
+        turn = [
+            {"role": "user", "content": "What did we decide?"},
+            {
+                "role": "assistant",
+                "content": "",
+                "tool_calls": [
+                    {
+                        "id": "",
+                        "type": "function",
+                        "function": {
+                            "name": "viking_search",
+                            "arguments": json.dumps({"query": "decision"}),
+                        },
+                    }
+                ],
+            },
+            {
+                "role": "tool",
+                "tool_call_id": "",
+                "name": "viking_search",
+                "content": json.dumps({"results": ["recall stuff"]}),
+            },
+            {
+                "role": "tool",
+                "tool_call_id": "",
+                "name": "shell_command",
+                "content": "important shell output",
+            },
+            {"role": "assistant", "content": "done"},
+        ]
+
+        batch = OpenVikingMemoryProvider._messages_to_openviking_batch(turn)
+
+        batch_text = json.dumps(batch)
+        # The unrelated (empty-id) shell result must survive.
+        assert "important shell output" in batch_text
+        # The recall tool result must still be excluded.
+        assert "recall stuff" not in batch_text
+        assert "viking_search" not in batch_text
+
     def test_messages_to_openviking_batch_preserves_responses_text_parts(self):
         turn = [
             {"role": "user", "content": [{"type": "input_text", "text": "hello"}]},