fix(memory): reject negative history cursors
This commit is contained in:
@@ -61,7 +61,7 @@ class MemoryStore:
|
|||||||
self.user_file = workspace / "USER.md"
|
self.user_file = workspace / "USER.md"
|
||||||
self._cursor_file = self.memory_dir / ".cursor"
|
self._cursor_file = self.memory_dir / ".cursor"
|
||||||
self._dream_cursor_file = self.memory_dir / ".dream_cursor"
|
self._dream_cursor_file = self.memory_dir / ".dream_cursor"
|
||||||
self._corruption_logged = False # rate-limit non-int cursor warning
|
self._corruption_logged = False # rate-limit invalid cursor warning
|
||||||
self._malformed_entry_logged = False # rate-limit bad history shape warning
|
self._malformed_entry_logged = False # rate-limit bad history shape warning
|
||||||
self._oversize_logged = False # rate-limit oversized-entry warning
|
self._oversize_logged = False # rate-limit oversized-entry warning
|
||||||
self._append_lock = threading.Lock() # serialize cursor allocation + append
|
self._append_lock = threading.Lock() # serialize cursor allocation + append
|
||||||
@@ -291,8 +291,8 @@ class MemoryStore:
|
|||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _valid_cursor(value: Any) -> int | None:
|
def _valid_cursor(value: Any) -> int | None:
|
||||||
"""Int cursors only — reject bool (``isinstance(True, int)`` is True)."""
|
"""Non-negative int cursors only; reject bool (``isinstance(True, int)`` is True)."""
|
||||||
if isinstance(value, bool) or not isinstance(value, int):
|
if isinstance(value, bool) or not isinstance(value, int) or value < 0:
|
||||||
return None
|
return None
|
||||||
return value
|
return value
|
||||||
|
|
||||||
@@ -315,7 +315,7 @@ class MemoryStore:
|
|||||||
if poisoned is not None and not self._corruption_logged:
|
if poisoned is not None and not self._corruption_logged:
|
||||||
self._corruption_logged = True
|
self._corruption_logged = True
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"history.jsonl contains a non-int cursor ({!r}); dropping it. "
|
"history.jsonl contains an invalid cursor ({!r}); dropping it. "
|
||||||
"Usually caused by an external writer; further occurrences suppressed.",
|
"Usually caused by an external writer; further occurrences suppressed.",
|
||||||
poisoned,
|
poisoned,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -104,7 +104,7 @@ class TestNextCursorRecovery:
|
|||||||
|
|
||||||
|
|
||||||
class TestReadUnprocessedWithCorruption:
|
class TestReadUnprocessedWithCorruption:
|
||||||
"""``read_unprocessed_history`` must skip entries with non-int cursors
|
"""``read_unprocessed_history`` must skip entries with invalid cursors
|
||||||
instead of crashing on comparison."""
|
instead of crashing on comparison."""
|
||||||
|
|
||||||
def test_skips_string_cursor_entries(self, store):
|
def test_skips_string_cursor_entries(self, store):
|
||||||
@@ -153,6 +153,7 @@ class TestCursorValidationInvariant:
|
|||||||
"""
|
"""
|
||||||
assert MemoryStore._valid_cursor(True) is None
|
assert MemoryStore._valid_cursor(True) is None
|
||||||
assert MemoryStore._valid_cursor(False) is None
|
assert MemoryStore._valid_cursor(False) is None
|
||||||
|
assert MemoryStore._valid_cursor(-1) is None
|
||||||
assert MemoryStore._valid_cursor(5) == 5
|
assert MemoryStore._valid_cursor(5) == 5
|
||||||
assert MemoryStore._valid_cursor(0) == 0
|
assert MemoryStore._valid_cursor(0) == 0
|
||||||
|
|
||||||
@@ -167,6 +168,18 @@ class TestCursorValidationInvariant:
|
|||||||
entries = store.read_unprocessed_history(since_cursor=0)
|
entries = store.read_unprocessed_history(since_cursor=0)
|
||||||
assert [e["cursor"] for e in entries] == [4, 5]
|
assert [e["cursor"] for e in entries] == [4, 5]
|
||||||
|
|
||||||
|
def test_negative_history_cursor_rejected(self, store):
|
||||||
|
"""A negative history cursor is corrupt and must not seed new writes."""
|
||||||
|
store.history_file.write_text(
|
||||||
|
'{"cursor": -5, "timestamp": "2026-04-01 10:00", "content": "negative"}\n',
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
store._cursor_file.unlink(missing_ok=True)
|
||||||
|
|
||||||
|
assert store.append_history("next") == 1
|
||||||
|
entries = store.read_unprocessed_history(since_cursor=0)
|
||||||
|
assert [e["cursor"] for e in entries] == [1]
|
||||||
|
|
||||||
def test_next_cursor_returns_max_not_just_last_int(self, store):
|
def test_next_cursor_returns_max_not_just_last_int(self, store):
|
||||||
"""Under adversarial corruption, file order ≠ numeric order. The
|
"""Under adversarial corruption, file order ≠ numeric order. The
|
||||||
recovery scan must return ``max(valid cursors) + 1``, not the
|
recovery scan must return ``max(valid cursors) + 1``, not the
|
||||||
@@ -187,7 +200,7 @@ class TestCursorValidationInvariant:
|
|||||||
assert store.append_history("safe next") == 101
|
assert store.append_history("safe next") == 101
|
||||||
|
|
||||||
def test_corruption_is_logged_exactly_once_per_store(self, store, caplog):
|
def test_corruption_is_logged_exactly_once_per_store(self, store, caplog):
|
||||||
"""Observability without spam: the first non-int cursor emits one
|
"""Observability without spam: the first invalid cursor emits one
|
||||||
warning, subsequent reads on the same store stay quiet. Without
|
warning, subsequent reads on the same store stay quiet. Without
|
||||||
this, a poisoned file produces one warning per agent turn."""
|
this, a poisoned file produces one warning per agent turn."""
|
||||||
import logging
|
import logging
|
||||||
@@ -213,7 +226,7 @@ class TestCursorValidationInvariant:
|
|||||||
loguru_logger.remove(handler_id)
|
loguru_logger.remove(handler_id)
|
||||||
|
|
||||||
corruption_warnings = [
|
corruption_warnings = [
|
||||||
r for r in caplog.records if "non-int cursor" in r.getMessage()
|
r for r in caplog.records if "invalid cursor" in r.getMessage()
|
||||||
]
|
]
|
||||||
assert len(corruption_warnings) == 1, (
|
assert len(corruption_warnings) == 1, (
|
||||||
"Expected exactly one corruption warning per store instance; "
|
"Expected exactly one corruption warning per store instance; "
|
||||||
|
|||||||
Reference in New Issue
Block a user