fix(memory/sync): don't clobber memories sharing a first line (#1976)

## Description

`ClaudeCodeAdapter.write_memories`
(`headroom/memory/sync_adapters/claude_code.py`) picks
each memory's file name from the **first line of its content only**:

```python
first_line = content.split("\n")[0][:60].strip()
slug = _sanitize_for_filename(first_line)
filename = f"headroom_{slug}.md"
```

So two *distinct* DB memories whose first lines slugify to the same
value map to the same
file. The existing "already on disk?" guard only skips when the on-disk
content hash equals
this memory's hash — for a genuine collision (same slug, different body)
it falls through and
`target.write_text(...)` overwrites the other memory. Data loss.

It also never converges. The overwritten memory never lands on disk, so
on the next
`sync_export` the adapter reads back the agent's files, doesn't find
that memory's hash in
`agent_hashes`, and re-exports it — overwriting the other one this time.
The pair ping-pongs
on every sync, and each round appends a fresh line to `MEMORY.md`.

This is realistic for headed/structured memories (e.g. several entries
that begin
`# Project conventions` or `The user prefers …`).

Closes: no issue filed — found while auditing the memory sync adapters.

## Fix

When the slug is already taken by a **different** memory (a distinct
`headroom_id` in the
existing file's frontmatter), disambiguate the file name with a short
content-hash suffix so
both survive. A matching `headroom_id` means it's an update of the same
memory, so the plain
slug file is rewritten as before — existing file names don't change, so
there's no migration
churn for the common (no-collision) case:

```python
existing_id = existing_fm.get("headroom_id", "")
if headroom_id and existing_id and existing_id != headroom_id:
    suffix = (content_hash or hashlib.sha256(content.encode()).hexdigest()[:16])[:8]
    filename = f"headroom_{slug}_{suffix}.md"
    ...
```

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)

## Changes Made

- `headroom/memory/sync_adapters/claude_code.py`: in `write_memories`,
disambiguate the file name with a content-hash suffix when the slug
already belongs to a different `headroom_id`; same-id updates still
rewrite the slug file in place.
- `tests/test_memory_sync.py`: add
`test_write_distinct_memories_sharing_first_line_do_not_clobber` (two
files survive) and `test_write_same_memory_updates_in_place` (no
duplicate on update).

## Testing

- [x] New regression tests added (`tests/test_memory_sync.py`)
- [x] Linting passes (`ruff check`) and formatting is clean (`ruff
format --check`)
- [ ] Full `pytest` deferred to CI (local-OOM reason below).

```text
$ uv run ruff check headroom/memory/sync_adapters/claude_code.py tests/test_memory_sync.py
All checks passed!
```

## Real Behavior Proof

- Environment: Windows 11, Python 3.10, headroom from this branch.
Importing `headroom` pulls in the torch/transformers stack and a full
`pytest` gets OOM-killed on this box, so I verified the write logic with
a dependency-free script (replicating `_sanitize_for_filename` /
`_parse_frontmatter` / the write loop against a real temp dir) and left
the full pytest to CI.
- Exact command / steps: wrote two memories that share the first line `#
Project conventions` but differ in body (distinct `headroom_id`),
through both the old and new logic, then wrote a same-id update.
- Observed result: the old logic reports `written=2` but leaves **one**
file (the first memory's body is gone); the new logic keeps both, and a
same-id update rewrites in place instead of duplicating:

```text
OLD: written=2 files=1 tabs=False fridays=True
NEW: written=2 files=2 tabs=True fridays=True
UPDATE: files=1 second=True
MEMORY COLLISION FIX VERIFIED
```

- Not tested: a full `sync_export`/`sync_import` round-trip through the
DB backend (needs the heavy stack). The fix is confined to the
file-naming decision in `write_memories`, and the new tests drive that
method directly. Full local `pytest` deferred to CI (OOM, per above).

## 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
- [ ] New and existing unit tests pass locally with my changes — ran
lint + a standalone logic check; full pytest deferred to CI (local OOM,
disclosed above)
- [x] I have updated the CHANGELOG.md if applicable

## Additional Notes

- No new dependencies; a small, migration-safe naming guard plus tests.
- @JerrettDavis tagging you — this one is a quiet data-loss path in the
Claude memory sync (a collision drops one memory and then thrashes on
every sync), so it may be worth a look when you get a chance.
This commit is contained in:
Abhay Singh 2026-07-10 21:17:31 +05:30 committed by GitHub
parent 4cb33cd9e3
commit 5e14b8c0f2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 69 additions and 1 deletions

View file

@ -38,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* **proxy/auth:** classify real Anthropic OAuth tokens correctly. `classify_auth_mode` matched OAuth on the `sk-ant-oat-` prefix, but real access tokens are `sk-ant-oat01-...` (a version number, no dash after `oat`), so every real subscription/OAuth token fell through to the `sk-` branch and was tagged `PAYG` — enabling aggressive lossy compression, auto `cache_control`, and `prompt_cache_key` injection on subscription-bound requests the classifier is meant to route to the passthrough-prefer path. The prefix is now the dash-less `sk-ant-oat` (still matches the legacy dashed shape). The existing parity tests only passed because they used a synthetic `sk-ant-oat-01-` fixture; a regression test now covers the real `sk-ant-oat01-` format.
* **install:** stop leaking a file descriptor on every `headroom install start`. `start_detached_agent()` opened the agent log file and handed it to `subprocess.Popen` but never closed the parent's copy, so each call leaked one fd (and pinned the log file open against rotation). The parent now closes its copy in a `try/finally` once the child has inherited it — the close also runs if `Popen` raises ([#1554](https://github.com/headroomlabs-ai/headroom/issues/1554)).
* **memory/sync:** stop the Codex AGENTS.md sync adapter from erasing previously-synced memories on every export. `sync_export` hands each adapter only the *delta* (memories the agent lacks), but `CodexAdapter.write_memories` rebuilt its whole managed section from just that delta — so each sync overwrote the section with only the new items, thrashing the file between disjoint subsets and never accumulating. It now merges the delta into the facts already present (deduped), matching the additive contract the ClaudeCode adapter already follows.
* **memory/sync:** stop the Claude Code sync adapter from clobbering distinct memories that share a first line. `write_memories` derived each file name from the first line of the content only (`headroom_{slug}.md`), so two different DB memories whose first lines slugify identically wrote to the same file and the second silently overwrote the first — and because the loser never landed on disk, the next sync re-exported it, ping-ponging the pair forever. When the slug is already taken by a *different* memory (distinct `headroom_id`) the file name is now disambiguated with a content-hash suffix; an update to the same memory still rewrites its slug file in place, so existing file names are unchanged.
* **cli/proxy:** honor `HEADROOM_MIN_TOKENS=0` / `HEADROOM_MAX_ITEMS=0`. The Click `proxy` command built these with `_get_env_int_optional(name) or 500`/`or 50`, so an explicit `0` — a legitimate value (`min_tokens_to_crush=0` means "crush every item") — was treated as falsy and silently replaced with the default. The `headroom proxy` argparse path already preserved `0` via `_get_env_int`, so the two entry points disagreed. The Click path now uses the same None-checking helper.
* **proxy:** strip the inbound `Content-Encoding`/`Transfer-Encoding` request headers on the Anthropic `/v1/messages` and OpenAI `/v1/chat/completions` paths before forwarding upstream. `read_request_json_with_bytes` already decompresses the inbound body (zstd/gzip/deflate/br), so the bytes forwarded upstream are plain JSON — but these two handlers left the original `content-encoding` header in place, so a client (or an edge proxy like a Cloudflare Worker) that sent a compressed body got its request rejected with upstream HTTP 400 because the provider tried to decompress already-decoded JSON. The `/v1/responses` handler already carried this fix (#1542); it is now applied to the messages and chat paths too.
* **proxy:** include the system prompt, tools, and the response-shaping request fields in the SemanticCache key. `_compute_key` hashed only `{model, messages}`, so two non-streaming requests with identical messages but a different top-level `system` prompt, tool set, sampling config, or output-shaping field collided on one key and the second caller was served the first's cached response — generated under different request semantics, in the default config (`cache_enabled` defaults on). The key now folds the request fields that shape generation — `temperature`/`top_p`/`top_k`/`max_tokens`/`stop`, plus OpenAI `tool_choice`/`response_format`/`parallel_tool_calls`/`seed`/`presence_penalty`/`frequency_penalty`/`logit_bias`/`n`/`logprobs`/`top_logprobs`/`reasoning_effort`/`verbosity`/`modalities` and Anthropic `thinking`/`tool_choice`/`output_config` — canonicalizing `system`/`tools` so a moved `cache_control` breakpoint does not fragment it, and the handlers snapshot the fields once at the cache read and reuse them at write so a body mutated by the pipeline cannot diverge the key. Non-streaming path only.

View file

@ -147,7 +147,12 @@ class ClaudeCodeAdapter(AgentMemoryAdapter):
source_agent = mem.get("source_agent", "unknown")
content_hash = mem.get("content_hash", "")
# Generate filename from content
# Generate filename from content. The slug is derived from the first
# line only, so two distinct memories that share a first line map to
# the same file — without the collision guard below the second would
# silently overwrite the first (data loss), and because the loser
# never lands on disk the next sync re-exports it, ping-ponging
# forever.
first_line = content.split("\n")[0][:60].strip()
slug = _sanitize_for_filename(first_line)
filename = f"headroom_{slug}.md"
@ -159,6 +164,21 @@ class ClaudeCodeAdapter(AgentMemoryAdapter):
existing_hash = hashlib.sha256(existing_body.strip().encode()).hexdigest()[:16]
if existing_hash == content_hash:
continue
# Same slug, different content. If the file on disk belongs to a
# *different* memory (distinct headroom_id), disambiguate with a
# content-hash suffix so we don't clobber it. A matching id is an
# update of the same memory, so the plain slug is rewritten as
# before (keeps existing filenames stable — no migration churn).
existing_id = existing_fm.get("headroom_id", "")
if headroom_id and existing_id and existing_id != headroom_id:
suffix = (content_hash or hashlib.sha256(content.encode()).hexdigest()[:16])[:8]
filename = f"headroom_{slug}_{suffix}.md"
target = self._memory_dir / filename
if target.exists():
_, dis_body = _parse_frontmatter(target.read_text(encoding="utf-8"))
dis_hash = hashlib.sha256(dis_body.strip().encode()).hexdigest()[:16]
if dis_hash == content_hash:
continue
# Build description (first 100 chars)
description = content.replace("\n", " ")[:100]

View file

@ -462,6 +462,53 @@ class TestClaudeCodeAdapter:
assert fm["source_agent"] == "codex"
assert "FastAPI" in body
@pytest.mark.asyncio
async def test_write_distinct_memories_sharing_first_line_do_not_clobber(self, memory_dir):
"""Two different memories that share a first line must not overwrite one
another. The filename slug is derived from the first line only, so before
the fix the second write clobbered the first (data loss)."""
adapter = ClaudeCodeAdapter(memory_dir)
written = await adapter.write_memories(
[
{
"content": "# Project conventions\nUse tabs for indentation.",
"headroom_id": "mem_a",
"content_hash": "hash_a",
},
{
"content": "# Project conventions\nDeploy on Fridays only.",
"headroom_id": "mem_b",
"content_hash": "hash_b",
},
]
)
assert written == 2
files = sorted(memory_dir.glob("headroom_*.md"))
# Both memories must survive on disk (distinct files).
assert len(files) == 2
bodies = "\n".join(f.read_text() for f in files)
assert "tabs for indentation" in bodies
assert "Deploy on Fridays only" in bodies
@pytest.mark.asyncio
async def test_write_same_memory_updates_in_place(self, memory_dir):
"""An update to the *same* memory (matching headroom_id) rewrites the
original slug file rather than spawning a disambiguated duplicate."""
adapter = ClaudeCodeAdapter(memory_dir)
await adapter.write_memories(
[{"content": "# Note\nfirst version", "headroom_id": "mem_x", "content_hash": "h1"}]
)
await adapter.write_memories(
[{"content": "# Note\nsecond version", "headroom_id": "mem_x", "content_hash": "h2"}]
)
files = list(memory_dir.glob("headroom_*.md"))
assert len(files) == 1
assert "second version" in files[0].read_text()
def test_fingerprint_changes_on_modification(self, memory_dir):
(memory_dir / "test.md").write_text("content 1")