Skip to content

Commit 7d8e4b0

Browse files
committed
fix: close 6 data-integrity/correctness findings from Oracle audit wave 1
Three parallel read-only Oracle audits (transform cache-stability, data integrity, workspaces security). Cache-stability core verified safe (6/6 replay mechanisms). Six source-confirmed bugs fixed; two design questions banked to .alfonso/oracle-loop-findings.md (NOT guessed). 1. Cheap-gate historian suppression (compartment-trigger.ts). With NO in-memory tail (post-restart / marker-drain lag), the trigger's cheap pre-gate used the scoped tagger floor for getTriggerTagTokenUpperBound. A collapsed floor above live eligible tags excluded their tokens → could falsely cheap-skip a needed historian fire (context overflow). Fix: use floor 0 for the bound when inMemoryTail is undefined (full active+dropped sum is still a valid upper bound, just looser; pre-boundary tags only inflate it). The inMemoryTail- present path is self-correcting (estimateUntaggedInMemoryTailUpperBound charges below-floor tags since coveredOwnerMessageIds is also floor-scoped). +test. 2. v22 backfill embedding loss (v22-deferred-backfill.ts). The legacy collision- merge path deleted a source memory row (FK-cascading its embedding) without transferring one to the survivor — same class as the live-path fix in 21ef95c, missed here. Fix: INSERT OR IGNORE the source embedding onto the target before delete. +test (real FK cascade under foreign_keys=ON). 3. migrate-session split-brain hardening (migrate-session.ts). (a) context.db BEGIN IMMEDIATE moved INSIDE the try with a txBegan guard so a throwing BEGIN (after OpenCode already committed) still compensates; rollback guarded so it can't mask the original error or skip compensation. (b) Refuse to apply when the OpenCode session row is missing (no half-migration). (c) Open context.db with PRAGMA foreign_keys=ON + busy_timeout=5000 — the CLI bypasses initializeDatabase, so without this the merge-delete leaves ORPHANED embeddings instead of cascading. +2 tests. 4. Dashboard category change didn't bump epoch (db.rs update_memory_category). A category edit is render- and visibility-changing (heading + workspace shared/non-shared flip) but only queued an m[1] "update" delta, leaving cached m[0] CAS-valid — a member session could keep receiving a now-non-shared foreign memory (or miss a now-shared one). Fix: bump member epochs (workspace fan-out inside the tx) like update_memory_status; no-op guard skips same-category; epoch-bump XOR delta. +1 test, existing test updated. Gate: plugin 2168/0, CLI 179/0, dashboard rust 93+21+10+20/0, tsc+biome clean.
1 parent bfce553 commit 7d8e4b0

8 files changed

Lines changed: 221 additions & 30 deletions

File tree

packages/cli/src/commands/migrate-session.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,4 +452,17 @@ describe("applyMigrateSession — memory actions", () => {
452452
expect(session.directory).toBe("/old/dir");
453453
expect(session.project_id).toBe("global");
454454
});
455+
456+
it("refuses to apply when the OpenCode session row is missing (no half-migration)", () => {
457+
const { oc, ctx } = setup();
458+
const plan = planMigrateSession(SID, "/home/u/benchmarks", makeDeps(oc, ctx));
459+
// Session vanishes between plan and apply (e.g. deleted while we worked).
460+
oc.prepare("DELETE FROM session WHERE id = ?").run(SID);
461+
expect(() => applyMigrateSession(plan, "leave", makeDeps(oc, ctx))).toThrow(/not found/i);
462+
// Context.db must be untouched — ownership stays on the source identity.
463+
const ownership = ctx
464+
.prepare("SELECT project_path FROM session_projects WHERE session_id = ?")
465+
.get(SID) as { project_path: string };
466+
expect(ownership.project_path).toBe(FROM);
467+
});
455468
});

packages/cli/src/commands/migrate-session.ts

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,14 @@ export function applyMigrateSession(
240240
const priorRow = deps.opencodeDb
241241
.prepare(`SELECT ${restoreCols.join(", ")} FROM session WHERE id = ?`)
242242
.get(plan.sessionId) as Record<string, string | null> | undefined;
243+
// If the session row vanished between planning and applying (it shouldn't —
244+
// the precondition is "OpenCode stopped"), refuse rather than mutate context
245+
// for a session OpenCode no longer has and leave nothing to compensate with.
246+
if (!priorRow) {
247+
throw new Error(
248+
`Session ${plan.sessionId} not found in opencode.db — aborting (is OpenCode still running, or was the session deleted?).`,
249+
);
250+
}
243251

244252
deps.opencodeDb.exec("BEGIN IMMEDIATE");
245253
try {
@@ -248,7 +256,11 @@ export function applyMigrateSession(
248256
.run(...params, plan.sessionId);
249257
deps.opencodeDb.exec("COMMIT");
250258
} catch (error) {
251-
deps.opencodeDb.exec("ROLLBACK");
259+
try {
260+
deps.opencodeDb.exec("ROLLBACK");
261+
} catch {
262+
// ignore — nothing committed yet, the throw below is what matters
263+
}
252264
throw error;
253265
}
254266

@@ -280,8 +292,13 @@ export function applyMigrateSession(
280292
let chunkEmbeddingsRestamped = 0;
281293
const epochsBumped: string[] = [];
282294

283-
deps.contextDb.exec("BEGIN IMMEDIATE");
295+
// BEGIN is INSIDE the try: if it throws (e.g. DB locked), OpenCode is already
296+
// committed, so we must still compensate. `txBegan` gates the rollback so we
297+
// never ROLLBACK a transaction that never started.
298+
let txBegan = false;
284299
try {
300+
deps.contextDb.exec("BEGIN IMMEDIATE");
301+
txBegan = true;
285302
// session_projects ownership → new identity (upsert).
286303
deps.contextDb
287304
.prepare(
@@ -351,9 +368,18 @@ export function applyMigrateSession(
351368

352369
deps.contextDb.exec("COMMIT");
353370
} catch (error) {
354-
deps.contextDb.exec("ROLLBACK");
355-
// Context.db rolled back atomically; now undo the already-committed
356-
// OpenCode move so the two databases stay consistent (no split-brain).
371+
// Roll back context.db only if a transaction actually began, and never let
372+
// a failing ROLLBACK mask the original error OR skip compensation.
373+
if (txBegan) {
374+
try {
375+
deps.contextDb.exec("ROLLBACK");
376+
} catch {
377+
// ignore — compensation below is what keeps the two DBs consistent
378+
}
379+
}
380+
// OpenCode was already committed; undo it so the two databases stay
381+
// consistent (no split-brain). Runs for ANY post-OpenCode-commit failure,
382+
// including a BEGIN that never started a transaction.
357383
compensateOpenCode();
358384
throw error;
359385
}
@@ -481,6 +507,18 @@ export async function runMigrateSessionCli(args: string[]): Promise<number> {
481507
const contextDbPath = defaultContextDbPath();
482508
const opencodeDb = new Database(opencodeDbPath);
483509
const contextDb = new Database(contextDbPath);
510+
// This CLI opens context.db directly (bypassing initializeDatabase), so the
511+
// pragmas the plugin normally sets are absent. foreign_keys=ON is REQUIRED:
512+
// the collision-merge path (rekeyMemoryRowWithCollisionMerge) relies on
513+
// memory_embeddings FK-cascading when a source memory row is deleted — without
514+
// it, deletes leave ORPHANED embedding rows. busy_timeout avoids instant-fail
515+
// if a plugin process touches the shared DB during the move.
516+
try {
517+
contextDb.exec("PRAGMA foreign_keys=ON");
518+
contextDb.exec("PRAGMA busy_timeout=5000");
519+
} catch {
520+
// best-effort; the move's own transactions are the real safety net
521+
}
484522
try {
485523
const deps = realDeps(opencodeDb, contextDb);
486524
const plan = planMigrateSession(sessionId, expandedTo, deps);

packages/dashboard/src-tauri/src/db.rs

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2868,14 +2868,20 @@ pub fn update_memory_category(
28682868
// Phase A: resolve the target row before opening a write transaction.
28692869
let target = lookup_memory_mutation_target(conn, memory_id)?;
28702870

2871-
// Phase B: re-check the target row, mutate, and queue once.
2871+
// No-op guard: changing a memory to its current category must NOT bump any
2872+
// epoch (that would force a needless hard fold across every workspace member).
2873+
if target.category.as_deref() == Some(new_category) {
2874+
return Ok(());
2875+
}
2876+
2877+
// Phase B: re-check the target row, mutate, and bump epochs once.
28722878
let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?;
28732879
verify_memory_project_path_unchanged(&tx, memory_id, &target.project_path)?;
28742880

2875-
let (content, normalized_hash): (String, String) = tx.query_row(
2876-
"SELECT content, normalized_hash FROM memories WHERE id = ?1",
2881+
let normalized_hash: String = tx.query_row(
2882+
"SELECT normalized_hash FROM memories WHERE id = ?1",
28772883
params![memory_id],
2878-
|row| Ok((row.get(0)?, row.get(1)?)),
2884+
|row| row.get(0),
28792885
)?;
28802886

28812887
// Pre-check the UNIQUE(project_path, category, normalized_hash) constraint
@@ -2903,15 +2909,17 @@ pub fn update_memory_category(
29032909
params![new_category, now_millis(), memory_id],
29042910
)?;
29052911

2906-
queue_memory_mutation(
2907-
&tx,
2908-
&target.project_path,
2909-
"update",
2910-
memory_id,
2911-
None,
2912-
Some(new_category),
2913-
Some(&content),
2914-
)?;
2912+
// A category change is visibility- AND render-changing: it alters the heading
2913+
// a memory renders under in <project-memory>, and (in a workspace) can flip a
2914+
// foreign memory between shared and non-shared. Both change m[0] bytes, so bump
2915+
// member epochs to force a hard fold — exactly like update_memory_status. The
2916+
// hard fold re-renders m[0] from true memory state, so we do NOT also queue an
2917+
// m[1] "update" delta (epoch-bump XOR delta, matching the status path). Fan-out
2918+
// is resolved INSIDE the write tx so a concurrent add-member can't miss it.
2919+
let project_identity = normalize_stored_project_path(&target.project_path);
2920+
let epoch_bump_identities =
2921+
crate::workspaces::workspace_member_identities_for_project(&tx, &project_identity)?;
2922+
crate::workspaces::bump_epochs_for_identities(&tx, &epoch_bump_identities)?;
29152923

29162924
tx.commit()?;
29172925
Ok(())

packages/dashboard/src-tauri/tests/db_mutations.rs

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -702,17 +702,33 @@ fn test_update_memory_category_success() {
702702
.expect("get category");
703703
assert_eq!(category, "NAMING");
704704

705-
// Verify mutation log row is written
706-
assert_eq!(
707-
mutation_log_rows(&conn),
708-
vec![(
709-
"git:project-a".to_string(),
710-
"update".to_string(),
711-
id,
712-
Some("NAMING".to_string()),
713-
Some("memory for git:project-a".to_string()),
714-
)]
715-
);
705+
// A category change is render- AND visibility-changing (heading + workspace
706+
// shared/non-shared flip), so it bumps the epoch to force a hard fold and
707+
// writes NO m[1] mutation-log row (epoch-bump XOR delta, like status changes).
708+
assert_eq!(memory_epoch(&conn, "git:project-a"), 12);
709+
assert_eq!(mutation_log_rows(&conn), vec![]);
710+
}
711+
712+
#[test]
713+
fn update_memory_category_noop_does_not_bump_epoch() {
714+
// Changing a memory to its CURRENT category must not bump any epoch (that
715+
// would force a needless hard fold across every workspace member).
716+
let mut conn = make_db();
717+
let id = insert_memory(&conn, "git:project-a", "active");
718+
// Re-setting a memory to its existing category must be a no-op.
719+
let current: String = conn
720+
.query_row(
721+
"SELECT category FROM memories WHERE id = ?1",
722+
params![id],
723+
|row| row.get(0),
724+
)
725+
.expect("get category");
726+
seed_project_state(&conn, "git:project-a", 5, 0);
727+
728+
db::update_memory_category(&mut conn, id, &current).expect("noop category update");
729+
730+
assert_eq!(memory_epoch(&conn, "git:project-a"), 5);
731+
assert_eq!(mutation_log_rows(&conn), vec![]);
716732
}
717733

718734
#[test]

packages/plugin/src/features/magic-context/v22-deferred-backfill.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,45 @@ describe("runDeferredV22Backfill", () => {
179179
expect(unresolved.count).toBe(0);
180180
});
181181

182+
test("collision-merge preserves the source row's embedding on the survivor (no FK-cascade loss)", async () => {
183+
// Regression: when a legacy source row carrying an embedding collides with
184+
// an existing target row that has NONE, the merge deletes the source —
185+
// FK-cascading its embedding away. Without the INSERT OR IGNORE transfer,
186+
// the survivor would be left unembedded (silent vector loss). The DB is
187+
// initialized with foreign_keys=ON, so the cascade is real here.
188+
const database = makeDb();
189+
const targetId = insertMemory(database, "/proj/canonical", "dup-hash");
190+
const sourceId = insertMemory(database, "/proj/symlinked", "dup-hash");
191+
// Only the SOURCE (later-deleted) row has an embedding.
192+
database
193+
.prepare(
194+
"INSERT INTO memory_embeddings (memory_id, embedding, model_id) VALUES (?, ?, 'm')",
195+
)
196+
.run(sourceId, new Uint8Array([1, 2, 3, 4]));
197+
198+
const summary = await runDeferredV22Backfill(database, {
199+
resolveIdentity: () => "git:sharedidentity",
200+
yieldToEventLoop: async () => {},
201+
});
202+
203+
expect(summary.status).toBe("completed");
204+
// Source row gone; survivor is the earlier (target) row. (.get() returns
205+
// null on node:sqlite / undefined on bun:sqlite for a missing row.)
206+
expect(
207+
database.prepare("SELECT id FROM memories WHERE id = ?").get(sourceId) ?? null,
208+
).toBeNull();
209+
// The survivor must now carry the adopted embedding — not lost to cascade.
210+
const surviving = database
211+
.prepare("SELECT COUNT(*) AS c FROM memory_embeddings WHERE memory_id = ?")
212+
.get(targetId) as { c: number };
213+
expect(surviving.c).toBe(1);
214+
// And no orphaned embedding rows remain anywhere.
215+
const total = database.prepare("SELECT COUNT(*) AS c FROM memory_embeddings").get() as {
216+
c: number;
217+
};
218+
expect(total.c).toBe(1);
219+
});
220+
182221
test("concurrent project_path mutation is a guarded no-op", async () => {
183222
const database = makeDb();
184223
const rowId = insertMemory(database, "/race", "race");

packages/plugin/src/features/magic-context/v22-deferred-backfill.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,16 @@ export async function runDeferredV22Backfill(
295295
LIMIT 1`,
296296
);
297297
const bumpSeenCount = db.prepare("UPDATE memories SET seen_count = ? WHERE id = ?");
298+
// Preserve an embedding on the surviving target BEFORE the source row's
299+
// embedding FK-cascades away on DELETE. Same fix as the live
300+
// collision-merge path (rekeyMemoryRowWithCollisionMerge): the two rows
301+
// are content-equivalent (same category + normalized_hash), so either
302+
// vector is valid; INSERT OR IGNORE keeps the target's if it has one,
303+
// adopts the source's otherwise — so a merged row never loses its vector.
304+
const preserveEmbedding = db.prepare(
305+
`INSERT OR IGNORE INTO memory_embeddings (memory_id, embedding, model_id)
306+
SELECT ?, embedding, model_id FROM memory_embeddings WHERE memory_id = ?`,
307+
);
298308
const deleteMemoryRow = db.prepare("DELETE FROM memories WHERE id = ?");
299309

300310
for (const row of resolvedRows) {
@@ -312,6 +322,7 @@ export async function runDeferredV22Backfill(
312322
if (mergedSeen !== (collision.seen_count ?? 1)) {
313323
bumpSeenCount.run(mergedSeen, collision.id);
314324
}
325+
preserveEmbedding.run(collision.id, row.id);
315326
deleteMemoryRow.run(row.id);
316327
changedRows += 1;
317328
changedIdentities.add(row.identity);

packages/plugin/src/hooks/magic-context/compartment-trigger.test.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,61 @@ describe("checkCompartmentTrigger", () => {
276276
expect(covered.partReads()).toBeGreaterThan(0);
277277
});
278278

279+
it("does NOT cheap-skip below-floor tags when inMemoryTail is undefined (no historian suppression)", () => {
280+
// Regression: with NO in-memory tail (post-restart / marker-drain lag),
281+
// the cheap-gate cannot account for below-floor tags via
282+
// estimateUntaggedInMemoryTailUpperBound (that path needs the tail). If a
283+
// collapsed floor sits ABOVE live eligible tags, a SCOPED bound would
284+
// exclude their tokens → falsely cheap-skip a needed historian fire. The
285+
// fix uses floor 0 for the bound when inMemoryTail is undefined, so the
286+
// gate stays conservative and falls through to the authoritative path,
287+
// which then fires on the meaningful eligible head.
288+
useTempDataHome("compartment-trigger-belowfloor-undefined-tail-");
289+
const sessionId = "ses-belowfloor-undefined";
290+
// Big narratable eligible head (m-1..m-6) + protected tail (m-7..m-11).
291+
createOpenCodeDb(sessionId, [
292+
{ id: "m-1", role: "user", text: "a ".repeat(3500) },
293+
{ id: "m-2", role: "assistant", text: "done" },
294+
{ id: "m-3", role: "user", text: "b ".repeat(3500) },
295+
{ id: "m-4", role: "assistant", text: "done" },
296+
{ id: "m-5", role: "user", text: "c ".repeat(3500) },
297+
{ id: "m-6", role: "assistant", text: "done" },
298+
{ id: "m-7", role: "user", text: "protected 1" },
299+
{ id: "m-8", role: "user", text: "protected 2" },
300+
{ id: "m-9", role: "user", text: "protected 3" },
301+
{ id: "m-10", role: "user", text: "protected 4" },
302+
{ id: "m-11", role: "user", text: "protected 5" },
303+
]);
304+
const db = openDatabase();
305+
// Tag the eligible head at LOW tag_numbers (1..3) carrying real tokens.
306+
insertCoveredMessageTag(db, sessionId, "m-1", 1, 3500);
307+
insertCoveredMessageTag(db, sessionId, "m-3", 2, 3500);
308+
insertCoveredMessageTag(db, sessionId, "m-5", 3, 3500);
309+
310+
// Pass a taggerFloorOverride ABOVE every tag (simulating a collapsed
311+
// floor) with NO in-memory tail. Pre-fix: the scoped bound excludes tags
312+
// 1..3 (bound 0, nullCount 0) → cheap-skip → shouldFire:false (WRONG).
313+
// Post-fix: floor 0 includes them → bound >> budget → fall through →
314+
// tail_size fires on the eligible head.
315+
const result = checkCompartmentTrigger(
316+
db,
317+
sessionId,
318+
makeSessionMeta(sessionId, 25),
319+
{ percentage: 25, inputTokens: 50_000 },
320+
25,
321+
65,
322+
1_000,
323+
undefined,
324+
undefined,
325+
undefined,
326+
undefined,
327+
undefined, // inMemoryTail undefined — the regressing condition
328+
10_000, // taggerFloorOverride well above the tags' numbers (1..3)
329+
);
330+
331+
expect(result.shouldFire).toBe(true);
332+
});
333+
279334
it("falls through when the in-memory upper bound equals the trigger budget", () => {
280335
useTempDataHome("compartment-trigger-memory-equality-");
281336
const db = openDatabase();

packages/plugin/src/hooks/magic-context/compartment-trigger.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -489,10 +489,21 @@ export function checkCompartmentTrigger(
489489
// sum is a uselessly-loose bound AND leaves nullCount stuck at the
490490
// legacy-row count forever (never backfilled), so the skip could never
491491
// trigger; scoping makes it a tight valid upper bound with nullCount≈0.
492+
// When there IS an in-memory tail, the scoped floor is safe: any live
493+
// tag sitting below the floor has its owner message in `inMemoryTail`
494+
// and is NOT in the (also-floor-scoped) covered-owner set, so
495+
// estimateUntaggedInMemoryTailUpperBound charges its true-raw tokens —
496+
// nothing is lost. But with NO in-memory tail (post-restart /
497+
// marker-drain lag) there is no such compensation: a scoped bound would
498+
// silently DROP the tokens of any live tool tag below the floor and
499+
// could falsely cheap-skip a needed historian fire (context overflow).
500+
// So fall back to floor 0 here — the full active+dropped sum is still a
501+
// valid UPPER bound (pre-boundary tags only inflate it), just looser.
502+
const boundFloor = inMemoryTail ? taggerFloor : 0;
492503
const { bound: persistedBound, nullCount } = getTriggerTagTokenUpperBound(
493504
db,
494505
sessionId,
495-
taggerFloor,
506+
boundFloor,
496507
);
497508
if (nullCount === 0) {
498509
const untaggedUpperBound = inMemoryTail

0 commit comments

Comments
 (0)