fix(memory/sync): make Codex AGENTS.md adapter additive (stop wiping memories) (#1674)

## Description

`sync_export` (in `headroom/memory/sync.py`) hands each adapter only the
**delta** — the memories the agent doesn't already have. It reads the
agent's
current memories, builds `agent_hashes`, and only puts a memory in
`to_export`
if its hash isn't already there:

```python
agent_hashes = {am.content_hash for am in await adapter.read_memories()}
for mem in existing_memories:
    if content_hash in agent_hashes:
        continue          # skip: agent already has it
    to_export.append(...)
exported = await adapter.write_memories(to_export)   # ← delta only
```

The `ClaudeCodeAdapter` is additive (a file per memory + index append),
so a
delta is correct for it. But `CodexAdapter.write_memories` rebuilt its
**entire**
`<!-- headroom:memory --> … <!-- /… -->` section from just the passed
delta and
spliced it back with `_MARKER_PATTERN.sub`. So every export
**overwrote** the
section with only the new items.

Concrete thrash:
- DB has A, B → first sync exports `[A, B]` → section = A, B 
- Add C → next sync's delta is `[C]` → section becomes **just C** (A, B
erased)
- Now the agent only has C → next sync's delta is `[A, B]` → section
becomes
  **A, B** (C erased) …

The file bounces between disjoint subsets and never holds the full set —
silent
memory loss on every sync.

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

## Fix

Make `CodexAdapter.write_memories` additive, matching the adapter
contract the
ClaudeCode adapter already follows: read the facts already in the
managed
section, merge the incoming delta into them (dedup by rendered
first-line), and
write the union. Return the count actually added. The function-based
`re.sub`
is kept so literal backslashes / `\u` in a memory aren't treated as
regex
escapes.

## Type of Change

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

## Changes Made

- `headroom/memory/sync_adapters/codex_agent.py`: `write_memories` now
merges the delta into the existing section instead of replacing the
whole section.
- `tests/test_memory_sync.py`: **two existing tests asserted the old
replace-the-whole-section behavior — i.e. they codified this bug.**
Updated them to the additive semantics (an existing managed fact is
preserved) and added `test_write_accumulates_across_syncs` covering the
delta-export-across-syncs scenario.
- `CHANGELOG.md`: Bug Fixes entry under Unreleased.

## Testing

- [x] New regression test added; two behavior-codifying tests corrected
- [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/codex_agent.py tests/test_memory_sync.py
All checks passed!
```

## Real Behavior Proof

- Environment: Windows 11, Python 3.12.11, headroom from this branch.
Importing `headroom` loads the torch/transformers stack and a full
`pytest` gets OOM-killed on this box, so I verified the merge logic with
a dependency-free script (only stdlib) and left the full pytest to CI.
- Exact command / steps: replicated `write_memories` (read existing
section bullets → merge delta → splice) against real temp files, then
ran the multi-sync scenario: export `[A, B]`, then export the delta
`[C]`, then re-export an existing fact; plus a literal-backslash memory
and a no-marker file.
- Observed result: after the delta export of C, A and B are still
present (no wipe); re-exporting an existing fact adds nothing;
backslashes land literally; a file with no marker keeps its surrounding
content:

```text
OK: A,B preserved after delta-export of C (no wipe)
OK: re-writing existing fact -> added 0, others intact
OK: literal backslashes preserved
OK: no-marker file -> section appended, existing preserved
CODEX MERGE LOGIC VERIFIED
```

- Not tested: a full DB→adapter `sync_export` run end-to-end (needs a
memory backend/embedder = the heavy stack); the delta contract is
confirmed by reading `sync.py`, and the adapter merge is covered by the
unit tests. 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

- The most reviewer-sensitive part is that I changed two existing tests.
They were asserting `"old fact" not in content` after a write — i.e.
they locked in the replace-the-whole-section behavior that causes the
wipe. Given `sync_export` only ever passes the delta, that behavior is
the bug; the updated tests assert the fact is preserved. Happy to
discuss if you'd rather fix this on the `sync_export` side instead (e.g.
pass the full set to replace-style adapters), but making the adapter
additive matches the existing ClaudeCode adapter and keeps the contract
uniform.
- @JerrettDavis tagging you — flagging the test change up front so it's
not a surprise in the diff.

---------

Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
This commit is contained in:
Abhay Singh 2026-07-09 23:13:01 +05:30 committed by GitHub
parent bfe4b4d21f
commit 7fd0c42ced
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 74 additions and 20 deletions

View file

@ -48,6 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* **wrap/codex:** `headroom unwrap codex` now removes the Headroom rtk instruction block from the Codex global `AGENTS.md`. `wrap codex` injects it there, but unwrap only restored `config.toml` and MCP state, so a plain `codex` launch kept following the "prefix shell commands with `rtk`" guidance and failed once the managed rtk binary was off PATH. Unwrap now strips the marker-fenced block (preserving the rest of the file), mirroring `unwrap copilot` ([#1421](https://github.com/headroomlabs-ai/headroom/issues/1421)).
* **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.
* **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:** 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.
* **learn (verbosity):** `--verbosity --apply --all` now aggregates the savings baseline across every project instead of overwriting it per project (last-project-wins), which previously left the output shaper with a tiny, unrepresentative baseline. The applied verbosity level comes from the project with the most samples ([#1288](https://github.com/headroomlabs-ai/headroom/pull/1288)).

View file

@ -68,32 +68,61 @@ class CodexAdapter(AgentMemoryAdapter):
return memories
async def write_memories(self, memories: list[dict[str, Any]]) -> int:
"""Write memories into the headroom section of AGENTS.md."""
"""Merge memories into the headroom section of AGENTS.md.
``sync_export`` hands this adapter only the *delta* memories the
agent doesn't already have (see ``AgentMemoryAdapter`` contract; the
sibling ClaudeCode adapter is additive for the same reason). So this
must accumulate: rebuilding the section from just ``memories`` would
erase every previously-synced fact on each run, thrashing the file
between disjoint subsets and never converging.
"""
if not memories:
return 0
# Build section content
lines = ["## Headroom Shared Memory", ""]
for mem in memories:
content = mem["content"].split("\n")[0].strip() # First line only
lines.append(f"- {content}")
lines.append("")
existing_content = self._path.read_text(encoding="utf-8") if self._path.exists() else ""
# Facts already in the managed section — preserve them (dedup by the
# rendered first-line, matching how read_memories reconstructs them).
facts: list[str] = []
seen: set[str] = set()
existing_match = _MARKER_PATTERN.search(existing_content)
if existing_match:
for line in existing_match.group(1).split("\n"):
stripped = line.strip()
if stripped.startswith("- "):
fact = stripped[2:].strip()
if fact and fact not in seen:
seen.add(fact)
facts.append(fact)
added = 0
for mem in memories:
fact = mem["content"].split("\n")[0].strip() # First line only
if fact and fact not in seen:
seen.add(fact)
facts.append(fact)
added += 1
lines = ["## Headroom Shared Memory", ""]
lines.extend(f"- {fact}" for fact in facts)
lines.append("")
section = f"{_MARKER_START}\n" + "\n".join(lines) + f"{_MARKER_END}"
# Merge into AGENTS.md
if self._path.exists():
content = self._path.read_text(encoding="utf-8")
if _MARKER_START in content:
content = _MARKER_PATTERN.sub(lambda _match: section, content)
# Splice the section back in. Use a function replacement (not a string
# template) so literal backslashes / \\u in a memory aren't treated as
# regex escapes.
if existing_content:
if _MARKER_START in existing_content:
content = _MARKER_PATTERN.sub(lambda _match: section, existing_content)
else:
content = content.rstrip() + "\n\n" + section + "\n"
content = existing_content.rstrip() + "\n\n" + section + "\n"
else:
self._path.parent.mkdir(parents=True, exist_ok=True)
content = section + "\n"
self._path.write_text(content, encoding="utf-8")
return len(memories)
return added
def fingerprint(self) -> str:
"""Hash of AGENTS.md contents."""

View file

@ -536,11 +536,14 @@ class TestCodexAdapter:
assert "Existing instructions" in content # Preserved
@pytest.mark.asyncio
async def test_write_replaces_existing_section(self, agents_md):
async def test_write_merges_into_existing_section(self, agents_md):
"""Additive: an existing managed fact is preserved when a new one is
written. ``sync_export`` hands the adapter only the delta, so a
replace-the-whole-section write would erase prior memories."""
agents_md.write_text(
"# Instructions\n\n"
"<!-- headroom:memory:start -->\n"
"## Old\n- old fact\n"
"## Headroom Shared Memory\n\n- old fact\n"
"<!-- headroom:memory:end -->\n"
)
@ -549,14 +552,14 @@ class TestCodexAdapter:
content = agents_md.read_text()
assert "new fact" in content
assert "old fact" not in content
assert "old fact" in content # preserved, not clobbered
@pytest.mark.asyncio
async def test_write_replaces_existing_section_with_literal_backslashes(self, agents_md):
async def test_write_preserves_existing_fact_with_literal_backslashes(self, agents_md):
agents_md.write_text(
"# Instructions\n\n"
"<!-- headroom:memory:start -->\n"
"## Old\n- old fact\n"
"## Headroom Shared Memory\n\n- old fact\n"
"<!-- headroom:memory:end -->\n"
)
@ -564,9 +567,30 @@ class TestCodexAdapter:
await adapter.write_memories([{"content": r"Use C:\Users\john.doe\repo and literal \u"}])
content = agents_md.read_text()
# Backslashes / \u land literally (function replacement, not a template).
assert r"C:\Users\john.doe\repo" in content
assert r"literal \u" in content
assert "old fact" not in content
assert "old fact" in content # preserved
@pytest.mark.asyncio
async def test_write_accumulates_across_syncs(self, agents_md):
"""Regression: exporting deltas across successive syncs must accumulate,
not thrash between disjoint subsets."""
adapter = CodexAdapter(agents_md)
await adapter.write_memories([{"content": "fact A"}, {"content": "fact B"}])
# Second sync only sees the new memory as a delta.
added = await adapter.write_memories([{"content": "fact C"}])
content = agents_md.read_text()
assert "fact A" in content
assert "fact B" in content
assert "fact C" in content
assert added == 1
# Re-writing an already-present fact adds nothing and keeps the rest.
again = await adapter.write_memories([{"content": "fact A"}])
assert again == 0
assert (await adapter.read_memories()).__len__() == 3
@pytest.mark.asyncio
async def test_read_empty_agents_md(self, agents_md):