[bawoodruff]

Problem

kg index floods duplicated primary key / COPY FROM JSON unavailable warnings. These are not cosmetic — they flag silent cross-project data loss.

The Entity/Observation primary key is global id alone (schema.go), but every clear/delete and nearly every read is scoped by project_id. When two projects share a DB file, a prior project's rows survive the project-scoped clear and collide with the current run's identically-id'd entities on CREATE, which are then silently dropped (old row wins). Relations bled too: CreateRelation's endpoint MATCH was project-blind, so edges could bind to the wrong project's node.

Fix — ADR Option A1

Full design: docs/kg-entity-key-fix-design.md (included in this PR).

• Namespaced key. Add pk STRING PRIMARY KEY = project_id <0x1F> id to Entity and Observation via one central helper (src/kglib/pk.go). id stays a plain, human-facing, journal-stable column.
  • Separator is 0x1F (Unit Separator), not the ADR's \x00: cgo truncates C strings at NUL, collapsing every pk back to its project_id (verified failure) — so NUL would have silently re-introduced the collisions.
• Project-qualified matches (the correctness core): CreateRelation matches endpoints on pk (closes the bleed); CreateObservation binds its parent by pk; GetEntity matches on pk with an id+project_id fallback for un-migrated DBs.
• Observation read paths (GetTopObservations, batchGetObservations) now scope through the parent entity's project.
• Embeddings write/invalidation scoped by project (SetEmbedding takes projectID, matches on pk; removed the project-blind entityProjectID).
• Idempotent writes: entity CREATE → MERGE on pk (ON CREATE populates the full row; ON MATCH refreshes only name/type/updated_at).
• Migration: bump FormatStamp.SchemaVersion → 2 so CheckFormat trips the existing archiveIfMismatch rebuild + journal replay (Kuzu v0.11.3 has no ADD PRIMARY KEY). The journal is the only copy of hand-writes — the old DB is archived, never deleted.
• Federated cross-graph joins continue to key on local id/name, never pk.

Tests

• Cross-project shared-id retention (both projects keep their own entities; shared ids resolve per-project).
• Zero-warning re-index (scans output for duplicated primary key / COPY FROM JSON unavailable).
• Observation non-bleed across projects (fail-before/pass-after verified).
• Schema-version rebuild + journal replay restores hand-writes.
• go build, go vet, gofmt -l, and go test ./... clean in both src/kg and src/kglib.

Review history

Implemented and reviewed via two engineer→reviewer rounds before this PR. The first review caught a real cross-project observation read bleed the initial tests missed (now fixed + regression-tested); a focused re-review confirmed all findings closed with no new defects.

Migration note for reviewers

Existing stamped DBs rebuild from source exactly once on first index after this lands (schema bump). Hand-written entities/relations replay from the journal. A truly unstamped legacy DB still reads fine; a write against one returns an actionable "run kg index" message rather than self-healing.

Generated with Claude Code

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: docs/kg-entity-key-fix-design.md, src/kglib/pk.go, src/kglib/schema.go, src/kglib/entity.go, src/kglib/observation.go, src/kglib/relation.go, src/kglib/embeddings.go, src/kglib/search.go, src/kglib/store.go, src/kglib/format.go, src/kg/migrate.go, src/kg/internal/knowledge/indexer.go, src/kg/internal/knowledge/log_indexer.go, src/kg/internal/knowledge/indexer_pdf.go, src/kg/internal/knowledge/store_wrappers.go, src/kg/internal/knowledge/indexer_crossproject_test.go, src/kg/personal.go, src/kg/helpers.go, plus test files touched by the diff.
• Overall verdict: REQUEST CHANGES level content, but per the mapping rule (Major, no Critical) the required verdict is comment.

The core fix is well-executed: the pk = project_id + 0x1F + id namespacing is applied consistently through every entity/observation write path (CreateEntity, createEntityWithID, CreateObservation, SetEmbedding, batchCreateEntities, bulkLoadExecEntities, bulkLoadExecObservations, bulkLoadRelations), the write-open gate correctly refuses pre-pk databases, GetEntity degrades gracefully for legacy read-only opens, and the schema-version stamp correctly forces archive+rebuild. I traced every call site of the changed signatures (SetEmbedding, batchGetObservations) and found no stale callers. One real gap remains in the same file the PR fixes.

Major Issues (should fix)

• src/kglib/relation.go:69-73 (GetRelations), :104-149 (DeleteRelation), :151-168 (TraverseRelations) — these three sibling functions still match the to/edge endpoint by bare id with no project_id filter (to.id = $to_id with only from.project_id checked, or no filter at all on to), the exact "project-blind endpoint match" pattern the ADR calls out and that CreateRelation (same file, lines 24-42) was fixed to avoid. The new comment on CreateRelation ("Endpoints are matched on pk, never on bare id") is therefore only true for that one function in the file. Today this is masked by an invariant the rest of the PR establishes (every edge-creation path — CreateRelation, bulkLoadRelations, CreateObservation, bulkLoadExecObservations — now binds both endpoints to the same projectID, so no current-schema database can contain a cross-project edge), but it is untested (the new indexer_crossproject_test.go only exercises entity/observation bleed, not DeleteRelation/GetRelations/TraverseRelations) and silently reintroduces the original bug class the moment any future write path creates a cross-project edge. Fix: qualify to (and, for GetRelations/TraverseRelations, the returned neighbour) by project_id or pk, matching the standard already applied to CreateRelation, GetEntity, and the observation reads.

Minor Issues (optional)

• src/kg/internal/knowledge/indexer.go:676-679 and src/kg/internal/knowledge/log_indexer.go:339-341 — the MERGE ... ON MATCH SET fallback refreshes name/type/updated_at but not visibility (and the log-indexer variant doesn't carry visibility at all). In the common path this is unreachable because clearCodeDerivedData empties this project's code-derived rows before the batch runs, so ON MATCH rarely fires today — but if that invariant ever changes, a symbol whose visibility flips on re-index would keep the stale value under the COPY-unavailable fallback. Consider adding e.visibility = $visibility to ON MATCH SET for completeness.
• src/kglib/pk.go — the "Unit Separator survives the cgo round trip, NUL does not" claim is the linchpin of the whole fix and is stated as "verified" but isn't exercised by an explicit round-trip unit test (e.g., write a pk containing 0x1f, reopen, assert the raw stored value and a MATCH by that exact string both work). Given how much of the fix depends on this exact byte surviving go-kuzu's C-string marshalling, a small dedicated test would pin the invariant against a future go-kuzu bump silently breaking it again.

Security Findings

• None.

Positive Observations

• The pk namespacing is centralized in one helper (kglib.PK) and re-exported rather than duplicated, and every write call site I traced (indexer, log indexer, PDF indexer, embeddings, entity/observation/relation CRUD) derives it from that single function.
• The write-open hard-refusal for pre-pk databases, paired with the schema-version stamp bump and archiveIfMismatch, gives a clean, well-tested migration story (TestSchemaVersionBumpRebuildsAndRestoresHandWrites) that correctly distinguishes a Kuzu-version mismatch from a schema-version mismatch in user-facing error text.
• GetEntity's fallback to (id, project_id) for legacy read-only opens is a thoughtful touch that keeps old graphs readable without forcing a rebuild just to view them.
• New regression tests (indexer_crossproject_test.go) directly reproduce the reported bug (shared code-derived IDs across two projects in one DB) and assert both the entity-count and observation-bleed symptoms described in the design doc.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: docs/kg-entity-key-fix-design.md, src/kglib/pk.go, src/kglib/schema.go, src/kglib/entity.go, src/kglib/observation.go, src/kglib/relation.go, src/kglib/embeddings.go, src/kglib/search.go, src/kglib/store.go, src/kglib/format.go, src/kg/migrate.go, src/kg/index.go, src/kg/internal/knowledge/indexer.go, log_indexer.go, log_app_indexer.go, indexer_pdf.go, store_wrappers.go, indexer_crossproject_test.go, plus the test files touched by the diff.
• The Major issue from the prior review round (GetRelations/DeleteRelation/TraverseRelations being project-blind on the to endpoint) is now fixed — both endpoints are qualified by project_id, and indexer_crossproject_test.go's new TestRelationReadsAndDeletesAreProjectScopedOnBothEnds exercises exactly that regression with a raw cross-project edge. I traced every other write/read path (CreateEntity, createEntityWithID, CreateObservation, CreateRelation, SetEmbedding, batchGetObservations, GetTopObservations, batchCreateEntities, bulkLoadExecEntities/Observations, bulkLoadRelations, batchCreateRelations's fallback) and found them all consistently namespaced via PK()/project_id. Federated cross-graph linking (graph_federated.go) correctly keys on local id/name, not pk, as it must. Two gaps remain.

Major Issues (should fix)

• src/kglib/embeddings.go:186-192 (SetObservationEmbedding) — this sibling of SetEmbedding was not updated: it still does MATCH (o:Observation {id: $id}) SET o.embedding = $embedding with no project_id/pk qualification at all, the exact project-blind-match pattern this PR fixes everywhere else (see the comment on SetEmbedding, lines 162-165, describing precisely this hazard). Called from BatchEmbed (line 77) after the caller already has projectID in scope but drops it before the write. Practical collision risk is low (observation ids are UUIDs), but it's an inconsistent, untested gap in the same invariant the rest of the PR establishes — fix by threading projectID through and matching via the parent Entity's pk/project_id the way GetTopObservations/batchGetObservations now do.
• src/kglib/store.go:113-141 interacting with src/kglib/format.go:266-270 and src/kg/migrate.go:148-151 — a database that is unstamped (no readable .format.json sidecar — e.g. a pre-stamping-era DB, or any DB whose sidecar was lost/corrupted independently of the .db directory) and also predates the pk column now has no self-healing path. CheckFormat returns FormatUnstamped, not FormatMismatch, for any nil/unreadable stamp regardless of pk presence (format.go:266-270), so archiveIfMismatch (migrate.go) treats it as fine and does nothing. kg index (index.go:124-134) then calls archiveIfMismatch (no-op) followed directly by knowledge.OpenStore read-write, which hits the new !store.hasPK gate (store.go:127-133) and hard-errors with "...must be rebuilt: run 'kg index' (or 'kg migrate'...)" — but kg index is the command that just failed, and kg migrate's own scope loop (migrate.go:148-151: if status != kglib.FormatMismatch { continue }) explicitly skips unstamped databases too, so neither documented remedy actually repairs it. This contradicts the stated design promise in migrate.go's own doc comment ("never an error and never a manual recovery") and isn't covered by any test (no test opens an unstamped+pre-pk DB read-write). Fix: have CheckFormat (or a parallel check archiveIfMismatch performs) also treat "unstamped AND missing pk column" as a mismatch requiring archive+rebuild, so the existing rebuild machinery actually reaches this case.

Minor Issues (optional)

• src/kglib/pk.go — the claim that the Unit Separator (0x1F) survives the cgo string round-trip where NUL does not is still asserted only in a comment, not pinned by a dedicated round-trip test, even though the whole fix depends on this exact byte surviving go-kuzu's marshalling. (Carried over from the prior review round; still true but unchanged in risk.)
• src/kg/internal/knowledge/indexer.go:677-679 and log_indexer.go:339-344 — the MERGE ... ON MATCH SET fallback paths still don't refresh visibility on re-index, masked today by clearCodeDerivedData emptying the project's rows first. (Carried over, unchanged.)

Security Findings

• None identified.

Positive Observations

• The previously-flagged cross-project relation-endpoint bug is fixed and now has an explicit regression test with a hand-crafted cross-project edge, which is the right way to pin this invariant.
• pk.go's switch from NUL to Unit-Separator as the join byte is a real, self-caught fix (NUL truncates across the cgo boundary) — good catch by the author before this even reached review.
• GetEntity's legacy fallback for pre-pk read-only opens, and the new hasPK/schema-version-aware error messages in store.go/migrate.go, are thoughtful compatibility handling for the common cases.
• TestSchemaVersionBumpRebuildsAndRestoresHandWrites and indexer_crossproject_test.go are well-targeted, scenario-driven tests rather than incidental coverage.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed (all 25 changed): docs/kg-entity-key-fix-design.md, src/kglib/pk.go, src/kglib/schema.go, src/kglib/entity.go, src/kglib/observation.go, src/kglib/relation.go, src/kglib/embeddings.go, src/kglib/search.go, src/kglib/store.go, src/kglib/format.go, src/kg/migrate.go, src/kg/internal/knowledge/{indexer.go,log_indexer.go,log_app_indexer.go,indexer_pdf.go,store_wrappers.go}, plus every touched test file (indexer_crossproject_test.go, migrate_test.go, embeddings_test.go, federated_test.go, journal_test.go, null_columns_test.go, search_test.go, visibility_test.go, graph_federated_test.go).
• Both Major gaps raised in prior review rounds on this PR are now closed, with dedicated regression tests:
  • relation.go's GetRelations/DeleteRelation/TraverseRelations now qualify the to/neighbour endpoint by project_id as well as from (lines 69-73, 104-149, 151-168), exercised end-to-end by the new TestRelationReadsAndDeletesAreProjectScopedOnBothEnds in indexer_crossproject_test.go, which builds a real cross-project edge via raw Cypher and asserts it survives a project-A delete and is invisible to project-A reads.
  • embeddings.go's SetObservationEmbedding now takes projectID and matches on pk (same pattern as SetEmbedding), called correctly from BatchEmbed with the in-scope projectID.
  • The unstamped-but-pre-pk "dead end" database (can't be write-opened, previously unrepaired by either kg index or kg migrate) is now detected via the new Store.HasPK()/databaseMissingPK and routed into the existing archive+rebuild machinery in both archiveIfMismatch (used by kg index) and migrate's own scan loop, covered by the new TestUnstampedPrePKDatabaseRebuildsAndRestoresHandWrites, which constructs a real pre-pk DB with a hand-written journal entry and asserts it survives the rebuild.
• I traced every write/read path touching pk/id/project_id (CreateEntity, createEntityWithID, batchCreateEntities, bulkLoadExecEntities, bulkLoadExecObservations, bulkLoadRelations, CreateObservation, CreateRelation, GetEntity, GetTopObservations, batchGetObservations, KeywordSearchFiltered, VectorSearch) and found them all consistently namespaced, with the GetEntity/openStoreWithConfig legacy fallback correctly gated on s.hasPK. No stale callers of the changed SetEmbedding/SetObservationEmbedding/batchGetObservations signatures remain. The kg module's go.mod uses a local replace for kglib, so the cross-module blast radius this repo structure usually risks doesn't apply here — kg always builds against this PR's kglib changes.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

• src/kglib/entity.go:122-128 (detectColumns) — the refactor from if len(row) > 1 && stringOrEmpty(row[1]) == "visibility" to switch name := stringOrEmpty(row[1]); name drops the len(row) > 1 bounds guard entirely; row[1] is now indexed unconditionally. CALL table_info('Entity') reliably returns multi-column rows in practice, so this is unlikely to trigger, but it's a defensive check that existed and was silently removed rather than deliberately judged unnecessary. Fix: restore a len(row) > 1 (or equivalent) guard before indexing.
• src/kg/migrate.go:62 + :196 — databaseMissingPK(dbPath) is called twice per unstamped database on a kg migrate run (once in the main loop's status check, once again inside archiveIfMismatch when indexScopeDB is reached), each doing a full read-only OpenStore/Close. Harmless but redundant; consider threading the already-computed missing bool through instead of re-deriving it.

Security Findings

None.

Positive Observations

• The two Major findings from earlier review rounds are fixed correctly and each is now pinned by a purpose-built regression test rather than just a comment — TestRelationReadsAndDeletesAreProjectScopedOnBothEnds and TestUnstampedPrePKDatabaseRebuildsAndRestoresHandWrites both construct the exact failure condition they guard against (a raw cross-project edge; a real pre-pk+unstamped DB with a journal) rather than asserting on an artifact.
• CurrentSchemaVersion/FormatStamp.SchemaVersion is a clean, minimal mechanism for forcing archive+rebuild on a PK-shape change that Kuzu can't ALTER in place, and it composes correctly with the existing Kuzu-version mismatch path (openDiagnostic, archiveIfMismatch now both report which of the two actually differs).
• PK() is the single, centrally-exported source of the namespacing format (kglib.PK, re-exported as knowledge.PK), so every bulk-load/COPY and every parameterized write across both modules derives the same byte format — the one place this could have silently drifted (indexer bulk paths vs. kglib writes) is instead unified.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: docs/kg-entity-key-fix-design.md, src/kglib/pk.go, pk_test.go, schema.go, entity.go, observation.go, relation.go, embeddings.go, embeddings_test.go, search.go, search_test.go, store.go, format.go, federated_test.go, journal_test.go, null_columns_test.go, visibility_test.go, src/kg/index.go, migrate.go, migrate_test.go, src/kg/internal/knowledge/{indexer.go, log_indexer.go, log_app_indexer.go, indexer_pdf.go, store_wrappers.go, indexer_crossproject_test.go, graph_federated_test.go}.
• This PR fixes the global-id-as-PK bug (cross-project collisions under a shared DB file) by introducing a project-namespaced pk = project_id + 0x1F + id column, rewriting every write/match path to key on it, and adding an archive+rebuild path for legacy unstamped/pre-pk databases.
• I independently traced every write path (CreateEntity, createEntityWithID, CreateObservation, CreateRelation, SetEmbedding, SetObservationEmbedding, batchCreateEntities, bulkLoadExecEntities/Observations, bulkLoadRelations) and every read path that needed project-scoping (GetEntity, GetRelations, DeleteRelation, TraverseRelations, GetTopObservations, batchGetObservations) and all are consistently namespaced, with no stale callers of the changed SetEmbedding/SetObservationEmbedding/batchGetObservations signatures.
• The two Major gaps raised in earlier review rounds on this same PR (project-blind to endpoint on GetRelations/DeleteRelation/TraverseRelations, and the unqualified SetObservationEmbedding) are fixed and each is pinned by a dedicated regression test (TestRelationReadsAndDeletesAreProjectScopedOnBothEnds, TestSetObservationEmbedding_ProjectScoped). The unstamped-pre-pk dead-end database is now routed into archiveIfMismatch/kg migrate's rebuild machinery, covered by TestUnstampedPrePKDatabaseRebuildsAndRestoresHandWrites.
• The two Minor items from earlier rounds are also now addressed: entity.go:122-131 (detectColumns) restores the len(row) <= 1 bounds guard before indexing, and the cgo round-trip claim in pk.go (Unit Separator survives, NUL doesn't) is now pinned by pk_test.go's TestPKSeparatorSurvivesCGoRoundTrip, which proves it against a real Store write + fresh read-only reopen rather than asserting it only in a comment.
• src/kg/go.mod uses a local replace github.com/cortexa-llc/mcp/kglib => ../kglib, so the kg module always builds against this PR's kglib changes — the usual cross-module blast-radius risk in this monorepo doesn't apply here.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

• src/kglib/pk.go:30-33 — the collision-freedom of pk rests entirely on the unenforced assumption that neither project_id nor id ever contains 0x1F; nothing validates or sanitizes project_id (derived from a repo directory base name) at the point it's accepted. Pathologically low risk on real filesystems, but consider a cheap assertion/sanitization in PK() (or at the project_id derivation site) rather than relying purely on convention — not blocking given how unlikely a directory name containing a Unit Separator byte is in practice.
• src/kg/internal/knowledge/log_indexer.go:329-341 (bulkLoadExecEntities MERGE fallback) still doesn't carry visibility in either ON CREATE/ON MATCH branch, unlike the code-indexer's equivalent in indexer.go. Likely intentional (exec-log entities don't use visibility today), but worth a one-line comment confirming that so a future reader doesn't mistake it for a second instance of the gap already fixed elsewhere in this PR.

Security Findings

None.

Positive Observations

• Every Major/Minor finding raised across this PR's prior review rounds is now fixed with a purpose-built regression test rather than just a comment, including a real cgo round-trip test for the separator-byte invariant the whole fix depends on.
• The cross-project test fixtures (indexer_crossproject_test.go) construct real collisions via raw Cypher rather than only unit-testing PK() in isolation, which is the right level to catch this class of bug.
• The migration path correctly distinguishes "unstamped but current" (leave alone) from "unstamped and pre-pk" (archive + rebuild), and threads the already-computed missing bool from kg migrate's loop into archiveIfMismatch to avoid a redundant read-only open — resolving the earlier redundancy note.

[cortexa-llc-reviewer]

Review Summary

• This PR has been through several review rounds; prior rounds' Major/Critical findings (project-blind relation endpoints, unqualified SetObservationEmbedding, the unstamped-pre-pk dead end) are all fixed and test-pinned, confirmed by re-reading relation.go, embeddings.go, and migrate.go/migrate_test.go.
• This pass focuses on what changed since the last approved round: src/kg/helpers.go/helpers_test.go (new 0x1F-sanitization guard on projectIDFromCwd, closing a previously-noted Minor), and the missingHint-threading added to src/kg/migrate.go's scan loop (closing another previously-noted Minor about a redundant read-only open). The threading change introduces a new correctness gap; the sanitization fix is incomplete at one other call site.

Major Issues (should fix)

• src/kg/migrate.go:199-216 — in the migrate command's scan loop, when a database's CheckFormat status is FormatUnstamped, databaseMissingPK(dbPath) is called to decide whether to rebuild it. If that call errors (e.g. the .db directory is corrupted or otherwise fails to open read-only — CheckFormat never opens the database, only its sidecar, so this is reachable independently of the stamp check), the condition mErr == nil && missing is false, so needsRebuild silently stays false and the loop just continues — but checked++ already ran at line 199, before this block. The database is counted as successfully checked and the run can end with "✅ %d database(s) checked; all readable by this build", which is false for this database. This directly violates the invariant the surrounding code documents a few lines above (unchecked "gates the summary, because the one thing this command must never do is print an all-clear for a database it never looked at") — every other failure mode in this function correctly calls failf, this one doesn't. Fix: on mErr != nil, call failf("scope %s: cannot determine whether %s predates the primary key: %v", label, dbPath, mErr) and continue, the same as the CheckFormat error path just above it.

Minor Issues (optional)

• src/kg/helpers.go:79 vs src/kg/graph.go:305 — the new guard comment on projectIDFromCwd claims it defends the 0x1F-separator invariant "at its single derivation site," but runFederatedGraph derives project_id for the federated-graph filter via filepath.Base(root) directly, bypassing projectIDFromCwd and its sanitization. graph_federated.go's layerIdentities/loadLayerGraph match this value literally against the stored project_id column (not via PK()), so in the (admittedly pathological) case where a project root's basename contains the 0x1F byte, indexed entities would carry the sanitized project_id while --federated would filter on the raw, un-sanitized one and silently return zero matches. Fix: use projectIDFromCwd(root) at graph.go:305 instead of filepath.Base(root) for consistency, or note explicitly in the helpers.go comment that it is not in fact the only derivation site.

Security Findings

None.

Positive Observations

• The two Minor items flagged in the previous (approved) review round are both addressed: the 0x1F/project_id sanitization guard is real and tested (TestProjectIDFromCwdReplacesSeparator), and the redundant double databaseMissingPK read-only open is now avoided via missingHint threading — the fix for the latter is what introduced the Major finding above, so the next round should re-verify the threading change rather than re-litigate the original redundancy.
• The core pk namespacing fix itself (schema, writes, relation endpoints, embeddings, migration/rebuild path) remains correctly and consistently applied everywhere it was re-examined in this pass.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: full diff (gh pr diff 17) across docs/kg-entity-key-fix-design.md, src/kglib/{pk.go,pk_test.go,schema.go,entity.go,observation.go,relation.go,embeddings.go,embeddings_test.go,search.go,search_test.go,store.go,format.go,federated_test.go,journal_test.go,null_columns_test.go,visibility_test.go}, src/kg/{helpers.go,helpers_test.go,graph.go,handle_server.go,index.go,migrate.go,migrate_test.go,migrate_scopes_test.go}, src/kg/internal/knowledge/{indexer.go,indexer_pdf.go,log_indexer.go,log_app_indexer.go,store_wrappers.go,indexer_crossproject_test.go,graph_federated_test.go}.
• This PR has been through several prior review rounds (visible in thread history). All previously-raised Critical/Major findings — project-blind GetRelations/DeleteRelation/TraverseRelations endpoints, unqualified SetObservationEmbedding, the unstamped-pre-pk dead-end database, the checked++-before-failf ordering bug in migrate.go's scan loop, and the graph.go:305/helpers.go separator-sanitization inconsistency — are confirmed fixed in this diff, each with a dedicated regression test (TestRelationReadsAndDeletesAreProjectScopedOnBothEnds, TestSetObservationEmbedding_ProjectScoped, TestUnstampedPrePKDatabaseRebuildsAndRestoresHandWrites, TestMigrateDoesNotCountScopeDatabasesWhosePKCannotBeDetermined, TestProjectIDFromCwdReplacesSeparator). I independently re-traced every write/read path touching pk/id/project_id and found them all consistently namespaced, with no stale callers of the changed signatures.
• One new, purely cosmetic issue surfaced this pass (see Minor below); no Critical or Major issues found.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

• src/kg/internal/knowledge/store_wrappers.go:33 — the comment on the re-exported PK says "the NUL-join format must match byte-for-byte," but src/kglib/pk.go:20-26 explicitly rejected NUL (it gets truncated by go-kuzu's cgo string marshalling) and uses the Unit Separator (0x1F) instead. The code is correct (it just calls kglib.PK), but the comment itself misstates the format it's describing. Fix: change "NUL-join" to "0x1F-separator" (or just "pk-derivation") to match pk.go's actual documented format.

Security Findings

None.

Positive Observations

• Every Major/Critical gap raised across this PR's five prior review rounds is now fixed and pinned by a purpose-built regression test rather than just a comment, including a real cgo round-trip test for the separator-byte invariant (pk_test.go) and a test proving the migrate scan loop correctly fails closed when pk-introspection errors (rather than silently printing an all-clear).
• The kg module's go.mod uses a local replace for kglib, so the usual cross-module blast-radius risk in this monorepo (three separate go.mods, no go.work) doesn't apply here — kg always builds against this PR's kglib changes.
• Design doc (docs/kg-entity-key-fix-design.md) is unusually thorough and the implementation matches its accepted design (Option A1) faithfully, including the documented rationale for rejecting NUL in favor of 0x1F.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed (full diff, gh pr diff 17): docs/kg-entity-key-fix-design.md, src/kglib/{pk.go,pk_test.go,schema.go,entity.go,observation.go,relation.go,embeddings.go,embeddings_test.go,search.go,search_test.go,store.go,format.go,federated_test.go,journal_test.go,null_columns_test.go,visibility_test.go}, src/kg/{helpers.go,helpers_test.go,graph.go,handle_server.go,index.go,migrate.go,migrate_test.go,migrate_scopes_test.go}, src/kg/internal/knowledge/{indexer.go,indexer_pdf.go,log_indexer.go,log_app_indexer.go,store_wrappers.go,indexer_crossproject_test.go,graph_federated_test.go}.
• This PR fixes the global-id-as-primary-key bug (cross-project collisions in a shared Kuzu DB file) via a project-namespaced pk = project_id + 0x1F + id column, rewritten write/match paths, and an archive+rebuild path for legacy unstamped/pre-pk databases.
• Independently re-traced every write path (CreateEntity, createEntityWithID, CreateObservation, CreateRelation, SetEmbedding, SetObservationEmbedding, batchCreateEntities, bulkLoadExecEntities/Observations, bulkLoadRelations) and read path (GetEntity, GetRelations, DeleteRelation, TraverseRelations, GetTopObservations, batchGetObservations) — all consistently namespaced on pk/project_id, with no stale callers of the changed signatures.
• All prior Major/Critical findings from earlier rounds of this PR (project-blind to endpoint on GetRelations/DeleteRelation/TraverseRelations; unqualified SetObservationEmbedding; the unstamped-pre-pk dead-end database; the checked++-before-resolution semantics in migrate.go's scan loop; the graph.go:305 vs helpers.go separator-sanitization bypass; the detectColumns bounds-guard removal; the store_wrappers.go "NUL-join" comment misstatement) remain fixed, each pinned by a dedicated regression test (TestRelationReadsAndDeletesAreProjectScopedOnBothEnds, TestSetObservationEmbedding_ProjectScoped, TestUnstampedPrePKDatabaseRebuildsAndRestoresHandWrites, TestMigrateDoesNotCountScopeDatabasesWhosePKCannotBeDetermined, TestProjectIDFromCwdReplacesSeparator, TestPKSeparatorSurvivesCGoRoundTrip).
• src/kg/go.mod uses a local replace github.com/cortexa-llc/mcp/kglib => ../kglib, so the usual cross-module blast-radius risk in this three-go.mod/no-go.work monorepo doesn't apply — kg always builds against this PR's kglib changes.

Critical Issues (must fix before merge)

None.

Major Issues (should fix)

None.

Minor Issues (optional)

None new. (Previously-noted minors — entity.go bounds guard, pk.go round-trip test, store_wrappers.go comment wording, graph.go:305 sanitization bypass — are all addressed in this revision.)

Security Findings

None.

Positive Observations

• Every Major/Critical gap raised across this PR's many prior review rounds is fixed and backed by a purpose-built regression test rather than just a comment fix, including a real cgo round-trip test for the 0x1F-separator invariant and a test proving the migrate scan loop fails closed (rather than printing a false all-clear) when pk-introspection errors.
• The design doc (docs/kg-entity-key-fix-design.md) is thorough and the implementation faithfully matches its accepted design, including the documented rationale for choosing 0x1F over NUL given go-kuzu's cgo string marshalling.
• Comments throughout the diff (CreateRelation, SetEmbedding, GetRelations, migrate.go) explicitly name the hazard each fix closes, making the invariants easy for future readers to verify rather than just trust.