fix(memory): make the markdown backend upsert, and forget every copy

[sulthannauval]

Replaces #434 — same commit, same diff. That PR was closed automatically when its branch was renamed from fix/084-markdown-memory-upsert to this one; no review had happened on it.

Summary

• Problem: two defects with one root cause in the markdown backend. store appended unconditionally, so storing a key twice wrote two lines. forget then breaked at the first file that matched, so a key present in more than one file lost one copy and kept the rest — while returning true.
• Why it matters: every other real backend upserts (SqliteMemory via ON CONFLICT(key) DO UPDATE, PostgresMemory matching it) and Memory is one trait, so this was a contract divergence. The duplicate inflated count() — which memory stats renders as Total: — showed the key twice in list(), and left get() returning whichever copy sorted first under a timestamp that is really the filename. Worse, MemoryForgetTool advertises "delete outdated facts or sensitive data" and renders Ok(true) as "Forgot memory: k" while get() still answered with the entry.
• What changed: extracted remove_key_from_file; forget now sweeps every file it owns; store clears the key from every file before writing, so a re-store under a different category moves the entry rather than duplicating it across two files.
• What did NOT change (scope boundary): the filename-as-timestamp shape is left alone — it is ugly and is what makes get()'s tie-break arbitrary, but changing it touches parse_entries_from_file, recall ordering and list ordering. Same for get()'s || e.content.contains(key) fallback, which lets a content substring resolve as a key: real, different defect, different blast radius. Neither is bundled here.

Evidence from probes against the previous behaviour:

forget returned true but entry survives: MemoryEntry {
  key: "dupe", content: "daily copy", category: Daily, ... }

assertion `left == right` failed: one key must count once
  left: 2
 right: 1

The control matters: the identical two-write-then-forget sequence on sqlite passes — one row, deleted cleanly. The doc comment on forget already argues that answering wrongly about a deletion is unacceptable; the break reintroduced the same class of wrong answer from the other direction.

Scope of impact: only reached when [memory] backend = "markdown". The default is sqlite (config/schema.rs).

Label Snapshot (required)

• Risk label (risk: low|medium|high): risk: medium
• Size label: auto-managed
• Scope labels: memory
• Module labels: —
• Contributor tier label: auto-managed
• If any auto-label is incorrect, note requested correction: —

Change Metadata

• Change type: bug
• Primary scope: memory

Linked Issue

• Closes #
• Related #
• Depends on # — none; independent of chore(deps): bump the rust-all group across 1 directory with 23 updates #82/chore(deps): bump which from 7.0.3 to 8.0.3 #83/fix(providers): repair rig tool-call continuation (decode + dedupe) #85 in this series
• Supersedes # — none

Validation Evidence (required)

cargo fmt --all -- --check                                              # clean
cargo +1.92.0 clippy --locked --all-targets -- -D clippy::correctness    # clean
BASE_SHA=<main> ./scripts/ci/rust_strict_delta_gate.sh                  # no blocking issues on changed lines
cargo test --lib -- memory::markdown                                     # 20 passed, 0 failed (14 existing unchanged)
cargo test --lib -- memory::                                             # 293 passed, 0 failed

Clippy is run under 1.92.0 because that is what .github/workflows/ci-run.yml pins; the local default toolchain is 1.97 and reports a large pre-existing diagnostic set that CI does not gate on.

• Evidence provided: six new tests. Four were confirmed to fail against the previous behaviour by restoring it (re-adding the break, removing store's call to forget) and re-running:

  • markdown_store_replaces_an_existing_key
  • markdown_store_moves_an_entry_across_categories
  • markdown_forget_sweeps_every_file
  • markdown_repeated_store_does_not_grow_the_file

  The other two — markdown_hand_written_lines_survive_store_and_forget, markdown_forget_absent_key_rewrites_nothing — guard behaviour this change could have broken rather than reproducing the bug, and pass either way by design.

• If any command is intentionally skipped: full-workspace cargo test was not run (disk-bound on this machine); the whole memory:: module was run instead.

Security Impact (required)

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No — same files (MEMORY.md, memory/*.md), same directory. store now performs a read-modify-write on files it previously only appended to, and may rewrite a sibling file when moving an entry between categories.

Worth stating positively: this improves a security-adjacent property. memory_forget exists partly to delete sensitive data, and it previously reported success while leaving a copy behind.

Privacy and Data Hygiene (required)

• Data-hygiene status: pass
• Redaction/anonymization notes: none needed — fixtures use k, dupe, old value/new value, core copy/daily copy.
• Neutral wording confirmation: confirmed.

Compatibility / Migration

• Backward compatible? Yes
• Config/env changes? No
• Migration needed? No — an existing workspace's duplicate lines are collapsed on the next store of that key, and are otherwise harmless (they read as two entries, which is what they already did).

Human Verification (required)

• Verified scenarios: store the same key twice → one entry, second value wins; store in Core then Daily → entry moves, old file no longer holds it; a key duplicated across MEMORY.md and a daily log → forget returns true and get() returns None.
• Edge cases checked: a hand-written line (not - **key**: value shaped) survives both a store of an unrelated key and a forget of an unrelated key; the # Long-Term Memory header survives; forget of an absent key rewrites nothing (asserted on both contents and mtime — contents alone is the weaker check); re-storing one key five times does not grow the file.
• What was not verified: no live run with [memory] backend = "markdown" configured in a real profile. All evidence is unit-level against MarkdownMemory directly.

Side Effects / Blast Radius (required)

• Affected subsystems: memory/markdown only. No other module changed.
• Potential unintended effects: store is now a read-modify-write across every file the backend owns instead of a single append, so it is O(files) per write. The markdown backend is the non-default, human-readable one and is not the performance path.
• Guardrails: unparseable lines are never touched, which is what bounds the rewrite. The hand_written_lines_survive test is the specific guard.

Agent Collaboration Notes (recommended)

• Agent tools used: Claude Code.
• Workflow/plan summary: plans/084-markdown-memory-appends-instead-of-upserting.md. The plan sequences the two fixes (drop the break first, then make store replace) because B is correct standalone while A is what stops the duplicate state being created.
• Verification focus: every regression test was mutation-checked — the previous behaviour was restored and the tests re-run. Four fail, two pass by design; both groups are named above rather than reported as a single green number.
• Confirmation: naming + architecture boundaries followed (AGENTS.md + CONTRIBUTING.md) — yes.

Rollback Plan (required)

• Fast rollback: git revert bf09536. Single commit, one file, no schema or config surface.
• Feature flags: none.
• Observable failure symptoms: MEMORY.md or a daily log losing operator-written prose, or losing its header; count() dropping unexpectedly on a markdown profile.

Risks and Mitigations

• Risk: store now rewrites files it previously only appended to, and MEMORY.md is a file humans edit by hand. A parsing mistake would eat operator prose.

  • Mitigation: remove_key_from_file drops only lines split_stored_entry parses into a key equal to the target; anything else — headers, prose, differently-shaped lines — is copied through verbatim. markdown_hand_written_lines_survive_store_and_forget pins exactly this.

• Risk: the trailing-blank-line trim is a third behaviour change riding along with the two defects.

  • Mitigation: it is not cosmetic. Without it the removal leaves the separator blank line behind and append_to_file adds another, so re-storing one key repeatedly grew the file by a blank line each time — and MEMORY.md is injected into the prompt, making that an unbounded token cost. markdown_repeated_store_does_not_grow_the_file pins it, and it fails against the previous behaviour.

• Risk: moving an entry between categories is a semantic choice, not a forced one — the alternative was to reject the re-store.

  • Mitigation: moving is what sqlite does (ON CONFLICT(key) DO UPDATE SET category = excluded.category), and matching it is the whole point of this PR. Stated explicitly in the plan and pinned by markdown_store_moves_an_entry_across_categories.

🤖 Generated with Claude Code

[github-actions]

PR intake checks found warnings (non-blocking)

Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.

• Incomplete required PR template fields: summary problem, summary why it matters, summary what changed, validation commands, security risk/mitigation, privacy status, rollback plan

Action items:

1. Complete required PR template sections/fields.
2. Remove tabs, trailing whitespace, and merge conflict markers from added lines.
3. Re-run local checks before pushing:
   • ./scripts/ci/rust_quality_gate.sh
   • ./scripts/ci/rust_strict_delta_gate.sh
   • ./scripts/ci/docs_quality_gate.sh

Run logs: https://github.com/RantAI-dev/RantAIClaw/actions/runs/31260808433

Detected blocking line issues (sample):

• none

Detected advisory line issues (sample):

• none

[sulthannauval]

Pushed 3647dc1.

CI caught a test that pinned the defect this PR removes — tests/memory_comparison.rs::compare_upsert:

// Markdown: append-only, count increases
assert!(md_count >= 2, "Markdown should keep both entries");

That is the append-only behaviour stated as the expectation. It now asserts markdown replaces, matching the sqlite assertions directly above it, and reads both backends through get() so the comparison is like-for-like. compare_forget in the same file was updated the same way when forget stopped returning false unconditionally and carries a comment saying so; this follows that precedent.

Why local validation missed it: it is an integration test under tests/, and the runs behind this branch were cargo test --lib only. Now also run locally: memory_comparison 7, memory_restart 14, migrate_legacy 10, profile_lifecycle 17, compat_v041_to_v050 2 — every integration binary that touches a memory surface.