Fix for #2914: stop agent from seeing deleted messages after Edit/Undo (from state.db)

[AlexeyDsov]

Problem

After Edit, Retry, or Undo in the WebUI, the agent could still see original messages from state.db even though the UI correctly showed only the truncated conversation. If you asked the agent "what messages do you see?", it would report both the old and new versions.

Note: This issue (#2914) was partially addressed in #2933, which introduced the truncation_watermark mechanism. However, two gaps remained: context_messages was not truncated in sync with messages, and the watermark filter only checked m_ts > watermark, allowing original edited messages (with older timestamps) to slip through.

Root Cause

When a user edits or truncates a message, the WebUI sidecar (.json session file) is updated correctly, but state.db is an append-only store — the original messages remain there forever. During session load, state.db rows are replayed and merged with the sidecar. The truncation_watermark mechanism was supposed to filter out pre-truncation rows, but had two gaps:

1. context_messages was not truncated alongside messages, so the agent's model-facing context retained rows the user deliberately removed.
2. Watermark filtering only checked m_ts > watermark — but original edited messages have an older timestamp (before the edit), so they passed through the filter unblocked.

Fix

• session_ops.py: retry_last() and undo_last() now truncate context_messages in sync with messages.
• models.py: reconciled_state_db_messages_for_session() now uses the sidecar's message IDs as authoritative — state.db rows whose content duplicates a sidecar message at a different timestamp are deduplicated, and rows preceding the watermark are properly excluded.
• truncation_watermark is persisted on every truncation operation (Edit, Retry, Undo) and survives reload.

Reproduction Examples

Example 1 — Edit leaks original into context:

1. Send: "What is the sum of angles in a triangle?" → Agent replies about 180°
2. Edit the message to: "What is the sum of angles in a square?" → Agent replies about 360°
3. Send: "List all my messages in this session" → Bug: Agent reports both "triangle" and "square", even though the UI only shows "square"

Example 2 — Edit then Undo leaks original:

1. Send: "What is the sum of angles in a triangle?" → Agent replies about 180°
2. Edit the message to: "What is the sum of angles in a square?" → Agent replies about 360°
3. Send another message: "What is the speed of light?"
4. Undo — removes the "speed of light" message
5. Send: "List all my previous messages" → Bug: Agent reports both the original "triangle" message AND the edited "square" message, even though the UI only shows "square"

Testing

12 regression tests in tests/test_issue2914_truncation_watermark.py:

• Merge filter (3) — state.db tail beyond watermark excluded; empty sidecar with watermark=0 blocks replay; full Edit → new turn → Undo scenario proves original messages no longer leak
• Truncate endpoint (2) — context_messages truncated in sync with messages; proves the bug without the fix
• Context clamp (5) — _clamp_context_to_watermark unit tests: filters beyond watermark, passes through when None, blocks all at zero, preserves messages without timestamp, handles empty input
• Watermark invariant (1) — watermark is NOT auto-cleared on save() even when newer messages exist

AI Assistance

This fix was developed with assistance from Qwen3.6-27B. The AI helped analyze the reconciliation logic, identify the dual-gap root cause (context_messages sync + watermark timestamp filtering), improve the code and draft the test scenarios.

[nesquena-hermes]

Review notes — looks right, two small follow-ups before merge

Reading the diff at api/models.py:607-621, api/routes.py:5733-5754, api/streaming.py:2622-2645 plus the 12 regression assertions in tests/test_issue2914_truncation_watermark.py. The dual-gap diagnosis matches what I see in the existing code — _maybe_clear_truncation_watermark() was unconditionally clearing the watermark whenever any message timestamp exceeded it, and the merge filter at models.py:3782-3789 only blocked m_ts > watermark, leaving the older pre-edit timestamps to slip through.

Code reference

The new merge filter at api/models.py:3802-3816:

# When a truncation watermark is active, state.db may contain original
# messages that were replaced by Edit (old content with old timestamp).
# The timestamp-based filter above catches messages AFTER the watermark,
# but messages BEFORE it (like the original pre-edit content) slip through.
if (
    watermark_timestamp is not None
    and timestamp is not None
    and timestamp < watermark_timestamp
    and key not in seen_message_keys
    and _session_message_content_key(msg) not in seen_content_keys
):
    continue

The content_key not in seen_content_keys guard is what protects legitimate pre-watermark history when the sidecar still contains it (e.g. after duplicate where context_messages was deep-copied). It only drops state.db rows whose content is unique to state.db — i.e. truly replaced messages. That's the right shape.

The _clamp_context_to_watermark helper at api/streaming.py:2622-2645 is invoked at all three _next_context_messages = … writeback paths in _run_agent_streaming (lines 5175, 5323, 6201) so it covers the streaming-done, mid-stream tool-result, and tool-loop-end branches. Good.

Two follow-ups worth resolving before merge

1. The _maybe_clear_truncation_watermark() no-op leaves dead code. The current diff turns the method body into pass but keeps the call site at models.py:639 and the helper at :607-621. That's fine for now, but it would be cleaner to either:

• Delete the call from save() and the method entirely, or
• Keep the method as a documented intentional no-op and add a single-line docstring that names the invariant ("never auto-clear on save — see Bug: /undo and message edit appear to succeed but have no visual effect #2914") so future maintainers don't reintroduce auto-clearing thinking it's a missed cleanup.

Recommendation: delete the call + method. The new test test_maybe_clear_truncation_watermark_does_not_clear_on_save (lines 393-427) pins the invariant from the outside, which is the right place to enforce it.

2. The _clamp_context_to_watermark filter uses m_ts <= _tw_ts (line 2637, generator expression — if (m_ts := _message_timestamp_as_float(m)) is None or m_ts <= _tw_ts). But the test asserts _clamp_context_to_watermark_zero_watermark_blocks_all expects everything dropped when watermark is 0.0. That works only because timestamps >= 1.0 in the test fixtures fail the <= 0.0 check. Worth confirming: is "messages with timestamp == watermark" intended to be kept or dropped?

Looking at the merge filter at models.py:3784-3789: it uses timestamp > watermark_timestamp (strict greater) to drop — so messages at exactly the watermark are kept. The <= in _clamp_context_to_watermark matches that semantics. Consistent. Just worth a one-line comment on streaming.py:2637 cross-referencing the merge filter so the asymmetry (<= to keep vs > to drop) is obvious to the next reader.

Test plan

Beyond what's already in tests/test_issue2914_truncation_watermark.py, one scenario worth thinking about for the next PR if you have time:

• Edit on a session that has a duplicate child. The fix in PR Fix session duplicate and branch field propagation #3101 adds deep-copy of context_messages + truncation_watermark to duplicate. But what happens if the parent is edited after the duplicate was made? The parent's state.db tail will now contain rows the child wouldn't want to inherit, but the child has its own sidecar so the child's reconcile should be insulated by its own truncation_watermark. Worth at least one cross-session test asserting that case, but probably out of scope for this PR — it's Fix session duplicate and branch field propagation #3101's territory.

Overall: this is a clear, well-investigated fix for a real data-leak bug. The test set is appropriately broad. Once the dead-code cleanup is decided one way or the other, I'd merge.

[nesquena-hermes]

Re-review of the force-pushed v2 (bc471c8) — both follow-ups resolved, LGTM

Re-fetched the branch (it was force-pushed to a single amended commit at bc471c8) and diffed it against origin/master. Both items I flagged in the last review are addressed cleanly.

Follow-up 1 (dead-code cleanup) — done, the better way

You took the delete option rather than the documented no-op. _maybe_clear_truncation_watermark() is gone and so is its save() call site. The diff at api/models.py:

-    def _maybe_clear_truncation_watermark(self) -> None:
-        watermark = _message_timestamp_as_float({"timestamp": self.truncation_watermark})
-        ...
-        if max_message_timestamp is not None and max_message_timestamp > watermark:
-            self.truncation_watermark = None

and at the save() body:

-        self._maybe_clear_truncation_watermark()

The invariant is now pinned from the outside by the merge-filter tests rather than an internal no-op — which is exactly where I'd want it. The stale test_maybe_clear_truncation_watermark_does_not_clear_on_save test was correctly dropped along with the method, so there's no dangling reference to a symbol that no longer exists.

Follow-up 2 (the <= vs > asymmetry) — comment added, plus a bonus

api/streaming.py:2638-2640 now carries the cross-reference I asked for, so the boundary semantics are self-documenting:

# Keep m_ts <= watermark (messages AT the watermark boundary are kept).
# Matches the merge filter in models.py merge_session_messages_append_only,
# which drops state.db rows with timestamp > watermark_timestamp (strict).
if (m_ts := _message_timestamp_as_float(m)) is None or m_ts <= _tw_ts

That matches the strict timestamp > watermark_timestamp drop in merge_session_messages_append_only at api/models.py:3782-3784, so a message exactly at the watermark is kept on both the merge path and the clamp path — consistent. The logger.info("clamping context_messages: %d → %d …") line you added is a nice touch for diagnosing this in the field without re-instrumenting.

Test coverage

The _clamp_context_to_watermark unit tests now cover the boundary cases the comment describes — test_clamp_context_to_watermark_zero_watermark_blocks_all (tests/...:343), _keeps_messages_without_timestamp (:360), and _empty_messages (:380). The no-timestamp-passes-through case (m_ts is None or m_ts <= _tw_ts) is pinned, which is the one easy-to-regress branch. 12 test functions total in the file.

Verdict

LGTM. Both follow-ups resolved, CI is green across 3.11 / 3.12 / 3.13, and the clamp is wired into all three _next_context_messages writeback paths (streaming.py:5181, :5329, :6207). No further blockers from my side — ready to merge.

The cross-session "edit-after-duplicate" scenario I mentioned remains #3101's territory, not a blocker here. Thanks for the quick turnaround, @AlexeyDsov.

[nesquena-hermes]

Thanks for the thorough work on this, @AlexeyDsov — the diagnosis of #2914 is correct (the > watermark filter missed original pre-edit content that sorts below the watermark, and context_messages wasn't being truncated in sync). Parts of this PR are exactly right: truncating context_messages alongside messages in the truncate endpoint, and the content-fingerprint filter in merge_session_messages_append_only.

But the review (independent maintainer trace + Opus advisor, both with empirical repros) found a severe regression in the watermark-clamp approach that we can't ship as-is. Marking hold + changes-requested.

The blocking problem: legitimate new turns are dropped from agent context after any Edit

_clamp_context_to_watermark keeps only messages with m_ts <= watermark. Combined with removing _maybe_clear_truncation_watermark, the watermark is now set only by truncate/undo/retry and never advanced on a normal turn (confirmed: truncation_watermark is only ever read in api/streaming.py:2633, never written). So:

1. User edits a message → truncate sets watermark = 2.0 (ts of the kept tail).
2. User sends a brand-new question (ts=3.0); agent replies (ts=4.0).
3. The post-result clamp runs with the stale watermark=2.0 and drops both ts=3.0 and ts=4.0 — the new turn vanishes from the agent's context on the next turn.

Empirical repro (run against this branch):

_clamp_context_to_watermark(session{watermark=2.0}, [u@1, a@2, u@3 "NEW Q", a@4 "NEW REPLY"])
→ ['first (pre-edit)', 'reply first']      # NEW Q and NEW REPLY dropped

This means every conversation that has ever had an Edit/Retry/Undo would silently lose all subsequent turns from the agent's context — a worse and much more frequent bug than #2914. There's also a secondary issue: because the auto-clear was removed, a watermark set once is now permanent — it survives across future compressions and will silently block legitimate older-context re-merges indefinitely.

What we'd need to merge this

Keep the good parts (context_messages truncation sync + the content-fingerprint merge filter). The watermark just can't double as a permanent deletion oracle. Concrete options (pick one):

1. Advance the watermark to the tail timestamp at the end of every successful turn (after the post-result s.save()). That keeps the deletion oracle alive for exactly the one immediate post-edit turn (where Bug: /undo and message edit appear to succeed but have no visual effect #2914 bites) and self-clears thereafter, so normal subsequent turns aren't clamped. Smallest change that keeps your fingerprint filter.
2. Restore a (stricter) _maybe_clear_truncation_watermark — clear the watermark once the next turn's content has been reconciled, rather than letting it persist forever. The original logic was on the right track, just too eager.
3. Use the dropped messages' content-fingerprints as the deletion oracle (snapshot them at truncate/undo time) instead of a timestamp watermark. Then the timestamp watermark can self-clear on the next turn since it no longer carries deletion semantics.

Tests to add

Please add an integration-level test that drives a real turn through the streaming path (_run_agent_streaming or the equivalent post-result merge) after an Edit, and asserts the next turn's context_messages contains the new user+assistant pair (not just the unit tests on _clamp_context_to_watermark in isolation). That's the case the current test suite doesn't cover and where the regression hides.

Really appreciate the depth here — the #2914 analysis is solid and the fingerprint filter is the right instinct. It's specifically the permanent-watermark clamp that needs rework before this is safe. Re-request review once the new-turn-survival path is covered and green.

[nesquena-hermes]

Pre-release regression gate caught a brick-class issue — holding

I picked this up to advance toward release (your v2 had my LGTM), cherry-picked the tip onto fresh master, and ran it through our pre-release gate (full test suite + Opus advisor + the Codex regression gate). The full suite passed, but both advisors independently flagged a brick-class regression, which I then reproduced empirically. Holding before it ships.

The regression

_clamp_context_to_watermark() is called on all three streaming-finalize paths (api/streaming.py:5292, 5440, 6348) and drops every message with timestamp > truncation_watermark. Combined with this PR removing _maybe_clear_truncation_watermark() from save() (the function that previously auto-cleared the watermark once newer messages arrived), nothing ever advances or clears the watermark after Edit/Retry/Undo sets it.

Result: the watermark becomes a permanent ceiling. Every turn after an Edit/Retry/Undo has its own new messages clamped out of context_messages — so the agent permanently loses memory of everything after the edit point. Symptom: do an Edit, ask a question (answers fine), then ask "what was my previous question?" — the agent only sees the pre-Edit conversation.

Why CI was green

The PR's regression test (test_edit_then_new_turn_then_undo_leaks_original_via_state_db) sets session.context_messages directly with the new turn already inlined — it never exercises the _clamp_context_to_watermark finalize path, so it passes while the real streaming flow breaks.

Empirical repro (clean, from a fresh stage worktree)

from api.streaming import _clamp_context_to_watermark
class S: session_id="post-edit"; truncation_watermark=101.0
result = [
    {"role":"user","content":"kept-q","timestamp":100.0},
    {"role":"assistant","content":"kept-a","timestamp":101.0},
    {"role":"user","content":"EDITED-q","timestamp":200.0},   # new turn after edit
    {"role":"assistant","content":"EDITED-a","timestamp":201.0},
]
_clamp_context_to_watermark(S(), result)
# → keeps only ['kept-q','kept-a'] — the new turn is dropped from agent context

Suggested fix (smallest, safest)

Drop the streaming clamp entirely and rely on the merge-side watermark filter in merge_session_messages_append_only (which already addresses #2914 on the state.db replay path), OR restore the _maybe_clear_truncation_watermark() auto-clear in save() so the watermark is a one-shot fence rather than a permanent ceiling. Keep the routes.py context_messages[:keep] slice and the new sub-watermark filter in merge_session_messages_append_only — those parts are correct.

Please also add a streaming-level integration test: Edit → new turn → second new turn, asserting the second turn sees the first new turn in the agent's context.

Re-applying hold + changes-requested. The diagnosis of #2914 itself is correct and the merge-side parts are good — this is fixable, just not shippable as-is. Thanks for the solid investigation work on the root cause.

[AlexeyDsov]

@nesquena-hermes thanks for the review. I rebased the branch onto the latest master and addressed your comments in the following two commits:

Changes:

• Removed _clamp_context_to_watermark() entirely (definition + 3 call sites in api/streaming.py)
• Kept the correct parts of the original PR: context_messages[:keep] in routes.py, sub-watermark content filter in merge_session_messages_append_only(), and removal of _maybe_clear_truncation_watermark() from save()
• Added tests:
• test_streaming_finalize_preserves_new_turns_after_edit — verifies new turns survive finalize
• test_streaming_finalize_does_not_leak_original_after_edit — verifies old replaced messages don't leak through finalize
• test_edit_does_not_leak_original_message_into_context_via_reconcile — verifies old replaced messages don't leak through reconcile

[nesquena-hermes]

Shipped in v0.51.197 (stage-batch9, via #3349). Thanks @AlexeyDsov — the diagnosis of #2914 was spot-on and the v3 (removing _clamp_context_to_watermark() entirely, timestamp-filtering stale pre-edit rows, and syncing context_messages on /api/session/truncate) is exactly right.

One maintainer fix applied on the way in: because Session.save() no longer auto-clears the watermark, the unconditional timestamp > watermark skip in merge_session_messages_append_only would have become a permanent ceiling — a genuine future state.db-only recovery row (compaction/recovery, missed by the sidecar) would be silently dropped from /api/session forever once the session advanced past the edit. I scoped the skip to only fire while the sidecar hasn't advanced past the watermark, and added 2 revert-verified regression tests (advanced-session recovery row merges; deleted-tail still filtered when not advanced). Co-authored-by attribution preserved.

Full suite 7162 passed, Opus + Codex regression gate both cleared (Codex re-verified the watermark-ceiling fix by execution). Closes #2914. 🎉