fix(openclaw): preserve retain turn numbering across resumes#953
Conversation
nicoloboschi
left a comment
There was a problem hiding this comment.
Overall: targeted, well-motivated fix — root cause addressed in both per-turn and chunked paths, helpers unit-tested. A few non-blocking suggestions:
1. Chunked retention loses a window on boundary miss (src/index.ts, new getRetentionTurnIndex check)
Strict conversationTurnCount % retainEveryN === 0 means if the hook fails or is skipped on the exact boundary turn, that window is lost forever — the next fire at turn 2N jumps to window index 2 and window 1 is never written. Old counter had the same shape so not a regression, but worth a comment documenting the trade-off.
2. nextDocumentSequence / documentSequenceBySession are now effectively dead (src/index.ts:1558)
With the hook always passing turnIndex into buildRetainRequest, the fallback counter is only reachable via external callers. That counter has the same restart-safety problem this PR is fixing. Consider removing it, or add a comment explaining why it's retained.
3. Missing end-to-end coverage for the actual bug (src/index.test.ts)
The helper tests and the explicit-turnIndex test are good, but they don't prove the hook itself wires countUserTurns(sessionEntry.messages) into buildRetainRequest correctly — which is the actual bug. Consider an integration-style test that invokes the hook with a pre-populated sessionEntry.messages (simulating a resumed conversation) and asserts the resulting document_id reflects the resumed turn rather than turn 1.
4. Nit: countUserTurns(messages: any[])
Consistent with surrounding code, but { role?: string }[] would cost nothing and document the shape the function actually cares about.
d2c40d7 to
66613c9
Compare
|
Thanks, Nicolo. I pushed an update that addresses the points you raised. I added a regression test for the real I also tightened
|
|
Actually @nicoloboschi, after checking out your Hermes PR#6428 it appears your desire is to concatenate all conversations from sessions (with a stable session_id) into a single document? With the latest addition to append v. replace in hindsight, is this your desired/preferred direction for all integrations long-term? If so, this PR is dead and the openclaw integration needs adjusted to match the new Hermes direction with 6428 (if it hasn't already been). I'm perfectly happy with either or both, as long as all of my raw source data makes it's way to hindsight and can be iterated over as the product improves. I personally would love to see Hindsight aggregate or append be the default + summarization of an entire conversation to pull relevant context vs. guessing at "memories/facts" based on individual turns. I am here using (and contributing to) Hindsight after having built my own memory tool, deciding the space was too large and many people were solving similar problems and that my efforts might be better focused helping others bring a larger vision to fruition. My initial take was each turn being stored individually, and an LLM gathering all related session_id-marked sessions, aggregating them, and then extracting/summarizing from that consolidation. The automatic append is more ideal, as it's one less knob to have to turn at the client-side, as long as the processing continues to iterate over a document once it has changed. Anyway, all that to say, feel free to kill this PR if it doesn't suite your vision |
ff13c8c to
6777bf0
Compare
|
thanks for your hard work! yes the pattern is session = document, we have upsert and delta updates already in place in the hindsight api - I'll close this PR |
Summary
Why
Today the OpenClaw integration tracks retain turn state in memory. That works until the gateway, pod, or application is restarted.
After a restart, an existing conversation can resume with its prior
sessionKey, but the in-memory counter is gone. The next retain call then starts back at turn 1, which reuses the samedocument_idand causes Hindsight to overwrite the original turn 1 document.In short, process lifetime was being treated as conversation lifetime, which is not sound.
What changed
The retain path now derives turn numbering from the actual persisted conversation history already available in
sessionEntry.messages.That means:
So if a resumed conversation is really on turn 7, the plugin writes turn 7, not turn 1 wearing a false moustache.
Why this shape
This keeps the fix narrow and avoids introducing new persistence machinery inside the plugin.
Rather than storing extra state or trying to recover counters across process restarts, the integration now uses the source of truth it already has: the conversation history OpenClaw persists and hands back to the hook on resume.
Testing
npx vitest run src/index.test.tsnpm run buildNote:
npm testin this package still reports one pre-existing unrelated failure insrc/backfill.test.ts; the OpenClaw plugin tests for this change pass.