Store SchemaRelationship declarations as relationship-table edges instead of schema-node JSON #2008

Overview

SchemaRelationship declarations (a schema's typed relationships to other types, e.g. project → tasks → task) are currently stored as a JSON array inside the declaring schema node's own properties.relationships — the same storage pattern used for SchemaField. This is architecturally leftover from the SurrealDB→SQLite migration: the original SurrealDB-era design (dd25b319, 2025-12-01) generated a real, dedicated edge table per declared relationship (SCHEMAFULL TYPE RELATION, named {source_type}_{relationship_name}_{target_type}) — a first-class SurrealDB graph primitive. The SQLite migration (f40c140e, #1222) flattened storage down to three generic tables (node, relationship, embedding) and never carried the per-relationship edge-table generation forward; SchemaRelationship ended up as descriptive JSON with no backing structure.

Since schema nodes are themselves ordinary rows in the node table (a project schema and a real project instance are both just node rows, distinguished by node_type/content, not by separate storage), a SchemaRelationship declaration is a well-typed edge between two existing node.ids — project (schema) → task (schema) — with no structural obstacle to storing it as a relationship table row using the exact same table instance-level edges already use. create_relationship's existing validation path already fetches a schema id via a plain get_node() call today (packages/core/src/services/node_service/relationship.rs:534-538) — the code already treats schema nodes as ordinary nodes at the point that matters. This isn't introducing conflation; it's finishing a design the SurrealDB era had and the SQLite migration dropped by accident, not by decision.

Reviewed by senior-architect-reviewer in two passes. Pass 1 (full independent code verification) — verdict: proceed as scoped, with amendments folded in below; explicitly rejected the alternative of leaving JSON storage alone and only patching the existing cache's invalidation gap (see "Rejected alternative"). Pass 2 (scoped follow-up covering proto/wire contracts, agent tool-schema text, the broader test suite, and seeded skill guidance) — findings folded in below; one genuine new gap found and confirmed by two independent reviews (see Acceptance Criteria).

Timing note: this is being scoped now because NodeSpace expects real users soon — landing this before instance data (real project↔task edges, etc.) accumulates avoids a migration on live user data. Still significant work; not a quick cleanup. Given #1999 is open and adds the first real relationship under the current model, there is a fairly near-term deadline here, not a vague "sometime pre-launch."

Problem Statement

Current storage (JSON-in-properties):

• SchemaNode::from_node() reads relationships out of node.properties.get("relationships"), silently defaulting to [] on any deserialization failure (.ok() + unwrap_or_default() — see Relationship viewer: view and manage typed relationships between nodes #1918's own review notes on this same fragility for fields). Correction from initial drafting, confirmed by reviewer: the live, actually-constructed SchemaNode is packages/core/src/models/schema_node.rs (re-exported as crate::models::SchemaNode, models/mod.rs:74) — not packages/nodespace-types/src/schema.rs's SchemaNode, which does not appear to be constructed or consumed by any live call site outside its own crate and looks like dead/parallel scaffolding. Confirm which is authoritative before implementation; treat the nodespace-types one as suspect.
• Four independent call sites parse this JSON today, not one or three — this was undercounted in earlier drafts, revised upward again by the follow-up review:
  1. NodeService::create_relationship — services/node_service/relationship.rs:534-538
  2. NodeService::get_inbound_relationships — services/node_service/relationship.rs:1085-1107 (the function actually called by rel_ops.rs:332 in production)
  3. get_node_relationships's outbound branch — ops/rel_ops.rs:298-302, whose own comment says it deliberately bypasses SchemaNode::from_node "to avoid a round-trip"
  4. packages/core/src/playbook/validation.rs:1184-1208's test-only create_schema helper — bypasses handle_create_schema entirely and hand-writes a relationships JSON value directly into node.properties via a raw NodeService::create_node() call. Found independently by two separate follow-up review passes, converging on the identical file/lines/root cause — see Acceptance Criteria.
• get_node_relationships (ops/rel_ops.rs:343-349) already carries a load-bearing comment describing and working around the exact ambiguity class this refactor's Open Question Set up Tauri + Svelte project structure #1 is about: two schemas both declaring assigned_to → person would surface each other's edges unless explicitly filtered by declaring source type. This is precedent, not a hypothetical — this codebase has already been bitten by under-scoped relationship_type-keyed queries once, one level down from what this issue proposes.
• InboundRelationshipCache (packages/core/src/services/relationship_cache.rs) was built to make "what relationships point at type X" fast, but is not called anywhere in production — get_inbound_relationships (the function real callers use) does a plain uncached get_all_schemas() scan and never touches the cache. InboundRelationshipCache::invalidate() is never called outside its own module; the cache's 60s TTL is the only thing that ever clears it, and it doesn't matter because nothing reads from it. Deleting it removes zero working behavior — there's no regression risk here, unlike earlier drafts of this issue implied.
  • (Correction: an earlier draft claimed this cache/relationship events have "zero references" in packages/desktop-app/src-tauri/src/watcher.rs and are "not wired into the domain-event broadcast at all." This was imprecise — watcher.rs does forward relationship:created/updated/deleted Tauri events to the frontend, and the frontend does consume them. The narrower, correct claim is above: the cache specifically is dead code, not the relationship-event broadcast generally.)
• packages/core/src/ops/context_ops.rs:161-237 (related_one_hop_schemas, parse_and_filter_non_core_schemas) — an unmentioned-until-review consumer: builds agent relationship-hop prompt context from SchemaNode.relationships off search results, via a synchronous filter_map calling SchemaNode::from_node(node).ok(). Its caller is already async, so restructuring for a DB-join-based read is tractable, but this call site needs to be in scope explicitly — it was missing from this issue's original reference list.
• packages/core/src/models/schema.rs:58-67's own module doc comment still describes the SurrealDB-era "Edge table storage... Edge table is single source of truth" model — stale documentation corroborating this issue's premise; should be corrected as part of this work.

What existed before the SQLite migration: dd25b319's Phase 2 (generate_relationship_ddl_statements()) generated a dedicated SCHEMAFULL TYPE RELATION table per declared relationship, with IN/OUT type constraints, tracking fields, and indexes — i.e., relationships were always intended to be real, queryable graph edges, not descriptive JSON. That capability was not carried forward when f40c140e moved off SurrealDB.

Rejected alternative

Leave JSON storage as-is and instead patch InboundRelationshipCache's invalidation gap. Rejected, per architect review: that cache isn't in the production code path today, so fixing its invalidation would address a problem nothing actually experiences. The real problems — four independently-drifting JSON parses, a missing reserved-name check (see Open Question 6), and a missing schema-deletion guard (see Acceptance Criteria) — are structural gaps this table-backed model fixes directly, not symptoms a cache fix would touch.

Proposed Solution

Store SchemaRelationship declarations as rows in the existing relationship table (packages/core/src/db/schema.sql:43-56), using the declaring and target schema nodes' ids as in_node/out_node — the same table, same REFERENCES node(id) constraints, and same indexes (idx_rel_out, idx_rel_in) already used for instance-level edges, just with schema-node ids at the endpoints instead of instance ids.

Concretely:

• relationship_type carries the relationship's declared name (e.g. "tasks") — see Open Question 1 for the required disambiguation mechanism, elevated to a blocking pre-implementation decision, not an open question to resolve during implementation
• Cardinality, reverse_name, reverse_cardinality, edge_fields, description — the per-declaration attributes currently inline in the SchemaRelationship JSON object — move into that row's existing relationship.properties JSON column (TEXT NOT NULL DEFAULT '{}'), which already exists for exactly this kind of per-edge metadata
• Consolidate the production declaration-read call sites onto one shared table-query path — create_relationship, get_inbound_relationships, and get_node_relationships's outbound branch should share one query helper, not three independently-rewritten JSON-to-table conversions
• create_schema/update_schema's relationship-handling code writes/deletes relationship table rows instead of mutating a JSON array on the schema node
• packages/proto/proto/node_service.proto requires zero changes. Confirmed by follow-up review: every relationship/schema RPC (GetSchemaDefinition/GetAllSchemas, CreateSchema/UpdateSchema, GetNodeRelationshipsResponse.relationships_json, GetRelatedNodesResponse.related_nodes_json) already carries data as opaque JSON strings by deliberate design — explicit code comments state this is intentional, specifically to let the aggregate shape "evolve without a proto change." The wire layer is already storage-agnostic.
• Agent tool-schema description text requires zero changes. Confirmed by follow-up review: create_relationship/get_related_nodes/create_schema/update_schema tool definitions (packages/agent/src/local_agent/tools.rs) contain zero references to JSON or storage internals — all description/parameter text is purely behavioral ("Use a relationship name defined on the relevant schema(s)...").
• Seeded skill guidance requires zero changes. Confirmed by follow-up review: the "Relationship Management" skill (packages/agent/src/skill_pipeline.rs:177-192, relationship_management_guidance()) is purely behavioral tool-call instruction (when to call create_relationship/get_related_nodes, find-before-link pattern) with zero mentions of JSON or storage location. User's own hypothesis on this — confirmed correct by direct evidence.

Resolved design decisions

Schema fetch must remain a single round-trip. Today, fetching a schema's definition (get_schema_definition/get_all_schemas) returns fields and relationships together in one SchemaNode object, in one call. That must not regress: SchemaNode::from_node() (or its replacement) still returns one unified object with both fields and relationships populated, from the caller's perspective in a single fetch — the backend internally does a table join/second query against relationship instead of a JSON parse to populate relationships, but no consumer (frontend SchemaNode type, RelationshipViewerModal, GenericSchemaForm, agent tool schema-fetch, etc.) should ever need to know or care whether relationships came from JSON or a table join, nor make a second explicit call to get them. Confirmed low-risk by review: the frontend blast radius is small — RelationshipViewerModal/GenericSchemaForm go through single Tauri commands (getNodeRelationships/getSchema) returning fully-assembled objects already; no frontend file needs to change for the storage migration itself as long as Rust-side handlers keep their current return shape.

Deleting a relationship declaration with live instance edges: block by default, no cascade-delete, no detach escape hatch. Once real users exist, silently destroying instance data (existing project↔task edges, etc.) as a side effect of a schema edit is not acceptable. Decision, superseding an earlier draft of this issue that proposed a "detach" escape hatch: block by default, full stop, for this issue's initial scope — no orphan/detach operation included here. Rationale: starting strict and loosening later (e.g. adding a confirm-and-cascade-delete flow on top of an existing block) is a backward-compatible, additive product change; starting permissive (or with an orphaning escape hatch that has its own discoverability problems) and tightening later risks breaking workflows users already built around it. If a real need for a supported cascade/detach path emerges post-launch, it should be a deliberate, separate follow-up decision — not default-shipped alongside the storage migration itself.

• Reject a relationship-declaration delete/rename/retarget if any instance-level relationship rows exist under that relationship_type for the declaring/target types. Return a clear error naming the count of affected edges, mirroring the existing required-field-removal error pattern (required: true blocking last-edge removal, per Relationship viewer: view and manage typed relationships between nodes #1918's PR Relationship editing command surface + required last-edge protection (#1918) #1993).
• This needs a corresponding validation query (count instance edges by relationship_type + endpoint type) before allowing the schema-side write — same shape as the disambiguation check needed for Open Question 1, likely implementable as one shared helper.
• New, review-surfaced gap this decision must also cover: delete_node (packages/core/src/db/sqlite_store/nodes.rs:432-450) has no relationship-aware guard at all today — it relies entirely on ON DELETE CASCADE on both in_node and out_node, with an existing code comment stating exactly that ("FK CASCADE handles relationship and embedding deletion"). Once schema-declaration edges are real relationship rows, deleting a schema node itself silently cascades away every declaration edge pointing at or from it — the identical silent-cascade failure mode the block-by-default decision above exists to prevent, reachable through a different door. This needs its own guard and is an explicit Acceptance Criterion below; it was missing from earlier drafts of this issue entirely.
• Note on scope: two of the three relationship-mutation handlers this decision governs (handle_add_schema_relationship, handle_remove_schema_relationship in schema/mod.rs) are confirmed not wired to any agent tool today (not in Tool::ALL, per their own doc comments at schema/mod.rs:385) — i.e. currently dead code from the agent's perspective. Implementation needs an explicit decision on whether to wire them up as part of this work or leave them dead; don't silently rewrite unreachable code without deciding this first.

No new caching layer in this issue's initial scope. The write path (create_relationship's declaration validation) already resolves to no-cache-needed per Open Question 4 below — a lateral move onto an indexed query, not a new cost. For the read path (viewer opens, schema fetches, agent relationship-hop context), an indexed relationship table query is cheap at current and expected near-term scale (a handful of schemas, modest relationship counts), and adding a cache now would be solving a problem nothing has yet demonstrated — the same reasoning that led to rejecting "just patch the old cache" as an alternative applies here in reverse: don't build a new one speculatively either. If real usage later shows read-path latency actually matters, add a thin read-through cache in front of the now-canonical table query at that point, invalidated correctly via watcher.rs's relationship events — unlike InboundRelationshipCache, which was never wired to those events at all. Treat "do we need a cache" as a question to answer with real data post-launch, not speculatively now.

Acceptance Criteria (draft — needs scoping pass before implementation)

• SchemaRelationship declarations are stored as relationship table rows between schema-node ids, not JSON in properties.relationships
• The correct, live SchemaNode implementation is confirmed (packages/core/src/models/schema_node.rs, pending verification — see Problem Statement) before any read-path rewrite begins; the JSON read path no longer reads relationships from properties — reads from the relationship table instead, in the SAME round-trip as fields (see Resolved design decisions — no caller-facing second fetch)
• create_relationship, get_inbound_relationships, and get_node_relationships's outbound branch are consolidated onto one shared table-query path — not three independently-rewritten JSON-to-table conversions
• InboundRelationshipCache is deleted entirely (confirmed dead in production — not "repurposed as a thin cache," per review finding that nothing reads from it today); no replacement caching layer is introduced in this issue's scope (see "No new caching layer" above)
• packages/core/tests/relationship_cache_test.rs is deleted alongside the cache, not migrated — it tests a component being removed, not coverage to preserve
• packages/core/src/playbook/validation.rs:1184-1208's test-only create_schema helper is rewritten to route through the real schema-creation path (handle_create_schema or the equivalent table write) instead of hand-writing relationships JSON directly into node.properties. Found independently by two separate follow-up review passes converging on the same file/lines/root cause. Without this fix, four tests will silently regress post-migration (empty relationship lists via the same .ok()/unwrap_or_default() swallowing pattern documented above, not a clear error) — test_unknown_relationship_type_fails, test_valid_relationship_type_passes, test_valid_multi_hop_path_passes, test_broken_path_relationship_without_target_type. (test_schema_impact_detects_path_traversal uses the same raw-JSON fixture but is confirmed unaffected — its target function never reads schema.relationships, it string-matches CEL rule text instead.)
• Existing schema relationship declarations (if any exist in shipped databases by the time this lands — currently none do, per live inspection) are migrated from JSON to relationship table rows
• create_schema/update_schema write/delete relationship table rows for relationship changes, atomically with the rest of the schema update
• A creation-time check rejects a schema-declared relationship whose name collides with a reserved/primitive relationship type (has_child, mentions, member_of, has_role, per BUILTIN_RELATIONSHIP_NAMES at packages/core/src/ops/rel_ops.rs:155) — this does not exist today under any storage model (confirmed by review: BUILTIN_RELATIONSHIP_NAMES is currently consulted only by the relationship viewer's display filter, rel_ops.rs:305,339, never by handle_create_schema/handle_update_schema/create_relationship's declaration path) and becomes a correctness risk, not just a display glitch, once declarations and primitives share one table
• Deleting/renaming/retargeting a relationship declaration is blocked by default when live instance edges exist under it (no detach/orphan escape hatch in this issue's scope — see Resolved design decisions)
• Deleting a schema node itself is blocked (or otherwise guarded) when relationship-declaration edges point at or from it, closing the delete_node/FK-CASCADE gap described above
• A decision is made and recorded on whether handle_add_schema_relationship/handle_remove_schema_relationship get wired to an agent tool as part of this work, or remain intentionally unreachable
• Instance-level create_relationship/rel_ops calls are guarded against accidentally writing a schema-declaration-shaped edge (either endpoint being a schema node) if that's not already structurally prevented — same disambiguation mechanism as Open Question 1
• packages/core/src/ops/context_ops.rs's related_one_hop_schemas/parse_and_filter_non_core_schemas (agent relationship-hop prompt context) is updated for the new read path and covered by a regression test — this consumer was missing from earlier scoping
• Regression test confirms all consolidated read paths (create, inbound lookup, outbound lookup) agree on the same declaration set post-migration — the highest-value new test, since today's independent parses can silently drift against each other with nothing catching it
• packages/core/src/models/schema.rs's stale SurrealDB-era "Edge table storage" module doc comment is corrected
• No new test failures (bun run test:all)
• Code passes bun run quality:fix

Open Questions

1. Relationship-type namespacing/disambiguation — a blocking pre-implementation decision, not an open question to resolve during implementation. Does a schema-declaration edge (project schema → task schema, relationship_type: "tasks") need to be distinguishable from an instance-level edge that happens to share the same relationship_type string? Resolve as: one shared helper (an "is this endpoint a schema node" check) reused by every relationship query, the delete-guard, and the reserved-name check alike — not three independent implementations of similar-but-not-identical scoping logic. This is precedented by the exact bug class already described in get_node_relationships's existing code comment (rel_ops.rs:343-349) for a structurally identical ambiguity one level down (two schemas sharing a relationship name).
2. edge_table field — confirmed dead, delete outright. InboundRelationship::edge_table is a SurrealDB-era holdover ("the edge table name for querying relationships"); confirmed via code search that no SQL or lookup anywhere reads it — it's populated by compute_edge_table_name() and never consumed. No re-evaluation needed; remove it.
3. Migration path / sequencing with Fix project/task schema field descriptions and add the missing project-task relationship #1999. No schema in the shipped database currently has any relationships defined, so there is no real data to migrate today — but Fix project/task schema field descriptions and add the missing project-task relationship #1999 adds the first one (project ↔ task). Fix project/task schema field descriptions and add the missing project-task relationship #1999 is written to tolerate either sequencing (it can implement in the current JSON format if it lands first, or the new table format if this issue lands first). Mild preference for this issue landing first, to avoid writing the declaration twice, but not a hard blocker either direction.
4. Validation-query cost on instance-edge creation — resolves cleanly in this proposal's favor, not a real cost concern. create_relationship's declaration lookup (relationship.rs:534-538) is already an uncached get_node() fetch + JSON parse on every instance-edge write today. A relationship-table query in the same spot is a lateral move onto an indexed lookup (idx_rel_in/idx_rel_out), not a new cost layer — there is no existing cache in front of this path to preserve or lose.
5. Sync/multi-database semantics — downgraded from a blocking unknown to a one-line confirmation. ADR-025 (SurrealDB Cloud) is superseded by ADR-036 (Supabase Postgres) — confirmed by reading ADR-036 directly, not just referencing it. ADR-036's write-path RLS example already queries tenant.relationships uniformly by relationship_type IN (...) with no schema/instance distinction anywhere in the design, and its decision No compilation errors or warnings #10 states explicitly that "the relationships table does NOT need its own sync_seq; the node trigger chain is sufficient" — a table-wide, undifferentiated statement. SYNC_TABLES = &["nodes", "relationships"] (cited in ADR-045) confirms relationships already syncs wholesale in shipped code. There is also a directly relevant, already-solved precedent: ADR-045 decided embeddings shouldn't sync at all, and the chosen mechanism was a type-filter at the sync layer (SYNC_TABLES excludes the table entirely; cloud-sync.md:399 shows mentions edges are already excluded from the synced relationships table by type today, reconciled locally instead). If schema-declaration edges ever need different sync treatment than instance edges, the same filter-by-type mechanism applies — no different storage shape required. Action: confirm this reading with whoever owns the sync design as a courtesy check, not as a blocking design question.
6. Reserved-name collision — elevated from "confirm it extends" to "write it from scratch," a required Acceptance Criterion above. Confirmed by review: no such check exists today, under any storage model. BUILTIN_RELATIONSHIP_NAMES is consulted only by the relationship viewer's display filter — never by schema-creation or relationship-declaration validation. A schema can declare a relationship literally named has_child today and nothing rejects it. This was a live, pre-existing gap even before this refactor; the refactor makes the consequence of leaving it unfixed worse (query-logic corruption risk, not just a display glitch) once declarations and primitives share one table.

Non-Goals

• Does not change SchemaField storage — fields describe a single node's own shape and have no natural "edge" representation; the JSON-in-properties pattern remains correct for them. This issue is scoped to relationships only, which are inherently edges between two nodes.
• Does not restore full SurrealDB-era per-relationship dedicated tables ({source_type}_{relationship_name}_{target_type}) — that granularity doesn't fit the SQLite three-table model this codebase has otherwise standardized on; this issue proposes reusing the existing shared relationship table, not recreating per-relationship tables.
• Does not include a cascade-delete or detach/orphan escape hatch for relationship-declaration removal in this issue's initial scope — pure block-by-default only (see Resolved design decisions). A confirm-and-cascade flow, if ever needed, is deliberate future follow-up work, not default-shipped here.
• Does not introduce a new caching layer for the read path in this issue's initial scope — see "No new caching layer" above; revisit post-launch only if real usage demonstrates a need.
• Does not require any proto, agent tool-schema, or seeded-skill-guidance changes — all three layers confirmed storage-agnostic by design via follow-up review.
• Does not block or depend on Fix project/task schema field descriptions and add the missing project-task relationship #1999/Relationships trigger shows even when a schema has no typed relationships defined #2007 shipping first — those work correctly under the current JSON storage model; this refactor can land before, after, or independent of them (see Open Question 3).
• Does not require a rollback story beyond "this must land before any relationship-bearing schema exists in a real user's database" — no live relationship data exists yet to roll back, but note Fix project/task schema field descriptions and add the missing project-task relationship #1999 is open and would be the first to create such data, so this is a real near-term sequencing constraint, not a vague future one.

Related Issues

• Relationship viewer: view and manage typed relationships between nodes #1918 (relationship viewer — the primary consumer of both the JSON-declared schema relationships and the relationship table's instance edges)
• Fix project/task schema field descriptions and add the missing project-task relationship #1999 (adds the first real SchemaRelationship declaration — a domain decision that holds regardless of this issue's storage-model outcome, per Fix project/task schema field descriptions and add the missing project-task relationship #1999's own note)
• Relationships trigger shows even when a schema has no typed relationships defined #2007 (Relationships button gating — reads schema.relationships/get_inbound_relationships, whichever storage model is current when it's implemented)