From a5b7aac7c0940bbdb983880cc9b77ace4b0a712a Mon Sep 17 00:00:00 2001 From: faj-design5260 Date: Thu, 10 Sep 2026 01:30:41 +0800 Subject: [PATCH 1/4] feat: archive invalid memories and refine management operations --- README.md | 10 +- docs/TODO.md | 12 + docs/design/index.md | 3 + docs/design/memory-lifecycle.md | 25 ++ docs/plans/memory-archive-management.md | 12 + docs/plans/memory-management-audit.md | 168 ++++++++++++ packages/cli/src/agent_memory/cli/main.py | 17 +- .../core/src/agent_memory/core/archive.py | 18 ++ .../core/src/agent_memory/core/context.py | 11 +- .../core/src/agent_memory/core/indexer.py | 4 +- .../core/src/agent_memory/core/injection.py | 5 +- packages/core/src/agent_memory/core/manage.py | 2 +- packages/core/src/agent_memory/core/paths.py | 14 +- .../core/src/agent_memory/core/prompts.py | 11 +- packages/core/src/agent_memory/core/recall.py | 17 +- packages/core/src/agent_memory/core/store.py | 160 ++++++++---- packages/mcp/src/agent_memory/mcp/tools.py | 22 +- skills/agent-memory/SKILL.md | 11 +- tests/system/test_archive_entries.py | 44 ++++ tests/unit/test_indexer.py | 8 +- tests/unit/test_memory_archive.py | 246 ++++++++++++++++++ tests/unit/test_storage.py | 3 +- 22 files changed, 752 insertions(+), 71 deletions(-) create mode 100644 docs/TODO.md create mode 100644 docs/design/index.md create mode 100644 docs/design/memory-lifecycle.md create mode 100644 docs/plans/memory-archive-management.md create mode 100644 docs/plans/memory-management-audit.md create mode 100644 tests/system/test_archive_entries.py create mode 100644 tests/unit/test_memory_archive.py diff --git a/README.md b/README.md index ff2f6575..d6463dbc 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,8 @@ $AGENT_MEMORY_STORE/ ├── schemas/ one file per type: its key fields, the field it groups by, write mode ├── decision/ memories live at //.md, placed by the schema │ └── agent-memory/ …/markdown-files-are-the-single-source-of-truth.md -├── archive/ append-only, out of the retrieval surface by default +├── archive/ evidence and invalid memory history +│ ├── memories/ invalid memories, preserving their original relative paths │ ├── provenance/ distillation evidence, kept forever │ └── sessions/ full trace copies, in case the host prunes its own ├── dream-reports/ one per sleep: what moved, what was proposed, evidence pointers @@ -95,6 +96,13 @@ and recall all operate on whole files, and a file is either active or invalid wi between. Frontmatter carries the stable name, a one-sentence abstract, the type and its schema fields, status, timestamps, links, weight, and provenance; the body is free markdown. +Invalidation moves a memory into `archive/memories/` and retains its raw evidence. Normal +Recall, Read, Trace and Context return active memories; `read --history`, `trace --history` +and temporal `recall --as-of` / `context --as-of` explicitly access history. Deep search can +still return raw evidence separately. New links must name another active memory in the +same store; `correct --link` replaces the full list. See the [operation audit and recovery +notes](docs/plans/memory-management-audit.md) for the boundaries and remaining policy choices. + ## Proof it works Measured on LongMemEval-S with a bounded haystack, 120 episodes, `claude -p` (Haiku 4.5) as diff --git a/docs/TODO.md b/docs/TODO.md new file mode 100644 index 00000000..fbcd0404 --- /dev/null +++ b/docs/TODO.md @@ -0,0 +1,12 @@ +# Follow-ups + +- Decide explicit confirmation and host-level authorization for permanent GC, import and + cross-store changes; CLI labels alone do not identify a human. +- Decide preimage retention and recovery guarantees for split and in-place correction, + including stores outside Git and multi-file Manage failures. +- Decide whether direct link/unlink delta commands, relation audit history and per-operation + bounds are needed; current correct replaces the complete list. +- Decide if explicit deep Raw search should suppress evidence cited exclusively by invalid + memories; shared evidence and historical queries must remain available. +- Correct explicit feedback persistence: Store.feedback currently returns an adjusted object + without writing the weight back to disk. diff --git a/docs/design/index.md b/docs/design/index.md new file mode 100644 index 00000000..b07f07da --- /dev/null +++ b/docs/design/index.md @@ -0,0 +1,3 @@ +# Design + +- [Memory lifecycle and management](memory-lifecycle.md) diff --git a/docs/design/memory-lifecycle.md b/docs/design/memory-lifecycle.md new file mode 100644 index 00000000..ffa80525 --- /dev/null +++ b/docs/design/memory-lifecycle.md @@ -0,0 +1,25 @@ +# Memory lifecycle and management + +Memory has two states: active and invalid. Supersession invalidates a predecessor with a +successor reference; deletion invalidates without one. Archive is a physical location, +not a third state. Invalid memories retain content, identity, validity dates, links and +provenance inside the existing Archive, separate from append-only raw evidence. + +Store persists invalidity before moving a file. Failed moves leave invalid truth available +for retry; failed projections leave file truth authoritative. Retrying deletion completes +archival and projection without changing the original invalidation time or successor. +Normal reads and recall check current truth; explicit history reads and temporal recall +retain history. Rebuild includes archived history. Existing invalid files remain readable +as history without an automatic bulk migration. Raw evidence remains independently available. + +Relationship additions require distinct active endpoints in the same store. Existing links +may remain as historical references. Correct replaces the complete link list, so removal +requires supplying the intended remaining list. Manage's automatic linking remains enabled. +Corrections require active memories and active successors. These are data validations, +not identity authorization or a scope permission system. + +Manage retains deterministic maintenance and capped executor decisions over existing +proposals. Reports and the decision ledger record outcomes; Git recovery depends on a +successful commit. Split and in-place corrections do not unconditionally preserve previous +content. Permanent collection remains outside the Manage and MCP menus; its human-only +label is not enforced authentication. Deployment owners must control shell/file access. diff --git a/docs/plans/memory-archive-management.md b/docs/plans/memory-archive-management.md new file mode 100644 index 00000000..2f9e01c4 --- /dev/null +++ b/docs/plans/memory-archive-management.md @@ -0,0 +1,12 @@ +# Memory archive and management + +Baseline: `34d12a2f8678d5561aba27bd8ff73c5ae4b6a258` (origin/main). +Branch: `feat/memory-archive-management`; isolated worktree, no experimental dependencies. + +1. Document lifecycle and management boundaries. +2. Add failing lifecycle, failure-recovery and adapter regression tests. +3. Extend the existing Archive and shared persistence path; isolate normal reads. +4. Validate relationship additions and active correction endpoints without a new permission model. +5. Run related tests, full CI checks and a temporary-store CLI scenario; review and commit. + +No real store migration, cleanup, deployment or merge is authorized by this plan. diff --git a/docs/plans/memory-management-audit.md b/docs/plans/memory-management-audit.md new file mode 100644 index 00000000..350cd525 --- /dev/null +++ b/docs/plans/memory-management-audit.md @@ -0,0 +1,168 @@ +# Memory archive and Manage audit + +Baseline: `34d12a2f8678d5561aba27bd8ff73c5ae4b6a258` from freshly fetched `origin/main`. +The branch starts from main, not the Progressive Read, Raw evidence or Optional Index branches. +The original dirty worktree was left intact. Paths below are relative to this repository; +`core/` abbreviates `packages/core/src/agent_memory/core/`. + +## Code facts at baseline + +- `core/record.py::STATUSES`, `MemoryRecord.from_text`, `validate`, `invalidate`: only + `active` and `invalid`. A missing status defaults to active; missing provenance defaults + to an empty list. Invalid requires `invalid_at`. Supersession is invalid with + `superseded_by`, not a third state. No archived/deleted enum. +- `core/store.py::record_many -> _write_one`: schema placement, validation, provenance, + body-change restrictions, persistence and projection. In-place abstract/link updates + retain the body; a changed nonempty body requires `supersedes`. `_predecessor` requires + an active, distinct existing memory. Before this change it rewrote the invalid + predecessor at its original path. +- `Store.correct`: separately permits in-place body, abstract, date, provenance and link + replacement; it previously accepted invalid sources and invalid successors. This is + broader than `record`, and is not a separate generic patch API. +- `Store.delete -> write`: invalidated in place; repeated delete returned immediately, + without repairing a failed index projection. `Store.write` also receives invalidation + from `core/manage.py::_merge_exact_duplicates`, `_merge`, `_supersede`; `_delete` delegates + to `Store.delete`. Thus invalidation is not confined to the CLI delete command. +- `core/archive.py::Archive.append_provenance/append_session`, `core/paths.py::StoreLayout`: + physical `archive/provenance//` Markdown and `archive/sessions/` session files. + There was no Memory archival API. `truth_files/type_of` excluded all Archive content. +- `Store.read/trace/find` had no active filter. `core/agentic.py::_run` already filtered + `find` results by active status. `Store.records`, `core/memory_md.py::render`, and + `core/reconcile.py::build` filtered active memories. `core/context.py::build` opened + Recall hits, while `core/injection.py::payload` trusted cached MEMORY.md bytes. +- `core/search_index.py::upsert` selects active/history FTS tables; `core/recall.py::_eligible` + relied on cached invalid_at. Explicit `as_of` consults the validity interval. A failed + projection could leave an old active row eligible. `core/indexer.py::sync/rebuild` + rebuilds records, FTS and Raw from disk; invalid files previously stayed in the normal tree. +- `Store._store_provenance/trace_record` and `core/sessions.py::parse_pointer/resolve` bind + multiple memories to shared session ranges. Missing sessions yield no messages. + Invalidation never deletes Raw. Explicit deep Recall independently retrieves Raw, with + `source=raw`; this is not progressive Memory-to-Raw reading. +- `core/migrate.py::migrate` handles legacy retired/stale layout and status conversion; + it is an explicit migration command, not a normal invalidate entry. It is unchanged and + was not run on user data. + +## Operation matrix + +CLI entries live in `packages/cli/src/agent_memory/cli/main.py`; MCP entries in +`packages/mcp/src/agent_memory/mcp/tools.py::SCHEMAS/dispatch`. All store operations act on +one configured root. Recall scope is a path-prefix search filter, not write authorization. + +| Operation | Baseline entry / decision and execution | Mutation / recovery / actual risk | Recommendation and this change | +|---|---|---|---| +| Read/Recall/Context/Trace | CLI; MCP read/recall; executor `agentic._run`; core executes | Read-only truth; usage logged. Read/Trace exposed invalid bodies/evidence | Autonomous. Active checks plus explicit history access implemented | +| Create/update | CLI record, MCP memory_record; executor `reconcile.check -> Store.record_many` | Creates file or changes metadata. Body replacement restricted in record; abstract replacement loses previous wording without Git | Autonomous within existing store. Added links validated; no new scope policy | +| Patch | `reconcile.OP_ALIASES` maps patch to update | Same record path, not an independent Manage operation | No new patch API | +| Correct | CLI correct, MCP memory_correct -> Store.correct | Body/abstract/link set replacement; no guaranteed preimage without Git | Retain existing autonomy; active source/successor validation implemented. Broader content policy deferred | +| Link | record/correct links; `Manage._add_cooccurrence_links` is deterministic T0 | Slug references, not filesystem hardlinks. T0 adds both directions from repeated cooccurrence. Wrong links affect interpretation; do not delete targets | Autonomous with distinct active new endpoints in this store. Added validation; MCP now exposes existing correct links capability | +| Unlink | No standalone CLI/MCP/Manage verb; `Store.correct(links=...)` replaces full list | Removes references only; target/Raw retained. Prior set needs Git or caller knowledge for exact restoration | Complete-set replacement documented; `links=[]` via core/MCP clears. Malformed MCP arrays rejected. Delta API deferred | +| Supersede | record supersedes, correct supersede_with; Manage proposal/T0 exact duplicates | Invalidates predecessor, stores successor name; old content retained. Incorrect choice hides current knowledge | Existing automatic authority retained. Physical archive implemented; invalid successor rejected by correct | +| Merge | `Manage._review -> decide -> _merge` or CLI decide | Creates merged content, invalidates old files, unions links/provenance. LLM can omit distinctions; multi-file partial failure possible | Existing proposal menu, revalidation and per-kind sleep cap retained. Invalid files now archived; no transaction framework | +| Split | `Manage._split`, decided as above | Rewrites original with first part, creates others; provenance restricted to original pointers. Original body not independently snapshotted | Recovery policy deferred; no broader authority added | +| Invalidate/delete | CLI delete -> Store.delete; Manage delete proposal | Invalidity, not physical deletion. False judgement removes useful current knowledge, but old file/evidence survives | Autonomous within existing entry/proposal bounds; archive and retry repair implemented | +| Archive | No baseline Memory operation; Archive only wrote Raw/provenance | This change adds Archive.archive_memory via Store; no standalone Agent command | Deterministic consequence of invalidation, not another permission tier | +| Date/weight maintenance | Manage._normalise_dates/_settle_weights, deterministic | Rewrites metadata with configured floor/ceiling; Git-dependent prior values | Unchanged | +| Feedback | CLI/MCP -> Store.feedback | **Only mutates returned object at baseline; does not persist weight** | Audit finding only, fix deferred | +| Group merge/cluster | Manage._merge_near_duplicate_groups/_cluster -> Store.record | Moves schema groups, retains stable name/content/evidence. No semantic scope ACL | Existing autonomy retained; cross-scope policy deferred | +| Redistill request | Manage._request_redistill -> Pending | Schedules uncited repeatedly accessed Raw | Counts archived citations too, so invalidation alone does not requeue old evidence | +| Physical delete | CLI gc -> Store.gc; absent from Manage proposal menu and MCP | Deletes invalid files including archived memories; Raw retained. Irreversible without backup | Human confirmation recommended; existing human-run label is **not enforced authentication**. No new GC policy implemented | +| Inspect/rebuild/export | CLI -> Store/indexer/portability | Rebuildable projections or output copy; export normally includes Archive | Autonomous local maintenance; archived history included in rebuild | +| Import/migrate | CLI -> portability.import_into / migrate.migrate | Writes files, can overwrite/import history; outside Manage controls | Explicit owner authorization recommended; unchanged | + +## Existing controls and their limits + +`Manage._review` uses `_caps`, the open proposal menu and `decide`; `_open` regenerates +proposals before each decision. A stale/unknown proposal cannot dictate arbitrary targets. +Direct CLI `decide` is not subject to the per-sleep cap. `core/reasoning.py::parse` accepts +verdicts and text, not arbitrary executable operations. The executor's write path has +reconcile handles, but direct CLI/MCP record/correct do not use that handle boundary. + +`core/locking.py::store_lock` serializes writers. This change puts correction/deletion +read-modify-write under that lock and rejects stale writes that would reactivate invalid +memories. A complete Manage sleep is not atomic. `core/ledger.py::DecisionLedger.append` +records verdicts; `Manage._write_report` records actions and `Manage._commit` attempts one +Git commit when configured. Failed operations can precede a ledger/report entry. No +pre-operation snapshot, guaranteed Git commit, restore command, actor authentication, +RBAC, approval service or host filesystem sandbox exists in this runtime. + +Autonomous operations: reads, existing record/update and T0 maintenance. Deterministic +checks: relation additions, active endpoints, existing proposal bounds, validity/schema +checks and writer locking. Higher-authority recommendations: permanent GC, bulk overwrite, +cross-store modifications and direct filesystem/DB changes. Product decisions remain in +[the follow-up list](../TODO.md). An Agent with shell/file access can bypass tool menus; +this PR does not claim otherwise. + +## Implemented lifecycle and recovery + +`Store.delete`, `Store.correct(supersede_with=...)`, `_write_one` predecessor invalidation, +and Manage's existing `Store.write` calls converge on `_persist`. It atomically replaces +file content with invalid metadata, then calls `Archive.archive_memory`, moving to +`archive/memories/`. `_project` refreshes MEMORY.md before SQLite. +`Indexer` includes archived memories in the existing history projection and rebuild. + +- Failed status persistence leaves the old file unchanged; temporary files are removed. +- Failed directory creation/move leaves an invalid file at its source. Retry `delete` on + that name; original invalid_at and successor remain. Destination collisions raise + FileExistsError and preserve both copies; resolve that collision with backed-up, + owner-reviewed recovery before retrying. No automatic overwrite or deletion. +- Successful move plus failed index projection leaves invalid truth in Archive. Default + Recall checks current file validity; Read/Trace scan when an indexed old path disappears. + Injection renders current active truth. Retry delete or rebuild repairs projections. +- Cached static MEMORY.md may remain stale after a move failure before projection, or an + external consumer may already hold injected context. Controlled injection renders fresh + truth; this cannot retract prior context or stop arbitrary direct filesystem reading. +- Read/Trace default to active; `include_invalid=True`, CLI `--history` and MCP read's + explicit include_invalid access history. CLI/MCP read report status and successor. + Temporal Context opens historical bodies; Recall scope still addresses original paths. +- Raw files are never moved or deleted by invalidation, including missing/shared evidence. + Deep Raw search remains explicit and unchanged; redistill counts historical citations. +- Existing valid old-format files work with omitted optional fields; old-location invalid + files are filtered without migration and archive when deletion is retried. Malformed + legacy state missing required invalid_at still requires repair; no speculative data fix. +- Multi-file supersede/merge remains nontransactional: a successor may be persisted before + predecessor archival fails. Inspect both names, then retry predecessor deletion to finish + archival; do not blindly replay a whole merge. Evidence remains in the old files. + +No bulk migration was executed. If owners later want old-location invalid files moved, +first enumerate those files with `Store.records(include_invalid=True)` and their paths, +back up the entire store including Archive and schemas, stop writers, then review the +list before applying existing delete per name. Verify history/Raw and rebuild. Rollback +restores that backup and rebuilds projections; this is a proposed runbook, not a migration +implemented or run by this PR. + +## Verification map + +Existing `tests/unit/test_storage.py`, `test_supersede_on_write.py`, `test_recall.py`, +`test_indexer.py`, `test_manage.py`, `test_manage_reasoning.py`, `test_reconcile.py` and +`tests/system/test_entry_equivalence.py` cover lifecycle, temporal retrieval, rebuild, +proposal caps/unknown verdicts, targets, Git/ledger and adapter parity. + +`tests/unit/test_memory_archive.py` adds behavioral isolation, physical archive, explicit +history/rebuild, idempotence, atomic-write/move/index failures, destination conflict, missing +optional fields, missing/shared Raw, retained links, rejected endpoints, stale resurrection, +Raw redistill and temporal scope. `tests/system/test_archive_entries.py` covers CLI/MCP +history and relationship parity, malformed arrays and unavailable destructive tools. +The existing dangling-link test now uses legacy file input: controlled new writes reject +unknown link targets, while index rebuild continues reporting historical dangling links. + +## Local validation results + +Executed against this worktree's six package source directories via PYTHONPATH, using the +existing Python 3.12 environment (no experimental branch source imported): + +```bash +export PYTHONPATH=packages/core/src:packages/cli/src:packages/mcp/src:packages/executor/src:packages/adapters/src:packages/harness/src +/home/codexlab/code/agent-memory/.venv/bin/python -m pytest -q --cov=agent_memory --cov-report=term-missing --cov-fail-under=85 +/home/codexlab/code/agent-memory/.venv/bin/ruff check . +/home/codexlab/code/agent-memory/.venv/bin/mypy +git diff --check +``` + +Results: 411 tests passed; coverage 91.14% (required 85%); Ruff passed; Mypy passed for +66 source files; diff whitespace check passed. An additional temporary-store CLI smoke +executed 11 commands: init, record, recall/read before delete, delete, recall/read after +delete, history read, repeated delete, rebuild and Context. All expected exit codes, +physical paths and empty active results were verified. No live user Memory was touched. + +CI equivalent: `uv sync --all-packages`, `uv run ruff check .`, `uv run mypy`, then +`uv run pytest -q --cov=agent_memory --cov-report=term-missing --cov-fail-under=85`. diff --git a/packages/cli/src/agent_memory/cli/main.py b/packages/cli/src/agent_memory/cli/main.py index e9eb4450..af2b9922 100644 --- a/packages/cli/src/agent_memory/cli/main.py +++ b/packages/cli/src/agent_memory/cli/main.py @@ -107,6 +107,7 @@ def _parser() -> argparse.ArgumentParser: opener = subparsers.add_parser("read", help="read one memory") opener.add_argument("name") opener.add_argument("--level", choices=LEVELS, default=LEVEL_FULL) + opener.add_argument("--history", action="store_true", help="include invalid memory history") opener.set_defaults(handler=_read) corrector = subparsers.add_parser("correct", help="update or supersede one memory") @@ -115,7 +116,12 @@ def _parser() -> argparse.ArgumentParser: corrector.add_argument("--body", default=None) corrector.add_argument("--body-file", default=None) corrector.add_argument("--supersede-with", default=None) - corrector.add_argument("--link", action="append", default=None) + corrector.add_argument( + "--link", + action="append", + default=None, + help="replace the complete link list; repeat for each retained target", + ) corrector.add_argument("--provenance", action="append", default=[]) corrector.set_defaults(handler=_correct) @@ -135,9 +141,10 @@ def _parser() -> argparse.ArgumentParser: tracer = subparsers.add_parser("trace", help="open the messages a memory cites") tracer.add_argument("name") + tracer.add_argument("--history", action="store_true", help="include invalid memory evidence") tracer.set_defaults(handler=_trace) - remover = subparsers.add_parser("delete", help="mark one memory invalid; the file stays") + remover = subparsers.add_parser("delete", help="invalidate and archive one memory") remover.add_argument("name") remover.set_defaults(handler=_delete) @@ -285,10 +292,12 @@ def _context(store: Store, args: argparse.Namespace) -> dict[str, object]: def _read(store: Store, args: argparse.Namespace) -> dict[str, object]: - result = store.read(args.name, level=args.level) + result = store.read(args.name, level=args.level, include_invalid=args.history) return { "name": result.record.name, "level": result.level, + "status": result.record.status, + "superseded_by": result.record.superseded_by, "abstract": result.record.abstract, "path": str(result.record.path), "outline": list(result.outline), @@ -352,7 +361,7 @@ def _archived_sessions(store: Store) -> list[str]: def _trace(store: Store, args: argparse.Namespace) -> dict[str, object]: - messages = store.trace(args.name) + messages = store.trace(args.name, include_invalid=args.history) return {"name": args.name, "messages": [message.as_dict() for message in messages]} diff --git a/packages/core/src/agent_memory/core/archive.py b/packages/core/src/agent_memory/core/archive.py index 1123a00d..c81c7341 100644 --- a/packages/core/src/agent_memory/core/archive.py +++ b/packages/core/src/agent_memory/core/archive.py @@ -7,7 +7,9 @@ from . import sessions from .clock import Clock +from .errors import FieldError, ValidationError from .paths import StoreLayout +from .record import MemoryRecord PROVENANCE_SUFFIX = ".md" SESSION_SUFFIX = sessions.SESSION_SUFFIX @@ -18,6 +20,22 @@ def __init__(self, layout: StoreLayout, clock: Clock | None = None): self._layout = layout self._clock = clock or Clock() + def archive_memory(self, record: MemoryRecord) -> pathlib.Path: + if record.is_active() or record.path is None: + raise ValidationError([FieldError("status", "only persisted invalid memories archive")]) + source = record.path + if self._layout.type_of(source) != record.type: + raise ValidationError([FieldError("path", "memory must belong to this store")]) + if self._layout.is_archived_memory(source): + return source + target = self._layout.archived_memories / source.relative_to(self._layout.root) + target.parent.mkdir(parents=True, exist_ok=True) + if target.exists(): + raise FileExistsError(f"archive destination already exists: {target}") + source.rename(target) + record.path = target + return target + def append_provenance(self, name: str, excerpt: str, source: str = "") -> pathlib.Path: folder = self._layout.provenance / name folder.mkdir(parents=True, exist_ok=True) diff --git a/packages/core/src/agent_memory/core/context.py b/packages/core/src/agent_memory/core/context.py index 6e88e609..876ff582 100644 --- a/packages/core/src/agent_memory/core/context.py +++ b/packages/core/src/agent_memory/core/context.py @@ -48,7 +48,12 @@ def build( full_text_entries = store.config.recall.context_full_text_entries rendered = [ - _entry(hit, _body(store, hit.name) if position < full_text_entries else "") + _entry( + hit, + _body(store, hit.name, include_invalid=as_of is not None) + if position < full_text_entries + else "", + ) for position, hit in enumerate(hits) ] return Context( @@ -63,8 +68,8 @@ def _entry(hit, body: str) -> str: return f"{head}\n{body}" if body.strip() else head -def _body(store: Store, name: str) -> str: +def _body(store: Store, name: str, include_invalid: bool = False) -> str: try: - return store.read(name, level=LEVEL_FULL).text + return store.read(name, level=LEVEL_FULL, include_invalid=include_invalid).text except Exception: return "" diff --git a/packages/core/src/agent_memory/core/indexer.py b/packages/core/src/agent_memory/core/indexer.py index e48d5482..2e44045f 100644 --- a/packages/core/src/agent_memory/core/indexer.py +++ b/packages/core/src/agent_memory/core/indexer.py @@ -95,7 +95,7 @@ def _is_raw(self, path: pathlib.Path) -> bool: def _present_hashes(self) -> dict[str, str]: present: dict[str, str] = {} - for path in self._layout.truth_files() + self._raw_files(): + for path in self._layout.truth_files(include_archive=True) + self._raw_files(): relative = str(path.relative_to(self._layout.root)) present[relative] = content_hash( path.read_text(encoding="utf-8"), self._config.index.hash_prefix_length @@ -116,6 +116,8 @@ def _load(self, path: pathlib.Path) -> MemoryRecord | None: record_module.validate(record, self._config, self._schemas.get(type_name)) except ValidationError: return None + if self._layout.is_archived_memory(path) and record.is_active(): + return None if record.type != type_name: return None return record diff --git a/packages/core/src/agent_memory/core/injection.py b/packages/core/src/agent_memory/core/injection.py index 3d83e1de..d5b6de30 100644 --- a/packages/core/src/agent_memory/core/injection.py +++ b/packages/core/src/agent_memory/core/injection.py @@ -1,9 +1,10 @@ -"""The injection track: a byte-prefix of MEMORY.md, never a summary of it. +"""The injection track: a byte-prefix of the root index rendered from current truth. Deterministic floor of the three read tracks — it costs no tool call and cannot miss.""" from __future__ import annotations +from . import memory_md from .store import Store NEWLINE = b"\n" @@ -14,7 +15,7 @@ def payload(store: Store) -> str: return "" if not store.layout.memory_index.exists(): return "" - data = store.layout.memory_index.read_bytes() + data = memory_md.render(store.records(), store.config, str(store.root)).encode("utf-8") budget = store.config.recall.injection_budget_bytes if len(data) <= budget: return data.decode("utf-8") diff --git a/packages/core/src/agent_memory/core/manage.py b/packages/core/src/agent_memory/core/manage.py index 0f444ebd..b07a9106 100644 --- a/packages/core/src/agent_memory/core/manage.py +++ b/packages/core/src/agent_memory/core/manage.py @@ -139,7 +139,7 @@ def sleep(self, reasoner: reasoning.Reasoner | None = None) -> DreamReport: actions.extend(self._merge_exact_duplicates(records)) actions.extend(self._merge_near_duplicate_groups()) actions.extend(self._cluster(self._store.records())) - actions.extend(self._request_redistill(self._store.records())) + actions.extend(self._request_redistill(self._store.records(include_invalid=True))) records = self._store.records() proposals = self.proposals(records=records, hits=hits) diff --git a/packages/core/src/agent_memory/core/paths.py b/packages/core/src/agent_memory/core/paths.py index 18c29eec..d2fb0d3e 100644 --- a/packages/core/src/agent_memory/core/paths.py +++ b/packages/core/src/agent_memory/core/paths.py @@ -9,6 +9,7 @@ MEMORY_INDEX_FILENAME = "MEMORY.md" ARCHIVE_DIRNAME = "archive" +MEMORIES_DIRNAME = "memories" PROVENANCE_DIRNAME = "provenance" SESSIONS_DIRNAME = "sessions" INDEX_DIRNAME = ".index" @@ -31,6 +32,13 @@ def memory_index(self) -> pathlib.Path: def archive(self) -> pathlib.Path: return self.root / ARCHIVE_DIRNAME + @property + def archived_memories(self) -> pathlib.Path: + return self.archive / MEMORIES_DIRNAME + + def is_archived_memory(self, path: pathlib.Path) -> bool: + return path.resolve().is_relative_to(self.archived_memories.resolve()) + @property def provenance(self) -> pathlib.Path: return self.archive / PROVENANCE_DIRNAME @@ -107,6 +115,8 @@ def type_of(self, path: pathlib.Path) -> str | None: relative = pathlib.Path(path).resolve().relative_to(self.root.resolve()) except ValueError: return None + if self.is_archived_memory(path): + relative = path.resolve().relative_to(self.archived_memories.resolve()) parts = relative.parts if len(parts) < len(("type", "file")) or parts[0] in self.reserved_dirnames: return None @@ -120,7 +130,7 @@ def groups_of(self, type_name: str) -> set[str]: return set() return {entry.name for entry in folder.iterdir() if entry.is_dir()} - def truth_files(self) -> list[pathlib.Path]: + def truth_files(self, include_archive: bool = False) -> list[pathlib.Path]: files: list[pathlib.Path] = [] if not self.root.is_dir(): return files @@ -128,4 +138,6 @@ def truth_files(self) -> list[pathlib.Path]: if not entry.is_dir() or entry.name in self.reserved_dirnames: continue files.extend(sorted(entry.rglob("*" + MEMORY_SUFFIX))) + if include_archive: + files.extend(sorted(self.archived_memories.rglob("*" + MEMORY_SUFFIX))) return files diff --git a/packages/core/src/agent_memory/core/prompts.py b/packages/core/src/agent_memory/core/prompts.py index 0113c657..11b52a7b 100644 --- a/packages/core/src/agent_memory/core/prompts.py +++ b/packages/core/src/agent_memory/core/prompts.py @@ -50,7 +50,8 @@ Values that move — a count, a goal, a price, a schedule, a status — almost always already have an entry holding the previous value. Search for it before writing the new one, and write the new one with `--supersedes `. That is what keeps "how many so far" answerable: the -current value is the one left standing, and the old value stays readable as history. +current value is the one left standing. Invalid memories move into Archive; read history +explicitly with `mem read --history` or `mem trace --history`. When the atom exists and the old content is simply wrong, write it again under the same name, which updates it in place. When the atom is new, create a new file. @@ -304,6 +305,14 @@ def repair(sheet: str, refused: str) -> str: such as `project` or `topic` name the subdirectory; pick an existing one, and pass `--create-group` only when a new one is genuinely needed. +## Relationship maintenance + +Use `mem record --link ` for links to existing active memories. To revise links, +`mem correct --link ` replaces the complete list; repeat `--link` for each +retained target. MCP `memory_correct` accepts `links`, with `[]` clearing the list. Choose +another active memory in this store as each new target. Existing historical links may stay. +Use these commands for changes so validation and indexing run together. + ## Write discipline {discipline} diff --git a/packages/core/src/agent_memory/core/recall.py b/packages/core/src/agent_memory/core/recall.py index c56eec46..511bbd91 100644 --- a/packages/core/src/agent_memory/core/recall.py +++ b/packages/core/src/agent_memory/core/recall.py @@ -102,10 +102,23 @@ def _eligible( moment = timestamp.parse(as_of) if as_of else None for row in rows: name = str(row["name"]) - if scope and not self._in_scope(str(row["path"]), scope): + path = self._store.root / str(row["path"]) + scope_path = ( + str(path.relative_to(self._store.layout.archived_memories)) + if self._store.layout.is_archived_memory(path) + else str(row["path"]) + ) + if scope and not self._in_scope(scope_path, scope): continue if moment is None: - if row["invalid_at"]: + current = self._store._at(path) + if ( + row["status"] != "active" + or row["invalid_at"] + or current is None + or not current.is_active() + or self._store.layout.is_archived_memory(path) + ): continue elif not self._current_at(row, moment): continue diff --git a/packages/core/src/agent_memory/core/store.py b/packages/core/src/agent_memory/core/store.py index eadee531..081a97cc 100644 --- a/packages/core/src/agent_memory/core/store.py +++ b/packages/core/src/agent_memory/core/store.py @@ -8,6 +8,7 @@ import dataclasses import pathlib +import tempfile from . import chunking, memory_md, placement, timestamp from . import record as record_module @@ -160,7 +161,7 @@ def _write_one(self, spec: dict[str, object]) -> MemoryRecord: ) supersedes = str(spec.get("supersedes") or "") or None derived_name = not spec.get("name") - if supersedes and derived_name and (self.root / placed.relative_path).exists(): + if supersedes and derived_name and self.find(placed.name) is not None: placed = self._successor_placement(placed) target = self.root / placed.relative_path existing = self._at(target) @@ -204,6 +205,7 @@ def _write_one(self, spec: dict[str, object]) -> MemoryRecord: self._enforce_update_only(existing, candidate) record_module.validate(candidate, self.config, schema) record_module.canonicalise_dates(candidate) + self._validate_links(candidate, existing) predecessor = self._predecessor(candidate, supersedes) for excerpt in _as_sequence(spec.get("provenance")): @@ -218,7 +220,7 @@ def _write_one(self, spec: dict[str, object]) -> MemoryRecord: if predecessor is not None and predecessor.path is not None: record_module.invalidate(predecessor, candidate.valid_from or now, candidate.name) predecessor.updated = now - predecessor.path.write_text(predecessor.to_text(), encoding="utf-8") + self._persist(predecessor) return candidate def _successor_placement(self, placed: placement.Placement) -> placement.Placement: @@ -262,11 +264,9 @@ def _reject_facts_dated_after_their_evidence(self, record: MemoryRecord) -> None [FieldError("valid_from", "later than the messages this memory cites")] ) - def trace(self, name: str) -> list[Message]: + def trace(self, name: str, *, include_invalid: bool = False) -> list[Message]: """Opens the messages a memory cites. The one read that reaches raw material by pointer.""" - current = self.find(name) - if current is None: - raise NotFoundError(f"no memory named {name}") + current = self._readable(name, include_invalid) stamp = self.clock.now().isoformat() self._log_access([AccessEntry(stamp, name, "", KIND_READ, self.agent)]) return self.trace_record(current) @@ -301,40 +301,54 @@ def correct( valid_from: str | None = None, provenance: list[str] | None = None, ) -> MemoryRecord: - current = self.find(name) - if current is None or current.path is None: - raise NotFoundError(f"no memory named {name}") - now = self.clock.timestamp() - if supersede_with: - successor = self.find(supersede_with) - if successor is None: - raise NotFoundError(f"no memory named {supersede_with}") - record_module.invalidate(current, successor.valid_from or now, supersede_with) - if abstract is not None: - current.abstract = abstract.strip() - if body is not None: - current.body = body - if links is not None: - current.links = list(links) - if valid_from is not None: - current.valid_from = valid_from - current.updated = now with store_lock(self.layout): + current = self.find(name) + if current is None or current.path is None: + raise NotFoundError(f"no memory named {name}") + if not current.is_active(): + raise ValidationError( + [FieldError("status", "correction requires an active memory")] + ) + now = self.clock.timestamp() + if supersede_with: + successor = self.find(supersede_with) + if successor is None: + raise NotFoundError(f"no memory named {supersede_with}") + if not successor.is_active(): + raise ValidationError( + [FieldError("supersede_with", "successor must be active")] + ) + record_module.invalidate(current, successor.valid_from or now, supersede_with) + if abstract is not None: + current.abstract = abstract.strip() + if body is not None: + current.body = body + if links is not None: + current.links = list(links) + if valid_from is not None: + current.valid_from = valid_from + current.updated = now + self._validate_write(current) for excerpt in provenance or []: current.provenance.append(self._store_provenance(current.name, excerpt)) - return self.write(current) + self._persist(current) + self._project() + return current def delete(self, name: str) -> MemoryRecord: - """Marks the record invalid. The file stays; physical removal is a human command.""" - current = self.find(name) - if current is None or current.path is None: - raise NotFoundError(f"no memory named {name}") - if not current.is_active(): + """Invalidate and archive one memory; retry completes archival and projection.""" + with store_lock(self.layout): + current = self.find(name) + if current is None or current.path is None: + raise NotFoundError(f"no memory named {name}") + if current.is_active(): + now = self.clock.timestamp() + record_module.invalidate(current, now) + current.updated = now + self._validate_write(current) + self._persist(current) + self._project() return current - now = self.clock.timestamp() - record_module.invalidate(current, now) - current.updated = now - return self.write(current) def gc(self) -> list[str]: """Physically removes invalid files. A human runs this; Manage cannot reach it.""" @@ -350,15 +364,50 @@ def gc(self) -> list[str]: def write(self, record: MemoryRecord) -> MemoryRecord: """Validate, persist, reproject. Agent writes and Manage rewrites share this path.""" - if record.path is None: - raise NotFoundError(f"{record.name} has no location on disk") - record_module.validate(record, self.config, self.schemas.get(record.type)) - record_module.canonicalise_dates(record) with store_lock(self.layout): - record.path.write_text(record.to_text(), encoding="utf-8") + self._validate_write(record) + self._persist(record) self._project() return record + def _validate_write(self, record: MemoryRecord) -> None: + if record.path is None or self.layout.type_of(record.path) != record.type: + raise ValidationError([FieldError("path", "memory must belong to this store")]) + existing = self.find(record.name) + if record.is_active() and ( + self.layout.is_archived_memory(record.path) + or (existing is not None and not existing.is_active()) + ): + raise ValidationError([FieldError("status", "an invalid memory cannot be reactivated")]) + record_module.validate(record, self.config, self.schemas.get(record.type)) + record_module.canonicalise_dates(record) + self._validate_links(record, existing) + + def _validate_links(self, record: MemoryRecord, existing: MemoryRecord | None) -> None: + added = set(record.links) - set(existing.links if existing else []) + for name in sorted(added): + target = self.find(name) + if name == record.name or target is None or not target.is_active(): + raise ValidationError( + [FieldError("links", f"{name} must name another active memory")] + ) + + def _persist(self, record: MemoryRecord) -> None: + if record.path is None: + raise NotFoundError(f"{record.name} has no location on disk") + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", dir=record.path.parent, delete=False + ) as handle: + temporary = pathlib.Path(handle.name) + try: + handle.write(record.to_text()) + handle.flush() + temporary.replace(record.path) + finally: + temporary.unlink(missing_ok=True) + if not record.is_active(): + self.archive.archive_memory(record) + def feedback(self, name: str, delta: float) -> MemoryRecord: current = self.find(name) if current is None or current.path is None: @@ -368,12 +417,12 @@ def feedback(self, name: str, delta: float) -> MemoryRecord: ) return current - def read(self, name: str, level: str = LEVEL_FULL) -> ReadResult: + def read( + self, name: str, level: str = LEVEL_FULL, *, include_invalid: bool = False + ) -> ReadResult: if level not in LEVELS: raise ValidationError([FieldError("level", f"must be one of {', '.join(LEVELS)}")]) - current = self.find(name) - if current is None: - raise NotFoundError(f"no memory named {name}") + current = self._readable(name, include_invalid) headings = tuple(entry.title for entry in chunking.outline(current.body, self.config)) if level == LEVEL_ABSTRACT: text = current.abstract @@ -385,19 +434,34 @@ def read(self, name: str, level: str = LEVEL_FULL) -> ReadResult: self._log_access([AccessEntry(stamp, name, "", KIND_READ, self.agent)]) return ReadResult(record=current, level=level, text=text, outline=headings) + def _readable(self, name: str, include_invalid: bool) -> MemoryRecord: + current = self.find(name) + if current is None or ( + not include_invalid + and ( + not current.is_active() + or (current.path is not None and self.layout.is_archived_memory(current.path)) + ) + ): + raise NotFoundError( + f"no active memory named {name}; use explicit history for invalid memories" + ) + return current + def find(self, name: str) -> MemoryRecord | None: with self._database.connect() as connection: row = SearchIndex(connection).row(name) path = (self.root / str(row["path"])) if row else self._scan_for(name) - return self._at(path) if path is not None else None + found = self._at(path) if path is not None else None + return found if found is not None else self._at(self._scan_for(name)) def records(self, include_invalid: bool = False) -> list[MemoryRecord]: found: list[MemoryRecord] = [] - for path in self.layout.truth_files(): + for path in self.layout.truth_files(include_archive=include_invalid): record = self._at(path) if record is None: continue - if include_invalid or record.is_active(): + if include_invalid or (record.is_active() and not self.layout.is_archived_memory(path)): found.append(record) return found @@ -415,8 +479,8 @@ def rebuild_index(self) -> IndexReport: return report def _project(self) -> IndexReport: - report = self._indexer.sync() memory_md.write(self.layout, self.records()) + report = self._indexer.sync() return report def _log_access(self, entries: list[AccessEntry]) -> None: @@ -429,7 +493,7 @@ def _at(self, path: pathlib.Path | None) -> MemoryRecord | None: return MemoryRecord.from_text(path.read_text(encoding="utf-8"), path) def _scan_for(self, name: str) -> pathlib.Path | None: - for path in self.layout.truth_files(): + for path in self.layout.truth_files(include_archive=True): if path.stem == name: return path return None diff --git a/packages/mcp/src/agent_memory/mcp/tools.py b/packages/mcp/src/agent_memory/mcp/tools.py index c0af16fb..036231b1 100644 --- a/packages/mcp/src/agent_memory/mcp/tools.py +++ b/packages/mcp/src/agent_memory/mcp/tools.py @@ -30,6 +30,7 @@ "properties": { "name": {"type": "string"}, "level": {"type": "string", "enum": list(LEVELS)}, + "include_invalid": {"type": "boolean", "description": "Explicit history access"}, }, "required": ["name"], }, @@ -55,6 +56,11 @@ "abstract": {"type": "string"}, "body": {"type": "string"}, "supersede_with": {"type": "string"}, + "links": { + "type": "array", + "items": {"type": "string"}, + "description": "Replace all links; empty list removes all links", + }, }, "required": ["name"], }, @@ -93,6 +99,13 @@ def dispatch(store: Store, tool: str, arguments: dict[str, object]) -> dict[str, def _require(tool: str, arguments: dict[str, object]) -> None: + if "links" in arguments and ( + not isinstance(arguments["links"], list) + or not all(isinstance(item, str) for item in arguments["links"]) + ): + raise ValidationError([FieldError("links", "must be an array of memory names")]) + if "include_invalid" in arguments and not isinstance(arguments["include_invalid"], bool): + raise ValidationError([FieldError("include_invalid", "must be a boolean")]) schema = SCHEMAS[tool] required = schema.get("required") missing = [ @@ -126,10 +139,16 @@ def _recall(store: Store, arguments: dict[str, object]) -> dict[str, object]: def _read(store: Store, arguments: dict[str, object]) -> dict[str, object]: - result = store.read(str(arguments["name"]), level=str(arguments.get("level") or LEVEL_FULL)) + result = store.read( + str(arguments["name"]), + level=str(arguments.get("level") or LEVEL_FULL), + include_invalid=arguments.get("include_invalid") is True, + ) return { "name": result.record.name, "level": result.level, + "status": result.record.status, + "superseded_by": result.record.superseded_by, "abstract": result.record.abstract, "path": str(result.record.path), "outline": list(result.outline), @@ -158,6 +177,7 @@ def _correct(store: Store, arguments: dict[str, object]) -> dict[str, object]: abstract=_optional(arguments, "abstract"), body=_optional(arguments, "body"), supersede_with=_optional(arguments, "supersede_with"), + links=_string_list(arguments["links"]) if "links" in arguments else None, ) return { "name": corrected.name, diff --git a/skills/agent-memory/SKILL.md b/skills/agent-memory/SKILL.md index 257dbb5a..057a8fbe 100644 --- a/skills/agent-memory/SKILL.md +++ b/skills/agent-memory/SKILL.md @@ -46,6 +46,14 @@ The store's `schemas/` directory lists the types and what each one is for. Group such as `project` or `topic` name the subdirectory; pick an existing one, and pass `--create-group` only when a new one is genuinely needed. +## Relationship maintenance + +Use `mem record --link ` for links to existing active memories. To revise links, +`mem correct --link ` replaces the complete list; repeat `--link` for each +retained target. MCP `memory_correct` accepts `links`, with `[]` clearing the list. Choose +another active memory in this store as each new target. Existing historical links may stay. +Use these commands for changes so validation and indexing run together. + ## Write discipline Recall first to see whether this atom already exists. @@ -53,7 +61,8 @@ Recall first to see whether this atom already exists. Values that move — a count, a goal, a price, a schedule, a status — almost always already have an entry holding the previous value. Search for it before writing the new one, and write the new one with `--supersedes `. That is what keeps "how many so far" answerable: the -current value is the one left standing, and the old value stays readable as history. +current value is the one left standing. Invalid memories move into Archive; read history +explicitly with `mem read --history` or `mem trace --history`. When the atom exists and the old content is simply wrong, write it again under the same name, which updates it in place. When the atom is new, create a new file. diff --git a/tests/system/test_archive_entries.py b/tests/system/test_archive_entries.py new file mode 100644 index 00000000..cd1d3cbe --- /dev/null +++ b/tests/system/test_archive_entries.py @@ -0,0 +1,44 @@ +import pytest +from agent_memory.cli.main import main +from agent_memory.core.errors import NotFoundError, ValidationError +from agent_memory.mcp.tools import dispatch + + +def test_cli_and_mcp_share_archive_and_relationship_boundaries(store, capsys): + first = dispatch( + store, + "memory_record", + {"type": "fact", "name": "quasar-old", "abstract": "Quasar old", "body": "Old evidence"}, + ) + dispatch( + store, + "memory_record", + {"type": "fact", "name": "quasar-new", "abstract": "Quasar new", "body": "New fact"}, + ) + dispatch(store, "memory_correct", {"name": first["name"], "links": ["quasar-new"]}) + assert store.find(first["name"]).links == ["quasar-new"] + with pytest.raises(ValidationError): + dispatch( + store, "memory_record", {"type": "fact", "abstract": "Bad link", "links": ["missing"]} + ) + dispatch(store, "memory_correct", {"name": first["name"], "supersede_with": "quasar-new"}) + with pytest.raises(NotFoundError): + dispatch(store, "memory_read", {"name": first["name"]}) + assert ( + dispatch(store, "memory_read", {"name": first["name"], "include_invalid": True})["text"] + == "Old evidence" + ) + assert main(["--store", str(store.root), "read", first["name"], "--history"]) == 0 + assert "Old evidence" in capsys.readouterr().out + for tool in ("memory_gc", "memory_delete", "memory_unlink"): + with pytest.raises(ValidationError): + dispatch(store, tool, {"name": first["name"]}) + + +@pytest.mark.parametrize("links", ["target", None, [None], {}]) +def test_malformed_relationship_input_cannot_clear_links(store, links): + store.record(type="fact", name="target", abstract="Target memory") + store.record(type="fact", name="source", abstract="Source memory", links=["target"]) + with pytest.raises(ValidationError): + dispatch(store, "memory_correct", {"name": "source", "links": links}) + assert store.find("source").links == ["target"] diff --git a/tests/unit/test_indexer.py b/tests/unit/test_indexer.py index 22eb9ea5..5dc47d11 100644 --- a/tests/unit/test_indexer.py +++ b/tests/unit/test_indexer.py @@ -49,15 +49,15 @@ def test_rebuild_is_idempotent(seeded): assert set(first.reindexed) == set(second.reindexed) -def test_a_dangling_link_is_reported_but_does_not_reject_the_write(store): +def test_a_legacy_dangling_link_is_reported_by_rebuild(store): written = store.record( abstract="Points at a memory that does not exist yet", type="fact", name="forward-reference", - links=["not-written-yet"], ) - assert written.path.exists() - report = store.sync_index() + written.links = ["not-written-yet"] + written.path.write_text(written.to_text()) + report = store.rebuild_index() assert ("forward-reference", "not-written-yet") in report.dangling_links diff --git a/tests/unit/test_memory_archive.py b/tests/unit/test_memory_archive.py new file mode 100644 index 00000000..515abe79 --- /dev/null +++ b/tests/unit/test_memory_archive.py @@ -0,0 +1,246 @@ +import pathlib + +import pytest +from agent_memory.core import context, injection +from agent_memory.core.errors import NotFoundError, ValidationError +from agent_memory.core.recall import Recall +from agent_memory.core.record import MemoryRecord + + +def memory(store, name="old-memory", **kwargs): + return store.record( + type="fact", name=name, abstract="Quasar queue timeout", body="Old fact", **kwargs + ) + + +def assert_isolated(store, name): + for level in ("abstract", "outline", "full"): + with pytest.raises(NotFoundError): + store.read(name, level=level) + with pytest.raises(NotFoundError): + store.trace(name) + assert name not in {hit.name for hit in Recall(store).recall("Quasar")} + assert name not in context.build(store, "Quasar").names + assert name not in injection.payload(store) + + +def test_archive_lifecycle_history_and_rebuild(store, clock): + old = memory(store) + source = old.path + assert store.read(old.name).text == old.body + assert old.name in {hit.name for hit in Recall(store).recall("Quasar")} + moment = clock.timestamp() + clock.advance(days=1) + invalid = store.delete(old.name) + assert invalid.path == store.layout.archived_memories / source.relative_to(store.root) + assert not source.exists() + assert invalid.path.exists() + assert invalid.status == "invalid" + assert_isolated(store, old.name) + for _ in range(2): + store.rebuild_index() + assert store.read(old.name, include_invalid=True).text == old.body + assert old.name in {h.name for h in Recall(store).recall("Quasar", as_of=moment)} + assert old.body in context.build(store, "Quasar", as_of=moment).text + before = invalid.path.read_bytes() + clock.advance(days=1) + assert store.delete(old.name).path.read_bytes() == before + + +def test_move_failure_is_invalid_retryable_and_keeps_raw(store, monkeypatch): + old = memory(store, provenance=["original evidence"]) + source = old.path + evidence = store.archive.provenance_of(old.name)[0] + original = pathlib.Path.rename + + def fail(path, target): + if path == source: + raise OSError("archive move failed") + return original(path, target) + + with monkeypatch.context() as patch: + patch.setattr(pathlib.Path, "rename", fail) + with pytest.raises(OSError, match="archive move failed"): + store.delete(old.name) + assert not MemoryRecord.from_text(source.read_text()).is_active() + assert evidence.exists() + assert_isolated(store, old.name) + assert store.delete(old.name).path.is_relative_to(store.layout.archived_memories) + + +def test_projection_failure_does_not_leak_and_retry_repairs_index(store, monkeypatch): + old = memory(store) + + def fail(): + raise OSError("index unavailable") + + with monkeypatch.context() as patch: + patch.setattr(store._indexer, "sync", fail) + with pytest.raises(OSError, match="index unavailable"): + store.delete(old.name) + assert_isolated(store, old.name) + assert store.find(old.name).path.is_relative_to(store.layout.archived_memories) + store.delete(old.name) + assert_isolated(store, old.name) + + +def test_shared_and_missing_raw_survive_invalidation(store): + store.archive.append_session("shared", ["user: quasar evidence"]) + old = memory(store, provenance=["sessions/shared#0-0", "sessions/missing#0-0"]) + other = memory(store, "other-memory", provenance=["sessions/shared#0-0"]) + before = store.trace(other.name) + store.delete(old.name) + assert store.trace(other.name) == before + assert store.trace(old.name, include_invalid=True) == before + assert store.find(old.name).provenance == old.provenance + + +def test_legacy_invalid_is_filtered_and_archived_on_retry(store): + old = memory(store) + text = old.path.read_text().replace("status: active", "status: invalid") + text = text.replace("invalid_at: null", "invalid_at: 2026-01-15") + old.path.write_text(text) + assert_isolated(store, old.name) + assert store.delete(old.name).path.is_relative_to(store.layout.archived_memories) + + +@pytest.mark.parametrize("links", [["missing"], ["source"]]) +def test_invalid_relationship_additions_are_rejected(store, links): + memory(store, "source") + with pytest.raises(ValidationError): + store.correct("source", links=links) + assert store.find("source").links == [] + + +def test_relationship_replacement_and_invalid_endpoints(store): + memory(store, "source") + memory(store, "target") + assert store.correct("source", links=["target"]).links == ["target"] + assert store.correct("source", links=[]).links == [] + store.delete("target") + with pytest.raises(ValidationError): + store.correct("source", links=["target"]) + with pytest.raises(ValidationError): + store.correct("source", supersede_with="target") + with pytest.raises(ValidationError): + store.correct("target", body="resurrection") + + +def test_destination_collision_preserves_both_copies(store): + old = memory(store) + target = store.layout.archived_memories / old.path.relative_to(store.root) + target.parent.mkdir(parents=True) + target.write_text("existing historical evidence") + with pytest.raises(FileExistsError): + store.delete(old.name) + assert target.read_text() == "existing historical evidence" + assert not MemoryRecord.from_text(old.path.read_text()).is_active() + assert_isolated(store, old.name) + + +def test_failed_atomic_status_write_preserves_active_original(store, monkeypatch): + old = memory(store) + before = old.path.read_bytes() + original = pathlib.Path.replace + + def fail(path, target): + if target == old.path: + raise OSError("status write failed") + return original(path, target) + + with monkeypatch.context() as patch: + patch.setattr(pathlib.Path, "replace", fail) + with pytest.raises(OSError, match="status write failed"): + store.delete(old.name) + assert old.path.read_bytes() == before + assert store.read(old.name).text == old.body + assert list(old.path.parent.iterdir()) == [old.path] + store.delete(old.name) + assert_isolated(store, old.name) + + +def test_stale_manage_write_cannot_resurrect_archived_memory(store): + stale = memory(store) + store.delete(stale.name) + stale.weight = store.config.weight.ceiling + with pytest.raises(ValidationError): + store.write(stale) + assert not stale.path.exists() + assert_isolated(store, stale.name) + + +def test_supersede_and_archive_retry_preserve_successor_and_original_time(store, clock): + old = memory(store) + clock.advance(days=1) + new = store.record(type="fact", name="new-memory", abstract="New fact", supersedes=old.name) + invalid = store.find(old.name) + clock.advance(days=1) + again = store.delete(old.name) + assert again.superseded_by == new.name + assert again.invalid_at == invalid.invalid_at + assert again.path == invalid.path + assert store.read(new.name).record.is_active() + + +def test_missing_optional_legacy_fields_remain_compatible(store): + old = memory(store) + text = "\n".join( + line + for line in old.path.read_text().splitlines() + if not line.startswith(("status:", "provenance:", "valid_from:")) + ) + old.path.write_text(text) + assert store.read(old.name).text == old.body + store.delete(old.name) + assert store.read(old.name, include_invalid=True).record.provenance == [] + + +def test_existing_historical_links_survive_other_metadata_updates(store): + target = memory(store, "target") + source = memory(store, "source", links=[target.name]) + store.delete(target.name) + updated = store.correct(source.name, abstract="Updated quasar wording") + assert updated.links == [target.name] + assert store.correct(source.name, links=[]).links == [] + + +def test_archive_rejects_active_memory_and_repeated_archive_is_noop(store): + old = memory(store) + with pytest.raises(ValidationError): + store.archive.archive_memory(old) + archived = store.delete(old.name) + before = archived.path.read_bytes() + assert store.archive.archive_memory(archived) == archived.path + assert archived.path.read_bytes() == before + + +def test_invalid_evidence_does_not_become_an_automatic_redistill_request(store): + from agent_memory.core.manage import ACTION_REDISTILL_REQUESTED, Manage + + store.archive.append_session("shared", ["user: quasar evidence"]) + old = memory(store, provenance=["sessions/shared#0-0"]) + store.delete(old.name) + for _ in range(store.config.manage.raw_hit_min): + Recall(store).recall("quasar", deep=True) + assert ACTION_REDISTILL_REQUESTED not in { + action.kind for action in Manage(store).sleep().actions + } + + +def test_unlink_does_not_delete_target_or_evidence(store): + target = memory(store, "target", provenance=["original evidence"]) + source = memory(store, "source", links=[target.name]) + before = target.path.read_bytes() + store.correct(source.name, links=[]) + assert store.read(target.name).text == target.body + assert target.path.read_bytes() == before + assert store.archive.provenance_of(target.name) + + +def test_temporal_scope_uses_original_memory_location(store, clock): + old = memory(store) + scope = str(old.path.parent.relative_to(store.root)) + moment = clock.timestamp() + clock.advance(days=1) + store.delete(old.name) + assert old.name in {h.name for h in Recall(store).recall("Quasar", scope=scope, as_of=moment)} diff --git a/tests/unit/test_storage.py b/tests/unit/test_storage.py index 5459efce..6f06aad4 100644 --- a/tests/unit/test_storage.py +++ b/tests/unit/test_storage.py @@ -20,6 +20,7 @@ def test_init_creates_the_schema_set_and_the_archive_buckets(store): def test_recorded_file_round_trips_through_frontmatter(store): + store.record(type="fact", name="file-truth-invariant", abstract="Files are truth") written = store.record( abstract="Deploys run from the release branch only", type="procedure", @@ -112,7 +113,7 @@ def test_provenance_excerpt_is_stored_and_retrievable_by_name(store): assert written.provenance -def test_delete_marks_invalid_in_place_without_losing_the_file(seeded): +def test_delete_archives_invalid_without_losing_the_file(seeded): removed = seeded.delete("file-truth-invariant") assert removed.status == STATUS_INVALID assert removed.path.exists() From c6b46c041cd3b6f031e7312a60384699cb985de1 Mon Sep 17 00:00:00 2001 From: faj-design5260 Date: Thu, 10 Sep 2026 15:55:18 +0800 Subject: [PATCH 2/4] chore: narrow memory archival change scope --- README.md | 4 +- docs/TODO.md | 12 -- docs/design/index.md | 2 +- docs/design/memory-lifecycle.md | 55 ++++-- docs/plans/memory-archive-management.md | 16 +- docs/plans/memory-management-audit.md | 168 ------------------ packages/cli/src/agent_memory/cli/main.py | 7 +- .../core/src/agent_memory/core/prompts.py | 8 - packages/core/src/agent_memory/core/store.py | 19 -- packages/mcp/src/agent_memory/mcp/tools.py | 11 -- skills/agent-memory/SKILL.md | 8 - tests/system/test_archive_entries.py | 55 ++++-- tests/unit/test_indexer.py | 8 +- tests/unit/test_memory_archive.py | 41 ----- tests/unit/test_storage.py | 1 - 15 files changed, 94 insertions(+), 321 deletions(-) delete mode 100644 docs/TODO.md delete mode 100644 docs/plans/memory-management-audit.md diff --git a/README.md b/README.md index d6463dbc..b7e6c3a8 100644 --- a/README.md +++ b/README.md @@ -99,9 +99,7 @@ fields, status, timestamps, links, weight, and provenance; the body is free mark Invalidation moves a memory into `archive/memories/` and retains its raw evidence. Normal Recall, Read, Trace and Context return active memories; `read --history`, `trace --history` and temporal `recall --as-of` / `context --as-of` explicitly access history. Deep search can -still return raw evidence separately. New links must name another active memory in the -same store; `correct --link` replaces the full list. See the [operation audit and recovery -notes](docs/plans/memory-management-audit.md) for the boundaries and remaining policy choices. +still return raw evidence separately. See the [memory lifecycle and recovery notes](docs/design/memory-lifecycle.md). ## Proof it works diff --git a/docs/TODO.md b/docs/TODO.md deleted file mode 100644 index fbcd0404..00000000 --- a/docs/TODO.md +++ /dev/null @@ -1,12 +0,0 @@ -# Follow-ups - -- Decide explicit confirmation and host-level authorization for permanent GC, import and - cross-store changes; CLI labels alone do not identify a human. -- Decide preimage retention and recovery guarantees for split and in-place correction, - including stores outside Git and multi-file Manage failures. -- Decide whether direct link/unlink delta commands, relation audit history and per-operation - bounds are needed; current correct replaces the complete list. -- Decide if explicit deep Raw search should suppress evidence cited exclusively by invalid - memories; shared evidence and historical queries must remain available. -- Correct explicit feedback persistence: Store.feedback currently returns an adjusted object - without writing the weight back to disk. diff --git a/docs/design/index.md b/docs/design/index.md index b07f07da..e321e9c7 100644 --- a/docs/design/index.md +++ b/docs/design/index.md @@ -1,3 +1,3 @@ # Design -- [Memory lifecycle and management](memory-lifecycle.md) +- [Memory lifecycle](memory-lifecycle.md) diff --git a/docs/design/memory-lifecycle.md b/docs/design/memory-lifecycle.md index ffa80525..07397def 100644 --- a/docs/design/memory-lifecycle.md +++ b/docs/design/memory-lifecycle.md @@ -1,4 +1,4 @@ -# Memory lifecycle and management +# Memory lifecycle Memory has two states: active and invalid. Supersession invalidates a predecessor with a successor reference; deletion invalidates without one. Archive is a physical location, @@ -12,14 +12,45 @@ Normal reads and recall check current truth; explicit history reads and temporal retain history. Rebuild includes archived history. Existing invalid files remain readable as history without an automatic bulk migration. Raw evidence remains independently available. -Relationship additions require distinct active endpoints in the same store. Existing links -may remain as historical references. Correct replaces the complete link list, so removal -requires supplying the intended remaining list. Manage's automatic linking remains enabled. -Corrections require active memories and active successors. These are data validations, -not identity authorization or a scope permission system. - -Manage retains deterministic maintenance and capped executor decisions over existing -proposals. Reports and the decision ledger record outcomes; Git recovery depends on a -successful commit. Split and in-place corrections do not unconditionally preserve previous -content. Permanent collection remains outside the Manage and MCP menus; its human-only -label is not enforced authentication. Deployment owners must control shell/file access. +## Recovery and history access + +`Store.delete`, `Store.correct(supersede_with=...)`, `_write_one` predecessor invalidation, +and Manage's existing `Store.write` calls converge on `_persist`. It atomically replaces +file content with invalid metadata, then calls `Archive.archive_memory`, moving to +`archive/memories/`. `_project` refreshes MEMORY.md before SQLite. +`Indexer` includes archived memories in the existing history projection and rebuild. + +- Failed status persistence leaves the old file unchanged; temporary files are removed. +- Failed directory creation/move leaves an invalid file at its source. Retry `delete` on + that name; original invalid_at and successor remain. Destination collisions raise + FileExistsError and preserve both copies; resolve that collision with backed-up, + owner-reviewed recovery before retrying. No automatic overwrite or deletion. +- Successful move plus failed index projection leaves invalid truth in Archive. Default + Recall checks current file validity; Read/Trace scan when an indexed old path disappears. + Injection renders current active truth. Retry delete or rebuild repairs projections. +- Cached static MEMORY.md may remain stale after a move failure before projection, or an + external consumer may already hold injected context. Controlled injection renders fresh + truth; this cannot retract prior context or stop arbitrary direct filesystem reading. +- Read/Trace default to active; `include_invalid=True`, CLI `--history` and MCP read's + explicit include_invalid access history. CLI/MCP read report status and successor. + Temporal Context opens historical bodies; Recall scope still addresses original paths. +- Raw files are never moved or deleted by invalidation, including missing/shared evidence. + Deep Raw search remains explicit and unchanged; redistill counts historical citations. +- Existing valid old-format files work with omitted optional fields; old-location invalid + files are filtered without migration and archive when deletion is retried. Malformed + legacy state missing required invalid_at still requires repair; no speculative data fix. +- Multi-file supersede/merge remains nontransactional: a successor may be persisted before + predecessor archival fails. Inspect both names, then retry predecessor deletion to finish + archival; do not blindly replay a whole merge. Evidence remains in the old files. + +No bulk migration was executed. If owners later want old-location invalid files moved, +first enumerate those files with `Store.records(include_invalid=True)` and their paths, +back up the entire store including Archive and schemas, stop writers, then review the +list before applying existing delete per name. Verify history/Raw and rebuild. Rollback +restores that backup and rebuilds projections; this is a proposed runbook, not a migration +implemented or run by this PR. + +## Remaining history policy + +Explicit deep Raw search remains independent. Whether it should suppress evidence cited +exclusively by invalid memories is deferred; shared evidence and history must remain available. diff --git a/docs/plans/memory-archive-management.md b/docs/plans/memory-archive-management.md index 2f9e01c4..e100899b 100644 --- a/docs/plans/memory-archive-management.md +++ b/docs/plans/memory-archive-management.md @@ -1,12 +1,10 @@ -# Memory archive and management +# Invalid memory archival -Baseline: `34d12a2f8678d5561aba27bd8ff73c5ae4b6a258` (origin/main). -Branch: `feat/memory-archive-management`; isolated worktree, no experimental dependencies. +Baseline: `34d12a2f8678d5561aba27bd8ff73c5ae4b6a258` (main). -1. Document lifecycle and management boundaries. -2. Add failing lifecycle, failure-recovery and adapter regression tests. -3. Extend the existing Archive and shared persistence path; isolate normal reads. -4. Validate relationship additions and active correction endpoints without a new permission model. -5. Run related tests, full CI checks and a temporary-store CLI scenario; review and commit. +Invalidation uses the existing Archive and shared persistence path. Normal Read, Recall, +Trace and Context exclude invalid memories; explicit history remains available. Index +rebuild retains archived history, and Raw/provenance survives invalidation. -No real store migration, cleanup, deployment or merge is authorized by this plan. +Lifecycle, adapter, failure recovery, idempotence and shared-evidence tests verify this +scope. No store migration, production cleanup or unrelated index feature is included. diff --git a/docs/plans/memory-management-audit.md b/docs/plans/memory-management-audit.md deleted file mode 100644 index 350cd525..00000000 --- a/docs/plans/memory-management-audit.md +++ /dev/null @@ -1,168 +0,0 @@ -# Memory archive and Manage audit - -Baseline: `34d12a2f8678d5561aba27bd8ff73c5ae4b6a258` from freshly fetched `origin/main`. -The branch starts from main, not the Progressive Read, Raw evidence or Optional Index branches. -The original dirty worktree was left intact. Paths below are relative to this repository; -`core/` abbreviates `packages/core/src/agent_memory/core/`. - -## Code facts at baseline - -- `core/record.py::STATUSES`, `MemoryRecord.from_text`, `validate`, `invalidate`: only - `active` and `invalid`. A missing status defaults to active; missing provenance defaults - to an empty list. Invalid requires `invalid_at`. Supersession is invalid with - `superseded_by`, not a third state. No archived/deleted enum. -- `core/store.py::record_many -> _write_one`: schema placement, validation, provenance, - body-change restrictions, persistence and projection. In-place abstract/link updates - retain the body; a changed nonempty body requires `supersedes`. `_predecessor` requires - an active, distinct existing memory. Before this change it rewrote the invalid - predecessor at its original path. -- `Store.correct`: separately permits in-place body, abstract, date, provenance and link - replacement; it previously accepted invalid sources and invalid successors. This is - broader than `record`, and is not a separate generic patch API. -- `Store.delete -> write`: invalidated in place; repeated delete returned immediately, - without repairing a failed index projection. `Store.write` also receives invalidation - from `core/manage.py::_merge_exact_duplicates`, `_merge`, `_supersede`; `_delete` delegates - to `Store.delete`. Thus invalidation is not confined to the CLI delete command. -- `core/archive.py::Archive.append_provenance/append_session`, `core/paths.py::StoreLayout`: - physical `archive/provenance//` Markdown and `archive/sessions/` session files. - There was no Memory archival API. `truth_files/type_of` excluded all Archive content. -- `Store.read/trace/find` had no active filter. `core/agentic.py::_run` already filtered - `find` results by active status. `Store.records`, `core/memory_md.py::render`, and - `core/reconcile.py::build` filtered active memories. `core/context.py::build` opened - Recall hits, while `core/injection.py::payload` trusted cached MEMORY.md bytes. -- `core/search_index.py::upsert` selects active/history FTS tables; `core/recall.py::_eligible` - relied on cached invalid_at. Explicit `as_of` consults the validity interval. A failed - projection could leave an old active row eligible. `core/indexer.py::sync/rebuild` - rebuilds records, FTS and Raw from disk; invalid files previously stayed in the normal tree. -- `Store._store_provenance/trace_record` and `core/sessions.py::parse_pointer/resolve` bind - multiple memories to shared session ranges. Missing sessions yield no messages. - Invalidation never deletes Raw. Explicit deep Recall independently retrieves Raw, with - `source=raw`; this is not progressive Memory-to-Raw reading. -- `core/migrate.py::migrate` handles legacy retired/stale layout and status conversion; - it is an explicit migration command, not a normal invalidate entry. It is unchanged and - was not run on user data. - -## Operation matrix - -CLI entries live in `packages/cli/src/agent_memory/cli/main.py`; MCP entries in -`packages/mcp/src/agent_memory/mcp/tools.py::SCHEMAS/dispatch`. All store operations act on -one configured root. Recall scope is a path-prefix search filter, not write authorization. - -| Operation | Baseline entry / decision and execution | Mutation / recovery / actual risk | Recommendation and this change | -|---|---|---|---| -| Read/Recall/Context/Trace | CLI; MCP read/recall; executor `agentic._run`; core executes | Read-only truth; usage logged. Read/Trace exposed invalid bodies/evidence | Autonomous. Active checks plus explicit history access implemented | -| Create/update | CLI record, MCP memory_record; executor `reconcile.check -> Store.record_many` | Creates file or changes metadata. Body replacement restricted in record; abstract replacement loses previous wording without Git | Autonomous within existing store. Added links validated; no new scope policy | -| Patch | `reconcile.OP_ALIASES` maps patch to update | Same record path, not an independent Manage operation | No new patch API | -| Correct | CLI correct, MCP memory_correct -> Store.correct | Body/abstract/link set replacement; no guaranteed preimage without Git | Retain existing autonomy; active source/successor validation implemented. Broader content policy deferred | -| Link | record/correct links; `Manage._add_cooccurrence_links` is deterministic T0 | Slug references, not filesystem hardlinks. T0 adds both directions from repeated cooccurrence. Wrong links affect interpretation; do not delete targets | Autonomous with distinct active new endpoints in this store. Added validation; MCP now exposes existing correct links capability | -| Unlink | No standalone CLI/MCP/Manage verb; `Store.correct(links=...)` replaces full list | Removes references only; target/Raw retained. Prior set needs Git or caller knowledge for exact restoration | Complete-set replacement documented; `links=[]` via core/MCP clears. Malformed MCP arrays rejected. Delta API deferred | -| Supersede | record supersedes, correct supersede_with; Manage proposal/T0 exact duplicates | Invalidates predecessor, stores successor name; old content retained. Incorrect choice hides current knowledge | Existing automatic authority retained. Physical archive implemented; invalid successor rejected by correct | -| Merge | `Manage._review -> decide -> _merge` or CLI decide | Creates merged content, invalidates old files, unions links/provenance. LLM can omit distinctions; multi-file partial failure possible | Existing proposal menu, revalidation and per-kind sleep cap retained. Invalid files now archived; no transaction framework | -| Split | `Manage._split`, decided as above | Rewrites original with first part, creates others; provenance restricted to original pointers. Original body not independently snapshotted | Recovery policy deferred; no broader authority added | -| Invalidate/delete | CLI delete -> Store.delete; Manage delete proposal | Invalidity, not physical deletion. False judgement removes useful current knowledge, but old file/evidence survives | Autonomous within existing entry/proposal bounds; archive and retry repair implemented | -| Archive | No baseline Memory operation; Archive only wrote Raw/provenance | This change adds Archive.archive_memory via Store; no standalone Agent command | Deterministic consequence of invalidation, not another permission tier | -| Date/weight maintenance | Manage._normalise_dates/_settle_weights, deterministic | Rewrites metadata with configured floor/ceiling; Git-dependent prior values | Unchanged | -| Feedback | CLI/MCP -> Store.feedback | **Only mutates returned object at baseline; does not persist weight** | Audit finding only, fix deferred | -| Group merge/cluster | Manage._merge_near_duplicate_groups/_cluster -> Store.record | Moves schema groups, retains stable name/content/evidence. No semantic scope ACL | Existing autonomy retained; cross-scope policy deferred | -| Redistill request | Manage._request_redistill -> Pending | Schedules uncited repeatedly accessed Raw | Counts archived citations too, so invalidation alone does not requeue old evidence | -| Physical delete | CLI gc -> Store.gc; absent from Manage proposal menu and MCP | Deletes invalid files including archived memories; Raw retained. Irreversible without backup | Human confirmation recommended; existing human-run label is **not enforced authentication**. No new GC policy implemented | -| Inspect/rebuild/export | CLI -> Store/indexer/portability | Rebuildable projections or output copy; export normally includes Archive | Autonomous local maintenance; archived history included in rebuild | -| Import/migrate | CLI -> portability.import_into / migrate.migrate | Writes files, can overwrite/import history; outside Manage controls | Explicit owner authorization recommended; unchanged | - -## Existing controls and their limits - -`Manage._review` uses `_caps`, the open proposal menu and `decide`; `_open` regenerates -proposals before each decision. A stale/unknown proposal cannot dictate arbitrary targets. -Direct CLI `decide` is not subject to the per-sleep cap. `core/reasoning.py::parse` accepts -verdicts and text, not arbitrary executable operations. The executor's write path has -reconcile handles, but direct CLI/MCP record/correct do not use that handle boundary. - -`core/locking.py::store_lock` serializes writers. This change puts correction/deletion -read-modify-write under that lock and rejects stale writes that would reactivate invalid -memories. A complete Manage sleep is not atomic. `core/ledger.py::DecisionLedger.append` -records verdicts; `Manage._write_report` records actions and `Manage._commit` attempts one -Git commit when configured. Failed operations can precede a ledger/report entry. No -pre-operation snapshot, guaranteed Git commit, restore command, actor authentication, -RBAC, approval service or host filesystem sandbox exists in this runtime. - -Autonomous operations: reads, existing record/update and T0 maintenance. Deterministic -checks: relation additions, active endpoints, existing proposal bounds, validity/schema -checks and writer locking. Higher-authority recommendations: permanent GC, bulk overwrite, -cross-store modifications and direct filesystem/DB changes. Product decisions remain in -[the follow-up list](../TODO.md). An Agent with shell/file access can bypass tool menus; -this PR does not claim otherwise. - -## Implemented lifecycle and recovery - -`Store.delete`, `Store.correct(supersede_with=...)`, `_write_one` predecessor invalidation, -and Manage's existing `Store.write` calls converge on `_persist`. It atomically replaces -file content with invalid metadata, then calls `Archive.archive_memory`, moving to -`archive/memories/`. `_project` refreshes MEMORY.md before SQLite. -`Indexer` includes archived memories in the existing history projection and rebuild. - -- Failed status persistence leaves the old file unchanged; temporary files are removed. -- Failed directory creation/move leaves an invalid file at its source. Retry `delete` on - that name; original invalid_at and successor remain. Destination collisions raise - FileExistsError and preserve both copies; resolve that collision with backed-up, - owner-reviewed recovery before retrying. No automatic overwrite or deletion. -- Successful move plus failed index projection leaves invalid truth in Archive. Default - Recall checks current file validity; Read/Trace scan when an indexed old path disappears. - Injection renders current active truth. Retry delete or rebuild repairs projections. -- Cached static MEMORY.md may remain stale after a move failure before projection, or an - external consumer may already hold injected context. Controlled injection renders fresh - truth; this cannot retract prior context or stop arbitrary direct filesystem reading. -- Read/Trace default to active; `include_invalid=True`, CLI `--history` and MCP read's - explicit include_invalid access history. CLI/MCP read report status and successor. - Temporal Context opens historical bodies; Recall scope still addresses original paths. -- Raw files are never moved or deleted by invalidation, including missing/shared evidence. - Deep Raw search remains explicit and unchanged; redistill counts historical citations. -- Existing valid old-format files work with omitted optional fields; old-location invalid - files are filtered without migration and archive when deletion is retried. Malformed - legacy state missing required invalid_at still requires repair; no speculative data fix. -- Multi-file supersede/merge remains nontransactional: a successor may be persisted before - predecessor archival fails. Inspect both names, then retry predecessor deletion to finish - archival; do not blindly replay a whole merge. Evidence remains in the old files. - -No bulk migration was executed. If owners later want old-location invalid files moved, -first enumerate those files with `Store.records(include_invalid=True)` and their paths, -back up the entire store including Archive and schemas, stop writers, then review the -list before applying existing delete per name. Verify history/Raw and rebuild. Rollback -restores that backup and rebuilds projections; this is a proposed runbook, not a migration -implemented or run by this PR. - -## Verification map - -Existing `tests/unit/test_storage.py`, `test_supersede_on_write.py`, `test_recall.py`, -`test_indexer.py`, `test_manage.py`, `test_manage_reasoning.py`, `test_reconcile.py` and -`tests/system/test_entry_equivalence.py` cover lifecycle, temporal retrieval, rebuild, -proposal caps/unknown verdicts, targets, Git/ledger and adapter parity. - -`tests/unit/test_memory_archive.py` adds behavioral isolation, physical archive, explicit -history/rebuild, idempotence, atomic-write/move/index failures, destination conflict, missing -optional fields, missing/shared Raw, retained links, rejected endpoints, stale resurrection, -Raw redistill and temporal scope. `tests/system/test_archive_entries.py` covers CLI/MCP -history and relationship parity, malformed arrays and unavailable destructive tools. -The existing dangling-link test now uses legacy file input: controlled new writes reject -unknown link targets, while index rebuild continues reporting historical dangling links. - -## Local validation results - -Executed against this worktree's six package source directories via PYTHONPATH, using the -existing Python 3.12 environment (no experimental branch source imported): - -```bash -export PYTHONPATH=packages/core/src:packages/cli/src:packages/mcp/src:packages/executor/src:packages/adapters/src:packages/harness/src -/home/codexlab/code/agent-memory/.venv/bin/python -m pytest -q --cov=agent_memory --cov-report=term-missing --cov-fail-under=85 -/home/codexlab/code/agent-memory/.venv/bin/ruff check . -/home/codexlab/code/agent-memory/.venv/bin/mypy -git diff --check -``` - -Results: 411 tests passed; coverage 91.14% (required 85%); Ruff passed; Mypy passed for -66 source files; diff whitespace check passed. An additional temporary-store CLI smoke -executed 11 commands: init, record, recall/read before delete, delete, recall/read after -delete, history read, repeated delete, rebuild and Context. All expected exit codes, -physical paths and empty active results were verified. No live user Memory was touched. - -CI equivalent: `uv sync --all-packages`, `uv run ruff check .`, `uv run mypy`, then -`uv run pytest -q --cov=agent_memory --cov-report=term-missing --cov-fail-under=85`. diff --git a/packages/cli/src/agent_memory/cli/main.py b/packages/cli/src/agent_memory/cli/main.py index af2b9922..c1437b2e 100644 --- a/packages/cli/src/agent_memory/cli/main.py +++ b/packages/cli/src/agent_memory/cli/main.py @@ -116,12 +116,7 @@ def _parser() -> argparse.ArgumentParser: corrector.add_argument("--body", default=None) corrector.add_argument("--body-file", default=None) corrector.add_argument("--supersede-with", default=None) - corrector.add_argument( - "--link", - action="append", - default=None, - help="replace the complete link list; repeat for each retained target", - ) + corrector.add_argument("--link", action="append", default=None) corrector.add_argument("--provenance", action="append", default=[]) corrector.set_defaults(handler=_correct) diff --git a/packages/core/src/agent_memory/core/prompts.py b/packages/core/src/agent_memory/core/prompts.py index 11b52a7b..2c45141d 100644 --- a/packages/core/src/agent_memory/core/prompts.py +++ b/packages/core/src/agent_memory/core/prompts.py @@ -305,14 +305,6 @@ def repair(sheet: str, refused: str) -> str: such as `project` or `topic` name the subdirectory; pick an existing one, and pass `--create-group` only when a new one is genuinely needed. -## Relationship maintenance - -Use `mem record --link ` for links to existing active memories. To revise links, -`mem correct --link ` replaces the complete list; repeat `--link` for each -retained target. MCP `memory_correct` accepts `links`, with `[]` clearing the list. Choose -another active memory in this store as each new target. Existing historical links may stay. -Use these commands for changes so validation and indexing run together. - ## Write discipline {discipline} diff --git a/packages/core/src/agent_memory/core/store.py b/packages/core/src/agent_memory/core/store.py index 081a97cc..bcb851ec 100644 --- a/packages/core/src/agent_memory/core/store.py +++ b/packages/core/src/agent_memory/core/store.py @@ -205,7 +205,6 @@ def _write_one(self, spec: dict[str, object]) -> MemoryRecord: self._enforce_update_only(existing, candidate) record_module.validate(candidate, self.config, schema) record_module.canonicalise_dates(candidate) - self._validate_links(candidate, existing) predecessor = self._predecessor(candidate, supersedes) for excerpt in _as_sequence(spec.get("provenance")): @@ -305,19 +304,11 @@ def correct( current = self.find(name) if current is None or current.path is None: raise NotFoundError(f"no memory named {name}") - if not current.is_active(): - raise ValidationError( - [FieldError("status", "correction requires an active memory")] - ) now = self.clock.timestamp() if supersede_with: successor = self.find(supersede_with) if successor is None: raise NotFoundError(f"no memory named {supersede_with}") - if not successor.is_active(): - raise ValidationError( - [FieldError("supersede_with", "successor must be active")] - ) record_module.invalidate(current, successor.valid_from or now, supersede_with) if abstract is not None: current.abstract = abstract.strip() @@ -381,16 +372,6 @@ def _validate_write(self, record: MemoryRecord) -> None: raise ValidationError([FieldError("status", "an invalid memory cannot be reactivated")]) record_module.validate(record, self.config, self.schemas.get(record.type)) record_module.canonicalise_dates(record) - self._validate_links(record, existing) - - def _validate_links(self, record: MemoryRecord, existing: MemoryRecord | None) -> None: - added = set(record.links) - set(existing.links if existing else []) - for name in sorted(added): - target = self.find(name) - if name == record.name or target is None or not target.is_active(): - raise ValidationError( - [FieldError("links", f"{name} must name another active memory")] - ) def _persist(self, record: MemoryRecord) -> None: if record.path is None: diff --git a/packages/mcp/src/agent_memory/mcp/tools.py b/packages/mcp/src/agent_memory/mcp/tools.py index 036231b1..6c57e34b 100644 --- a/packages/mcp/src/agent_memory/mcp/tools.py +++ b/packages/mcp/src/agent_memory/mcp/tools.py @@ -56,11 +56,6 @@ "abstract": {"type": "string"}, "body": {"type": "string"}, "supersede_with": {"type": "string"}, - "links": { - "type": "array", - "items": {"type": "string"}, - "description": "Replace all links; empty list removes all links", - }, }, "required": ["name"], }, @@ -99,11 +94,6 @@ def dispatch(store: Store, tool: str, arguments: dict[str, object]) -> dict[str, def _require(tool: str, arguments: dict[str, object]) -> None: - if "links" in arguments and ( - not isinstance(arguments["links"], list) - or not all(isinstance(item, str) for item in arguments["links"]) - ): - raise ValidationError([FieldError("links", "must be an array of memory names")]) if "include_invalid" in arguments and not isinstance(arguments["include_invalid"], bool): raise ValidationError([FieldError("include_invalid", "must be a boolean")]) schema = SCHEMAS[tool] @@ -177,7 +167,6 @@ def _correct(store: Store, arguments: dict[str, object]) -> dict[str, object]: abstract=_optional(arguments, "abstract"), body=_optional(arguments, "body"), supersede_with=_optional(arguments, "supersede_with"), - links=_string_list(arguments["links"]) if "links" in arguments else None, ) return { "name": corrected.name, diff --git a/skills/agent-memory/SKILL.md b/skills/agent-memory/SKILL.md index 057a8fbe..413341d8 100644 --- a/skills/agent-memory/SKILL.md +++ b/skills/agent-memory/SKILL.md @@ -46,14 +46,6 @@ The store's `schemas/` directory lists the types and what each one is for. Group such as `project` or `topic` name the subdirectory; pick an existing one, and pass `--create-group` only when a new one is genuinely needed. -## Relationship maintenance - -Use `mem record --link ` for links to existing active memories. To revise links, -`mem correct --link ` replaces the complete list; repeat `--link` for each -retained target. MCP `memory_correct` accepts `links`, with `[]` clearing the list. Choose -another active memory in this store as each new target. Existing historical links may stay. -Use these commands for changes so validation and indexing run together. - ## Write discipline Recall first to see whether this atom already exists. diff --git a/tests/system/test_archive_entries.py b/tests/system/test_archive_entries.py index cd1d3cbe..dc0bbe36 100644 --- a/tests/system/test_archive_entries.py +++ b/tests/system/test_archive_entries.py @@ -1,10 +1,10 @@ import pytest from agent_memory.cli.main import main -from agent_memory.core.errors import NotFoundError, ValidationError +from agent_memory.core.errors import NotFoundError from agent_memory.mcp.tools import dispatch -def test_cli_and_mcp_share_archive_and_relationship_boundaries(store, capsys): +def test_cli_and_mcp_share_archive_boundaries(store, capsys): first = dispatch( store, "memory_record", @@ -15,12 +15,6 @@ def test_cli_and_mcp_share_archive_and_relationship_boundaries(store, capsys): "memory_record", {"type": "fact", "name": "quasar-new", "abstract": "Quasar new", "body": "New fact"}, ) - dispatch(store, "memory_correct", {"name": first["name"], "links": ["quasar-new"]}) - assert store.find(first["name"]).links == ["quasar-new"] - with pytest.raises(ValidationError): - dispatch( - store, "memory_record", {"type": "fact", "abstract": "Bad link", "links": ["missing"]} - ) dispatch(store, "memory_correct", {"name": first["name"], "supersede_with": "quasar-new"}) with pytest.raises(NotFoundError): dispatch(store, "memory_read", {"name": first["name"]}) @@ -30,15 +24,40 @@ def test_cli_and_mcp_share_archive_and_relationship_boundaries(store, capsys): ) assert main(["--store", str(store.root), "read", first["name"], "--history"]) == 0 assert "Old evidence" in capsys.readouterr().out - for tool in ("memory_gc", "memory_delete", "memory_unlink"): - with pytest.raises(ValidationError): - dispatch(store, tool, {"name": first["name"]}) -@pytest.mark.parametrize("links", ["target", None, [None], {}]) -def test_malformed_relationship_input_cannot_clear_links(store, links): - store.record(type="fact", name="target", abstract="Target memory") - store.record(type="fact", name="source", abstract="Source memory", links=["target"]) - with pytest.raises(ValidationError): - dispatch(store, "memory_correct", {"name": "source", "links": links}) - assert store.find("source").links == ["target"] +def test_cli_invalidation_retains_history_and_excludes_normal_reads(store, capsys): + prefix = ["--store", str(store.root)] + assert ( + main( + prefix + + [ + "record", + "--type", + "fact", + "--name", + "quasar-cli", + "--abstract", + "Quasar fact", + "--body", + "Original evidence", + ] + ) + == 0 + ) + capsys.readouterr() + assert main(prefix + ["read", "quasar-cli"]) == 0 + assert "Original evidence" in capsys.readouterr().out + assert main(prefix + ["recall", "Quasar"]) == 0 + assert "quasar-cli" in capsys.readouterr().out + for _ in range(2): + assert main(prefix + ["delete", "quasar-cli"]) == 0 + capsys.readouterr() + assert store.find("quasar-cli").path.is_relative_to(store.layout.archived_memories) + assert main(prefix + ["read", "quasar-cli"]) != 0 + capsys.readouterr() + for command in ("recall", "context"): + assert main(prefix + [command, "Quasar"]) == 0 + assert "quasar-cli" not in capsys.readouterr().out + assert main(prefix + ["read", "quasar-cli", "--history"]) == 0 + assert "Original evidence" in capsys.readouterr().out diff --git a/tests/unit/test_indexer.py b/tests/unit/test_indexer.py index 5dc47d11..22eb9ea5 100644 --- a/tests/unit/test_indexer.py +++ b/tests/unit/test_indexer.py @@ -49,15 +49,15 @@ def test_rebuild_is_idempotent(seeded): assert set(first.reindexed) == set(second.reindexed) -def test_a_legacy_dangling_link_is_reported_by_rebuild(store): +def test_a_dangling_link_is_reported_but_does_not_reject_the_write(store): written = store.record( abstract="Points at a memory that does not exist yet", type="fact", name="forward-reference", + links=["not-written-yet"], ) - written.links = ["not-written-yet"] - written.path.write_text(written.to_text()) - report = store.rebuild_index() + assert written.path.exists() + report = store.sync_index() assert ("forward-reference", "not-written-yet") in report.dangling_links diff --git a/tests/unit/test_memory_archive.py b/tests/unit/test_memory_archive.py index 515abe79..695637fe 100644 --- a/tests/unit/test_memory_archive.py +++ b/tests/unit/test_memory_archive.py @@ -104,28 +104,6 @@ def test_legacy_invalid_is_filtered_and_archived_on_retry(store): assert store.delete(old.name).path.is_relative_to(store.layout.archived_memories) -@pytest.mark.parametrize("links", [["missing"], ["source"]]) -def test_invalid_relationship_additions_are_rejected(store, links): - memory(store, "source") - with pytest.raises(ValidationError): - store.correct("source", links=links) - assert store.find("source").links == [] - - -def test_relationship_replacement_and_invalid_endpoints(store): - memory(store, "source") - memory(store, "target") - assert store.correct("source", links=["target"]).links == ["target"] - assert store.correct("source", links=[]).links == [] - store.delete("target") - with pytest.raises(ValidationError): - store.correct("source", links=["target"]) - with pytest.raises(ValidationError): - store.correct("source", supersede_with="target") - with pytest.raises(ValidationError): - store.correct("target", body="resurrection") - - def test_destination_collision_preserves_both_copies(store): old = memory(store) target = store.layout.archived_memories / old.path.relative_to(store.root) @@ -195,15 +173,6 @@ def test_missing_optional_legacy_fields_remain_compatible(store): assert store.read(old.name, include_invalid=True).record.provenance == [] -def test_existing_historical_links_survive_other_metadata_updates(store): - target = memory(store, "target") - source = memory(store, "source", links=[target.name]) - store.delete(target.name) - updated = store.correct(source.name, abstract="Updated quasar wording") - assert updated.links == [target.name] - assert store.correct(source.name, links=[]).links == [] - - def test_archive_rejects_active_memory_and_repeated_archive_is_noop(store): old = memory(store) with pytest.raises(ValidationError): @@ -227,16 +196,6 @@ def test_invalid_evidence_does_not_become_an_automatic_redistill_request(store): } -def test_unlink_does_not_delete_target_or_evidence(store): - target = memory(store, "target", provenance=["original evidence"]) - source = memory(store, "source", links=[target.name]) - before = target.path.read_bytes() - store.correct(source.name, links=[]) - assert store.read(target.name).text == target.body - assert target.path.read_bytes() == before - assert store.archive.provenance_of(target.name) - - def test_temporal_scope_uses_original_memory_location(store, clock): old = memory(store) scope = str(old.path.parent.relative_to(store.root)) diff --git a/tests/unit/test_storage.py b/tests/unit/test_storage.py index 6f06aad4..9a983d32 100644 --- a/tests/unit/test_storage.py +++ b/tests/unit/test_storage.py @@ -20,7 +20,6 @@ def test_init_creates_the_schema_set_and_the_archive_buckets(store): def test_recorded_file_round_trips_through_frontmatter(store): - store.record(type="fact", name="file-truth-invariant", abstract="Files are truth") written = store.record( abstract="Deploys run from the release branch only", type="procedure", From abd575e2acb07295b8140772282e02e37c46d25b Mon Sep 17 00:00:00 2001 From: faj-design5260 Date: Wed, 16 Sep 2026 23:20:29 +0800 Subject: [PATCH 3/4] fix: make invalid-memory archival atomic --- docs/design/memory-lifecycle.md | 35 +++--- .../phase-a-fix-gates.log | 24 ++++ .../phase_a_b_report.md | 119 ++++++++++++++++++ .../core/src/agent_memory/core/archive.py | 2 +- packages/core/src/agent_memory/core/store.py | 55 ++++++-- tests/unit/test_memory_archive.py | 93 +++++++++++++- 6 files changed, 294 insertions(+), 34 deletions(-) create mode 100644 experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log create mode 100644 experiments/runs/archive-management-12-20260916/phase_a_b_report.md diff --git a/docs/design/memory-lifecycle.md b/docs/design/memory-lifecycle.md index 07397def..5b5569bc 100644 --- a/docs/design/memory-lifecycle.md +++ b/docs/design/memory-lifecycle.md @@ -5,9 +5,10 @@ successor reference; deletion invalidates without one. Archive is a physical loc not a third state. Invalid memories retain content, identity, validity dates, links and provenance inside the existing Archive, separate from append-only raw evidence. -Store persists invalidity before moving a file. Failed moves leave invalid truth available -for retry; failed projections leave file truth authoritative. Retrying deletion completes -archival and projection without changing the original invalidation time or successor. +Store moves unchanged active bytes into Archive before committing invalid metadata there. +Failed moves or status writes restore the original active file. Failed projections leave +archived file truth authoritative; retrying deletion completes projection without changing +the original invalidation time or successor. Normal reads and recall check current truth; explicit history reads and temporal recall retain history. Rebuild includes archived history. Existing invalid files remain readable as history without an automatic bulk migration. Raw evidence remains independently available. @@ -15,22 +16,24 @@ as history without an automatic bulk migration. Raw evidence remains independent ## Recovery and history access `Store.delete`, `Store.correct(supersede_with=...)`, `_write_one` predecessor invalidation, -and Manage's existing `Store.write` calls converge on `_persist`. It atomically replaces -file content with invalid metadata, then calls `Archive.archive_memory`, moving to -`archive/memories/`. `_project` refreshes MEMORY.md before SQLite. +and Manage's existing `Store.write` calls converge on `_persist`. It prepares invalid +content in a temporary file, moves the original bytes to +`archive/memories/`, then replaces those bytes with invalid content. +If the replacement fails, it moves the original bytes back. `_project` refreshes MEMORY.md +before SQLite. `Indexer` includes archived memories in the existing history projection and rebuild. -- Failed status persistence leaves the old file unchanged; temporary files are removed. -- Failed directory creation/move leaves an invalid file at its source. Retry `delete` on - that name; original invalid_at and successor remain. Destination collisions raise - FileExistsError and preserve both copies; resolve that collision with backed-up, +- Failed status persistence or move leaves the active source unchanged; temporary files + are removed. A retry can perform the invalidation normally. +- Destination collisions raise FileExistsError before the source moves; both the active + source and pre-existing target stay unchanged. Resolve the collision with backed-up, owner-reviewed recovery before retrying. No automatic overwrite or deletion. - Successful move plus failed index projection leaves invalid truth in Archive. Default Recall checks current file validity; Read/Trace scan when an indexed old path disappears. Injection renders current active truth. Retry delete or rebuild repairs projections. -- Cached static MEMORY.md may remain stale after a move failure before projection, or an - external consumer may already hold injected context. Controlled injection renders fresh - truth; this cannot retract prior context or stop arbitrary direct filesystem reading. +- Cached static MEMORY.md may remain stale after a successful move followed by projection + failure, or an external consumer may already hold injected context. Controlled injection + renders fresh truth; this cannot retract prior context or stop arbitrary filesystem reads. - Read/Trace default to active; `include_invalid=True`, CLI `--history` and MCP read's explicit include_invalid access history. CLI/MCP read report status and successor. Temporal Context opens historical bodies; Recall scope still addresses original paths. @@ -39,9 +42,9 @@ file content with invalid metadata, then calls `Archive.archive_memory`, moving - Existing valid old-format files work with omitted optional fields; old-location invalid files are filtered without migration and archive when deletion is retried. Malformed legacy state missing required invalid_at still requires repair; no speculative data fix. -- Multi-file supersede/merge remains nontransactional: a successor may be persisted before - predecessor archival fails. Inspect both names, then retry predecessor deletion to finish - archival; do not blindly replay a whole merge. Evidence remains in the old files. +- A single successor write rolls back its new file if predecessor archival fails. A Manage + merge involving several memories still commits each successful write separately; inspect + individual records before retrying a partially completed merge. No bulk migration was executed. If owners later want old-location invalid files moved, first enumerate those files with `Store.records(include_invalid=True)` and their paths, diff --git a/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log b/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log new file mode 100644 index 00000000..cc937b64 --- /dev/null +++ b/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log @@ -0,0 +1,24 @@ +Archive Management Phase A fix verification — 2026-09-16 +Working tree: feature-checkout, feat/memory-archive-management +Pre-fix HEAD: c6b46c041cd3b6f031e7312a60384699cb985de1 +Environment: Python 3.12.3; uv workspace with all packages/extras; UV_CACHE_DIR=/tmp/uv-cache + +Command: UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/unit/test_memory_archive.py tests/system/test_archive_entries.py +Result: 17 passed + +Command: UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/unit/test_memory_archive.py tests/unit/test_recall.py tests/unit/test_supersede_on_write.py tests/unit/test_indexer.py tests/unit/test_context.py tests/unit/test_manage.py tests/unit/test_manage_reasoning.py tests/system/test_cli.py tests/system/test_archive_entries.py tests/system/test_sleep_stores.py +Result: 123 passed in 21.22s + +Command: UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q +Result: 406 passed in 34.08s + +Command: UV_CACHE_DIR=/tmp/uv-cache uv run ruff check . +Result: All checks passed! + +Command: UV_CACHE_DIR=/tmp/uv-cache uv run mypy +Result: Success: no issues found in 66 source files + +Command: git diff --check +Result: exit 0, no output + +Phase B: existing three paired answer probes retained; no answer reruns or expansion. diff --git a/experiments/runs/archive-management-12-20260916/phase_a_b_report.md b/experiments/runs/archive-management-12-20260916/phase_a_b_report.md new file mode 100644 index 00000000..8e9247ee --- /dev/null +++ b/experiments/runs/archive-management-12-20260916/phase_a_b_report.md @@ -0,0 +1,119 @@ +# Archive Management: contract and answer-sensitive probes + +## Scope and identities + +This follow-up does **not** run the original active-only 12×2 exam. The pre-fix treatment +code was `c6b46c041cd3b6f031e7312a60384699cb985de1`; the feature-plan baseline is +`34d12a2f8678d5561aba27bd8ff73c5ae4b6a258`. The dirty current worktree was not +switched. The fix and contract tests were developed in the isolated `feature-checkout/`. +Answer probes used exact Git archive snapshots of the two pre-fix commits and are not +rerun after this lifecycle-only fix. + +## Phase A — Archive feature contract: PASS + +The original Phase A audit was **FAIL** despite 72 passing tests (`phase-a-tests.log`): +`Store._persist` wrote invalid status to the active file before `Archive.archive_memory` +attempted the move. A move failure or destination collision left an invalid file under +the active tree. The old tests expected that retryable half-state. + +The fix prepares invalid content in a temporary file under Archive, moves unchanged +active bytes to the archive path, then replaces the archived bytes with invalid content. +A move failure leaves the active source untouched. If the subsequent status replacement +fails, the original bytes move back to the active source. A pre-existing destination +raises `FileExistsError` without changing either file. A single `Store.record` supersede +rolls back its candidate file when predecessor archival fails. Projection still follows +the successful filesystem transition; a projection failure is repaired by retry/rebuild. +The failure guarantee covers operation errors with a working rollback filesystem; it is +not a cross-file crash transaction. + +| Required behavior | Evidence | +|---|---| +| Delete invalidates and moves | `test_archive_lifecycle_history_and_rebuild`; `test_cli_invalidation_retains_history_and_excludes_normal_reads` | +| Supersede archives predecessor | `test_supersede_and_archive_retry_preserve_successor_and_original_time`; `test_cli_and_mcp_share_archive_boundaries` | +| Manage invalidation archives | **Added** `test_manage_duplicate_archives_only_invalid_copy_and_rebuild_keeps_active` | +| Default Read denies invalid/archived | `assert_isolated`; both CLI/MCP system tests | +| Normal Recall excludes it | `assert_isolated`; both CLI/MCP system tests | +| Rebuild keeps it out of active retrieval | `test_archive_lifecycle_history_and_rebuild`; added Manage/rebuild test | +| Provenance/history survives | `test_shared_and_missing_raw_survive_invalidation`; `test_archive_lifecycle_history_and_rebuild` | +| Active memory is not misarchived | `test_archive_rejects_active_memory_and_repeated_archive_is_noop`; added Manage/rebuild test | +| Status-write failure restores active original | `test_failed_atomic_status_write_preserves_active_original` | +| Move failure preserves active and retry succeeds | `test_move_failure_preserves_active_and_retry_succeeds` | +| Destination collision preserves both unchanged | `test_destination_collision_preserves_both_copies` | +| Failed supersede leaves no successor | `test_supersede_move_failure_keeps_predecessor_and_no_successor` | +| Failed `Store.correct` supersede keeps both originals active | `test_correct_move_failure_keeps_original_and_successor` | +| Projection failure can be retried | `test_projection_failure_does_not_leak_and_retry_repairs_index` | +| Repeated archive/delete is idempotent | `test_archive_lifecycle_history_and_rebuild`; `test_archive_rejects_active_memory_and_repeated_archive_is_noop` | + +Post-fix focused Archive/CLI/MCP contract tests: **17 passed**; broader lifecycle, +recall, index, Manage and CLI suites: **123 passed**. Full pytest: **406 passed**. +Ruff, mypy and `git diff --check` passed. The exact gate commands and +outputs are in `phase-a-fix-gates.log`. No Manage, Vector, Progressive, Observation or +Evidence Sufficiency policy was changed. + +## Phase B — real path and three paired answers + +The Codex Host uses `workspace-write` and adds the Store directory, so direct filesystem +commands are permitted. The native Agent prompt and skill instruct `mem context`, Recall, +and Read; they do not suggest `ls`/`grep` fallback. Baseline `mem read ` still +returns the invalid record, while Archive rejects it by default. Explicit history works +on Archive. `command_exposure.json` records actual CLI and `rg --files` checks of all three +case Stores. Thus the potential filesystem/direct-read path exists, but permission alone +does not show that the Agent uses it. + +Three cases came from the official LongMemEval source sessions and existing memories: +Negroni attempts 5→10, Crash Course videos 12→15, and Ticket to Ride high score 124→132. +`sensitive-probes-v3/manifest.json` stores the question/truth, source-session IDs, +original memory file hashes, field mapping needed by the target's schema, and a hash of +each canonical Store. Each canonical Store was created once through `Store.record` from +the original old/new memory text, with the source raw session archive copied unchanged; +both arms then received byte-identical copies. Both arms called the same official +`Store.correct(old, supersede_with=new)` operation. The baseline left the invalid file in +its active tree; Archive moved it under `archive/memories`. The generated `MEMORY.md`, +config and final answer prompt hash matched within each pair. + +An earlier preparation attempt with whole historical Stores is retained in +`sensitive-probes/` and marked abandoned. It made no answer calls: that corpus uses a +newer config and folder layout incompatible with the target code. The completed v3 set +uses source-grounded entries written through the target's API. + +| Question | Baseline answer | Archive answer | Judge | Agent commands in both arms | Archive-sensitive evidence | +|---|---|---|---|---|---| +| `603deb26` | 10 times | 10 times | both correct | `mem context` | old 5-attempt memory not surfaced | +| `5831f84d` | 15 Crash Course videos | 15 videos | both correct | `mem context` | old 12-video memory not surfaced | +| `0e4e4c46` | 132 points | 132 points | both correct | `mem context` | old 124-point memory file not surfaced | + +For all 6 answer runs, `mem context` caused one Recall and one Read of the **new active** +memory. No Agent `ls`, `grep`, `rg`, `cat`, direct `mem read `, or other filesystem +fallback appeared in the captured Host traces. The per-case Recall and Read ID sets were +identical across arms. The old invalid memory file was never surfaced. In `0e4e4c46`, +the new active memory's body itself mentions the previous 124-point score; Archive does not +remove historical values embedded in current valid evidence. + +Summary: Baseline **3/3**, Archive **3/3**; correctness transitions 0; Recall used 6/6, +memory Read 6/6, filesystem fallback 0/6, direct old-memory Read 0/6, archive-sensitive +file exposure 0/6, changed Recall sets 0/3, changed Read sets 0/3. One answer string changed +wording without changing its factual answer. Mean answer latency was 20.08 s versus +21.42 s; Codex reported 19,271 versus 19,585 answer tokens. These small differences are +not an archive quality signal. Judge used the same Codex model and the branch's five-vote +yes/no rubric, so it provides votes rather than a free-text reason. Each Host/Judge call +used one attempt; a failed in-sandbox Host preflight and the subsequent successful +out-of-sandbox preflight are recorded separately and were not answer retries. + +Raw results and commands are in `sensitive-probes-v3/results/`, +`sensitive-probes-v3/logs/`, `sensitive-probes-v3/paired_analysis.json`, and +`sensitive-probes-v3/command_exposure.json`. + +## Decision + +**NO E2E-SENSITIVE PATH FOUND** in these three source-grounded probes. The code permits +direct-read and filesystem exposure in principle, and the baseline's invalid file is +indeed reachable by those commands. The Agent actually followed `mem context` in every +run and saw the same active evidence in both arms. Do not expand to 12 answer pairs on +this evidence. Treat the current branch primarily as lifecycle/integration behavior; +the Archive feature as lifecycle/integration correctness, with the failed-move contract +now covered by Phase A tests. No answer-quality improvement is claimed. + +The probes intentionally use minimal two-memory Stores reconstructed through official +record APIs because historical frozen Stores use a newer incompatible config/layout. +They demonstrate the observed path for these three questions, not that filesystem fallback +can never occur with larger Stores or different questions. diff --git a/packages/core/src/agent_memory/core/archive.py b/packages/core/src/agent_memory/core/archive.py index c81c7341..7d7eaafd 100644 --- a/packages/core/src/agent_memory/core/archive.py +++ b/packages/core/src/agent_memory/core/archive.py @@ -30,7 +30,7 @@ def archive_memory(self, record: MemoryRecord) -> pathlib.Path: return source target = self._layout.archived_memories / source.relative_to(self._layout.root) target.parent.mkdir(parents=True, exist_ok=True) - if target.exists(): + if target.exists() or target.is_symlink(): raise FileExistsError(f"archive destination already exists: {target}") source.rename(target) record.path = target diff --git a/packages/core/src/agent_memory/core/store.py b/packages/core/src/agent_memory/core/store.py index bcb851ec..ced12e28 100644 --- a/packages/core/src/agent_memory/core/store.py +++ b/packages/core/src/agent_memory/core/store.py @@ -213,13 +213,31 @@ def _write_one(self, spec: dict[str, object]) -> MemoryRecord: candidate.provenance.append(pointer) self._reject_facts_dated_after_their_evidence(candidate) target.parent.mkdir(parents=True, exist_ok=True) - target.write_text(candidate.to_text(), encoding="utf-8") - if moved_from is not None and moved_from != target: - moved_from.unlink(missing_ok=True) - if predecessor is not None and predecessor.path is not None: - record_module.invalidate(predecessor, candidate.valid_from or now, candidate.name) - predecessor.updated = now - self._persist(predecessor) + previous_target = ( + target.read_bytes() if predecessor is not None and target.exists() else None + ) + previous_moved = ( + moved_from.read_bytes() + if predecessor is not None and moved_from is not None and moved_from.exists() + else None + ) + try: + self._persist(candidate) + if moved_from is not None and moved_from != target: + moved_from.unlink(missing_ok=True) + if predecessor is not None and predecessor.path is not None: + record_module.invalidate(predecessor, candidate.valid_from or now, candidate.name) + predecessor.updated = now + self._persist(predecessor) + except Exception: + if predecessor is not None: + if previous_target is None: + target.unlink(missing_ok=True) + else: + target.write_bytes(previous_target) + if moved_from is not None and previous_moved is not None: + moved_from.write_bytes(previous_moved) + raise return candidate def _successor_placement(self, placed: placement.Placement) -> placement.Placement: @@ -376,18 +394,33 @@ def _validate_write(self, record: MemoryRecord) -> None: def _persist(self, record: MemoryRecord) -> None: if record.path is None: raise NotFoundError(f"{record.name} has no location on disk") + source = record.path + moving = not record.is_active() and not self.layout.is_archived_memory(source) + destination = ( + self.layout.archived_memories / source.relative_to(self.root) if moving else source + ) + destination.parent.mkdir(parents=True, exist_ok=True) with tempfile.NamedTemporaryFile( - mode="w", encoding="utf-8", dir=record.path.parent, delete=False + mode="w", encoding="utf-8", dir=destination.parent, delete=False ) as handle: temporary = pathlib.Path(handle.name) try: handle.write(record.to_text()) handle.flush() - temporary.replace(record.path) + if moving: + # Move the unchanged active bytes first. A failed move leaves the + # source active; a failed status write moves those bytes back. + self.archive.archive_memory(record) + try: + temporary.replace(destination) + except Exception: + destination.rename(source) + record.path = source + raise + else: + temporary.replace(destination) finally: temporary.unlink(missing_ok=True) - if not record.is_active(): - self.archive.archive_memory(record) def feedback(self, name: str, delta: float) -> MemoryRecord: current = self.find(name) diff --git a/tests/unit/test_memory_archive.py b/tests/unit/test_memory_archive.py index 695637fe..3065b2f8 100644 --- a/tests/unit/test_memory_archive.py +++ b/tests/unit/test_memory_archive.py @@ -47,9 +47,11 @@ def test_archive_lifecycle_history_and_rebuild(store, clock): assert store.delete(old.name).path.read_bytes() == before -def test_move_failure_is_invalid_retryable_and_keeps_raw(store, monkeypatch): +def test_move_failure_preserves_active_and_retry_succeeds(store, monkeypatch): old = memory(store, provenance=["original evidence"]) source = old.path + before = source.read_bytes() + target = store.layout.archived_memories / source.relative_to(store.root) evidence = store.archive.provenance_of(old.name)[0] original = pathlib.Path.rename @@ -62,10 +64,14 @@ def fail(path, target): patch.setattr(pathlib.Path, "rename", fail) with pytest.raises(OSError, match="archive move failed"): store.delete(old.name) - assert not MemoryRecord.from_text(source.read_text()).is_active() + assert source.read_bytes() == before + assert MemoryRecord.from_text(source.read_text()).is_active() + assert not target.exists() assert evidence.exists() + assert store.read(old.name).text == old.body + assert store.delete(old.name).path == target + assert not source.exists() assert_isolated(store, old.name) - assert store.delete(old.name).path.is_relative_to(store.layout.archived_memories) def test_projection_failure_does_not_leak_and_retry_repairs_index(store, monkeypatch): @@ -106,23 +112,26 @@ def test_legacy_invalid_is_filtered_and_archived_on_retry(store): def test_destination_collision_preserves_both_copies(store): old = memory(store) + before = old.path.read_bytes() target = store.layout.archived_memories / old.path.relative_to(store.root) target.parent.mkdir(parents=True) target.write_text("existing historical evidence") with pytest.raises(FileExistsError): store.delete(old.name) assert target.read_text() == "existing historical evidence" - assert not MemoryRecord.from_text(old.path.read_text()).is_active() - assert_isolated(store, old.name) + assert old.path.read_bytes() == before + assert MemoryRecord.from_text(old.path.read_text()).is_active() + assert store.read(old.name).text == old.body def test_failed_atomic_status_write_preserves_active_original(store, monkeypatch): old = memory(store) before = old.path.read_bytes() + archive_path = store.layout.archived_memories / old.path.relative_to(store.root) original = pathlib.Path.replace def fail(path, target): - if target == old.path: + if target == archive_path: raise OSError("status write failed") return original(path, target) @@ -131,6 +140,7 @@ def fail(path, target): with pytest.raises(OSError, match="status write failed"): store.delete(old.name) assert old.path.read_bytes() == before + assert not archive_path.exists() assert store.read(old.name).text == old.body assert list(old.path.parent.iterdir()) == [old.path] store.delete(old.name) @@ -160,6 +170,51 @@ def test_supersede_and_archive_retry_preserve_successor_and_original_time(store, assert store.read(new.name).record.is_active() +def test_supersede_move_failure_keeps_predecessor_and_no_successor(store, monkeypatch): + old = memory(store) + before = old.path.read_bytes() + original = pathlib.Path.rename + + def fail(path, target): + if path == old.path: + raise OSError("archive move failed") + return original(path, target) + + with monkeypatch.context() as patch: + patch.setattr(pathlib.Path, "rename", fail) + with pytest.raises(OSError, match="archive move failed"): + store.record( + type="fact", name="new-memory", abstract="New fact", supersedes=old.name + ) + assert old.path.read_bytes() == before + assert store.read(old.name).record.is_active() + assert store.find("new-memory") is None + assert store.record( + type="fact", name="new-memory", abstract="New fact", supersedes=old.name + ).is_active() + assert_isolated(store, old.name) + + +def test_correct_move_failure_keeps_original_and_successor(store, monkeypatch): + old = memory(store) + successor = memory(store, name="new-memory") + before = old.path.read_bytes() + original = pathlib.Path.rename + + def fail(path, target): + if path == old.path: + raise OSError("archive move failed") + return original(path, target) + + with monkeypatch.context() as patch: + patch.setattr(pathlib.Path, "rename", fail) + with pytest.raises(OSError, match="archive move failed"): + store.correct(old.name, supersede_with=successor.name) + assert old.path.read_bytes() == before + assert store.read(old.name).record.is_active() + assert store.read(successor.name).record.is_active() + + def test_missing_optional_legacy_fields_remain_compatible(store): old = memory(store) text = "\n".join( @@ -203,3 +258,29 @@ def test_temporal_scope_uses_original_memory_location(store, clock): clock.advance(days=1) store.delete(old.name) assert old.name in {h.name for h in Recall(store).recall("Quasar", scope=scope, as_of=moment)} + + +def test_manage_duplicate_archives_only_invalid_copy_and_rebuild_keeps_active(store): + from agent_memory.core.manage import ACTION_DUPLICATE_MERGED, Manage + + original = memory(store, name="first-copy") + duplicate = memory(store, name="second-copy") + original_path = original.path + duplicate_path = duplicate.path + + report = Manage(store).sleep() + assert ACTION_DUPLICATE_MERGED in {action.kind for action in report.actions} + archived = store.find(duplicate.name) + assert archived.status == "invalid" + assert archived.superseded_by == original.name + assert archived.path == store.layout.archived_memories / duplicate_path.relative_to(store.root) + assert not duplicate_path.exists() + assert original_path.exists() + assert store.read(original.name).record.is_active() + + store.rebuild_index() + Manage(store).sleep() + assert original_path.exists() + assert store.find(original.name).path == original_path + assert store.find(duplicate.name).path == archived.path + assert duplicate.name not in {hit.name for hit in Recall(store).recall("Quasar")} From 2fc8aca68a9cefabd814a3dc3edec9e9f69dc6c6 Mon Sep 17 00:00:00 2001 From: faj-design5260 Date: Wed, 16 Sep 2026 23:26:27 +0800 Subject: [PATCH 4/4] docs: correct archive contract test count --- .../runs/archive-management-12-20260916/phase-a-fix-gates.log | 2 +- .../runs/archive-management-12-20260916/phase_a_b_report.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log b/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log index cc937b64..5839f18c 100644 --- a/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log +++ b/experiments/runs/archive-management-12-20260916/phase-a-fix-gates.log @@ -4,7 +4,7 @@ Pre-fix HEAD: c6b46c041cd3b6f031e7312a60384699cb985de1 Environment: Python 3.12.3; uv workspace with all packages/extras; UV_CACHE_DIR=/tmp/uv-cache Command: UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/unit/test_memory_archive.py tests/system/test_archive_entries.py -Result: 17 passed +Result: 18 passed in 1.97s Command: UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/unit/test_memory_archive.py tests/unit/test_recall.py tests/unit/test_supersede_on_write.py tests/unit/test_indexer.py tests/unit/test_context.py tests/unit/test_manage.py tests/unit/test_manage_reasoning.py tests/system/test_cli.py tests/system/test_archive_entries.py tests/system/test_sleep_stores.py Result: 123 passed in 21.22s diff --git a/experiments/runs/archive-management-12-20260916/phase_a_b_report.md b/experiments/runs/archive-management-12-20260916/phase_a_b_report.md index 8e9247ee..cef67c67 100644 --- a/experiments/runs/archive-management-12-20260916/phase_a_b_report.md +++ b/experiments/runs/archive-management-12-20260916/phase_a_b_report.md @@ -44,7 +44,7 @@ not a cross-file crash transaction. | Projection failure can be retried | `test_projection_failure_does_not_leak_and_retry_repairs_index` | | Repeated archive/delete is idempotent | `test_archive_lifecycle_history_and_rebuild`; `test_archive_rejects_active_memory_and_repeated_archive_is_noop` | -Post-fix focused Archive/CLI/MCP contract tests: **17 passed**; broader lifecycle, +Post-fix focused Archive/CLI/MCP contract tests: **18 passed**; broader lifecycle, recall, index, Manage and CLI suites: **123 passed**. Full pytest: **406 passed**. Ruff, mypy and `git diff --check` passed. The exact gate commands and outputs are in `phase-a-fix-gates.log`. No Manage, Vector, Progressive, Observation or