feat(memory): preserve corroborating episode evidence - #340
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/storage-db.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/storage-db.ts:1070">
P3: The v79 table and index definition is duplicated in `migrations.ts`. Future schema changes can leave fresh databases initialized here and upgraded databases migrated by v79 with different constraints or indexes; keep one canonical definition, preferably the migration, and remove this duplicate.</violation>
</file>
<file name="packages/plugin/src/hooks/magic-context/rust-mode-transform.ts">
<violation number="1" location="packages/plugin/src/hooks/magic-context/rust-mode-transform.ts:908">
P2: During authority preparation, this code runs a separate evidence query for every memory. Load evidence in one query and group it by `memory_id` to avoid making large-project handoffs unnecessarily slow.</violation>
</file>
<file name="packages/plugin/src/hooks/magic-context/module-state-sync.ts">
<violation number="1" location="packages/plugin/src/hooks/magic-context/module-state-sync.ts:1489">
P2: This map calls getMemoryEvidence(args.pass.db, memory.id) once per memory row, each issuing its own prepared-statement SQL query against memory_evidence. For a full force sync this is an N+1 query pattern on the module-state-sync hot path: with hundreds or thousands of memories it runs that many separate SELECTs while serializing the payload. Batch the evidence lookup into a single query (WHERE memory_id IN (...)) keyed by memory id, then assign per row.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/memory/promotion.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/memory/promotion.ts:65">
P2: When a legacy memory has no provenance row, a later historian session records evidence but leaves `seen_count` unchanged. Update the idempotent evidence path to preserve the legacy aggregate and count this newly observed session, so corroboration does not silently undercount migrated memories.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/migrations-v79.test.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/migrations-v79.test.ts:10">
P3: The v79 backfill test only covers the happy path (a memory with non-null source_session_id). The migration's two other contract branches are unverified: rows with NULL source_session_id must be excluded from backfill, and COALESCE(source_type, 'historian') must produce 'historian' when source_type is absent. Add assertions for those cases so a regression (e.g. dropping the WHERE clause and backfilling dangling evidence) is caught.</violation>
</file>
<file name="packages/pi-plugin/src/tools/ctx-memory.ts">
<violation number="1" location="packages/pi-plugin/src/tools/ctx-memory.ts:475">
P1: When an exact hash already exists in a Rust-module-managed project, this call records evidence through the unguarded existing-row branch. The old path checked TypeScript authority via `updateMemorySeenCount`, so add the same authority guard before dedup updates or route this write through the Rust facade.</violation>
</file>
<file name="packages/pi-plugin/src/tools/ctx-memory.test.ts">
<violation number="1" location="packages/pi-plugin/src/tools/ctx-memory.test.ts:1181">
P2: The test's title promises to verify that content-bound evidence is preserved through a merge, but it only checks the shape of the three rows: every `content_hash` is asserted with `expect.any(String)` and no row validates which hash it holds. As written, the assertion passes even if the canonical memory's own hash was written onto all copied rows, if all three hashes are identical (evidence dedupe/versioning broken), or if the copied hashes point at unrelated content. To actually verify the feature, assert each `content_hash` equals the expected normalized hash of its source content (and that the three hashes are distinct).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| } | ||
|
|
||
| const memory = insertMemory(deps.db, { | ||
| const insertResult = insertMemoryIdempotent(deps.db, { |
There was a problem hiding this comment.
P1: When an exact hash already exists in a Rust-module-managed project, this call records evidence through the unguarded existing-row branch. The old path checked TypeScript authority via updateMemorySeenCount, so add the same authority guard before dedup updates or route this write through the Rust facade.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/tools/ctx-memory.ts, line 475:
<comment>When an exact hash already exists in a Rust-module-managed project, this call records evidence through the unguarded existing-row branch. The old path checked TypeScript authority via `updateMemorySeenCount`, so add the same authority guard before dedup updates or route this write through the Rust facade.</comment>
<file context>
@@ -471,26 +472,19 @@ export function createCtxMemoryTool(
- }
-
- const memory = insertMemory(deps.db, {
+ const insertResult = insertMemoryIdempotent(deps.db, {
projectPath: projectIdentity,
category: rawCategory,
</file context>
| .all(canonical?.id), | ||
| ).toEqual([ | ||
| { | ||
| content_hash: expect.any(String), |
There was a problem hiding this comment.
P2: The test's title promises to verify that content-bound evidence is preserved through a merge, but it only checks the shape of the three rows: every content_hash is asserted with expect.any(String) and no row validates which hash it holds. As written, the assertion passes even if the canonical memory's own hash was written onto all copied rows, if all three hashes are identical (evidence dedupe/versioning broken), or if the copied hashes point at unrelated content. To actually verify the feature, assert each content_hash equals the expected normalized hash of its source content (and that the three hashes are distinct).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/tools/ctx-memory.test.ts, line 1181:
<comment>The test's title promises to verify that content-bound evidence is preserved through a merge, but it only checks the shape of the three rows: every `content_hash` is asserted with `expect.any(String)` and no row validates which hash it holds. As written, the assertion passes even if the canonical memory's own hash was written onto all copied rows, if all three hashes are identical (evidence dedupe/versioning broken), or if the copied hashes point at unrelated content. To actually verify the feature, assert each `content_hash` equals the expected normalized hash of its source content (and that the three hashes are distinct).</comment>
<file context>
@@ -1112,3 +1112,89 @@ describe("createCtxMemoryTool", () => {
+ .all(canonical?.id),
+ ).toEqual([
+ {
+ content_hash: expect.any(String),
+ source_message_id: null,
+ source_type: "agent",
</file context>
| UNIQUE(project_path, category, normalized_hash) | ||
| ); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS memory_evidence ( |
There was a problem hiding this comment.
P3: The v79 table and index definition is duplicated in migrations.ts. Future schema changes can leave fresh databases initialized here and upgraded databases migrated by v79 with different constraints or indexes; keep one canonical definition, preferably the migration, and remove this duplicate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/storage-db.ts, line 1070:
<comment>The v79 table and index definition is duplicated in `migrations.ts`. Future schema changes can leave fresh databases initialized here and upgraded databases migrated by v79 with different constraints or indexes; keep one canonical definition, preferably the migration, and remove this duplicate.</comment>
<file context>
@@ -1067,6 +1067,18 @@ export function initializeDatabase(db: Database): void {
UNIQUE(project_path, category, normalized_hash)
);
+ CREATE TABLE IF NOT EXISTS memory_evidence (
+ memory_id INTEGER NOT NULL REFERENCES memories(id) ON DELETE CASCADE,
+ content_hash TEXT NOT NULL,
</file context>
| import { initializeDatabase, LATEST_SUPPORTED_VERSION } from "./storage-db"; | ||
|
|
||
| describe("migration v79: memory evidence", () => { | ||
| it("creates the evidence identity and backfills known source sessions", () => { |
There was a problem hiding this comment.
P3: The v79 backfill test only covers the happy path (a memory with non-null source_session_id). The migration's two other contract branches are unverified: rows with NULL source_session_id must be excluded from backfill, and COALESCE(source_type, 'historian') must produce 'historian' when source_type is absent. Add assertions for those cases so a regression (e.g. dropping the WHERE clause and backfilling dangling evidence) is caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/migrations-v79.test.ts, line 10:
<comment>The v79 backfill test only covers the happy path (a memory with non-null source_session_id). The migration's two other contract branches are unverified: rows with NULL source_session_id must be excluded from backfill, and COALESCE(source_type, 'historian') must produce 'historian' when source_type is absent. Add assertions for those cases so a regression (e.g. dropping the WHERE clause and backfilling dangling evidence) is caught.</comment>
<file context>
@@ -0,0 +1,49 @@
+import { initializeDatabase, LATEST_SUPPORTED_VERSION } from "./storage-db";
+
+describe("migration v79: memory evidence", () => {
+ it("creates the evidence identity and backfills known source sessions", () => {
+ const db = new Database(":memory:");
+ try {
</file context>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/storage-identity-merge.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/storage-identity-merge.ts:206">
P1: When an identity merge is rerun after a source row has a legacy baseline, this call counts that baseline again and inflates `seen_count`. Make the transferred legacy baseline idempotent by skipping already-superseded source rows or recording and consuming the baseline on the source.</violation>
</file>
<file name="crates/mc-module/src/lib.rs">
<violation number="1" location="crates/mc-module/src/lib.rs:1702">
P2: When a state-sync memory contains `evidence: null`, this field becomes `None`, so `into_row` treats it as omitted and preserves stale evidence instead of rejecting the malformed snapshot. Reject explicit `null` while accepting only an omitted field or an array, matching the cross-runtime contract.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const targetId = collision.id; | ||
| const mergedSeen = Math.max(Number(collision.seen_count ?? 1), Number(row.seen_count ?? 1)); | ||
| const mergedSeen = | ||
| mergeMemoryEpisodeEvidence(db, targetId, [sourceId]) ?? |
There was a problem hiding this comment.
P1: When an identity merge is rerun after a source row has a legacy baseline, this call counts that baseline again and inflates seen_count. Make the transferred legacy baseline idempotent by skipping already-superseded source rows or recording and consuming the baseline on the source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/storage-identity-merge.ts, line 206:
<comment>When an identity merge is rerun after a source row has a legacy baseline, this call counts that baseline again and inflates `seen_count`. Make the transferred legacy baseline idempotent by skipping already-superseded source rows or recording and consuming the baseline on the source.</comment>
<file context>
@@ -201,23 +202,9 @@ function mergeMemoryRow(
- evidenceRow?.count ?? 0,
- );
+ const mergedSeen =
+ mergeMemoryEpisodeEvidence(db, targetId, [sourceId]) ??
+ Math.max(Number(collision.seen_count ?? 1), Number(row.seen_count ?? 1));
const sourceClassifiedAt = Number(row.classified_at ?? 0);
</file context>
| #[serde(default)] | ||
| mural_cue_rejection_count: i64, | ||
| #[serde(default)] | ||
| evidence: Option<Vec<ModuleMemoryEvidenceWire>>, |
There was a problem hiding this comment.
P2: When a state-sync memory contains evidence: null, this field becomes None, so into_row treats it as omitted and preserves stale evidence instead of rejecting the malformed snapshot. Reject explicit null while accepting only an omitted field or an array, matching the cross-runtime contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mc-module/src/lib.rs, line 1702:
<comment>When a state-sync memory contains `evidence: null`, this field becomes `None`, so `into_row` treats it as omitted and preserves stale evidence instead of rejecting the malformed snapshot. Reject explicit `null` while accepting only an omitted field or an array, matching the cross-runtime contract.</comment>
<file context>
@@ -1699,7 +1699,7 @@ struct ModuleMemoryWire {
mural_cue_rejection_count: i64,
#[serde(default)]
- evidence: Vec<ModuleMemoryEvidenceWire>,
+ evidence: Option<Vec<ModuleMemoryEvidenceWire>>,
}
</file context>
|
Retriggering the immutable head because the only failed check was a pre-code OpenCode version-download failure in the non-required 0.0.0.0 smoke probe. |
|
Ran two independent cross-family reviews of this PR against the design in #335, then verified every finding below myself against the PR branch ( First: the invariant we cared about holds, and our prescription was wrong. #335 argued for That's strictly better than what we asked for — it preserves content-version provenance that our 2-part key would have destroyed, and Also confirmed clean: Two findings both reviewers reached independentlyBoth reduce to the same root: the evidence table has no session-lifecycle story. Flagging now rather than later because nothing gates promotion on evidence yet ( 1.
Genuinely two-sided though, and the second reviewer argued the other side well: deleting evidence means a memory silently loses corroboration it legitimately earned. Its proposed third option — roll a terminal session's distinct contribution into a compact per-memory count, then drop the detail rows — preserves the count without keeping dead session references forever. Your call which semantics you want; the current state looks like neither choice was made explicitly. 2. No harness discriminator, which makes safe cleanup impossible. This one I can speak to from operational experience rather than reading. Yesterday I found 1,992 of 6,443
Three Shoulds, lower confidence — evidence attached, judge for yourself
Growth, for sizingNo retention, pruning, cap, or sweep. Rows grow as What we could not checkRust evidence tests didn't run — the cloned workspace was missing What I did verify by reading the PR branch directly: Happy to send a PR for the |
alfonso-magic-context
left a comment
alfonso-magic-context
left a comment
There was a problem hiding this comment.
Reviewed in depth — and worth saying first: you and @iceteaSA (#335) independently converged on the same missing primitive, which is strong signal the gap is real. The design direction is right: content-bound (memory, content hash, session) episodes make independent-session corroboration computable without adding a category, changing the uniqueness key, or touching rendered memory bytes.
Two blockers before this can merge:
- Rust-mode historian promotion records no episodes.
to_store_fact()leavessource_session_idasNone,publish_historian_chunk()callspromote_facts_tx()without the publishing session, and the exact-duplicate skip path never records evidence. In rust transform mode the historian is the main autonomous memory producer, so as delivered the corroboration signal would silently not accumulate exactly where it matters most. The new Rust tests cover direct insert/facade writes but not historian publication — that path needs both the wiring and a test. memory_evidenceneeds to join the authority guard fence. It mirrors from the module authority store, but the TypeScript side has no guard triggers for it, andrecordMemoryEvidence()doesn't assert write permission itself — today it's protected only incidentally because episode writes also touch the guarded parent row. An evidence-only write path added later could mutate a MODULE-authority project's read model unguarded. It should get its own triggers plus coverage in the armed-replay test alongside the six existing guarded tables.
Change requests are narrow and the core design stands. If you'd rather we take the fence work (it's deep in our authority machinery), say so and we'll layer it as a follow-up commit on your PR.
|
Design update that affects this PR: we've taken the corroboration question to a public design discussion in #335 (full reasoning there, including why session-keyed episodes conflict with the operating model we're converging on and what we're proposing instead — deterministic decay + a two-step historian observation-reconciliation). Parking this PR pending that thread's outcome rather than asking for the rework from the earlier review — your input on the #335 direction is very welcome, and the review's rust-wiring and authority-fence findings will apply to whatever variant emerges. |
3 participants
Summary
seen_countmonotonic and based on distinct corroborating sessions without reducing legacy aggregate countsFixes #335
Related to #334
Attribution boundary
OpenCode supplies the assistant message that owns the tool call, not a verified originating user message. Primary writes therefore remain
source_type=agent; the dormantuservariant is not falsely populated.source_message_idis the host-native assistant tool owner on OpenCode and null on Pi/Rust. A future verified host user-message link can activateusersafely.Semantic boundary
Evidence records observations of exact content versions. It does not claim paraphrased memories are the same assertion; explicit merge retains the original hashes.
Verification
mc-storecompiled against publiccortexkit\/commons; 136/136 store tests pass. Fullmc-moduleworkspace tests remain blocked by unavailablesubconsciouspath dependenciesNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Preserves content-bound, per-session episode evidence for memories and makes duplicate writes idempotent within a session. Old: duplicate writes bumped seen_count without provenance and evidence-only updates sometimes didn’t sync; new: evidence keyed by content hash and session recomputes seen_count from distinct sessions, and evidence-only changes now sync and seed mirrors in all runtimes.
memory_evidence(PK:memory_id, content_hash, source_session_id) with a session index; backfill frommemories; bumpLATEST_SUPPORTED_VERSIONto 79; replay-safe for armed stores.insertMemoryIdempotentandrecordMemoryEvidenceinstead of ad‑hoc seen_count bumps; same-session repeats are no-ops; distinct sessions add corroboration. Attribution requires a verified owner forsource_message_id; tool writes defaultsource_type=agentand never inferuser.Written for commit 1975b8a. Summary will update on new commits.
Greptile Summary
The PR adds content-versioned, per-session evidence for durable memories across the TypeScript, Pi, and Rust stores.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR W[Memory write or repeat] --> TS[(TypeScript memory store)] TS --> E[(Per-session evidence)] E --> C[Recompute monotonic seen_count] E --> Q[Request Rust memory sync] Q --> R[Rewind memory watermarks] R --> P[Build state-sync payload] P --> RS[(Rust memory store)] RS --> M[Mirror or authority transfer] M --> TSReviews (7): Last reviewed commit: "style(pi): format memory evidence regres..." | Re-trigger Greptile
Context used (3)