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).
This commit is contained in:
kshitijk4poor 2026-06-19 13:53:39 +05:30
parent 5a856bdfa3
commit fcac0f94d4
2 changed files with 49 additions and 4 deletions

View file

@ -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

View file

@ -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"}]},