Skip to content

fix(hindsight): flatten retained conversation messages - #94968

Closed
zhuxiucai wants to merge 1 commit into
NousResearch:mainfrom
zhuxiucai:fix/hindsight-flat-conversation
Closed

zhuxiucai wants to merge 1 commit into
NousResearch:mainfrom
zhuxiucai:fix/hindsight-flat-conversation

Conversation

@zhuxiucai

Copy link
Copy Markdown

What does this PR do?

Hermes currently serializes each retained user/assistant pair as a JSON array and then wraps buffered pairs in another array, producing [[user, assistant], ...].

Hindsight's structured conversation chunker and append-array merge both expect one flat JSON array of message objects. The nested payload therefore falls back to plain-text chunking; after multiple appends, the stored document can become newline-concatenated JSON arrays instead of one valid conversation.

This change keeps buffered pairs structured in memory and flattens them only when serializing the retain payload. Both normal retain dispatch and flush-on-session-switch now send [user, assistant, ...].

Related Issue

No matching issue or PR found after searching the repository.

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/hindsight/__init__.py
    • buffer user/assistant pairs as structured messages instead of pre-serialized arrays;
    • serialize buffered pairs as one flat conversation for both regular retain and session-switch flush;
    • preserve existing append watermark, metadata, timestamps, tags, and legacy replace behavior.
  • tests/plugins/memory/test_hindsight_provider.py
    • assert metadata-rich retains use a flat message array;
    • assert buffered session-switch flushes remain flat and ordered;
    • assert append mode sends only the new user/assistant pair as a mergeable flat array.

How to Test

  1. Run scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py -q — 85 tests pass.
  2. Run scripts/run_tests.sh tests/plugins/memory/ -q — 363 tests pass across 24 files.
  3. Against a real self-hosted Hindsight 0.9.2 instance, retain two turns through the patched provider with update_mode="append"; read the stored document and verify it parses as four flat message objects in user, assistant, user, assistant order. Recall the synthetic facts, then delete the canary bank.

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 the repository's required scripts/run_tests.sh wrapper and all relevant tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on macOS arm64 against a Linux arm64 Hindsight service

Documentation & Housekeeping

  • Documentation update — N/A; behavior is internal and covered by the helper docstring/tests
  • cli-config.yaml.example update — N/A; no config keys changed
  • CONTRIBUTING.md / AGENTS.md update — N/A; no architecture or workflow changed
  • Cross-platform impact considered; change is stdlib JSON/list handling
  • Tool descriptions/schemas update — N/A; no tool schema changed

Screenshots / Logs

Real canary verification:

message_count=4
all_flat_dicts=true
roles=[user, assistant, user, assistant]
recall_count=4

@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 area/memory Memory subsystem: store, providers, sync, background reviews sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Aug 25, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Duplicate of #51270: both changes flatten Hindsight retained turn payloads from nested pairs into the flat message array required for turn-aware chunking. #51270 is the earlier open implementation.

@teknium1

Copy link
Copy Markdown
Collaborator

Closing: the bundled Hindsight provider this PR patches has moved out of this repo.

Thanks @zhuxiucai for this contribution. In #119888 (merge 9d799e0531c; removal commit 4cbf862abe4) the in-tree plugins/memory/hindsight/ provider was removed — Hindsight now installs from the plugin catalog and its code lives in vectorize-io/hindsight hindsight-integrations/hermes (maintained by @nicoloboschi). There is no longer any code in this repo for the Hindsight half of this PR to patch, so we are closing every open PR against the bundled provider rather than leaving them stranded.

Triage notes:

  • Flattens the Hindsight retain payload from [[user, assistant], ...] to [user, assistant, ...] in sync_turn and the session-switch flush; touches only plugins/memory/hindsight/init.py and its tests. Never landed before removal (the nested join was still present at 4cbf862) and the same nesting is still in the pinned upstream plugin (content = "[" + ",".join(turns) + "]" at hindsight-integrations/hermes/init.py:1332, turns json.dumps'd at :1377-1378), so this is a real fix to carry to vectorize-io/hindsight. Duplicate of the earlier, still-open fix: flatten sync_turn content to enable turn-aware chunking for hindsight plugin #51270 (qxxaa), which should get the same credit.
  • Still relevant at the catalog pin (dc750388)? Yes — the same code is at hindsight-integrations/hermes/__init__.py:1332 in the upstream tree. It is listed with your credit in Fixes from Hermes-side PRs worth carrying into hindsight-integrations/hermes vectorize-io/hindsight#4662 so it is not lost; if you want to carry the fix yourself, please open it against vectorize-io/hindsight — it would be welcome there.

If you believe this was closed in error, comment and we will reopen.

(Bulk-closed in the hindsight-move close pass.)

@teknium1 teknium1 closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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