From 3c2c4503a725c3c1e65ab45351602bf68548e3d3 Mon Sep 17 00:00:00 2001 From: serhiizghama Date: Mon, 22 Jun 2026 03:24:39 +0700 Subject: [PATCH 1/2] fix(storage): scope graph nodes per project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Graph `nodes` are project-scoped data, but uniqueness was enforced globally on `name` and `upsertNode` fetched by `name` with no project filter. When two projects produced a node with the same name — e.g. a `main`/`master` branch node, which is the common case — the second project's `INSERT OR IGNORE` was dropped and the unscoped `SELECT` returned the first project's node id, so its records were linked to another project's entity. A `reset` of the first project then deleted the shared node, leaving the second project with dangling links. Make node uniqueness per project: - Schema migration V2 rebuilds `nodes` with `UNIQUE(project_id, name)` instead of the global `name UNIQUE`. Node ids (referenced by `record_entities`/`edges`) are preserved, so existing links survive. - `upsertNode` scopes its lookup with `AND project_id = ?`. Both halves are required: the SELECT fix alone can't work while the global UNIQUE still blocks the second project's insert. The rebuild drops the table that `record_entities`/`edges` reference, so `applyMigrations` now disables foreign keys around the migration per the SQLite schema-change procedure and re-checks integrity before re-enabling them — `PRAGMA foreign_keys` is a no-op inside a transaction, and `defer_foreign_keys` is insufficient because dropping a parent leaves a deferred-constraint count a later rename can't clear. Closes #78 --- docs/SPEC.md | 3 +- packages/agentctx/src/hooks/entities.ts | 11 ++-- packages/agentctx/src/storage/schema.ts | 71 ++++++++++++++++++++++--- 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/docs/SPEC.md b/docs/SPEC.md index 21f4911..e6d8e06 100644 --- a/docs/SPEC.md +++ b/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, diff --git a/packages/agentctx/src/hooks/entities.ts b/packages/agentctx/src/hooks/entities.ts index e06a55a..46ced41 100644 --- a/packages/agentctx/src/hooks/entities.ts +++ b/packages/agentctx/src/hooks/entities.ts @@ -8,7 +8,7 @@ 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(), @@ -16,9 +16,12 @@ export function upsertNode(db: Database, projectId: string, kind: NodeKind, name 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`); } diff --git a/packages/agentctx/src/storage/schema.ts b/packages/agentctx/src/storage/schema.ts index 5247137..e11bf71 100644 --- a/packages/agentctx/src/storage/schema.ts +++ b/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"); } } From 149be402620458746e936435ebdaba78018b20c6 Mon Sep 17 00:00:00 2001 From: serhiizghama Date: Mon, 22 Jun 2026 03:24:45 +0700 Subject: [PATCH 2/2] test(storage): cover per-project node scoping and the V2 migration - entities.test.ts: two projects sharing a node name get distinct ids, a record links to its own project's node, single-project upsert stays idempotent, and uniqueness holds per (project_id, name). - migrations.test.ts: the V1->V2 rebuild preserves node ids and their record links, re-enables foreign keys with no violations, and admits the same node name in a different project afterward. --- .../agentctx/test/storage/entities.test.ts | 81 +++++++++++++++++++ .../agentctx/test/storage/migrations.test.ts | 75 +++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 packages/agentctx/test/storage/entities.test.ts create mode 100644 packages/agentctx/test/storage/migrations.test.ts diff --git a/packages/agentctx/test/storage/entities.test.ts b/packages/agentctx/test/storage/entities.test.ts new file mode 100644 index 0000000..bcfadfe --- /dev/null +++ b/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); + }); +}); diff --git a/packages/agentctx/test/storage/migrations.test.ts b/packages/agentctx/test/storage/migrations.test.ts new file mode 100644 index 0000000..fa34be8 --- /dev/null +++ b/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; + + 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(); + }); +});