fix: protect dream history during compaction

fix: protect dream history during compaction in HKUDS/nanobot.

Open

hamb1ywants to merge intofrom

2 commits5 files changed+54-4

Fixes #4055.

Summary

Make history compaction preserve entries newer than the Dream cursor
Pass the current Dream cursor from scheduled and manual Dream cleanup
Add regression coverage for protected compaction behavior

Verification

uv run pytest \
  tests/agent/test_memory_store.py::TestHistoryWithCursor::test_compact_history_drops_oldest \
  tests/agent/test_memory_store.py::TestHistoryWithCursor::test_compact_history_preserves_entries_after_dream_cursor \
  tests/agent/test_memory_store.py::TestHistoryWithCursor::test_compact_history_fills_cap_with_recent_processed_entries \
  tests/command/test_builtin_dream.py::test_dream_internal_run_silences_progress \
  -q

uv run ruff check \
  nanobot/agent/memory.py \
  nanobot/cli/commands.py \
  nanobot/command/builtin.py \
  tests/agent/test_memory_store.py \
  tests/command/test_builtin_dream.py

Activity

fix: protect dream history during compaction

e0cbf47Jul 2

hamb1y-bot-hkuds-nanobot4mo ago

Review — fix: protect dream history during compaction

What this PR does: Adds protect_after_cursor: int | None = None parameter to MemoryStore.compact_history(). When set, entries with cursor > protect_after_cursor are always retained (even if they exceed max_history_entries). Remaining cap is filled with newest non-protected entries. If protected entries alone exceed cap, all are kept and a warning is logged. Both Dream call sites (on_cron_job in cli/commands.py and the manual Dream command in command/builtin.py) now pass protect_after_cursor=store.get_last_dream_cursor().

What's good

Backwards-compatible signature. protect_after_cursor=None skips the new logic and falls through to the original entries[-self.max_history_entries:] path. Existing callers unaffected.
Algorithm is correct. Protected indices are computed first, then remaining cap is filled backwards from the newest non-protected entries. This preserves "newest entries win" semantics for the non-protected set while guaranteeing protected entries survive.
Warning when protected > cap. logger.warning("History compaction kept {} unprocessed entries beyond cap {}", ...) — operators get visibility when the cap is structurally too small for the unprocessed volume.
Both Dream call sites updated. Scheduled (cli/commands.py) and manual (command/builtin.py) Dream both pass the cursor. No path missed.
Test mock updated to capture compact_history_protect_after_cursor. This verifies the parameter is actually being threaded through, not just that the function accepts it.
Three test cases cover: preserves entries after cursor, fills cap with recent processed entries, and the existing drops_oldest test still passes (backwards compat).

Concerns / questions

_valid_cursor is referenced but not defined in the diff. Presumably pre-existing. Verify it handles cursor being None, a string, or a non-integer gracefully — the walrus assignment (cursor := self._valid_cursor(entry.get("cursor"))) followed by cursor is not None suggests it returns int | None. If it raises on malformed input, the compaction would crash. Worth a defensive test.
remaining can go negative if len(protected_indices) > max_history_entries. The if remaining < 0: branch handles this correctly (skip the fill loop, log warning). But the else branch's fill loop has if remaining <= 0: break — since remaining starts >= 0 in this branch and only decreases, this is safe.
No test for the "protected exceeds cap" warning case. A test with max_history_entries=2 and 5 protected entries would verify the warning fires and all 5 are kept. Currently the warning path is untested.

Verdict

Approve. Clean, well-tested, correctly backwards-compatible. The "protected exceeds cap" test would close the last gap.