Skip to content

Deduplicate a Bot-formed memory per Bot, and keep forgetting person-wide - #788

Open
Chebaleomkar wants to merge 2 commits into
CopilotKit:mainfrom
Chebaleomkar:fix/memory-dedup-per-bot
Open

Chebaleomkar wants to merge 2 commits into
CopilotKit:mainfrom
Chebaleomkar:fix/memory-dedup-per-bot

Conversation

@Chebaleomkar

Copy link
Copy Markdown
Contributor

Fixes #714, as decided there: option 1, with forgetting kept person-wide.

What this changes

formMemory deduplicated a Bot-formed memory per person and content, but retrieve gives a Bot only the rows it formed. A second Bot forming a fact the first already held got duplicate: true with the first Bot's row, so it could never recall the fact.

  • The dedup lookup, the onConflictDoNothing target and personal_memories_formed_digest_idx now include formedByAgentId. Migration 0057_memory_dedup_per_bot rebuilds the index and keeps every row. Existing rows cannot collide under the wider key, so there is no backfill.
  • Forgetting stays the person's. The lookup also matches a forgotten row from any Bot, and takes it first. So a fact the person deleted is answered forgotten: true to every Bot, even one that had already formed its own copy.
  • The Memory page already labels each row with the Bot that formed it, so two copies of one fact read as two Bots' memories. No UI change.

Where it runs

Server: save_observed_memory and background research, the two callers of formMemory. remember_personal_fact uses create and is unchanged.

Boundary and audit

No change. A Bot still recalls only what it formed, from apps it still holds.

Changelog

An entry under Unreleased, plus a "Before upgrading" bullet for the migration.

Tradeoffs

A server still on the previous release fails to save a newly formed Bot memory once 0057 has run, because its conflict target needs the old index. Keeping the old index for a release does not avoid this: while it exists it still refuses the second Bot's copy, which is the bug itself. I used the single index swap and stated the window in the changelog. The same conclusion was reached in the review on #766, which covers the index and the same-Bot cases. This PR adds person-wide forgetting and the reset case NathanTarbert asked for, on the next free migration number.

Proof

The tests land in their own commit before the fix. Against PostgreSQL 17 with every migration applied:

  • On main, two of the new tests fail. "the same fact formed by two Bots is kept for each" receives duplicate: true, and "resetting one Bot leaves another Bot's copy" finds no row for the other Bot. The forgetting test passes on main and guards that behaviour against this change.
  • With the fix, memory-store.integration, bot-lifecycle.integration and memory pass 38/38. memory-intelligence.integration, proactive-store.integration and proactive pass 24/24.
  • Server tsc, Biome format and lint, and drizzle-kit check are clean. The live index reads (owner_user_id, formed_by_agent_id, import_digest) WHERE formed_by = 'bot'.

Thanks to @kvnloo for tracing the schema contract on #714.

Two of these fail on main: the second Bot is answered as a duplicate
of the first Bot's row, so it never keeps the fact it formed.
formMemory deduplicated on (person, content), but retrieval gives a Bot
only the rows it formed. Include formedByAgentId in the lookup, the
onConflictDoNothing target and personal_memories_formed_digest_idx
(migration 0057). A tombstone from any Bot still answers forgotten, so
forgetting stays person-wide.

Fixes CopilotKit#714

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A fact one Bot remembered is reported as remembered to a second Bot, which then never sees it

1 participant