feat: store-boundary write dedup (normalized dedup_hash, both add paths)

[gabaum10]

Summary

Store-boundary write dedup for memory. Every mx memory add — and every fact-type add — now hashes a normalized dedup_hash (title+body, lowercased, punctuation-stripped) and checks it against same-owner rows before the upsert. On a hit, the write skips and exits 0 with a duplicate_of pointer to the row that already holds the content. No new row, no edges, no embedding.

The enforcement lives in one shared pre-upsert helper wired into both write paths: the plain add_one path and the fact-type add-batch inline path. There was no single choke point before, so the two paths could disagree about what counted as a duplicate. Now they don't.

Candidate lookup is a same-owner fetch keyed on the session_id field — visibility-filtered, params bound. Hit -> skip and short-circuit. Miss -> the write proceeds exactly as it does today.

Design notes

• Owner scope is a hard AND predicate. Owner is part of the match, not a filter layered on top. None matches null-owner rows, so a public unowned add dedups against its own kind rather than colliding with owned content.
• DedupIndex is keyed (owner, hash) so a mixed-owner batch dedups per-owner instead of smearing across owners inside a single add-batch.
• Skips return Ok and short-circuit ahead of edge-building and embedding. A duplicate costs nothing downstream.
• session_id = None bypasses dedup and emits an observable json note rather than silently doing nothing. The bounded fallback for the no-session case is deferred, not forgotten.
• Fix round: one dedup_gate() helper. All three new-entry write funnels (standard add_one, single-add --type, batch inline --type) now call one shared dedup_gate() — the check, the bypass-signal, and the fail-open lookup-error handling all live in one place instead of being hand-rolled per funnel.

Honest disclosures

• The write boundary now confirms content existence under a claimed owner (that's what duplicate_of tells you). This is bounded to the pre-existing owner-claim authz — it exposes no more than a caller could already assert about their own owner scope, but it is a new confirmation surface and worth naming. Extended per fix-round review: add-batch's per-line source_agent/owner makes this sharper in practice (one batch call can probe many owner+content combinations at once) — same bound, not a new hole. Pinned by batch_per_line_owner_claim_confirms_existence_of_matching_private_content.
• TOCTOU: the in-process DedupIndex is per-process best-effort. Two concurrent writers can still both land the same content — the check-then-write window is real. This matches the pre-existing concurrency posture; we did not make it worse, and we did not close it. Fix round: a candidate-lookup failure (transient read blip) now fails the gate OPEN rather than aborting the write — a mechanism that already tolerates a check-then-write race should tolerate a read blip the same way.
• Legacy title-only content_hash is untouched. dedup_hash (title+body, W447) is a distinct, additional mechanism. The old hash keeps doing its old job.

Housekeeping

Motivated by ~20 double-write pairs traced across W342–W445 — regenerated duplicates that exact-match checks slid right past because a regenerated body differs by a comma. Normalization is what catches those.

Session-scope coverage (raccoon review, garbaggio — RESOLVED): the ~20 historical double-write pairs were traced (W447) to same-session dual generations (same session_id, same resonance, minutes apart); session-scoped dedup covers the observed failure class; cross-session near-duplicates are deliberately out of scope.

Sibling context for the merger: PR #401 (exclude-tags) is open on this repo. Both it and this PR touch src/handlers/memory.rs and src/surreal_db/knowledge.rs, and PR #399 touches the same add-path functions. Landing/merge order across the three is Cory's call — flagging the overlap so it's a decision, not a surprise.

Fix round (raccoon review response, 8 raccoons + 3 Opus deep-divers)

Both blockers fixed, the structural finding implemented, the test-authority gap closed, and every tail finding fixed or documented. Full per-finding disposition table went to Lens for verification alongside this PR. Summary:

• [Blocker] dedup_hash title/body boundary collapse — fixed. Hashes the normalized title/body as a length-prefixed pair (not a joined string), so a duplicate that shifts the split point (title one turn, title+body the next) no longer collapses to the same hash. dedup_hash is recomputed on every call and never persisted, so there's no migration concern. Regression test added.
• [Blocker] fail-CLOSED on candidate-lookup error — fixed. All three funnels now fail OPEN on a lookup error (warn to stderr on all three funnels; the standard path's --json also carries a dedup_lookup_warning field; write proceeds) via the shared dedup_gate(). Unit test injects a lookup failure and asserts the write still lands.
• [Major] test-authority gap — fixed. The primary-path skip test (and every other skip test that only read self-reported CLI output) now also asserts the actual persisted row count via mx memory list --json.
• [Structural] gate copy-pasted 3x — implemented. One dedup_gate() helper; all three funnels call it. garbaggio's UNIQUE-index redesign was NOT implemented (see design option below) — this fix is the DRY consolidation only, not a store-level constraint.
• Tail findings (9): bypass-signal now consistent across all four funnels (fixed structurally via the shared gate); --allow-duplicate silent-overwrite caveat documented on cli.rs --help, docs/src/memory.typ, and mx-docs.md (perturbing the id was considered and rejected — see disclosure above); json-mode double-signal nuance fixed (stderr note now gated on plain mode only); smart-quote/em-dash/ellipsis normalization extended (one-directional safe, false-negative-only); public+owned vs public+unowned dedup gap documented + pinned by a regression test (not fixed — would need an owner-predicate redesign); claimed-owner oracle disclosure extended to the batch per-line owner vector + regression test; unbounded candidate fetch documented as a follow-up (not fixed — see design option below); skip-payload id key added; duplicate_of last-seen-not-canonical documented; batch-summary test assertions anchored to full lines instead of loose substrings.

Design option for a future PR (not implemented here)

garbaggio's structural alternative, surfaced but deliberately not built this round: persist dedup_hash as a field on knowledge and back it with DEFINE INDEX ... UNIQUE ON knowledge FIELDS session, owner, dedup_hash (the schema already has the pattern — tagged_with_unique, applies_to_unique, relates_to_unique). That would make "store-boundary" literally true and close the acknowledged TOCTOU gap at the database level, at the cost of mapping a constraint-violation error to WriteOutcome::Skipped and a migration to backfill dedup_hash on existing rows. Cory's call whether this is worth a follow-up PR — flagging so it's a decision, not a surprise. Also unaddressed this round: get_entries_for_session has no result limit, so a very long-lived session pays a full rehash of its group on every write (bounded in practice; a follow-up if it stops being bounded).

Test plan

• cargo test — 1,255 tests green (1,203 baseline + 10 new: 2 in knowledge.rs, 2 in handlers::memory::dedup_gate_tests, 6 in tests/dedup_write_boundary.rs; 3 existing skip-tests strengthened with store-level row-count assertions), 0 failed, 3 pre-existing ignored.
• cargo fmt --check — clean.
• cargo clippy --all-targets -- -D warnings — clean.
• docs/build.sh regenerated (docs/src/memory.typ touched — dedup section rewritten to cover all four funnels, the fail-open behavior, the allow-duplicate caveat, and the coverage-boundary open questions).

No merge from me — Cory merges.

[gabaum10]

Self-review disclosure

This PR was reviewed by our own automated raccoon panel (trash-panda-eugenics) — 8 raccoons + 3 Opus deep-divers, run internally before requesting Cory's review. Disclosing it as a self-review, not an independent external one, so the provenance is on the record. The verdict was REQUEST_CHANGES; both blockers are now fixed at 4979fccb89848400e41a84f831302c25cbf8eedb. Full compiled report below.

---

🦝 Raccoon Review: store-boundary write dedup (normalized dedup_hash, both add paths)

Verdict: REQUEST_CHANGES | 8 raccoons, 3 on Opus, all GREEN — 4 convergent clusters, 10 singular

Confidence: I'd block on the first two. The title/body hash collision is cross-model — two Opus raccoons and my Sonnet correctness-hunter landed on it independently, rim-licker ran the actual function, and I confirmed the newline-gets-collapsed path myself against PR HEAD. The fail-closed lookup is a single Opus read, but it's traced and I verified the ?-abort at both funnels. The test-authority gap is proven by bug injection, not asserted — fix it and it becomes the safety net for the other two. The structural DRY critique (three raccoons) is real but it's maintainability on a feature that works. The security oracle is author-disclosed and bounded. The tail is genuine but non-blocking.

Blocking: 1, 2

This is a careful, unusually well-tested PR, and the crew spent as much effort ruling things out as flagging them — the poisoned-record hazard, the owner-binding silent-no-op trap, the trait/return-type compile surface, the batch count bookkeeping, and the empty-owner predicate all came back clean (see the ruled-out list at the bottom — that's signal too). Two introduced correctness defects gate it: dedup_hash silently collapses the title/body boundary (a false-positive skip = silent write-path data loss), and the new candidate lookup fails closed, turning a transient DB read blip into an aborted memory write that pre-PR would have succeeded. Underneath those: the gate is copy-pasted three times rather than being the "store-boundary choke point" the PR sells, the primary path's skip test never checks actual row count (proven to hide a silent double-write), and the documented "bypass is never silent" guarantee holds on only one of four funnels.

Key Findings

[Major] src/knowledge.rs:197 — dedup_hash erases the title/body boundary → silent false-positive skip (Two Opus and my Sonnet correctness-hunter found this independently; rim-licker ran the function.)
Fix: normalize title and body separately and combine, or join with a sentinel that survives whitespace-collapse.

Analysis

dedup_hash(title, body) hashes normalize_for_dedup(format!("{title}\n{body}")). But \n is whitespace, not ASCII punctuation, so normalize_for_dedup's terminal .split_whitespace().join(" ") (knowledge.rs:170) collapses it exactly like any other space. The hash then depends only on the flattened word sequence of title+body, not on where the split falls. Two genuinely distinct entries whose combined token stream matches across the split point collapse to one hash — and because the gate runs before the upsert and a hit returns WriteOutcome::Skipped (exit 0, "Already saved", no row written), the second entry is silently dropped and reported as success.

rim-licker verified by extracting and running the real functions:

dedup_hash("a b", "")          == dedup_hash("a", "b")            // both → "a b"
dedup_hash("Ship it", "now.")  == dedup_hash("Ship it now", "")   // both → "ship it now"

That second case is exactly the variance an LLM regenerating the same fact produces — folding a sentence into the title one turn, splitting it title+body the next. This is the identity guarantee the whole feature rests on, and it's the one class of near-duplicate the tests never exercise: every existing test_dedup_hash_* recases the same split.

DJ verified against PR HEAD (915195d): the \n is the only separator and it is unconditionally collapsed. Suggested fix: blake3_hex over the pair (normalize_for_dedup(title), normalize_for_dedup(body)), plus a regression test pinning dedup_hash("Ship it","now.") != dedup_hash("Ship it now","").

Test coverage: uncovered — no test moves content across the title/body boundary.

[Major] src/handlers/memory.rs:577 — write-boundary dedup fails CLOSED; a transient read blip aborts the write (Roach on Opus, singular — went deeper than the diff, into the production config.)
Fix: on ensure_group lookup error, log to stderr and proceed with the write rather than ?-aborting. Align all three funnels on that policy.

Analysis

add_one (577) and the single-add --type fact path (1251) call dedup.ensure_group(...)?. That puts a new fallible read ahead of the write on every session-scoped mx memory add. Pre-PR there was no such query — the write went straight to upsert_knowledge. So a transient get_entries_for_session failure now kills a write that previously succeeded.

The production config runs MX_SURREAL_MODE=network (confirmed in tests/dedup_write_boundary.rs's own isolation comment) — exactly where a SELECT can hit a transient WebSocket blip independent of the subsequent upsert. This is an availability regression, and it contradicts the PR's own best-effort/TOCTOU-tolerant contract: if a duplicate slipping through a concurrent-writer race is acceptable, a duplicate slipping through a lookup blip should be acceptable too — by proceeding, not aborting.

The three funnels don't even agree on the policy: the batch fact path (2620) degrades gracefully (if let Err(e) → push to entry_errors → continue), but that still drops that line's write. Fail open and make all three consistent.

DJ verified against PR HEAD: ?-abort at 577 and 1251; graceful continue at 2620.

Test coverage: uncovered — no test injects a lookup error; CountingStore passes through to a real in-memory store, so the fail path never runs.

[Major] tests/dedup_write_boundary.rs:127 — the primary path's skip test never checks the actual row count (mystery-meat, singular — proven by bug injection, not asserted.)
Fix: assert persisted row count (mx memory list --json length, or db.inner.count() in dedup_gate_tests) after a duplicate-skip on the standard path, mirroring what the fact-type test already does.

Analysis

standard_path_recased_duplicate_is_skipped_exactly_one_entry — the flagship test for the PR's central claim — only inspects the CLI's self-reported JSON (skipped, status, duplicate_of). It never asks the store how many rows actually exist. mystery-meat proved this is a live gap, not a theoretical one: it patched add_one so a detected Duplicate still falls through the full write path (landing a second row) while still returning Skipped, and:

• All 5 dedup_gate_tests unit tests stayed green.
• Running the injected-bug binary: the CLI printed a clean "skipped": true, while mx memory list showed two rows persisted.
• The integration test did fail — but only by incidental side effect (stray (embed skipped) lines breaking a strict JSON parse), not because it checked the store.

Exactly one test in the ~730-line suite (single_add_fact_type_path_..., line 443) does the honest thing and asserts entries.len() == 1 via mx memory list. The primary path has no equivalent — so the suite would not catch a silent-double-write regression of precisely the kind Findings 1 and 2 describe. Fixing this is cheap and it's the safety net for the other two fixes. (mystery-meat reverted its experimental edit; its worktree was isolated and clean.)

Test coverage: the gap IS in the suite — no primary-path test independently confirms store row count after a skip.

[Major] src/handlers/memory.rs:576 — the gate is copy-pasted 3×; "store-boundary" isn't quite true (structural — non-blocking) (Three of mine converged: trashboat, garbaggio, el-notario.)
Fix: extract one dedup_gate() helper the three sites call; ideally push it into a single KnowledgeStore::add_new(...) so a fourth funnel physically cannot bypass it.

Analysis

The PR leads with "shared choke point" and "store-boundary," but the gate (ensure_group → check → skip → record) is hand-rolled at three handler sites — add_one (576/642), the single-add --type path (1249), and the batch fact-type path (2619/2671). What's actually shared is the DedupIndex struct and dedup_hash, not the gate. upsert_knowledge itself gates nothing, so any funnel that doesn't hand-roll the check writes a duplicate — and the code's own comment admits the --type path was "a THIRD new-entry write funnel the census missed." A structure that already lost track of one funnel can't promise a fourth won't slip; Findings 5 (bypass signal on 1/4 funnels) and the divergent skip strings are already symptoms of that drift.

garbaggio adds the sharp structural alternative: this codebase already ships the store-level tool for the guarantee — schema/surrealdb-schema.surql defines compound UNIQUE indexes (tagged_with_unique, applies_to_unique, relates_to_unique). A persisted dedup_hash field plus DEFINE INDEX ... UNIQUE ON knowledge FIELDS session, owner, dedup_hash would make "store-boundary" literally true and close the acknowledged TOCTOU, at the cost of mapping a constraint-violation error to WriteOutcome::Skipped. dedup_hash is currently never persisted (recomputed on every read/write).

Not in the blocking set — the feature works today. This is about the durability of the guarantee and honest framing.

Test coverage: partial — each funnel is tested independently, which is exactly what lets them drift; no test asserts they share one implementation.

🦝 What else the crew noticed (9 findings)

[Minor] src/handlers/memory.rs:1249 — "bypass is never silent" holds on only 1 of 4 funnels (3/8: trashboat, lil-grabby, roach)
Fix: emit the bypass signal on the two fact funnels and the batch standard path when session_id is None, or narrow the docs to the plain mx memory add path.

Analysis

docs/src/memory.typ and mx-docs.md promise a session-less write surfaces the bypass ("dedup":"bypassed_no_session" in --json, a stderr note in plain mode). Only the standard add_one caller path emits it (memory.rs:1480-1503). The single-add --type path (1249), the batch inline fact-type path (2619), and the batch standard path emit nothing on a None session — dedup silently no-ops. For three of four funnels the documented guarantee is false. (el-notario adds an adjacent nuance: on the standard path the eprintln! fires unconditionally, so --json mode gets both the stderr note and the json field, contrary to the doc's mode-exclusive phrasing.) DJ-verified against PR HEAD.

Test coverage: partial — the standard --json path is pinned; neither fact funnel's None-session behavior is asserted.

[Minor] src/cli.rs:691 — --allow-duplicate silently overwrites rather than creating a distinct row (el-notario; test/cache perspectives from mystery-meat + trashboat)
Fix: document the caveat on all four doc surfaces, or perturb the id on forced writes so a genuinely distinct row is produced; add an entry-count assertion.

Analysis

generate_id (knowledge.rs:276) hashes path_hint:title and upsert_knowledge is an UPSERT keyed on that id, so a byte-identical forced re-add reproduces the same id and overwrites the original row in place (new updated_at, same id) instead of creating a second entry. The flag's "force the write through" only holds for recased/reworded near-duplicates. The PR's own fact-type test comment names this caveat, but it never reached cli.rs --help, CLAUDE.md, memory.typ, or mx-docs.md, and no test asserts the resulting entry count. mystery-meat independently flagged the test-side twin (the standard-path allow_duplicate test reuses one title, so generate_id collision means it can't distinguish "forced through" from "no-op upsert"). trashboat flagged a different allow_duplicate hazard: forced writes are neither checked nor recorded, so a later normal add in the same cached group won't dedup against the forced row.

Test coverage: uncovered — no allow_duplicate test asserts entry count after a byte-identical re-add.

[Minor] docs/src/memory.typ:161 — session-scoped dedup may miss the majority of the duplicates it was built to stop (garbaggio — DJ override to minor: strategic question, needs author data)
Fix: confirm whether the 20 traced W342–W445 pairs were same-session (this fix helps) or cross-session (it's a no-op for them) before treating W447 as closed.

Analysis

The coverage-boundary note excludes "cross-session regenerations (a fact re-derived in a later wake under a different session_id)," and the author equates "different session_id" with "a later wake." But the motivation is "~20 double-write pairs traced across W342–W445" — a ~100-wide span of work items that almost certainly spans many wakes. If so, a same-session-scoped fix catches only a minority of the motivating class. The mechanism was validated only against synthetic same-session tests; nothing in the PR shows the 20 real pairs were checked against the chosen scope. This is the sharpest strategic note in the review — I'm holding it at minor because it needs the author's data, not because it's a demonstrated defect. If mostly cross-session, the primary mechanism likely wants owner-scoped matching with a bounded time window, session-scoping demoted to a tightening.

Test coverage: existing tests are same-session by construction; none asks "does this fix the pairs it was built to fix."

[Minor] src/handlers/memory.rs:1389 — public+owned vs public+unowned identical content never dedup (false negative) (garbaggio)
Fix: document the gap alongside the existing caveats, or treat visibility='public' rows as one candidate pool regardless of owner tag.

Analysis

entry_owner = if is_private { Some(...) } else { owner } (1389-1394), so a public entry can still carry Some(owner) via --owner X without --private (independent flags). A public+owned entry and a later public+unowned entry with identical normalized title+body — both visible to every reader, the exact case this feature targets — never dedup, because the None-owner candidate fetch is owner IS NONE, which strictly excludes the owned row. The existing cross_owner_public_entry_is_not_a_dedup_candidate test guards the false positive (two owned public entries from different owners) and leaves this false negative open.

Test coverage: uncovered — the mirror-image case of the existing test.

[Minor] src/handlers/memory.rs:379 — claimed-owner dedup pre-check is a private-content existence oracle (2/8: el-notario, trashboat — author-disclosed)
Fix: keep the disclosure and extend it to the batch per-line source_agent vector; add a regression test pinning the accepted behavior.

Analysis

ensure_group queries the store as the caller-supplied owner (AgentContext::for_agent(owner); on the batch path owner = agent:{source_agent}, per-line and caller-controllable), and a Duplicate hit returns duplicate_of: kn-XXXX before the read-after-write permission verify that would reject a forged-owner write. schema/surrealdb-schema.surql defines owner as a plain option<string> with no PERMISSIONS clause. So --owner <victim> --private lets a caller learn whether <victim> already privately holds that exact normalized content, gated only by guessing title+body. The PR's "Honest disclosures" section names this for the single-add path; the batch per-line source_agent is the sharper form. Bounded to the pre-existing owner-claim authz and accepted by the author — the actionable gap is the missing regression test and extending the disclosure.

Test coverage: uncovered — no test drives a caller claiming an owner it doesn't control with --private.

[Minor] src/knowledge.rs:173 — only ASCII punctuation is stripped; smart quotes / em-dashes don't dedup (lil-grabby)
Fix: extend the filter to the common LLM Unicode glyphs (U+2018/2019/201C/201D/2013/2014/2026), or qualify the docs to say "ASCII punctuation."

Analysis

normalize_for_dedup uses filter(|c| !c.is_ascii_punctuation()). Confirmed: dedup_hash("dont","") != dedup_hash("don’t","") (smart apostrophe kept). LLM regenerations routinely swap straight quotes for smart quotes, hyphens for em-dashes, ... for … — the exact recase/repunctuate class this gate targets — and those won't dedup. The docs say "punctuation stripped" unqualified; only the code comment says ASCII. This is a false negative (misses dupes — the safe direction), but it undercuts the stated coverage.

Test coverage: uncovered — the normalize test uses only ASCII . and ,.

[Minor] src/store.rs:484 — unbounded candidate fetch → O(n) rehash per invocation on long-lived sessions (garbaggio)
Fix: if the DB-level UNIQUE index isn't adopted, scope get_entries_for_session to a bounded recent-time window.

Analysis

get_entries_for_session has no LIMIT, and DedupIndex has no cross-invocation cache, so every mx invocation that first touches a (session, owner) group fetches and re-hashes every row ever written to that group. For a long-lived session this O(n) full-scan-and-hash recurs on every future write. The PR's "single query per batch" claim is about query count, not per-query cost or the rehash work. Bounded in practice (memory sessions aren't usually thousands of rows) — a follow-up, not a block.

Test coverage: query count is bounded per group; the cost of a large group is unexercised.

[Minor] src/handlers/memory.rs:1460 — skip output shape diverges from success (missing id, changed plain-text phrase, new batch line) (2/8: the-snitch, garbaggio — companion-facing)
Fix: echo the duplicate's id under the same id key in the skip payload; document the two plain-text phrases; finish syncing the ~/.soren pocket-persist executor doc + schema.

Analysis

Three related shape changes a caller can observe: (1) the --json skip payload has no id key (unlike every success payload), so jq -r .id silently gets null on a skip; (2) plain mode prints Already saved as {id} ... where success prints Added entry: {id}, so a regex on Added entry: (kn-\S+) silently misses while exit stays 0; (3) add-batch grows a new [N] Already saved: kn-XXXX per-line shape (2635, 2903). the-snitch traced the real downstream consumer — the ~/.soren pocket-persist executor — and found one sibling doc updated for this but the mechanical-executor doc (mirrored verbatim into pocket-persist.js) still only expects ids from "Added entry" lines, with no dedup-skip count field in its return schema. No live jq/regex consumer was found in the reachable scripts today, so this is a latent/companion-facing contract change, not a confirmed in-repo break — and the fix on the consumer side lives in ~/.soren, which the author owns, per the CLAUDE.md maintenance rule this PR itself adds.

Test coverage: partial — the skip --json is asserted to be pure JSON, but not the absence of id; no CI test exercises the ~/.soren consumer.

[Note] src/handlers/memory.rs:386 — hygiene: arbitrary duplicate_of when legacy dups share a hash; unanchored substring test matches (el-notario, mystery-meat)
Fix: doc-comment that duplicate_of is last-seen (not canonical) when legacy duplicates already collide; anchor batch-summary test assertions to the full line.

Analysis

DedupIndex.seen is a HashMap keyed (session, owner, hash), so when the DB already holds multiple distinct rows sharing one normalized hash under the same group — precisely the historical W342–W445 pairs — only the last one iterated survives and is reported as duplicate_of; which one is incidental to query order, not canonical/earliest. Separately, several batch-summary test assertions use contains("1 added"), which would also match "21 added" at larger magnitudes — harmless today, fragile if reused. Grouped as one note so they don't dilute the real findings.

Test coverage: none seeds two pre-existing same-hash rows; the substring assertions are self-described.

🦝 What the crew ruled OUT (verified clean — that's signal too)

The crew spent real effort disproving plausible bugs. These came back clean:

• Poisoned-record hazard — roach confirmed the read-back verify precedes record() on the batch fact path (memory.rs:2653-2668), same as add_one. Not a hazard.
• Owner-binding silent no-op (bare agent_id vs agent:{id}) — roach + mystery-meat confirmed all funnels bind entry.owner. Correctly wired.
• add_one return-type / new trait method breaking callers or compilation — the-snitch grepped the tree: add_one is private with all call sites in-diff; all three KnowledgeStore implementors updated. Compiles clean.
• add-batch summary-line format change — the-snitch found no reachable companion parses that sentence positionally. Safe.
• Empty-string owner Some("") — rim-licker hand-traced the bind/clause pairing; self-consistent round-trip. The questionable input is pre-existing, out of diff.
• type::thing('session', ...) — rim-licker: mirrors an unchanged pre-existing convention.
• Batch added/skipped/failed count double-count or drop — rim-licker + mystery-meat traced every continue/push path; mutually exclusive by construction.
• CountingStore a vacuous mock — mystery-meat read all ~70 methods; genuine pass-through to a real in-memory store plus one counter.
• wake_ritual.rs unreachable!() stub — mystery-meat + the-snitch confirmed the mock never reaches the dedup path.
• fact-type allow_duplicate test a tautology — mystery-meat: it has real teeth; honestly disclosed, not vacuous.

---

8 opinionated trash pandas, three Opus deep-divers, zero coordination between them. Two of them ran the code instead of trusting it — the collision and the double-write were reproduced, not argued. The convergence is the signal; the reproductions are the receipts.

[gabaum10]

Disposition receipt (Lens-verified)

Every raccoon finding, its disposition, and where it landed. Verified against the pushed tree at 4979fccb89848400e41a84f831302c25cbf8eedb.

Blockers & structural

ID	Finding	Disposition
K1	dedup_hash title/body boundary collapse	Fixed — knowledge.rs length-prefixed pair hash + regression test
K2	fail-CLOSED on candidate-lookup error	Fixed — fail-open shared gate + FailingStore unit test
K3	test-authority gap (CLI self-report only)	Fixed — store-level row-count assertions via mx memory list --json
K4	gate copy-pasted 3×	Fixed — one dedup_gate(), all four funnels

Tail (11)

#	Finding	Disposition
1	bypass-signal inconsistent across funnels	Fixed — consistent across all funnels via the shared gate
2	json-mode double-signal	Fixed — stderr note gated to plain mode only
3	--allow-duplicate silent-overwrite caveat	Documented — 3 surfaces (cli.rs --help, memory.typ, mx-docs.md)
4	smart-quote/em-dash/ellipsis normalization	Fixed — one-directional normalization + test
5	public-owned vs public-unowned dedup gap	Pinned by regression test (owner-predicate redesign deferred)
6	claimed-owner oracle disclosure	Extended to batch per-line owner vector + test
7	unbounded candidate fetch	Documented follow-up (bounded in practice)
8	skip payload missing id	Fixed — id key added to skip payload
9	duplicate_of last-seen-not-canonical	Documented — doc comment
10	batch-summary loose-substring assertions	Anchored to full lines
11	session-scope coverage open question	Resolved — traced to same-session dual generations (W447); cross-session out of scope

Suite delta: 1,166 → 1,255 green.

[coryzibell]

VERDICT: REQUEST CHANGES

One blocking finding. Not because the code is broken — it is, in fact, unusually careful — but because the ticket named false-positive dedup "the worst failure mode here," and there is one such surface that is neither closed nor documented. Everything else is a comment.

One-line summary: Structurally sound, heavily-tested, honestly-disclosed dedup gate wired into all three write funnels — but the dedup identity is title+body only, so identical content re-filed under a different category or tags in the same session is silently dropped (exit 0), an undocumented false-positive data-loss class.

---

Findings, ranked

1. [BLOCKER] Dedup identity excludes category and tags — undocumented false-positive data loss.
src/knowledge.rs dedup_hash(title, body) hashes title+body only. src/surreal_db/knowledge.rs get_entries_for_session_async scopes candidates by session + owner + visibility — no category predicate. Net: within one session+owner, two entries with byte-identical title+body but a different category (or different tags) collide. The second write is skipped, exit 0, "Already saved as {other-category-id}." The caller believes it is saved; it is not — it points at a row in a different category.

This is exactly the false-positive class the ticket flagged. It is not a considered tradeoff: the W447 evidence (same-session regenerations) shares category, so adding category to the hash strictly reduces false positives at zero cost to the target use case. Its omission is an unforced gap. The PR uses a "documented-gap + pinned regression test" pattern for the public-owned/public-unowned case (public_owned_and_public_unowned_identical_content_do_not_dedup) — the category/tags dimension has neither a test nor a mention.

Fix: fold category (and decide on tags) into the dedup identity — either into the length-prefixed hash (add a third length prefix, exactly as the LANDMINE FOR THE NEXT EDITOR doc-comment already warns) or into the candidate query scope. If cross-category dedup is somehow intended, make it a decree with a pinning test like the owner case. Either way, the identity contract must be explicit about what it treats as "the same."

2. [COMMENT / acknowledged] "Store-boundary" is a misnomer — the gate is application-level, TOCTOU-open.
DedupIndex (src/handlers/memory.rs) is per-process read-then-write with no DB UNIQUE constraint. Two concurrent mx invocations both land. The PR discloses this honestly and defers the garbaggio DEFINE INDEX ... UNIQUE ON knowledge FIELDS session, owner, dedup_hash option. Acceptable given the disclosure and that the observed failure (regenerations "minutes apart" = sequential processes) is caught by the DB candidate fetch. But the title oversells; the real close is the deferred unique index. Note for merge: the unique-index option is where finding #1's category dimension must also land (FIELDS session, owner, category, dedup_hash).

3. [COMMENT / acknowledged] get_entries_for_session is unbounded.
No LIMIT; every write in a session rehashes the whole session group — O(n²) over a long-lived session. Disclosed as a follow-up. Bounded in practice today. Fine to defer, but it is on the record.

4. [COMMENT] Cross-invocation dedup depends on deterministic title.
add_one does not transform title/body (verified: entry.title = args.title.clone()), so the hash matches what's stored — good. But if title is auto-generated from content upstream for empty-title adds (docs reference this) and that generation is non-deterministic, a regenerated duplicate gets a different title → different hash → dedup misses. False-negative only (a dup slips through), not data loss. Worth a one-line confirmation that title resolution is deterministic before the hash is computed.

5. [MERGE-ORDER] PR #399 and #401 collide on the same functions.
This PR changes add_one's signature (adds dedup: &mut DedupIndex, allow_duplicate: bool) and return type (Result<KnowledgeEntry> → Result<WriteOutcome>), and rewrites the batch match arms. PR #399 (non-fatal post-write side effects) touches the same add-path functions — a textual conflict on add_one is near-certain, and semantically #399's side effects must sit after this PR's Skip early-return (a skip is a no-op → no side effects, which is correct). PR #401 (exclude-tags) also touches memory.rs + knowledge.rs. Whichever lands second re-threads. The PR already flags this; confirming it is a real, visible conflict, not a phantom. (Agreed landing plan below.)

---

What is right (the acceptable part)

• src/knowledge.rs dedup_hash: length-prefixed title/body pair fed to blake3. The boundary-collapse blocker ("Ship it"/"now." vs "Ship it now"/"") is genuinely fixed, with a regression test (test_dedup_hash_does_not_collapse_title_body_boundary). The LANDMINE doc-comment is exactly the readability standard I ask for — it tells the next editor why, not just what.
• Fail-open on lookup error, unified in one dedup_gate() — with a fault-injecting unit test (lookup_failure_fails_open_and_the_write_still_lands) that asserts the row actually persists. Correct: a mechanism that tolerates a write race must tolerate a read blip.
• All three funnels (standard add_one, single-add --type, batch inline --type) route through the one gate; the previously-missed inline fact path is covered and tested (single_add_fact_type_path_*, batch_fact_type_path_*, standard_batch_path_also_dedups_via_add_one).
• No migration risk: dedup_hash is never persisted, recomputed from candidate rows on every check, so legacy unhashed entries participate in dedup on the next same-session write. Clean.
• record() fires only after a verified durable upsert, never on the skip or failed-write path — so a failed earlier batch line can't poison a distinct later one. Correct.
• Skip is Ok + exit 0 + duplicate_of pointer, id key mirrors success payloads, plain-mode phrasing kept distinct from "Added entry:". Caller is told.
• Test/logic ratio: ~2,100 lines test / ~700 lines logic / ~270 docs. The bulk is tests and fixtures, as it must be for a data-loss-risk feature.

---

Merge order (agreed cross-PR plan)

Recommended landing order across the three concurrent memory PRs: #401 → #402 → #399. This PR lands second — after #401, before #399.

• #401 first (exclude-tags, read-path only): independent, lands first to minimize rebase churn.
• #402 second (this PR): lands next carrying the category/tags dedup-identity fix (finding 1). Design the identity contract once, correctly, before anything rebases onto it.
• #399 third (non-fatal post-write side effects): rebases on top and threads its structured degraded-state field (embed_deferred) into this PR's already-richer --json schema (skipped / duplicate_of / status), so the JSON contract is designed once, coherently. Note for #399's rebase: this PR's Skip early-return correctly means no post-write side effects run on a dedup skip — #399's degraded-state logic sits after the write, past the early-return, and never fires on a skip.
• Deferred UNIQUE index (whenever it lands): it must include the category dimension too — DEFINE INDEX ... UNIQUE ON knowledge FIELDS session, owner, category, dedup_hash — consistent with finding 1.

---

Fix finding #1. The rest is acceptable.

[gabaum10]

Round 1 review addressed: 1 fixed, 0 deferred, 0 skipped.

Finding	File	Disposition	Evidence
F1: Dedup identity excluded category/tags — identical title+body re-filed under a different category in the same session silently collided (false-positive data loss)	src/surreal_db/knowledge.rs:727-777, src/knowledge.rs:233+	Fixed	c5799a3 — get_entries_for_session/_async now take a category param and the SQL WHERE clause adds AND category = type::thing('category', $category) as a bound query parameter (not interpolated), scoping dedup candidates by category rather than folding a third field into the hash (avoiding the LANDMINE ambiguity the hash's own doc-comment warned about). dedup_hash gets a new IDENTITY CONTRACT doc-comment stating category is enforced at the candidate-query layer and tags are deliberately excluded (edge-modeled, not a scalar row field). DedupIndex group/seen keys now include category so a same-hash entry from a different category's fetch can never shadow another category's check. All three write funnels (add_one, single-add --type, batch inline --type) thread category through the shared gate. New/updated tests: identical_content_different_category_does_not_dedup, identical_content_different_tags_still_dedups, test_get_entries_for_session_category_scope_excludes_other_categories. Deferred items (TOCTOU-open gate, unbounded candidate fetch, #399/#401 merge order) are comment-lane per triage and out of scope for this fix.

---

Also in this push, two test-harness patches (pre-existing, environmental — unrelated to this PR's logic): trigger_check.rs and triggers_cli.rs both inherited ambient MX_SURREAL_* network env from dev shells, connecting "isolated" tests to a live DB where namespace-scoped sessions get IAM-refused on schema DDL. Both now pin embedded mode + strip the ambient vars, mirroring the isolation pattern dedup_write_boundary.rs already carried. CI was never affected (no ambient env there). Note for a possible follow-up, your call: three test files now hand-roll the same six-line isolation block — a shared spawn_isolated_mx() helper would keep the fourth from being written wrong.

[coryzibell]

Re-review (Verdictia Stern) — @ b9d6d0c

VERDICT: APPROVE — conditioned on one mechanical rebase item (below), which the build forces anyway.

Prior BLOCKER — dedup identity ignored category → RESOLVED ✅

Fixed in two layers, correctly:

• Candidate query (src/surreal_db/knowledge.rs:766): AND category = type::thing('category', $category), bound param — wrong-category rows never fetched.
• In-memory key: DedupIndex keyed (session_id, owner, category, dedup_hash) (src/handlers/memory.rs:363), with a doc-comment correctly explaining why both layers are needed.

Proven, not asserted: identical_content_different_category_does_not_dedup (memory.rs:5476) — identical title+body under two categories both land as Written, second candidate query fires, and same-category recase still dedups. Store-layer pin at surreal_db/tests.rs:3331.

Bonus: the \n-join title/body boundary collapse (self-review finding-1) is fixed via a length-prefixed pair hash (knowledge.rs:254), pinned by test, with a LANDMINE doc-comment warning that a third hashed field needs its own prefix. Curt approval on that comment specifically.

Deferred UNIQUE index: still not landed (nothing persisted → no migration risk); the required shape FIELDS session, owner, category, dedup_hash is now on the record in the DedupIndex doc-comment.

Tags divergence — documented accepted false-negative (note, not blocker)

Identical title+body with different tags still dedups; identical_content_different_tags_still_dedups (memory.rs:5560) pins it deliberately. A visible, tested, documented decision rather than silent behavior. Acceptable.

Prior positives — all hold, some improved ✅

• Fail-open now structural: three hand-rolled gates collapsed into one dedup_gate (memory.rs:577) that logs and returns Proceed on error, tested with a FailingStore.
• All 3 funnels share the single gate (memory.rs:722, 1400, 2809) — divergence now requires editing the gate, not forgetting a copy.
• Skip early-return before upsert/verify/edge/embed/anchor and before record() (memory.rs:733) — fix: durable memory writes no longer exit 1 when post-write embed/anchor fails #399's dependency is safe.
• Newest two commits are test-env isolation only; nothing rode in.

⚠️ Merge vs current main — one certain mechanical break

git merge-tree vs main (now containing #405 / 1a309c0) is textually clean, BUT: #405 added owned_private_matching as a required KnowledgeStore method. This branch's two new test doubles — CountingStore (memory.rs:4429) and FailingStore (memory.rs:4766) — predate it and will fail post-rebase with E0046. Fix during rebase: one-line delegating impl on each. MockStore in wake_ritual.rs is unaffected.

Findings

Sev	Location	Finding
W	memory.rs:4429, :4766	Test doubles miss owned_private_matching → E0046 after rebase onto #405; add delegating stubs
N	memory.rs:5560	Tag-divergence dedup: documented accepted false-negative; no action

the smallest nod. Fix the two stubs on rebase. The rest is correct. Lands SECOND (#401 → #402 → #399).

[gabaum10]

Rebased onto main (post-#405); delegating owned_private_matching stubs added to CountingStore + FailingStore per review. cargo check --all-targets + full suite green.