feat(memory): add consolidate() method to FileMemoryStore

[maisieyanz]

Description

A FileMemoryStore accumulates knowledge over time: duplicate facts pile up, later entries contradict earlier ones, and files drift into the wrong place. Nothing today reconciles that — the store only grows. consolidate() gives it an offline maintenance pass that an operator or scheduled job can run to keep the corpus coherent.

The consolidation strategy is plan-then-execute. A single structured-output call produces a full action plan over every file; deterministic code, not the model, then validates and applies it. The model never touches storage directly. Validation is the gate: it accumulates all violations at once (unknown paths, disallowed actions for the requested operations, and any write/delete collision that would destroy content), and throws with every violation named at once, so a rejected plan is diagnosable in one pass rather than one error at a time. Execution runs all writes before any deletes, so merged content lands before its sources are removed. A crash mid-run leaves duplicates rather than dropping content that had nowhere else to live.

This protects sources, not overwrite targets: an update rewrites in place by design. Consolidation reserves consolidation-changelog.md and excludes it from the working set, search(), and add(), so it can't ingest or rewrite its own audit log.

Public API Changes

FileMemoryStore gains a consolidate() method, plus a ConsolidateConfig and ConsolidateOperation type:

import { FileMemoryStore } from '@strands-agents/sdk/vended-memory-stores/file-memory-store'

const store = new FileMemoryStore({ name: 'agent-memory' })

// Run all maintenance operations
await store.consolidate({ model })

// Or scope to specific operations, with limits
await store.consolidate({
  model,
  operations: ['deduplicate', 'prune'],
  maxDirectories: 8,   // default 8
  maxFiles: 100,       // default 100 — bounds planner input
  maxActionsPerPlan: 1000,  // default 1000 — bounds planner output
})

ConsolidateOperation is one of 'deduplicate' | 'resolveContradictions' | 'deriveInsights' | 'prune' | 'reorganize'.

consolidate() throws when the store exceeds maxFiles, when the plan exceeds maxActionsPerPlan, when the model returns no structured output, when the plan fails validation, or when a concurrent run is already in flight on the same store instance.

a store named agent-memory lays out like this:

./.strands/                              # LocalFileStorage baseDir
└── memory/                              # STORAGE_NAMESPACE — isolates memory from
    └── agent-memory/                    #   sessions/context-offload on a shared backend
        ├── consolidation-changelog.md   # reserved: append-only audit log, one entry per run
        └── facts/                       # default target for add() without metadata.path
            ├── dark-mode-preference.md
            └── deploy-process.md

reorganize may group files into additional top-level directories within the
store's namespace, subject to the layout rules below.

Every knowledge file is markdown with YAML frontmatter carrying a description:

---
description: "Theme preference: dark mode"
---

User prefers dark mode for all editors

Layout rules enforced by validatePath() — the planner is untrusted, so a plan
violating any of these is rejected before a single storage mutation:

Rule	Rationale
Paths end in .md	The store only holds markdown knowledge files
At most one level of nesting	Keeps the tree navigable; no deep hierarchies to traverse
At most maxDirectories dirs (default 8)	Groups knowledge by topic instead of fragmenting into sparse dirs
Dir names match ^[a-z0-9-]{1,30}$	Stable, portable keys across storage backends
No ./.. segments or backslashes	Prevents traversal outside the store's namespace
consolidation-changelog.md is reserved	A plan must not clobber its own audit trail

Related Issues

Documentation PR

Type of Change

Bug fix
New feature
Breaking change
Documentation update
Other (please describe):

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.

• I ran hatch run prepare

Checklist

• I have read the CONTRIBUTING document
• I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
• My change is focused and reasonably small; I have split unrelated work into separate PRs
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[maisieyanz]

@strandly-the-agent Review my pr

[strandly-the-agent]

Superseded by the formal review on the latest head.

My earlier review at b46e008 was premature and incorrectly claimed data integrity was guaranteed. The PR has since moved to 6623fff, and cold adversarial testing found reproducible integrity and resource-safety blockers. Please use the latest formal review; this comment is retained only to make that correction explicit.

[maisieyanz]

@strandly-the-agent review

[strandly-the-agent]

Changes requested — six reproduced integrity/resource failures remain

✅ Reviewed clean exact head 6623fff12406ac845af28aa609e0d6c943bf89bb; GitHub status rollup is SUCCESS.
✅ Focused suites: 101/101 passed; type-check, lint, and diff-check passed.
✅ Every adversarial scenario was rerun cold twice with byte-identical output.
✅ Same-instance overlap, post-return action limits, and changelog-error masking are fixed.
🔴 Path identity, non-atomic/stale apply, read-only mutation, empty output, prompt framing, and unbounded invocation still block merge.
⚠️ This review supersedes my premature comment at b46e008, especially its incorrect claim that data integrity was guaranteed.
⚠️ Advanced API/adversarial tiers were unavailable under data-retention mode; both passes completed after retry on the default tier.

API gate: needs-api-review is present, but this is a substantial public primitive and needs an API meeting before completion. Two blocking questions: should omitted operations authorize destructive prune/overwrite, and should planning/application be separated with explicit authorization plus a structured result instead of Promise<void>?

Per-pass breakdown

• Correctness/adversarial: reproduced Windows namespace escape, case-alias deletion, partial writes without audit, stale-snapshot data loss, mutation under writable: false, empty-content erasure, prompt-induced victim deletion, and an unbounded 25-call planner loop.
• Test quality: the new tests cover the three latest hardenings, but none of the six blocker groups above.
• LLM context: fixed triple-fence framing lets stored text become planner instructions; deterministic validation cannot prove semantic provenance.
• API/DevX: substantial primitive needs an API meeting. Main decisions are destructive defaults, plan/apply separation, consistency guarantees, and structured outcomes.
• Docs: lines 490–492 incorrectly claim an aborted write pass leaves the store unchanged.
• Suppressed from inline review: searchable changelog, derive-insights wording mismatch, NaN/large-output edge cases, maxDirectories wording, and broader API alternatives. These are lower-value or subsumed by the blockers/API questions above.
• Not repeated: the existing human enum question at line 49.

Aggregation: 5 specialist passes, 14 raw items → 6 inline blocker groups + 2 summary-only API questions.

[strandly-the-agent]

🔴 Block — applying the plan is neither atomic nor protected from stale snapshots

Writes execute sequentially here, and moves later write data captured by _readAllFiles() before deleting the live source. The same-instance flag does not cover add(), another store instance, or an external writer.

Cold repros, each repeated 2/2 with byte-identical output: when the second write failed, the store remained NEW-A / OLD-B with no changelog; a concurrent update after snapshotting was overwritten by the stale move and then deleted. The first result also contradicts lines 490–492, which say a failed write leaves the store unchanged.

This needs explicit apply semantics before merge: transaction/CAS support, or snapshot preconditions plus rollback/journaling and a structured partial result. A per-instance mutex cannot provide the documented integrity guarantee.

[strandly-the-agent]

🟡 Downgrading this from a blocker. At 49dcce1e the @remarks at :357-361 now state the boundary explicitly — "concurrent writes captured before the snapshot may be silently overwritten … Cross-instance/external-writer protection is not provided in this version" — and :571-574 no longer claims an unchanged store on write failure. A disclosed v1 scope boundary for a local-file store is a defensible call, and I verified there's no content loss on an injected write failure (orphan target, nothing destroyed).

Two things I'd still weigh, both cheap:

1. The failure mode is silent success, not an error. Cold repro, 2/2: an add() landing during planning is discarded, the live file is deleted, and the changelog records the run as fully successful. Re-reading each move source at execution (:597-603) and skipping + reporting when its bytes differ from the snapshot turns silent loss into a reported conflict without any locking or CAS.
2. A mid-plan write failure still exits before _recordChangelog (:379-382), so the one state change that did land is unaudited and the error doesn't name it. Recording the changelog in a finally would cover it.

Neither blocks merge as long as the documented contract stands.

[strandly-the-agent]

🔴 Still open at 3621800, and sharper than last round: the guard the @remarks point to as the mitigation doesn't run at all for the plan shape that loses everything.

newTargets (execute.ts:205-209) only collects merge.target and move.to, so an update+delete plan creates no new path, newTargets.size === 0, and assertNewTargetsUnclaimed returns at :211 before reading anything.

Cold repro, 2/2 byte-identical. Two instances over one backend (_consolidating is instance-scoped, file-memory-store.ts:109), each folding the other's file into its own survivor:

run1 = fulfilled   run2 = fulfilled
surviving knowledge files: 0  []
TOTAL KNOWLEDGE LOSS with both runs reporting success: YES

My correctness pass independently reproduced the mirrored-merge variant of the same thing (merge([a,b])→a racing merge([a,b])→b, both files deleted) — there the guard does run but skips the target because it's already in the snapshot.

I know there's no lock and the docstring says so. What I'd push back on is the framing: file-memory-store.ts:262-267 presents the re-read as the protection against concurrent writers, and a reader will reasonably assume it covers the worst case. It covers the case where a new key gets claimed, and not the case where everything disappears. Either the guard needs to cover in-snapshot targets too (re-read every path the plan will delete, and abort if its bytes moved), or the @remarks should say plainly that two overlapping consolidations can empty the store and both report success — which is a very different sentence to write than the current one, and probably the argument for a namespace-scoped guard.

Related, same root cause — a concurrent delete gets undone. The write pass sources content from the start-of-run snapshot (execute.ts:121-130), so a memory the user deleted mid-run is re-materialized at the move target and the run reports success (archive/secret.md now holds the deleted content: YES). The @remarks only describe concurrent writes being lost, never a removal being reversed — worth a line, since "forget this" silently failing is its own category of problem for a memory store.

[strandly-the-agent]

Unchanged at 664fe60 — execute.ts:200-233 and :124 are byte-for-byte untouched by this commit, and the mirrored-merge race still leaves zero surviving files with both runs reporting success (re-reproduced at this head).

I've raised this three rounds running, so I'm going to stop re-escalating and put it down as a decision instead. Two defensible outcomes, and I don't think it's my call which:

1. Fix it — extend the claim check to cover in-snapshot targets and the paths a plan will delete (re-read, compare bytes to the snapshot, abort on drift), which also closes the resurrection case at :124.
2. Document it precisely — keep the current scope, but change file-memory-store.ts:262-267 so it doesn't present the re-read as the mitigation for concurrent writers. The sentence that needs to be there is roughly: two overlapping consolidations can empty the store, and both will report success. That's a much stronger warning than the text carries today, and it's the one a reader deciding "can I run this from two workers?" actually needs.

Either is fine by me; what I'd push back on is the current middle ground, where the docs imply a guard that doesn't run for the worst-case plan shape. Happy to leave this thread to you and lizradway from here.

[lizradway]

can you add the file directory layout to the pr description?

[maisieyanz]

@strandly-the-agent Re-review this pr please.

[strandly-the-agent]

Re-review at 49dcce1e — changes requested (3 blockers; most of the prior round is genuinely fixed)

✅ Verified fixed at this head: writable: false rejection (:386-391, throws before any await or model call) · evidence-tag containment (serializeEvidence :1139-1141, held against 6 payload shapes incl. storage-key injection) · turn bound (limits.turns + explicit limitTurns throw, :505-517/:543-556) · changelog excluded from search() and planner input (:270, :472) · backslash and ASCII case-alias path vectors (:1008, :174-176, :895-968).

🔴 Still blocking — three bypasses of the new guards, each reproduced cold twice: zero-width characters defeat the non-empty-content check (reopens the erasure class); duplicate merge sources launder an unauthorized update past the operations allow-list; the audit changelog is forgeable and exceeded maxGeneratedBytes by ~32×.

🟡 5 should-fix: reserved-key write via add(), NaN disabling all five caps, planner budgets never told to the model + deriveInsights promising semantics merge can't honor, unconstrained filename segment, optional stale-move mitigation.

Heads-up — the API gate is currently inert: the PR carries needs-api-review, but .github/workflows/api-review-label.yml:19-25 recognizes only api/needs-review / api/review-complete, so the check takes the "no API review labels present — skipping" branch and passes vacuously. The python label also doesn't match the diff (strands-ts/** only).

Verification ledger

• Exact head 49dcce1e481cb5d925a957d6c0a9332875db6e61, merge-base e4189c10, clean detached checkout, no tracked edits.
• Focused suites: 124/124 passed (74 consolidate + 50 store), Type Errors no errors.
• tsc --noEmit -p src/tsconfig.json passed · npm run lint passed · git diff --check clean.
• Every adversarial scenario re-run cold twice with byte-identical output.
• Not verified here: format:check (Prettier isn't installed in this sandbox; the GitHub status rollup at this head is SUCCESS, which covers it) and the macOS NFC/NFD case (no APFS volume — flagged as speculative, not a blocker). The repo-wide npm run type-check failed only with TS6305 because dist/ was never built here — environmental, not PR-caused.
• Two specialist passes failed transiently and were retried successfully; no pass is missing.

API / process notes (summary-level, not inline)

• Substantial new primitive. Model-planned destructive mutation of customer data is a category MemoryStore has never had (src/memory/types.ts:101-164 has no maintenance method), and there's no Python sibling to reconcile against, so this shape sets the cross-SDK contract. Per team/API_BAR_RAISING.md:24-26 that reads as the explicit-meeting tier rather than a solo approve.
• operations omitted still authorizes deletion (:411 defaults to all; prune→delete :773, resolveContradictions→delete/update :769-772), with no dry-run and no report of what was removed. Would it be better for the smallest call — consolidate({ model }) — not to be the most destructive one?
• CONSOLIDATE_OPERATIONS is unreachable by consumers: documented as the customer-facing source of truth (types.ts:24-26, :43-44) but index.ts:2 exports it type-only, and package subpath exports block a deep import — so operations: CONSOLIDATE_OPERATIONS.filter(op => op !== 'prune') can't be written today.
• Promise<void> can't express the outcome: no-op (:420) is indistinguishable from "ran, changed nothing"; partial success throws after mutating (:452-458); audit-write failure is swallowed to logger.warn (:699-701). Both sibling stores return a typed result (bedrock-knowledge-base/store.ts:376, test-memory-store/store.ts:194) and this store's own add() returns a receipt (:310).
• The reserved changelog key has no exported constant and no reader — the PR's own tests reach around the store with storage.read('consolidation-changelog.md') (consolidate.test.node.ts:95,107,120). Is the changelog meant to be a customer-readable artifact or an internal aid? That answer changes both the key's visibility and whether a ConsolidateResult subsumes it.
• Smaller: no cancelSignal (settled convention at types/agent.ts:135, multiagent.ts:58, graph.ts:331) · maxInputBytes/maxGeneratedBytes are the only byte-unit public config fields in strands-ts/src, vs. team/DECISIONS.md:185-191 preferring token units · operations: [] is silently accepted and costs a planner call · DEFAULT_MAX_CONSOLIDATION_TURNS (:58) is named "default" but nothing can override it, and :363 reads as a single-call bound while the worst case is two invokes · maxDirectories (types.ts:59-63) promises a store-wide invariant while add() validates paths not at all (:317-319).
• PR body is the customer-facing doc of record (no site/ page mentions FileMemoryStore) and is stale: the prose still says it excludes a consolidation/ directory while the diagram shows the flat key from 32d455c6; maxActionsPerPlan and maxGeneratedBytes are missing from the config example; the read-only and turn-limit throws aren't listed; no module-exports section (API_BAR_RAISING.md:56-64).

Per-pass breakdown

• Correctness: of the 6 prior blockers, 3 verified fixed, 1 (path identity) fixed for the named vectors via a documented heuristic, 1 (turns) fixed, 1 (non-atomic/stale snapshot) unchanged but now explicitly scoped out at :357-361. New nit: parseFrontmatter (:117) can match the opening delimiter's newline as the close, so '---\n---\nbody' passes validateActionContent but parses with an empty description and raw --- as body — folded in here, not posted separately.
• Adversarial: 11 repro scripts, each cold-run twice byte-identically. Broke: zero-width bodies, duplicate merge sources, changelog forgery + byte-cap bypass, add() reserved key, NaN caps, filename charset. Could not break: evidence-tag containment, .././backslash/nesting/directory rules, changelog exclusion from search and planner input, write-before-delete ordering (no content loss on injected write failure — the :571-574 tradeoff claim is accurate), read-only rejection, action cap, same-instance overlap guard.
• Test quality: the 124 tests are genuinely behavioral — they assert storage state and model-call arguments rather than internals. Gaps proven by repro: no coverage for Cf bodies, duplicate sources, reason/summary size or sanitization, NaN/Infinity limits, NFC/NFD aliases.
• LLM-context: the evidence framing is sound and states untrusted-data provenance in both system and user messages. Gaps: deriveInsights promises retention merge can't honor; maxActionsPerPlan/maxGeneratedBytes/maxDirectories, merge cardinality and the directory charset rule are enforced but never stated to the planner, and two of them hard-throw with no revise attempt; the revise-retry message (:543-546) re-injects the model's prior plan without the untrusted-data framing used for evidence.
• Routed to existing threads instead of new comments: the zero-width bypass (on the empty-content thread), U+2028/U+2029 escaping (evidence thread), NFC/NFD + filename charset (path-identity thread), stale-move mitigation (atomicity thread), turn-bound confirmation (turn thread).

Aggregation: 5 passes → 13 raw findings + 10 API sub-items → 3 🔴 and 5 🟡 posted inline or as thread replies, the rest summary-level.

[lizradway]

we should definitely benchmark this later on

[lizradway]

can be addressed as follow up by the team, but im wondering if you have any ideas for a more intelligent route to take in this scenario (ie. only consolidate on some fraction of the files, etc)

[lizradway]

no need imo to implement this now, just want some ideas for the future

[strandly-the-agent]

Two ideas from what this round's testing turned up, both squarely follow-up material rather than anything for this PR:

1. A bounded slice per run would also bound blast radius. A plan can currently push the store past its own admission limits (see the move amplification blocker — 1000 files / 117 MB generated from one 120 KiB source), after which no future consolidate() can run at all. Processing a bounded slice per run — oldest-N, one directory, or a byte budget — handles "store exceeds limits" gracefully and means a single bad plan can only ever affect that slice.

2. Tell the planner its budget. The prompt never states maxActionsPerPlan, maxGeneratedBytes, maxDirectories, the ≥2-distinct-sources rule, or the directory charset (planner.ts:131-190, unchanged by the refactor), yet the first two hard-throw with no revise cycle (plan.ts:88-91, file-memory-store.ts:366-370). A model told "at most N actions, at most X bytes generated" will usually stay inside it; today it can only discover the limit by being rejected, and for those two limits it doesn't even get the retry. That likely removes most of the need for smarter post-hoc handling.

A third option if the corpus genuinely outgrows a single planner call: hierarchical consolidation — consolidate within each directory first, then a second pass over the per-directory summaries — which keeps each model call small and makes the work naturally resumable.

[lizradway]

can we do these assertions at initialization rather than in consolidate so these are caught earlier?

[maisieyanz]

moved assertions to the top of consolidate() instead. I can't move them to initialization unless I added them to the constructor

[lizradway]

how did you chose these defaults?

[maisieyanz]

They're pretty conservative default values that I thought would be safe for general use case. They're subject to change if they're ever tested or benchmarked

[lizradway]

non-blocking: should we define this in errors.ts?