fix(storage): scope graph nodes per project (fix #78)

docs/SPEC.md

@@ -119,7 +119,8 @@ CREATE VIRTUAL TABLE records_fts USING fts5(title, body, content=records, conten
 CREATE TABLE nodes (
   id TEXT PRIMARY KEY, project_id TEXT,
   kind TEXT,                               -- file | symbol | package | module | branch
-  name TEXT UNIQUE
+  name TEXT NOT NULL,
+  UNIQUE(project_id, name)                 -- per-project: same name (e.g. `main` branch) may exist in different projects
 );
 CREATE TABLE edges (
   id TEXT PRIMARY KEY,

packages/agentctx/src/hooks/entities.ts

@@ -8,17 +8,20 @@ import { ulid } from "../storage/ulid.js";
 
 export type NodeKind = "file" | "symbol" | "package" | "module" | "branch";
 
-/** Insert-or-fetch a node by its unique name. Returns the node id. */
+/** Insert-or-fetch a node by its `(project_id, name)`. Returns the node id. */
 export function upsertNode(db: Database, projectId: string, kind: NodeKind, name: string): string {
   db.prepare("INSERT OR IGNORE INTO nodes (id, project_id, kind, name) VALUES (?, ?, ?, ?)").run(
     ulid(),
     projectId,
     kind,
     name,
   );
-  const row = db.prepare("SELECT id FROM nodes WHERE name = ?").get(name) as
-    | { id: string }
-    | undefined;
+  // Scope the lookup to the project: nodes are unique per `(project_id, name)`,
+  // so two projects sharing a node name (e.g. a `main` branch) must not resolve
+  // to each other's node and cross-link their records.
+  const row = db
+    .prepare("SELECT id FROM nodes WHERE project_id = ? AND name = ?")
+    .get(projectId, name) as { id: string } | undefined;
   if (row === undefined) {
     throw new Error(`node "${name}" vanished after upsert`);
   }

packages/agentctx/src/storage/schema.ts

@@ -87,8 +87,41 @@ CREATE TABLE sessions (
 );
 `;
 
+/**
+ * V2 — issue #78: `nodes` uniqueness must be per project, not global.
+ *
+ * V1 declares `name TEXT NOT NULL UNIQUE`, but nodes are project-scoped data
+ * (`deleteProjectData` removes `nodes WHERE project_id = ?`). A global unique on
+ * `name` means two projects that produce a node with the same name — e.g. a
+ * `main` / `master` branch node, which is the common case — collide: the second
+ * project's `INSERT OR IGNORE` is dropped and it silently reuses the first
+ * project's node id, cross-linking its records to another project's entity.
+ *
+ * Rebuild `nodes` with a composite `UNIQUE(project_id, name)`. Node ids (the
+ * PRIMARY KEY referenced by `record_entities` and `edges`) are preserved, so
+ * existing links survive the table swap. `applyMigrations` disables foreign
+ * keys around the migration (the SQLite-recommended procedure for table
+ * rebuilds) and re-checks integrity before re-enabling them.
+ */
+const SCHEMA_V2 = `
+CREATE TABLE nodes_new (
+  id TEXT PRIMARY KEY,
+  project_id TEXT,
+  kind TEXT,
+  name TEXT NOT NULL,
+  UNIQUE(project_id, name)
+);
+
+INSERT INTO nodes_new (id, project_id, kind, name)
+  SELECT id, project_id, kind, name FROM nodes;
+
+DROP TABLE nodes;
+
+ALTER TABLE nodes_new RENAME TO nodes;
+`;
+
 /** Ordered migrations; index N migrates user_version N → N+1. Append-only. */
-export const MIGRATIONS: readonly string[] = [SCHEMA_V1];
+export const MIGRATIONS: readonly string[] = [SCHEMA_V1, SCHEMA_V2];
 
 export const SCHEMA_VERSION = MIGRATIONS.length;
 
@@ -97,12 +130,34 @@ export function currentSchemaVersion(db: Database): number {
 }
 
 export function applyMigrations(db: Database): void {
-  for (let version = currentSchemaVersion(db); version < MIGRATIONS.length; version++) {
-    const migration = MIGRATIONS[version];
-    if (migration === undefined) break;
-    db.transaction(() => {
-      db.exec(migration);
-      db.pragma(`user_version = ${version + 1}`);
-    })();
+  const startVersion = currentSchemaVersion(db);
+  if (startVersion >= MIGRATIONS.length) return;
+
+  // Table-rebuild migrations (CREATE new / copy / DROP old / RENAME) require
+  // foreign keys to be disabled, and `PRAGMA foreign_keys` is a no-op inside a
+  // transaction — so toggle it here, around the per-migration transactions, per
+  // the SQLite procedure for schema changes. `defer_foreign_keys` is not enough:
+  // dropping a parent table leaves a deferred-constraint count that a later
+  // rename can't clear, so COMMIT fails even when no row is actually dangling.
+  const foreignKeysEnabled = db.pragma("foreign_keys", { simple: true }) === 1;
+  if (foreignKeysEnabled) db.pragma("foreign_keys = OFF");
+  try {
+    for (let version = startVersion; version < MIGRATIONS.length; version++) {
+      const migration = MIGRATIONS[version];
+      if (migration === undefined) break;
+      db.transaction(() => {
+        db.exec(migration);
+        db.pragma(`user_version = ${version + 1}`);
+      })();
+    }
+
+    if (foreignKeysEnabled) {
+      const violations = db.pragma("foreign_key_check") as unknown[];
+      if (violations.length > 0) {
+        throw new Error(`migration left ${violations.length} foreign key violation(s)`);
+      }
+    }
+  } finally {
+    if (foreignKeysEnabled) db.pragma("foreign_keys = ON");
   }
 }

packages/agentctx/test/storage/entities.test.ts

@@ -0,0 +1,81 @@
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { linkRecordToEntity, upsertNode } from "../../src/hooks/entities.js";
+import { insertRecord } from "../../src/storage/records.js";
+import { type TempDb, openTempDb } from "./helpers.js";
+
+describe("upsertNode — per-project node scoping (issue #78)", () => {
+  let tmp: TempDb;
+
+  beforeEach(() => {
+    tmp = openTempDb();
+  });
+
+  afterEach(() => {
+    tmp.cleanup();
+  });
+
+  it("gives two projects sharing a node name distinct node ids", () => {
+    const a = upsertNode(tmp.db, "projectA", "branch", "main");
+    const b = upsertNode(tmp.db, "projectB", "branch", "main");
+
+    expect(a).not.toBe(b);
+  });
+
+  it("links a record to its own project's node, not another project's", () => {
+    // Project A claims the `main` branch node first.
+    const nodeA = upsertNode(tmp.db, "projectA", "branch", "main");
+
+    // Project B's record must link to project B's `main` node.
+    const recordB = insertRecord(tmp.db, {
+      projectId: "projectB",
+      type: "decision",
+      title: "Release cut",
+      body: "Tagged from main",
+      source: "cli",
+    });
+    const nodeB = upsertNode(tmp.db, "projectB", "branch", "main");
+    linkRecordToEntity(tmp.db, recordB.id, nodeB);
+
+    expect(nodeB).not.toBe(nodeA);
+
+    const linkedNodeId = (
+      tmp.db
+        .prepare("SELECT entity_id FROM record_entities WHERE record_id = ?")
+        .get(recordB.id) as { entity_id: string }
+    ).entity_id;
+    expect(linkedNodeId).toBe(nodeB);
+
+    const linkedProjectId = (
+      tmp.db.prepare("SELECT project_id FROM nodes WHERE id = ?").get(linkedNodeId) as {
+        project_id: string;
+      }
+    ).project_id;
+    expect(linkedProjectId).toBe("projectB");
+  });
+
+  it("is idempotent within a project — same (project, name) resolves to one node", () => {
+    const first = upsertNode(tmp.db, "projectA", "branch", "main");
+    const second = upsertNode(tmp.db, "projectA", "branch", "main");
+
+    expect(second).toBe(first);
+
+    const count = (
+      tmp.db
+        .prepare("SELECT COUNT(*) AS n FROM nodes WHERE project_id = ? AND name = ?")
+        .get("projectA", "main") as { n: number }
+    ).n;
+    expect(count).toBe(1);
+  });
+
+  it("enforces uniqueness per (project_id, name), not globally on name", () => {
+    upsertNode(tmp.db, "projectA", "branch", "main");
+    upsertNode(tmp.db, "projectB", "branch", "main");
+
+    const names = (
+      tmp.db.prepare("SELECT name FROM nodes WHERE name = 'main'").all() as Array<{ name: string }>
+    ).length;
+    // Both projects keep their own `main` node — the global UNIQUE would have
+    // collapsed these to a single row.
+    expect(names).toBe(2);
+  });
+});

packages/agentctx/test/storage/migrations.test.ts

@@ -0,0 +1,75 @@
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import Database from "better-sqlite3";
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { MIGRATIONS, SCHEMA_VERSION, applyMigrations } from "../../src/storage/schema.js";
+
+/**
+ * Issue #78: the V2 migration rebuilds `nodes` to swap the global `name UNIQUE`
+ * for `UNIQUE(project_id, name)`. A rebuild drops the table that
+ * `record_entities`/`edges` reference, so these tests pin the upgrade path:
+ * existing node ids (and their inbound links) must survive, and foreign keys
+ * must be re-enabled and intact afterward.
+ */
+describe("schema migration V1 → V2 (issue #78)", () => {
+  let dir: string;
+  let db: InstanceType<typeof Database>;
+
+  beforeEach(() => {
+    dir = mkdtempSync(join(tmpdir(), "agentctx-mig-"));
+    db = new Database(join(dir, "agentctx.db"));
+    db.pragma("foreign_keys = ON");
+    // Bring the database up to V1 only, then seed a linked node as a v0.1
+    // install would have it.
+    db.transaction(() => {
+      db.exec(MIGRATIONS[0] as string);
+      db.pragma("user_version = 1");
+    })();
+    db.prepare(
+      `INSERT INTO records (id, project_id, type, title, body, valid_from, recorded_at, source)
+       VALUES ('rec1', 'projectA', 'decision', 't', 'b', 'x', 'x', 'cli')`,
+    ).run();
+    db.prepare(
+      "INSERT INTO nodes (id, project_id, kind, name) VALUES ('node1', 'projectA', 'branch', 'main')",
+    ).run();
+    db.prepare("INSERT INTO record_entities (record_id, entity_id) VALUES ('rec1', 'node1')").run();
+  });
+
+  afterEach(() => {
+    if (db.open) db.close();
+    rmSync(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
+  });
+
+  it("advances to the current schema version", () => {
+    applyMigrations(db);
+    expect(db.pragma("user_version", { simple: true })).toBe(SCHEMA_VERSION);
+  });
+
+  it("preserves node ids and their record links across the nodes rebuild", () => {
+    applyMigrations(db);
+
+    const node = db.prepare("SELECT id, project_id, name FROM nodes WHERE id = 'node1'").get();
+    expect(node).toEqual({ id: "node1", project_id: "projectA", name: "main" });
+
+    const link = db.prepare("SELECT entity_id FROM record_entities WHERE record_id = 'rec1'").get();
+    expect(link).toEqual({ entity_id: "node1" });
+  });
+
+  it("re-enables foreign keys with no integrity violations", () => {
+    applyMigrations(db);
+    expect(db.pragma("foreign_keys", { simple: true })).toBe(1);
+    expect(db.pragma("foreign_key_check")).toEqual([]);
+  });
+
+  it("allows the same node name in a different project after the upgrade", () => {
+    applyMigrations(db);
+    expect(() =>
+      db
+        .prepare(
+          "INSERT INTO nodes (id, project_id, kind, name) VALUES ('node2', 'projectB', 'branch', 'main')",
+        )
+        .run(),
+    ).not.toThrow();
+  });
+});