Fix re-audit Councils A+B findings (FK scope, identity divergence, lease split-brain, child leak, Pi primer seed)

Ufuk Altinok · alfonso-magic-context · Ufuk Altinok · commit 5f2e81dfd086 · 2026-06-24T14:12:40.000+02:00
Round-2 blind re-audit, councils A (embedding) + B (dreamer/smart-notes): Council A (both source-confirmed, both touch this round's code): - A#1: v49 ran a GLOBAL PRAGMA foreign_key_check after the FIRST table rebuild, before the git/chunk orphan pre-cleans ran — a pre-existing orphan in a later table failed the migration closed. Scoped assertForeignKeyIntegrity to the just-rebuilt table. +regression test seeding a chunk-table orphan. - A#2 (regression I introduced adding truncate to identity): the provider constructor's own this.modelId omitted truncate while canonical getEmbeddingProviderIdentity included it → writes/reads under divergent model_ids when truncate set. Mirrored truncate into the constructor. +parity test asserting provider.modelId === getEmbeddingProviderIdentity(config) across all identity-affecting fields. Council B: - cortexkit#5: buildHiddenAgentConfig dropped user permission/tools under lockPermissions but NOT prompt/system — a user dreamer.prompt could clobber the privacy- hardened retrospective prompt. Now strips prompt/system under lock too. +2 tests. - #1: failed map/verify/classify children (memory-pool text) were kept on the !phaseFailed path. Delete on success AND failure; still honor keep_subagents (memory-pool text, not raw user transcripts — unlike retrospective). Removed now-dead phaseFailed in all three runners. - cortexkit#3: lease heartbeat reclaimed an expired-but-free lease even after a >TTL gap, risking split-brain after a multi-minute stall (a sibling could have run in the gap). Declare lost when the gap exceeds a full TTL; short ≤TTL self-inflicted expiry still reclaims. +split-brain regression test. - Pi primer seed (single-reporter, real): scheduled refresh-primers never got primerRawProviderFactory (only the manual /ctx-dream path did) → scheduled Pi ran closed-book, defeating the open-book redesign. Threaded the factory through ProjectRegistration → scheduled executor; Pi supplies its JSONL factory. OpenCode unaffected (buildPrimerSeed reads opencode.db directly). Accepted/false-positive (documented A50/A51): #2 smart-note egress is the deliberate v1 design (manifest advisory, SSRF + secret-denylist are the real boundaries); content-edit-not-invalidating-check is correct (check keys on the trigger, not the body). Gate: plugin 2516/0, Pi 509/0, both lint-clean. Co-authored-by: Alfonso [Magic Context] <288211368+alfonso-magic-context@users.noreply.github.com>
diff --git a/docs/AUDIT-KNOWN-ISSUES.md b/docs/AUDIT-KNOWN-ISSUES.md
@@ -819,3 +819,29 @@ window where it GCs off a stale trusted registration. Pi never calls the GC at
 all. Persisting the latch (a migration + cross-process write coordination) would
 add complexity for a window that does not exist given this ordering. If the
 ensureRegistered-before-GC pairing is ever broken, revisit this.
+
+### A50. Smart-note compiled checks expose readFile + httpGet in one sandbox (egress is the accepted v1 design)
+Compiled smart-note checks run in a QuickJS WASM sandbox with BOTH a `readFile`
+capability (repo-relative, denylist-guarded) and an `httpGet` capability
+(SSRF-guarded, public hosts). An audit recurringly flags this as an exfiltration
+P0: a prompt-injected `surface_condition` could author a check that reads a repo
+file and POSTs it to an attacker host, and the capability manifest is advisory
+(not a runtime allowlist). This is the DELIBERATE v1 decision (see
+`.alfonso/plans/smart-notes-compiled-checks-spec.md`): v1 allows all external
+egress, the manifest is an audit artifact not a security boundary, and the actual
+boundaries are the fail-closed SSRF guard (blocks loopback/RFC1918/link-local/
+metadata, pins the socket to the validated IP) and the secret denylist
+(`.env*`, `.npmrc`, `.git`, `secrets/`). Rationale: the host agent that authored
+the note already reads files and hits the network with FAR fewer limits than the
+sandbox, so the marginal risk of sanctioned in-sandbox egress is small, and
+per-note user-approved egress was judged not worth the v1 friction. Per-note
+egress approval remains the planned hardening if real-world misuse appears. Not a
+regression; do not re-flag without new evidence of exploitation.
+
+### A51. Content edits do not invalidate a smart-note's compiled check (correct — the check keys on the trigger, not the body)
+An audit flagged that editing a smart note's `content` leaves its compiled check
+stale. Verified a FALSE POSITIVE: the compiled check evaluates the
+`surface_condition` (the trigger), never the note body. `updateNote` resets the
+whole compiled-check lifecycle ONLY when `surface_condition` changes
+(`smartConditionChanged` in `storage-notes.ts`), which is correct — a body edit
+doesn't change what the check tests, so re-compiling would be wasted work.
diff --git a/packages/pi-plugin/src/dreamer/index.ts b/packages/pi-plugin/src/dreamer/index.ts
@@ -155,6 +155,10 @@ export function registerPiDreamerProject(opts: PiDreamerOptions): void {
 		// /ctx-dream path already uses.
 		retrospectiveRawProvider: () =>
 			new PiRetrospectiveRawProvider({ projectCwd: opts.projectDir }),
+		// SCHEDULED refresh-primers likewise needs the Pi JSONL factory so its
+		// open-book seed renders raw U:/TC: lines; without it the scheduled task
+		// silently ran closed-book (the manual /ctx-dream path already wires this).
+		primerRawProviderFactory: createPiPrimerRawProviderFactory(),
 	}).then((timerCleanup) => {
 		if (cancelled) {
 			// Registration was cancelled before timer setup completed —
diff --git a/packages/plugin/src/agents/hidden-agent-registrations.ts b/packages/plugin/src/agents/hidden-agent-registrations.ts
@@ -270,21 +270,40 @@ export function buildHiddenAgentConfig(
     const {
         permission: overridePermission,
         tools: overrideTools,
+        // Pulled out so they can be conditionally re-added: under lockPermissions
+        // a user `prompt`/`system` override must NOT replace the hardened
+        // system prompt (see below). Spreading `...rest` below would otherwise
+        // clobber the positional `prompt` arg, since `...rest` lands after it.
+        prompt: overridePrompt,
+        system: overrideSystem,
         ...rest
     } = (overrides ?? {}) as {
         permission?: Record<string, unknown>;
         tools?: Record<string, boolean>;
+        prompt?: unknown;
+        system?: unknown;
         [key: string]: unknown;
     };
     // When locked (privacy-critical agents), the user `tools` enable/disable map
-    // is ALSO dropped — it is a privilege-escalation surface (a user
-    // `dreamer.tools: { bash: true }` would otherwise re-enable a denied tool on
-    // the retrospective agent). Unlocked agents keep their `tools` override.
-    const restOverrides = lockPermissions
-        ? rest
-        : overrideTools !== undefined
-          ? { ...rest, tools: overrideTools }
-          : rest;
+    // AND the `prompt`/`system` text are ALL dropped — each is a
+    // privilege/behavior-escalation surface. A user `dreamer.tools: { bash: true }`
+    // would otherwise re-enable a denied tool; a user `dreamer.prompt`/`system`
+    // would otherwise replace the privacy-hardened prompt that, e.g., keeps the
+    // retrospective agent from transcribing raw user text. Unlocked agents keep
+    // their `tools`/`prompt`/`system` overrides.
+    const promptOverrides: Record<string, unknown> = lockPermissions
+        ? {}
+        : {
+              ...(overridePrompt !== undefined ? { prompt: overridePrompt } : {}),
+              ...(overrideSystem !== undefined ? { system: overrideSystem } : {}),
+          };
+    const restOverrides: Record<string, unknown> = lockPermissions
+        ? { ...rest, ...promptOverrides }
+        : {
+              ...rest,
+              ...promptOverrides,
+              ...(overrideTools !== undefined ? { tools: overrideTools } : {}),
+          };
     const basePermission = buildAllowOnlyPermission(allowedTools, agentLabel);
     return {
         prompt,
diff --git a/packages/plugin/src/features/magic-context/dreamer/classify.ts b/packages/plugin/src/features/magic-context/dreamer/classify.ts
@@ -187,7 +187,6 @@ async function classifyOneChunk(
     signal: AbortSignal,
 ): Promise<{ classified: number; changed: number }> {
     let agentSessionId: string | null = null;
-    let phaseFailed = false;
     const startedAt = Date.now();
     try {
         const createResponse = await args.client.session.create({
@@ -249,7 +248,6 @@ async function classifyOneChunk(
         recordInvocation(args, startedAt, { status: "completed", messages: run.output });
         return applyClassifications(args, chunk, run.validated);
     } catch (error) {
-        phaseFailed = true;
         const desc = describeError(error);
         log(
             `[dreamer] classify chunk failed: ${desc.brief}`,
@@ -259,7 +257,10 @@ async function classifyOneChunk(
         if (signal.aborted) throw error;
         return { classified: 0, changed: 0 };
     } finally {
-        if (agentSessionId && !phaseFailed && !shouldKeepSubagents()) {
+        // Delete on success AND failure (the failed child still holds the
+        // memory-pool snapshot from the prompt). keep_subagents still honored —
+        // memory-pool text, not raw user transcripts.
+        if (agentSessionId && !shouldKeepSubagents()) {
             await args.client.session
                 .delete({
                     path: { id: agentSessionId },
diff --git a/packages/plugin/src/features/magic-context/dreamer/lease.test.ts b/packages/plugin/src/features/magic-context/dreamer/lease.test.ts
@@ -1,6 +1,6 @@
 /// <reference types="bun-types" />
 
-import { describe, expect, it } from "bun:test";
+import { describe, expect, it, spyOn } from "bun:test";
 import { mkdtempSync, rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -231,6 +231,47 @@ describe("startLeaseHeartbeat", () => {
         closeQuietly(db);
     });
 
+    it("declares lost (not reclaim) when the lease lapsed past a full TTL — split-brain guard", async () => {
+        // A >TTL stall (e.g. machine sleep): our lease lapsed and a sibling could
+        // have acquired AND mutated in the gap. Even though the lease may now be
+        // free, blindly reclaiming + continuing on our stale snapshot is
+        // split-brain — the heartbeat must declare lost. (A short ≤TTL gap still
+        // reclaims; see the transient-tolerant test above.)
+        const db = makeDb();
+        const realNow = Date.now();
+        const clock = { value: realNow };
+        const nowSpy = spyOn(Date, "now").mockImplementation(() => clock.value);
+        try {
+            expect(acquireLease(db, "holder-a")).toBe(true);
+            let lostReason: string | null = null;
+            // Short interval so a real timer beat fires; the FIRST synchronous
+            // beat confirms ownership at t0 (lastConfirmedAt = realNow).
+            const hb = startLeaseHeartbeat(
+                db,
+                "holder-a",
+                DREAMING_LEASE_KEY,
+                (reason) => {
+                    lostReason = reason;
+                },
+                20,
+            );
+            expect(hb.lost).toBe(false);
+
+            // Jump the clock 3 minutes (> 2min TTL): our own lease has lapsed
+            // (isLeaseActive false), so the next beat's renewLease fails and the
+            // gap exceeds the TTL.
+            clock.value = realNow + 3 * 60 * 1000;
+            await sleep(60);
+
+            expect(hb.lost).toBe(true);
+            expect(lostReason).toContain("past TTL");
+            hb.stop();
+        } finally {
+            nowSpy.mockRestore();
+            closeQuietly(db);
+        }
+    });
+
     it("declares lost exactly once when a different holder actively owns the lease", async () => {
         const db = makeDb();
         expect(acquireLease(db, "holder-a")).toBe(true);
diff --git a/packages/plugin/src/features/magic-context/dreamer/lease.ts b/packages/plugin/src/features/magic-context/dreamer/lease.ts
@@ -187,10 +187,28 @@ export function startLeaseHeartbeat(
     const beat = () => {
         if (lost) return;
         try {
-            // renew-or-reclaim: renewLease keeps it if still ours; acquireLease
-            // reclaims an expired/free lease and only returns false when a
-            // different holder is actively in possession.
-            if (renewLease(db, holderId, leaseKey) || acquireLease(db, holderId, leaseKey)) {
+            // Continuous ownership: renewLease keeps it if still ours. This is
+            // the always-safe path — we never lost the lease.
+            if (renewLease(db, holderId, leaseKey)) {
+                lastConfirmedAt = Date.now();
+                return;
+            }
+            // renewLease failed → we are no longer the recorded holder OR the
+            // lease lapsed. If the gap since our last confirmed beat exceeds a
+            // full TTL, the lease was provably claimable by another process for a
+            // meaningful window — a sibling could have acquired AND mutated in the
+            // gap (a >2min stall / machine sleep), so blindly reclaiming a now-free
+            // lease and continuing on our stale snapshot is split-brain. Declare
+            // lost instead. A SHORT delay (≤ TTL, e.g. a slightly-late 60s beat
+            // causing self-inflicted expiry) still recovers via reclaim below.
+            if (Date.now() - lastConfirmedAt > LEASE_DURATION_MS) {
+                declareLost("lease lapsed past TTL — another holder may have run");
+                return;
+            }
+            // reclaim an expired-but-free lease after only a short gap (our own
+            // delayed beat); returns false only when a different holder is
+            // actively in possession.
+            if (acquireLease(db, holderId, leaseKey)) {
                 lastConfirmedAt = Date.now();
                 return;
             }
diff --git a/packages/plugin/src/features/magic-context/dreamer/map-memories.ts b/packages/plugin/src/features/magic-context/dreamer/map-memories.ts
@@ -141,7 +141,6 @@ async function mapOneBatch(
     signal: AbortSignal,
 ): Promise<{ mapped: number; independent: number }> {
     let agentSessionId: string | null = null;
-    let phaseFailed = false;
     const startedAt = Date.now();
     try {
         const createResponse = await args.client.session.create({
@@ -199,7 +198,6 @@ async function mapOneBatch(
         recordInvocation(args, startedAt, { status: "completed", messages: run.output });
         return await applyBatchMappings(args, batch, run.validated);
     } catch (error) {
-        phaseFailed = true;
         const desc = describeError(error);
         log(
             `[dreamer] map-memories batch failed: ${desc.brief}`,
@@ -211,7 +209,10 @@ async function mapOneBatch(
         if (signal.aborted) throw error;
         return { mapped: 0, independent: 0 };
     } finally {
-        if (agentSessionId && !phaseFailed && !shouldKeepSubagents()) {
+        // Delete on success AND failure (the failed child still holds the
+        // memory-pool snapshot from the prompt). keep_subagents still honored —
+        // memory-pool text, not raw user transcripts.
+        if (agentSessionId && !shouldKeepSubagents()) {
             await args.client.session
                 .delete({
                     path: { id: agentSessionId },
diff --git a/packages/plugin/src/features/magic-context/dreamer/verify.ts b/packages/plugin/src/features/magic-context/dreamer/verify.ts
@@ -134,7 +134,6 @@ async function verifyOneBatch(
     signal: AbortSignal,
 ): Promise<{ verified: number; updated: number; archived: number }> {
     let agentSessionId: string | null = null;
-    let phaseFailed = false;
     const startedAt = Date.now();
     try {
         const createResponse = await args.client.session.create({
@@ -192,7 +191,6 @@ async function verifyOneBatch(
         recordInvocation(args, startedAt, { status: "completed", messages: run.output });
         return await applyVerifyManifest(args, batch, run.validated);
     } catch (error) {
-        phaseFailed = true;
         const desc = describeError(error);
         log(
             `[dreamer] verify batch failed: ${desc.brief}`,
@@ -202,7 +200,13 @@ async function verifyOneBatch(
         if (signal.aborted) throw error;
         return { verified: 0, updated: 0, archived: 0 };
     } finally {
-        if (agentSessionId && !phaseFailed && !shouldKeepSubagents()) {
+        // Delete the child regardless of success/failure (a FAILED child still
+        // holds the memory-pool snapshot fed into the prompt — leaving it only on
+        // the failure path leaked them on disk). Still honor keep_subagents: this
+        // child carries curated project memories (already in context.db), not raw
+        // user text, so the user's explicit data-collection opt-in wins — unlike
+        // the retrospective child, which is purged unconditionally.
+        if (agentSessionId && !shouldKeepSubagents()) {
             await args.client.session
                 .delete({
                     path: { id: agentSessionId },
diff --git a/packages/plugin/src/features/magic-context/memory/embedding-openai.test.ts b/packages/plugin/src/features/magic-context/memory/embedding-openai.test.ts
@@ -1,6 +1,54 @@
 import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
+import type { EmbeddingConfig } from "../../../config/schema/magic-context";
+import { getEmbeddingProviderIdentity } from "./embedding-identity";
 import { embeddingModelsMatch, OpenAICompatibleEmbeddingProvider } from "./embedding-openai";
 
+describe("provider modelId matches canonical identity (write/read must agree)", () => {
+    // The provider's own this.modelId is what WRITES are stored under; the
+    // registry/GC/reads resolve via getEmbeddingProviderIdentity(config). Any
+    // identity-affecting field present in one but not the other splits the
+    // vector space → silent zero-results + GC of valid vectors. Lock parity
+    // across every identity-affecting field.
+    const cases: Array<{ name: string; config: EmbeddingConfig }> = [
+        {
+            name: "endpoint+model only",
+            config: { provider: "openai-compatible", endpoint: "http://h/v1", model: "m" },
+        },
+        {
+            name: "with api_key + input_type",
+            config: {
+                provider: "openai-compatible",
+                endpoint: "http://h/v1",
+                model: "m",
+                api_key: "k",
+                input_type: "passage",
+            },
+        },
+        {
+            name: "with truncate set",
+            config: {
+                provider: "openai-compatible",
+                endpoint: "http://h/v1",
+                model: "m",
+                truncate: "END",
+            },
+        },
+    ];
+    for (const c of cases) {
+        test(c.name, () => {
+            const provider = new OpenAICompatibleEmbeddingProvider({
+                endpoint: c.config.endpoint,
+                model: c.config.model,
+                apiKey: c.config.provider === "openai-compatible" ? c.config.api_key : undefined,
+                inputType:
+                    c.config.provider === "openai-compatible" ? c.config.input_type : undefined,
+                truncate: c.config.provider === "openai-compatible" ? c.config.truncate : undefined,
+            });
+            expect(provider.modelId).toBe(getEmbeddingProviderIdentity(c.config));
+        });
+    }
+});
+
 describe("embeddingModelsMatch token-boundary semantics", () => {
     test("exact match", () => {
         expect(embeddingModelsMatch("qwen3-embedding-4b", "qwen3-embedding-4b")).toBe(true);
diff --git a/packages/plugin/src/features/magic-context/memory/embedding-openai.ts b/packages/plugin/src/features/magic-context/memory/embedding-openai.ts
@@ -138,6 +138,12 @@ export class OpenAICompatibleEmbeddingProvider implements EmbeddingProvider {
             model: this.model,
             ...(this.apiKey ? { api_key: this.apiKey } : {}),
             ...(this.inputType ? { input_type: this.inputType } : {}),
+            // truncate participates in identity (it changes which text an
+            // over-long input embeds). MUST mirror getEmbeddingProviderIdentity
+            // exactly — a missing field here makes the provider write under a
+            // different model_id than reads/GC resolve, silently zeroing results
+            // and reaping valid vectors.
+            ...(this.truncate ? { truncate: this.truncate } : {}),
         });
     }
 
diff --git a/packages/plugin/src/features/magic-context/migrations-v49.test.ts b/packages/plugin/src/features/magic-context/migrations-v49.test.ts
@@ -136,4 +136,40 @@ describe("migration v49 — per-model embedding coexistence", () => {
             closeQuietly(db);
         }
     });
+
+    test("reaches v49 with a pre-existing orphan in the chunk table (per-table FK check, not global)", () => {
+        // A global foreign_key_check after the FIRST (memory) rebuild would also
+        // inspect the still-old chunk table and fail closed on an orphan there —
+        // before the chunk orphan pre-clean ever runs. The per-table check + each
+        // table's own pre-clean must let the migration finish. The chunk table is
+        // the sharpest case: no prior migration ever cleaned it.
+        const db = new Database(":memory:");
+        try {
+            initializeDatabase(db);
+            runMigrations(db);
+
+            // Inject a chunk orphan (no parent compartment) with FK enforcement off,
+            // then roll v49 back and re-run.
+            db.exec("PRAGMA foreign_keys=OFF");
+            db.prepare(
+                `INSERT INTO compartment_chunk_embeddings (
+                    compartment_id, session_id, project_path, harness, window_index,
+                    start_ordinal, end_ordinal, chunk_hash, model_id, dims, vector, created_at
+                 ) VALUES (999999, 'ses-orphan', 'git:orphan', 'opencode', 0, 0, 1, 'h', 'm', 4, x'01020304', 1)`,
+            ).run();
+            db.exec("PRAGMA foreign_keys=ON");
+            db.prepare("DELETE FROM schema_migrations WHERE version = 49").run();
+
+            expect(() => runMigrations(db)).not.toThrow();
+            expect(countRows(db, "compartment_chunk_embeddings")).toBe(0);
+            expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]);
+            expect(
+                db
+                    .prepare("SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1")
+                    .get(),
+            ).toEqual({ version: LATEST_MIGRATION_VERSION });
+        } finally {
+            closeQuietly(db);
+        }
+    });
 });
diff --git a/packages/plugin/src/features/magic-context/migrations.ts b/packages/plugin/src/features/magic-context/migrations.ts
diff --git a/packages/plugin/src/index-refresh.test.ts b/packages/plugin/src/index-refresh.test.ts
diff --git a/packages/plugin/src/plugin/dream-timer.ts b/packages/plugin/src/plugin/dream-timer.ts
