Files
chopratejas--headroom/tests/test_memory_wrapper.py
Abhay Singh e00c6ff81c fix(memory): don't crash inline memory extraction on a non-object <memory> block (#2470)
## Description

`parse_response_with_memory` extracts an inline `<memory>...</memory>`
block from a model response and parses its JSON:

```python
try:
    data = json.loads(memory_json)
    memories = data.get("memories", [])
except json.JSONDecodeError as e:
    logger.warning(f"Failed to parse memory JSON: {e}")
```

The block content is fully model-controlled. `json.loads` succeeds on
any valid JSON, including a non-object such as a bare array
(`<memory>["x"]</memory>`), a string, or a number. `data.get("memories",
[])` then raises `AttributeError: 'list' object has no attribute 'get'`,
which the `except json.JSONDecodeError` does not catch, so the inline
memory path crashes on output a model can realistically produce.

## Fix

Guard the parsed value: read `memories` only when the block is a JSON
object, and accept it only when it is a list (logging and ignoring
otherwise). Malformed JSON is still handled by the existing decode
guard, and a well-formed object is unchanged. This mirrors the
non-object hardening already applied to the batch JSONL path.

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)

## Changes Made

- `headroom/memory/inline_extractor.py`: only read `memories` from a
dict-typed parsed block, and only when the field is a list; log and
ignore other shapes.
- `tests/test_memory_wrapper.py`: regression covering a non-object
memory block, a non-list `memories` field, malformed JSON, and a
well-formed block.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed

### Test Output

```text
$ python -m pytest tests/test_memory_wrapper.py -q
6 passed

# with the fix reverted, the new test fails with
# AttributeError: 'list' object has no attribute 'get'

$ uvx ruff@0.15.17 check headroom/memory/inline_extractor.py tests/test_memory_wrapper.py
All checks passed!
$ uvx mypy@1.20.2 --ignore-missing-imports headroom/memory/inline_extractor.py
Success: no issues found in 1 source file
```

## Real Behavior Proof

- Environment: Windows 11, Python 3.12, project venv (`uv sync --extra
proxy`), `uvx ruff@0.15.17` / `uvx mypy@1.20.2`, pytest in the venv.
- Exact command / steps: called the real `parse_response_with_memory`
with a `<memory>["x"]</memory>` block, a `{"memories": "nope"}` block, a
malformed block, and a valid block; then reverted `inline_extractor.py`
and re-ran.
- Observed result: with the fix all four return cleanly (empty memories
for the bad shapes, the parsed list for the valid one) and the memory
block is still stripped from the content; with the fix reverted the
non-object block raises `AttributeError: 'list' object has no attribute
'get'`. Ran against the actual module.
- Not tested: a live end-to-end chat where a model emits a non-object
memory block.

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] I have updated the CHANGELOG.md if applicable
2026-08-12 00:12:50 -05:00

227 lines
7.8 KiB
Python

from __future__ import annotations
from pathlib import Path
from types import SimpleNamespace
import pytest
from headroom.memory.config import EmbedderBackend
from headroom.memory.wrapper import MemoryWrapper, _MemoryAPI, with_memory
class FakeMemory:
def __init__(self) -> None:
self.search_results: list[object] = []
self.add_calls: list[dict[str, object]] = []
self.query_results: list[object] = []
self.clear_result = 0
async def search(self, **kwargs): # noqa: ANN003
self.last_search = kwargs
return self.search_results
async def add(self, **kwargs): # noqa: ANN003
self.add_calls.append(kwargs)
return SimpleNamespace(id=f"mem-{len(self.add_calls)}", **kwargs)
async def query(self, filter_value): # noqa: ANN001, ANN201
self.last_filter = filter_value
return self.query_results
async def clear_scope(self, **kwargs): # noqa: ANN003
self.last_clear = kwargs
return self.clear_result
def make_client(content: str = "raw response") -> tuple[object, object]:
response = SimpleNamespace(choices=[SimpleNamespace(message=SimpleNamespace(content=content))])
def create(**kwargs): # noqa: ANN003, ANN202
create.kwargs = kwargs
return response
client = SimpleNamespace(chat=SimpleNamespace(completions=SimpleNamespace(create=create)))
return client, response
def test_memory_wrapper_lazy_initialization_and_factory(monkeypatch: pytest.MonkeyPatch) -> None:
client, _response = make_client()
fake_memory = FakeMemory()
seen: dict[str, object] = {}
async def fake_create(config): # noqa: ANN001
seen["config"] = config
return fake_memory
monkeypatch.setattr("headroom.memory.wrapper.HierarchicalMemory.create", fake_create)
wrapper = MemoryWrapper(
client,
user_id="alice",
db_path="memory.db",
top_k=7,
session_id="session-1",
agent_id="agent-1",
embedder_backend=EmbedderBackend.OPENAI,
openai_api_key="sk-test",
)
assert wrapper.chat.completions._wrapper is wrapper
assert wrapper._initialized is False
api = wrapper.memory
assert isinstance(api, _MemoryAPI)
assert wrapper._initialized is True
assert wrapper._memory is fake_memory
assert seen["config"].db_path == Path("memory.db")
assert seen["config"].embedder_backend == EmbedderBackend.OPENAI
assert seen["config"].openai_api_key == "sk-test"
wrapped = with_memory(client, user_id="bob", session_id="s2", agent_id="a2", top_k=3)
assert isinstance(wrapped, MemoryWrapper)
assert wrapped._client is client
assert wrapped._user_id == "bob"
assert wrapped._session_id == "s2"
assert wrapped._agent_id == "a2"
assert wrapped._top_k == 3
def test_inject_memories_handles_empty_and_inserts_context() -> None:
client, _response = make_client()
fake_memory = FakeMemory()
wrapper = MemoryWrapper(client, user_id="alice", _memory=fake_memory)
no_user = [{"role": "assistant", "content": "skip"}]
assert wrapper._inject_memories(no_user) == no_user
messages = [{"role": "user", "content": "Question?"}]
assert wrapper._inject_memories(messages) == messages
fake_memory.search_results = [
SimpleNamespace(memory=SimpleNamespace(content="Prefers Python")),
SimpleNamespace(memory=SimpleNamespace(content="Works on APIs")),
]
original = [
{"role": "system", "content": "System"},
{"role": "user", "content": "Question?"},
{"role": "user", "content": "Follow-up"},
]
injected = wrapper._inject_memories(original)
assert original[1]["content"] == "Question?"
assert injected[1]["content"].startswith(
"<context>\n- Prefers Python\n- Works on APIs\n</context>\n\n"
)
assert injected[2]["content"] == "Follow-up"
assert fake_memory.last_search == {
"query": "Follow-up",
"user_id": "alice",
"session_id": None,
"top_k": 5,
}
def test_store_memories_persists_only_nonempty_content() -> None:
client, _response = make_client()
fake_memory = FakeMemory()
wrapper = MemoryWrapper(
client,
user_id="alice",
session_id="session-1",
agent_id="agent-1",
_memory=fake_memory,
)
wrapper._store_memories([{"content": "Remember this"}, {"content": ""}, {}])
assert fake_memory.add_calls == [
{
"content": "Remember this",
"user_id": "alice",
"session_id": "session-1",
"agent_id": "agent-1",
"importance": 0.7,
}
]
def test_wrapped_completions_create_injects_parses_and_stores(
monkeypatch: pytest.MonkeyPatch,
) -> None:
client, response = make_client("raw completion")
wrapper = MemoryWrapper(client, user_id="alice", _memory=FakeMemory())
stored: list[list[dict[str, str]]] = []
monkeypatch.setattr(
wrapper,
"_inject_memories",
lambda messages: [{"role": "user", "content": "enhanced"}],
)
monkeypatch.setattr(
"headroom.memory.wrapper.inject_memory_instruction",
lambda messages, short=True: (
messages + [{"role": "system", "content": "memory-instruction"}]
),
)
monkeypatch.setattr(
"headroom.memory.wrapper.parse_response_with_memory",
lambda content: SimpleNamespace(
content="clean response",
memories=[{"content": "saved memory"}],
),
)
monkeypatch.setattr(wrapper, "_store_memories", lambda memories: stored.append(memories))
result = wrapper.chat.completions.create(
messages=[{"role": "user", "content": "hello"}], model="x"
)
assert result is response
assert response.choices[0].message.content == "clean response"
assert client.chat.completions.create.kwargs["messages"] == [
{"role": "user", "content": "enhanced"},
{"role": "system", "content": "memory-instruction"},
]
assert stored == [[{"content": "saved memory"}]]
def test_memory_api_methods_delegate_to_underlying_memory() -> None:
fake_memory = FakeMemory()
memory_one = SimpleNamespace(id="m1", content="alpha")
memory_two = SimpleNamespace(id="m2", content="beta")
fake_memory.search_results = [
SimpleNamespace(memory=memory_one),
SimpleNamespace(memory=memory_two),
]
fake_memory.query_results = [memory_one, memory_two]
fake_memory.clear_result = 2
api = _MemoryAPI(fake_memory, user_id="alice", session_id="session-1", agent_id="agent-1")
assert api.search("alpha", top_k=3) == [memory_one, memory_two]
added = api.add("new memory", importance=0.9)
assert added.content == "new memory"
assert api.get_all() == [memory_one, memory_two]
assert api.clear() == 2
assert api.stats() == {"total": 2}
assert fake_memory.last_clear == {"user_id": "alice"}
def test_parse_response_with_memory_tolerates_non_object_memory_block() -> None:
from headroom.memory.inline_extractor import parse_response_with_memory
# The model controls the <memory> block. A valid-JSON non-object (a bare
# array) or a non-list `memories` field must not crash the parse: the
# `json.loads` succeeds, so the JSONDecodeError guard does not apply, and
# `.get`/iteration on the wrong type would otherwise raise.
assert parse_response_with_memory('hi <memory>["x"]</memory> bye').memories == []
assert parse_response_with_memory('<memory>{"memories": "nope"}</memory>').memories == []
# Malformed JSON is still handled, and a well-formed block still parses.
assert parse_response_with_memory("<memory>{not json</memory>").memories == []
parsed = parse_response_with_memory(
'text <memory>{"memories": [{"content": "User likes Python"}]}</memory>'
)
assert parsed.memories == [{"content": "User likes Python"}]
assert parsed.content == "text"