From 7fd0c42ced9ecdf2a5411ff85d554b9e39ceb0b6 Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Thu, 9 Jul 2026 23:13:01 +0530 Subject: [PATCH] fix(memory/sync): make Codex AGENTS.md adapter additive (stop wiping memories) (#1674) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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** ` … ` 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 --- CHANGELOG.md | 1 + headroom/memory/sync_adapters/codex_agent.py | 57 +++++++++++++++----- tests/test_memory_sync.py | 36 ++++++++++--- 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10252f14f..9d0368fb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)). diff --git a/headroom/memory/sync_adapters/codex_agent.py b/headroom/memory/sync_adapters/codex_agent.py index 993ebf5a1..01f9da07a 100644 --- a/headroom/memory/sync_adapters/codex_agent.py +++ b/headroom/memory/sync_adapters/codex_agent.py @@ -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.""" diff --git a/tests/test_memory_sync.py b/tests/test_memory_sync.py index 3ff0f0346..26802f4c8 100644 --- a/tests/test_memory_sync.py +++ b/tests/test_memory_sync.py @@ -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" "\n" - "## Old\n- old fact\n" + "## Headroom Shared Memory\n\n- old fact\n" "\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" "\n" - "## Old\n- old fact\n" + "## Headroom Shared Memory\n\n- old fact\n" "\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):