Skip to content

fix: flatten sync_turn content to enable turn-aware chunking for hindsight plugin - #51270

Open
qxxaa wants to merge 3 commits into
NousResearch:mainfrom
qxxaa:fix/hindsight-flat-conversation-array
Open

fix: flatten sync_turn content to enable turn-aware chunking for hindsight plugin#51270
qxxaa wants to merge 3 commits into
NousResearch:mainfrom
qxxaa:fix/hindsight-flat-conversation-array

Conversation

@qxxaa

@qxxaa qxxaa commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Problem

sync_turn() produces a nested JSON array when building the retain payload:

# Each turn stored as a JSON array string:
turn = json.dumps(self._build_turn_messages(...))  # '[{user},{asst}]'

# At retain time, joined into:
content = "[" + ",".join(turns_to_retain) + "]"
# Produces: [[{user},{asst}],[{user},{asst}],...]

Hindsight's server-side chunk_text() (in engine/retain/fact_extraction.py) detects conversation format via:

parsed = json.loads(text)
if isinstance(parsed, list) and all(isinstance(turn, dict) for turn in parsed):
    return _chunk_conversation(parsed, max_chars)

This detection has no fallback handling for nested arrays. It silently fails the isinstance(turn, dict) check (outer elements are lists, not dicts) and drops through to RecursiveCharacterTextSplitter. The text splitter has no awareness of conversation turn boundaries and can split mid-message, producing chunks that begin without speaker attribution.

When an extraction LLM receives a chunk with no role prefix, it has no basis for determining who said what. Any first-person statement in such a fragment will be attributed to whichever speaker the model guesses, and that guess propagates through consolidation as a persistent fact.

Fix

One-line change in sync_turn(). Store each turn as comma-joined JSON objects instead of a JSON array:

# Before:
turn = json.dumps(self._build_turn_messages(...), ensure_ascii=False)

# After:
turn = ",".join(json.dumps(m, ensure_ascii=False) for m in self._build_turn_messages(...))

The two join sites (sync_turn line 1571, on_session_switch line 1759) already wrap with "[" + ",".join(...) + "]" and now produce a flat array:

[{"role":"user","content":"Q: ...","timestamp":"..."},{"role":"assistant","content":"JARVIS: ...","timestamp":"..."},...]

This passes the all(isinstance(turn, dict)) check and routes to _chunk_conversation(), which packs complete turns without splitting mid-message.

Affected code paths

Site Line Context
sync_turn() 1544 Turn serialisation at storage time

The join sites at lines 1571 (sync_turn retain) and 1759 (on_session_switch flush) are unchanged. They produce correct output once the stored elements are flat.

Regression risk

None. The change only affects the internal serialisation format of _session_turns entries. The final content string sent to aretain_batch is still a valid JSON array of message dicts. The only difference is nesting depth. Hindsight's extraction pipeline processes the same message objects regardless of how they arrived.

Test changes

  • Updated test_sync_turn_retains_metadata_rich_turn - assertions now reference flat indices (content[0] not content[0][0]).
  • Updated test_sync_turn_every_n_turns - len(content) == 6 (3 turns x 2 messages) instead of len(content) == 3 (3 pairs).
  • Added test_sync_turn_produces_flat_conversation_array - explicitly verifies the flat-dict contract that enables Hindsight's turn-aware chunking path.

@qxxaa qxxaa changed the title Fix/hindsight flat conversation array fix: flatten sync_turn content to enable turn-aware chunking for hindsight plugin Jun 23, 2026
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jun 23, 2026
@qxxaa

qxxaa commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Tested on my environment with a live Hindsight deployment. After applying this fix, conversation chunks now consistently start with [{"role": and route through _chunk_conversation correctly.

Before the fix, the nested [[{...}]] structure caused Hindsight's server-side chunker to fall through to sentence-boundary splitting, producing chunks that started mid-sentence with no speaker attribution. With the flat serialization, multiple append cycles produce valid JSON arrays and chunking respects turn boundaries.

There's a complementary server-side fix I've submitted separately to Hindsight (vectorize-io/hindsight#2412) to handle the "\n".join() corruption on their end. The two fixes are independent but together resolve the full data path..

@qxxaa

qxxaa commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi, the complementary server-side fix for this (vectorize-io/hindsight#2412) merged back in June. Without this PR, the client is still sending nested arrays that bypass Hindsight's turn-aware chunking, causing it to fall back to blind text splitting and misattribute speakers.

The data integrity issue is on the sending side. Would appreciate this getting another look

@qxxaa

qxxaa commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Hi @alt-glitch - would you consider bumping this to P2? I don't think P3/cosmetic is the right fit here.

This is a data integrity bug, not a cosmetic issue. Without this fix, every Hindsight retain cycle sends nested arrays ([[{user},{asst}]]) that fail the all(isinstance(turn, dict)) check in chunk_text(). The fallback RecursiveCharacterTextSplitter splits mid-message with no turn awareness, producing chunks without speaker attribution. The extraction LLM then has to guess who said what, and wrong guesses persist through consolidation as permanent facts in the user's memory store.

The server-side companion fix (vectorize-io/hindsight#2412) merged back in June. That handles the "\n".join() corruption on the Hindsight side, but the hermes client is still the one sending malformed nested arrays. Both halves need to be fixed for the full data path to work correctly.

It's a one-line change with three updated tests and zero regression risk. Happy to address any feedback if needed.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The premise is verified on current main: sync_turn() stores each turn as a JSON array at plugins/memory/hindsight/__init__.py:1621, then wraps the joined fragments in another array at :1648. The same stored fragments feed the session-switch flush at :1836, so the proposed change correctly flattens both outbound paths. The linked upstream Hindsight companion fix merged as 78d32cd16cc10af20f552f0158ef4ee409d67e10 and explicitly recognizes flat arrays of message dictionaries.

Problems

  • The new flat-array assertion covers the normal sync_turn() retain path, but TestSessionSwitchBufferFlush::test_buffered_turns_flushed_before_clear only checks that content strings are present. It does not protect the flat top-level-dictionary contract for the independent flush assembly at plugins/memory/hindsight/__init__.py:1836.

Suggested changes

  • Parse the flushed item["content"] in that session-switch test and assert all top-level elements are dictionaries in user/assistant order.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026
@qxxaa
qxxaa force-pushed the fix/hindsight-flat-conversation-array branch from 4c3bbfe to 7d5594f Compare July 15, 2026 10:00
@qxxaa

qxxaa commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Added the flat-dict contract assertion to TestSessionSwitchBufferFlush::test_buffered_turns_flushed_before_clear as suggested - parses the flushed item["content"] and asserts all top-level elements are dicts in user/assistant order.

Full test suite passes (117/117).

Note: the unrelated test_normalize_codex_response_salvage_is_xai_scoped failure in CI is pre-existing on main - not related to this change.

@qxxaa
qxxaa force-pushed the fix/hindsight-flat-conversation-array branch from 7d5594f to 26a56a1 Compare July 16, 2026 10:36
@qxxaa

qxxaa commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to absorb the issuer_kind pinning fix (8fa8aabb, #64844) - clears the unrelated test_normalize_codex_response_salvage_is_xai_scoped CI failure.

@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
@qxxaa
qxxaa force-pushed the fix/hindsight-flat-conversation-array branch 3 times, most recently from 2e60a16 to f8c91b9 Compare August 4, 2026 07:42
@qxxaa
qxxaa force-pushed the fix/hindsight-flat-conversation-array branch 2 times, most recently from 9e50c9a to 0bf0346 Compare August 14, 2026 09:57
qxxaa added 3 commits August 14, 2026 17:17
sync_turn serialises each turn pair as a JSON array and stores it in
_session_turns. At retain time, joining these produces a nested array:
[[{user},{asst}],[{user},{asst}],...]. Hindsight's chunk_text() detects
conversation arrays via:

    all(isinstance(turn, dict) for turn in parsed)

The nested structure fails this check (outer elements are lists, not
dicts) and falls through to RecursiveCharacterTextSplitter which splits
on sentence/paragraph boundaries with no turn awareness.

This causes chunks to begin mid-message without speaker attribution.
The extraction LLM then misattributes the orphaned text — e.g.
assigning an assistant's first-person statement to the user.

Fix: store each turn as comma-joined JSON objects rather than a JSON
array. The existing join sites ('[' + ','.join(...) + ']') now produce
a flat array of message dicts that passes the isinstance check and
routes to _chunk_conversation(), which packs complete turns and never
splits mid-message.

Verified: chunks produced after this fix all begin with complete
{\"role\": ..., \"content\": ..., \"timestamp\": ...} objects with full
speaker prefixes intact."
Add flat-dict contract assertion to session-switch flush test as
suggested by sweeper review.
@qxxaa
qxxaa force-pushed the fix/hindsight-flat-conversation-array branch from 0bf0346 to 6140dc9 Compare August 14, 2026 17:18
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 P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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