[malibio]

Closes #2008.

What changed

Schema relationship declarations (project → tasks → task) are no longer JSON inside the declaring schema node's properties.relationships — each declaration is a row in the existing relationship table between the declaring and target schema nodes, finishing the design the SQLite migration dropped:

• in_node = declaring schema node, out_node = target schema node (self-edge when target_type: None), relationship_type = declared name, full SchemaRelationship JSON in the row's properties (authoritative).
• Migration v004 converts any existing JSON declarations to rows (self-edge fallback for dangling targets) and strips the relationships key from every schema node. Covered by a migration test.

Consolidated read path (single round-trip preserved)

SqliteStore::get_schema_declarations / get_all_schema_declarations are the only declaration readers; get_schema_node/get_all_schemas hydrate SchemaNode.relationships from them, so callers still get fields + relationships in one fetch. All five former independent JSON parses now agree by construction (the issue counted four — delete_relationship's required-last-edge guard at relationship.rs:813 was a fifth):

1. create_relationship validation
2. delete_relationship required-guard
3. get_inbound_relationships (via get_all_schemas)
4. rel_ops::get_node_relationships outbound branch
5. context_ops agent relationship-hop context (search hits re-resolved from the hydrated corpus)

A new integration test (all_read_paths_agree_on_the_declaration_set) locks the agreement in.

Single write path + guards

NodeService::set_schema_relationships (used by create_schema/update_schema and core-schema seeding) is the only declaration writer. It enforces:

• Reserved names: a declaration may not take a builtin structural name (has_child, mentions, member_of, has_role) — BUILTIN_RELATIONSHIP_NAMES moved to models::schema and is shared by every consumer. This check did not exist before under any storage model.
• Block-by-default: removing or retargeting a declaration with live instance edges is rejected, naming the edge count. No cascade, no detach (per issue decision).
• Emits relationship domain events per actual change (diffed replace — unchanged declarations keep their row/id), so declaration edges replicate like instance edges.

Additional guards:

• delete_node/bulk_delete/delete_subtree_atomic refuse to delete a schema node that declaration edges point at or from (closes the ON DELETE CASCADE silent-destroy gap).
• create_relationship refuses a schema node on either end of an instance write.

Wire contract unchanged

Correction to the issue's analysis: nodespace-types::SchemaNode is not dead — the desktop app parses the daemon's schema responses through it, reading properties.relationships off the wire. The daemon's get_all_schemas/get_schema_definition now assemble the wire node via SchemaNode::into_wire_node() (relationships embedded in wire properties only), so no frontend or proto change was needed and none was made. into_node() is the storage shape and never carries relationships.

Deletions

• InboundRelationshipCache + relationship_cache_test.rs (confirmed dead in production; no replacement cache per issue scope)
• SchemaRelationship.edge_table + compute_edge_table_name (SurrealDB holdovers, zero readers)
• handle_add_schema_relationship/handle_remove_schema_relationship — recorded decision for the issue's open item: these were tool-unreachable dead code and update_schema's add_relationships/remove_relationships covers the capability, so they are deleted rather than wired up
• SqliteStore::update_schema (no callers; raw-properties writer, now a footgun)

Known limitations (noted deliberately)

• Schema-node property writes and declaration writes are two sequenced operations, not one transaction (full atomicity would require threading a tx through create_node_with_parent). Declarations are written first in update_schema so a live-edge rejection leaves the schema fully untouched.
• Sync (ADR-036): declaration edges ride the same relationships table sync + relationship events as instance edges; if they ever need different treatment, ADR-045's filter-by-type mechanism applies. Flagging for whoever owns sync design, per the issue's courtesy-check note.

Test evidence

• Rust: 46 test binaries across core/agent/daemon/types/cli — 2,145 passed, 0 failed
• New integration suite schema_relationship_declarations_test.rs: 9 tests (storage shape, read-path agreement, reserved names, live-edge blocks, schema-delete guard, endpoint scoping)
• Frontend: 191 files / 4,439 tests — identical to baseline
• Pre-push gate (test:all + e2e): passed

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2010 (issue #2008)

Review Type: Initial Review (no prior reviews on this PR; full diff origin/main...HEAD, 30 files, +1552/−1409, single commit 8cd08563)

Code Review Summary

This is a well-executed structural migration: schema relationship declarations move from properties.relationships JSON into first-class relationship-table edges, with a genuinely consolidated read path (get_schema_declarations / get_all_schema_declarations are the only readers, hydrated into SchemaNode.relationships), a diffed transactional replace as the write path, a correct v004 migration with test, reserved-name enforcement that did not previously exist, live-instance-edge blocking, and schema-delete guards. The into_node() (storage) vs into_wire_node() (wire) split is a clean resolution of the wire-contract question, and the CLI/desktop consumers I checked (packages/cli/src/commands/schema.rs, diagnostics.rs) consume the wire node generically and are unaffected (the wire actually gains the relationships key on get_all_schemas, which previously returned raw storage nodes). Dead code (InboundRelationshipCache, edge_table/compute_edge_table_name, SqliteStore::update_schema, the two tool-unreachable schema-relationship handlers) is deleted rather than deprecated, per project policy. The new 9-test integration suite is high-value — all_read_paths_agree_on_the_declaration_set locks in exactly the drift class the issue was written about.

One genuine gap keeps this from an immediate approve: the "single write path" invariant has a bypass through two public instance-edge mutation APIs (details below). It is a small, contained fix that mirrors the guard this PR already added to create_relationship.

Requirements Check (issue #2008 acceptance criteria)

Criterion	Status
Declarations stored as relationship rows between schema-node ids, not JSON	✅ relationships.rs:1305+, verified by declarations_are_relationship_rows_not_properties_json
Live SchemaNode confirmed (models/schema_node.rs); JSON read path removed; same round-trip	✅ get_schema_node/get_all_schemas hydrate in one caller-facing fetch; zero remaining properties.get("relationships") readers outside a test assertion
create_relationship / get_inbound_relationships / get_node_relationships outbound consolidated onto one shared query path	✅ all route through get_schema_node/get_all_schemas → get_schema_declarations (plus the fifth reader, delete_relationship's required-guard)
InboundRelationshipCache deleted, no replacement cache	✅ (two stale doc-comment references remain — nit below)
relationship_cache_test.rs deleted, not migrated	✅
playbook/validation.rs test-only create_schema helper rewritten through the real write path	✅ validation.rs:1180+ (and graph_resolver.rs fixture likewise)
Existing JSON declarations migrated	✅ migration v004 + v004_moves_json_declarations_to_relationship_rows
create_schema/update_schema write declaration rows atomically with the rest of the schema update	⚠️ Partial, documented: declaration replace is transactional, but node-properties write is a separate sequenced operation. Declarations-first ordering means a live-edge rejection leaves the schema untouched; the residual window (declarations committed, properties write fails) is acknowledged in the PR body. Acceptable deviation for a single-user local store — noted, not blocking
Reserved/builtin-name check at declaration time	✅ schema/mod.rs:190 + re-check in set_schema_relationships; tested for both create and update paths, including no-half-created-schema
Delete/rename/retarget blocked when live instance edges exist	⚠️ Enforced on the update_schema path (tested for remove and retarget) — but bypassable via delete_relationship/update_relationship_properties (🔴 below)
Schema-node delete guarded against FK-CASCADE destroying declarations	✅ delete_node, bulk_delete, delete_subtree_atomic via assert_schema_deletable; both ends tested. (delete_children_subtree_unchecked/delete_nodes_by_ids_unchecked remain unguarded — internal-only by contract, acceptable; see 🟢)
Decision recorded on handle_add/remove_schema_relationship	✅ deleted as tool-unreachable dead code; decision recorded in the PR body, capability covered by update_schema
Instance-level create_relationship/rel_ops guarded against schema-node endpoints	⚠️ create_relationship guarded and tested; the criterion's rel_ops mutation surface (delete_relationship, update_relationship_properties) is not (🔴 below)
context_ops relationship-hop context updated + regression test	✅ re-resolves hits from the hydrated corpus; related_one_hop_schemas unit tests updated. The re-resolution mapping inside build_workspace_context itself has no direct test (needs an embedding-service harness) — acceptable given the hydration source is integration-tested
Regression test: all read paths agree on one declaration set	✅ all_read_paths_agree_on_the_declaration_set covers five paths
Stale SurrealDB-era doc comment in models/schema.rs corrected	✅
No new test failures (test:all)	✅ as evidenced: Rust 2145/0 across 46 binaries, frontend 191 files/4439 = baseline, pre-push gate passed
Passes quality:fix	✅ (rustfmt churn in unrelated files corroborates it ran)

Open Question 1 (disambiguation) is resolved as endpoint-node_type scoping plus builtin-name exclusion, centralized in sqlite_store/relationships.rs (builtin_exclusion_sql, the join-scoped queries) — one mechanism, as required. declaration_edges_do_not_leak_into_instance_queries covers the shared-name case directly.

Findings

🔴 Critical

• packages/core/src/services/node_service/relationship.rs:801 (delete_relationship) and :912 (update_relationship_properties): declaration edges are mutable through the instance-edge APIs, bypassing every new guard. delete_relationship("assembly", "widgets", "widget") with schema-node ids resolves the declaration row via get_relationship_id (line 875) and deletes it (line 882) — no live-instance-edge check, so this silently produces exactly the orphaned-instance-edges state the block-by-default decision exists to prevent. Likewise update_relationship_properties will overwrite a declaration row's properties — the authoritative SchemaRelationship JSON — with arbitrary edge-attribute JSON; the corrupted row then fails parse_declaration_row and the declaration silently vanishes from every read path (warn-and-skip). Both methods are exposed through the daemon RPC surface (packages/daemon/src/services/node_service.rs:857,877 → rel_ops), and schema ids are guessable human-readable type names, so this is accidental-misuse territory, not just adversarial. This contradicts the store-layer comment that set_schema_declarations and friends "are the ONLY paths that read or write declaration rows" (sqlite_store/relationships.rs, section header) and the issue's criterion that instance-level create_relationship/rel_ops calls be guarded on either endpoint being a schema node. Fix: mirror the guard create_relationship gained — refuse schema-node endpoints in both methods (scoped to non-builtin relationship names so structural edges on schema nodes, e.g. description-subtree has_child, keep working), and add the two negative tests to schema_relationship_declarations_test.rs.

🟡 Improvements

• packages/core/src/db/migrations/v004_schema_relationship_edges.rs:59-100: migration will convert a builtin-named JSON declaration into a live structural edge. The reserved-name check is new, so a pre-v004 dev database can legally contain a declaration named e.g. has_child. v004 inserts it verbatim as a relationship_type = 'has_child' row between two schema nodes — which hierarchy traversals then treat as a real parent/child edge (delete_children_subtree_unchecked's recursive CTE would follow it and delete the target schema as a "child"). Shipped databases hold no declarations, so exposure is dev-only, but the fix is one is_builtin_relationship(&name) check in the loop: skip with a tracing::warn!, matching the existing no-name handling. Cheap insurance against a nasty corruption mode.
• packages/core/src/services/node_service/mod.rs:1402-1414 vs packages/core/src/services/node_service/schema.rs:387-402: the "single write path" doc is inaccurate for seeding. set_schema_relationships's doc comment says core-schema seeding routes through it, but seeding calls store.set_schema_declarations directly — skipping the reserved-name validation and event emission. Core schemas are compile-time-defined so the practical risk is low, but the invariant claim should match the code: either route seeding through the service method, or (if events during seeding are undesirable) correct both doc comments and add a debug_assert!/check that seeded declarations pass is_builtin_relationship. Otherwise the first core schema that ships a declaration will quietly bypass the name check.

🟢 Nitpicks

• Nit: packages/core/src/services/node_service/relationship.rs:487-490 and :817-820 — two hand-rolled matches!(… "member_of" | "has_child" | "mentions" | "has_role") lists survive right next to the newly shared BUILTIN_RELATIONSHIP_NAMES/is_builtin_relationship. The PR centralized this constant precisely so lists can't drift; these two call sites should use it (DRY).
• Nit: packages/daemon/src/services/local_agent_service.rs:114,1203 — doc comments still reference the deleted InboundRelationshipCache (“contrast InboundRelationshipCache's…”). Dangling references to removed code; reword.
• Nit: packages/daemon/src/services/node_service.rs:1240-1253 — a schema node whose properties fail SchemaNode::from_node makes get_schema_node return Ok(None) (warn-and-skip), so get_schema_definition falls through to the "not a schema (type=schema)" error — accurate-ish but misleading for a corrupt schema. Low priority; a distinct "schema node unparsable" message would aid debugging.
• Nit: packages/core/src/db/sqlite_store/relationships.rs (set_schema_declarations) — a direct caller passing two declarations with the same name inserts twice (existing.remove only consumes the first), and idx_rel_unique won't catch it when the targets differ. The handlers dedupe upstream, but a one-line duplicate-name rejection in set_schema_relationships would make the invariant hold for all callers (defense in depth, same spirit as the reserved-name re-check).
• Nit: delete_children_subtree_unchecked / delete_nodes_by_ids_unchecked (sqlite_store/nodes.rs) remain outside assert_schema_deletable. Their contracts are internal-only (description subtrees, import pruning) so this is acceptable — but worth one sentence in their doc comments stating they must never be handed schema-node ids now that declarations cascade.
• Nit: bun.lock — the configVersion removal is unrelated tooling-version churn bundled into this PR; harmless, but ideally lockfile drift lands separately.

Recommendation: REQUEST CHANGES

The PR is a clear net improvement to code health and closes issue #2008's structural goals with strong test coverage. The single blocker is the 🔴 declaration-mutation bypass in delete_relationship/update_relationship_properties — it silently defeats the PR's own headline invariant (block-by-default, single write path) through public API surface, and the fix is small and pattern-matched to code already in this PR. The two 🟡 items are strongly recommended alongside it; nits are optional.

[malibio]

Automated pragmatic-code-review: REQUEST CHANGES (posted as a comment — GitHub blocks review actions on one's own pull request). Full findings in the review comment above.

[malibio]

Address-review summary (commit 2)

All findings from the initial review addressed — none skipped:

🔴 Critical — declaration-mutation bypass (FIXED)
delete_relationship and update_relationship_properties now reject a non-builtin edge whose source is a schema node, mirroring create_relationship's guard. Deleting a declaration through the instance API (bypassing live-edge protection) and corrupting a declaration row's authoritative SchemaRelationship JSON are both refused with errors pointing at update_schema. Builtin edges stay exempt (a schema's description subtree legitimately carries has_child edges). Two negative tests added (delete_relationship_refuses_to_delete_a_declaration_edge, update_relationship_properties_refuses_to_corrupt_a_declaration_edge).

🟡 Migration builtin-name hazard (FIXED) — v004 drops (with warning) a legacy declaration named after a builtin structural relationship instead of converting it into a live structural edge between schema nodes; migration test extended with a has_child case.

🟡 Seeding bypass (FIXED) — the name invariants (reserved builtin names + per-schema uniqueness) moved into SqliteStore::set_schema_declarations itself, so seeding and any direct caller hit the same backstop; NodeService::set_schema_relationships docs corrected to state seeding's actual path. This also implements the duplicate-name-rejection nit.

🟢 Nits (ALL FIXED) — builtin matches! lists deduplicated onto is_builtin_relationship; stale InboundRelationshipCache doc references removed from local_agent_service.rs; daemon get_schema_definition now returns internal for a present-but-unparseable schema node vs failed_precondition for a non-schema node; _unchecked delete paths document that schema-node ids must never route through them; unrelated bun.lock churn reverted.

Tests: declarations suite 11/11, migrations 7/7, daemon green, quality:fix clean, pre-push gate (test:all + e2e) passed.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The critical finding was addressed with real logic changes to two public API methods plus store-layer validation changes — not style-only edits. A delta review of the new commit should verify the guards match the intended scoping (non-builtin + schema-source only).

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2010 (issue #2008)

Review Type: Re-Review (delta 8cd08563..2ea572c8, 10 files, +165/−20; prior verdict REQUEST CHANGES)

Previous Review Summary

Prior findings: 1 Critical, 2 Improvements, 7 Nits. Addressed: 10/10. Not addressed: 0. Each verified against the delta, not just the summary comment:

Prior finding	Status	Verification
🔴 Declaration-mutation bypass via delete_relationship / update_relationship_properties	✅ Fixed	packages/core/src/services/node_service/relationship.rs:814-827 and :942-959 — both methods now reject a non-builtin edge whose source is a schema node, with errors pointing at update_schema. Scoping is correct on both axes I was asked to check: (a) builtin edges on schema nodes keep working — both guards sit inside !is_builtin_relationship(...), so a schema's description-subtree has_child edges remain deletable/reorderable; (b) the ops-layer wrappers can't sidestep it — packages/core/src/ops/rel_ops.rs:93-118 delegate straight to these NodeService methods, so the daemon RPC surface (packages/daemon/src/services/node_service.rs:857,877) inherits the guards. Source-only checking is sufficient: create_relationship refuses a schema node on either endpoint (relationship.rs:528-560), so the only non-builtin schema-touching edges that exist are declarations (schema source). Two negative tests added and passing (delete_relationship_refuses_to_delete_a_declaration_edge, update_relationship_properties_refuses_to_corrupt_a_declaration_edge), both also asserting the declaration survives intact.
🟡 v004 converts builtin-named legacy declaration into a live structural edge	✅ Fixed	packages/core/src/db/migrations/v004_schema_relationship_edges.rs:61-72 — is_builtin_relationship check drops the declaration with tracing::warn! instead of inserting it; placed before target parsing so even a malformed builtin-named entry is dropped. Migration test extended with a has_child declaration in the fixture and a zero-rows assertion (migrations/mod.rs:394-411).
🟡 Seeding bypasses name validation; doc claims single write path	✅ Fixed	packages/core/src/db/sqlite_store/relationships.rs:1401-1430 — reserved-builtin-name and per-schema duplicate-name checks now enforced inside set_schema_declarations itself, so core-schema seeding and any direct caller hit the same backstop. NodeService::set_schema_relationships doc corrected to state seeding's actual path (schema.rs:388-393). This also resolves the duplicate-name nit.
🟢 Hand-rolled builtin matches! lists	✅ Fixed	Both call sites (relationship.rs:487, :814) now use is_builtin_relationship.
🟢 Stale InboundRelationshipCache doc refs	✅ Fixed	Both removed (local_agent_service.rs:114, :1200).
🟢 Daemon corrupt-schema vs non-schema error	✅ Fixed	daemon/src/services/node_service.rs:1248-1257 — parsable-failure now Status::internal, non-schema stays failed_precondition, absent id stays NotFound. Correct status semantics (stored-data bug vs caller error).
🟢 _unchecked delete doc contracts	✅ Fixed	sqlite_store/nodes.rs:680-683, :707-710 — both document why they bypass assert_schema_deletable and that schema-node ids must never route through them.
🟢 bun.lock churn	✅ Fixed	git diff origin/main..HEAD -- bun.lock is empty.

Findings (delta)

Critical Issues

None.

Suggested Improvements

None.

Nitpicks

• Nit: packages/core/src/services/node_service/relationship.rs:950 — update_relationship_properties now performs an extra get_node(source_id) point-read per non-builtin call solely for the guard. Negligible on a local SQLite store and the symmetric delete_relationship already had the fetch; noted only for completeness. No action needed.

Verification Evidence

• schema_relationship_declarations_test: 11 passed / 0 failed (includes both new negative tests).
• Migration unit tests: 7 passed / 0 failed (includes extended v004 builtin-drop case).
• Store-layer duplicate/reserved-name checks are redundant with (not replacing) the friendlier service-layer validation in set_schema_relationships — defense in depth, no behavior change on the happy path.
• Pre-push gate (test:all + e2e) passed on the fix commit per push logs.

Recommendation: APPROVE

The single blocker is closed with correctly-scoped guards (non-builtin + schema-source, unreachable-around via rel_ops), both improvements landed at the right layers (migration-time drop; store-layer invariant backstop), and every nit was taken. The delta introduces no new issues. Net positive; ready to merge.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Address-review (round 2)

Re-review verdict was APPROVE with zero actionable findings. One nit was noted and is deliberately skipped: the guard in update_relationship_properties costs one extra get_node point-read per non-builtin call — the reviewer marked it 'no action needed', the fetch is the guard itself, and a single point-read against local SQLite is negligible. Would revisit only if profiling ever shows relationship edits as a hot path.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: No code changed in this round. The PR is ready for merge (pending approvals).

🤖 Generated with Claude Code