Skip to content

fix(openviking): preserve structured turn sync attribution - #48894

Closed
ehz0ah wants to merge 3 commits into
NousResearch:mainfrom
ehz0ah:pr-37251-structured-sync-fix
Closed

fix(openviking): preserve structured turn sync attribution#48894
ehz0ah wants to merge 3 commits into
NousResearch:mainfrom
ehz0ah:pr-37251-structured-sync-fix

Conversation

@ehz0ah

@ehz0ah ehz0ah commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Supersedes #34763 and #37251. The structured-sync implementation from #37251 was cherry-picked into this branch with the original authorship preserved, then followed up with fixes for OpenViking attribution and structured text handling.

This makes OpenViking structured turn sync preserve the data OpenViking needs long-term: Responses-format text parts, assistant-owned tool results, and Hermes peer attribution.

The original structured sync path added messages/batch support and ToolPart payloads, but still had two correctness gaps:

  • Hermes Responses-style content blocks such as input_text / output_text were dropped by the OpenViking provider's local text extraction.
  • Tool result messages could lose the assistant ownership/peer attribution OpenViking uses when extracting assistant-side cases, patterns, tools, and skills.

This PR centralizes Hermes message text flattening, reuses it from the OpenViking provider, and makes the Codex app-server runtime pass the canonical messages transcript through to memory sync so OpenViking can store tool calls/results instead of falling back to plain user/assistant text.

Related Issue

Supersedes #34763 and #37251. The #37251 implementation commit was cherry-picked with authorship preserved.

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

  • Added agent/message_content.py with a shared flatten_message_text() helper for string content and structured OpenAI/Responses content blocks.
  • Updated plugins/memory/openviking/__init__.py to:
    • preserve input_text, output_text, and text content;
    • convert completed tool results into assistant-owned OpenViking ToolPart payloads;
    • apply Hermes peer_id only to assistant messages;
    • keep OpenViking recall tool output out of the re-ingested session transcript.
  • Updated agent/codex_runtime.py so the Codex app-server path passes the projected canonical messages list into external memory sync.
  • Added/updated regression coverage for text flattening, structured OpenViking batch conversion, assistant peer attribution, and Codex runtime memory sync handoff.

How to Test

  1. scripts/run_tests.sh tests/agent/test_message_content.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/run_agent/test_codex_app_server_integration.py
    • Result: 152 passed.
  2. .venv/bin/python -m pytest -q tests/agent/test_message_content.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/run_agent/test_codex_app_server_integration.py
    • Result: 152 passed.
  3. .venv/bin/python -m ruff check agent/message_content.py agent/codex_runtime.py plugins/memory/openviking/__init__.py tests/agent/test_message_content.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/run_agent/test_codex_app_server_integration.py
    • Result: all checks passed.
  4. Live local integration smoke test:
    • Started current OpenViking locally in api_key mode with separate model/API and OpenViking auth keys.
    • Called Hermes OpenVikingMemoryProvider.sync_turn(...) against the real server.
    • Read back /api/v1/sessions/{session_id}/context.
    • Verified four stored messages: user, assistant, assistant, assistant; assistant messages carried peer_id: hermes; exactly one ToolPart preserved tool_id, tool_name, tool_input, completed tool_status, and tool output; input_text / output_text were preserved.

Full-suite note: I also attempted scripts/run_tests.sh. It did not complete because this local worktree/environment fails unrelated tests outside this PR's surface, including ACP tests with ModuleNotFoundError: No module named 'acp' and existing non-OpenViking failures. The focused touched-path suite above passes through the repo's canonical wrapper.

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: macOS 15.7.4

Documentation & Housekeeping

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

Screenshots / Logs

N/A. This is a memory sync/storage correctness fix with unit and local integration verification.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jun 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: #37251 (structured turn sync implementation, cherry-picked here with authorship preserved), #34763 (full turn-context sync feature request). This PR supersedes both by adding the OpenViking attribution + structured-text-flattening fixes (input_text/output_text preservation, assistant-owned ToolPart payloads, peer_id scoping) the earlier path lacked.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks for the clean salvage here, @ehz0ah — the cherry-pick of #37251 is faithful (identical git patch-id) and the authorship/attribution is exactly right.

I reviewed the full PR and verified every claim (faithful cherry-pick, real supersession of #34763/#37251, correct release.py mapping, 152 tests pass, ruff clean, and the keep_recent_count: 0 / flatten_message_text paths are both non-regressions). A parallel review pass turned up two things worth fixing:

  1. A real bug — an empty tool_call_id (reachable in the canonical transcript) poisoned the batch skip set with "" and could silently drop unrelated tool results.
  2. A reuse win — _sync_trace_enabled() re-implemented utils.env_var_enabled.

I've carried all of your commits forward verbatim (authorship preserved) and added a single follow-up commit with those two fixes plus a mutation-checked regression test in #48924. Closing this in favor of that PR so the fixes ride along. Full credit to you, @huangxun375-stack (#37251), and @pty819 (#34763) — all preserved in the salvage branch.

kshitijk4poor added a commit that referenced this pull request Jun 19, 2026
…-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage #48894)
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…4-structured-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage NousResearch#48894)
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
…4-structured-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage NousResearch#48894)
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
…4-structured-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage NousResearch#48894)
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…4-structured-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage NousResearch#48894)
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
…4-structured-sync

fix(openviking): structured turn sync — guard empty tool_id, reuse env_var_enabled (salvage NousResearch#48894)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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.

3 participants