Skip to content

Integrate OpenViking structured turn sync - #37251

Closed
huangxun375-stack wants to merge 1 commit into
NousResearch:mainfrom
huangxun375-stack:main
Closed

Integrate OpenViking structured turn sync#37251
huangxun375-stack wants to merge 1 commit into
NousResearch:mainfrom
huangxun375-stack:main

Conversation

@huangxun375-stack

@huangxun375-stack huangxun375-stack commented Jun 2, 2026

Copy link
Copy Markdown

What does this PR do?

This PR fixes OpenViking turn ingestion so the memory provider persists Hermes canonical messages with their structured content instead of flattening the turn into plain user/assistant text.

Before this change, sync_turn() only used user_content and assistant_content, so tool calls and tool results from the completed turn were not stored in OpenViking. Hermes already passes the full canonical messages list to memory providers; this PR makes the OpenViking provider consume that message list and write it through OpenViking's structured /messages/batch API.

Completed Hermes tool calls are stored as OpenViking ToolParts with:

  • tool_id
  • tool_name
  • tool_input
  • tool_output
  • tool_status

This preserves the actual turn structure in OpenViking storage and keeps Hermes as the source of truth for the live transcript.

Related Issue

Related to #34763

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/memory/openviking/__init__.py

    • Added sync_turn(messages=...) handling for Hermes canonical turn messages.
    • Writes structured OpenViking /messages/batch payloads with parts.
    • Converts Hermes assistant tool_calls and matching role=tool messages into OpenViking ToolParts.
    • Keeps pending ToolParts only for interrupted tool calls without results.
  • tests/openviking_plugin/test_openviking.py

    • Added coverage for structured batch payloads.
    • Added coverage for completed tool call/result folding.
    • Added coverage for error tool status mapping and interrupted pending calls.

How to Test

  1. Run the targeted OpenViking provider tests:

    py -m pytest tests\openviking_plugin\test_openviking.py tests\plugins\memory\test_openviking_provider.py tests\run_agent\test_memory_sync_interrupted.py -q -o addopts=''
  2. Confirm the tests pass.

Observed result:

51 passed, 1 skipped

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

py -m pytest tests\openviking_plugin\test_openviking.py tests\plugins\memory\test_openviking_provider.py tests\run_agent\test_memory_sync_interrupted.py -q -o addopts=''

51 passed, 1 skipped

@huangxun375-stack
huangxun375-stack force-pushed the main branch 2 times, most recently from 80d7977 to 60eac1a Compare June 2, 2026 08:08
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers labels Jun 2, 2026
@ehz0ah

ehz0ah commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for the update. This is much closer to what OpenViking needs: I checked it against the current OpenViking source, and the broad direction is right. /sessions/{id}/messages/batch accepts parts, ToolPart has the fields this PR is sending, commit accepts keep_recent_count, and the old top_k issue appears fixed because this now sends limit to search/find.

I still think there are two important issues to fix before merging.

First, _message_text() is too narrow for Hermes canonical messages. It currently handles plain strings, {"type": "text", "text": ...}, and dicts with content, but Hermes also uses Responses-format text blocks such as:

{"type": "input_text", "text": "user message"}
{"type": "output_text", "text": "assistant answer"}

With that shape, structured sync can silently drop the assistant text. For example, a turn like:

[
    {"role": "user", "content": [{"type": "input_text", "text": "hello"}]},
    {"role": "assistant", "content": [{"type": "output_text", "text": "answer"}]},
]

currently converts into only the user message in the OpenViking batch, so the assistant answer never reaches OpenViking.

The clean fix is to use one Hermes-owned text extraction helper for canonical message content, or at minimum make this helper support the full set of text-bearing block types:

TEXT_PART_TYPES = {"text", "input_text", "output_text", "summary_text"}

I would keep the migration narrow in this PR: add or use a shared helper, wire it into OpenViking structured sync, and add regression coverage for input_text / output_text. I would not migrate every other provider/parser here, since this PR is specifically about OpenViking ingestion.

Second, completed tool results should not be emitted as role: "user". OpenViking accepts that mechanically, but its memory extraction treats roles as meaningful: user-role content is user evidence, while assistant-role content is where tool/case/skill evidence belongs. A Hermes role=tool result is part of the assistant's tool execution, not something the user said.

The better shape is to keep ToolParts on assistant-role messages, for example:

{
    "role": "assistant",
    "parts": [
        {"type": "text", "text": "I will inspect that."},
        {
            "type": "tool",
            "tool_id": "call_123",
            "tool_name": "shell_command",
            "tool_input": {"command": "rg foo"},
            "tool_output": "...",
            "tool_status": "completed",
        },
    ],
}

The final assistant answer can remain a later assistant message. Consecutive assistant messages are fine here; they preserve the actual assistant tool execution followed by the final response.

One smaller coverage gap: agent/codex_runtime.py already appends turn.projected_messages into the local messages list, but then calls _sync_external_memory_for_turn() without passing messages. If structured ToolPart sync is expected on that runtime path too, that call should pass messages=messages; otherwise OpenViking only receives flattened user/assistant text there.

So my suggested path is:

  1. Add/use a canonical Hermes text extraction helper and use it in OpenViking sync.
  2. Emit completed ToolParts under role: "assistant", not role: "user".
  3. Pass messages=messages from agent/codex_runtime.py if that runtime should get structured OpenViking ingestion.
  4. Add tests for input_text / output_text, assistant-role ToolParts, and the codex runtime handoff if changed.

I do not think OpenViking itself needs changes for this PR; the server-side contract already supports the structured payload. The remaining fixes are about sending the right Hermes-side shape into that contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants