[malibio]

Closes #2941
Closes #3026

Problem

create_relationship accepted a write through an in-direction declaration's own name (e.g. old --superseded_by--> new) and stored it under that name with the caller's endpoint order. One logical edge then had two storage shapes: forward-name reads missed it, cardinality was bypassed on both ends, and check_node_completeness had to check both shapes.

Change

• Normalize on write (relationship.rs): create_relationship_in_tx, remove_relationship_in_tx, delete_relationship and update_relationship_properties rewrite an in-name call to its forward spelling first. The endpoints are swapped and the name becomes the declaration's reverseName. The existing forward validation (target type, cardinality + reverse_cardinality replace, edge-field enums, required last-edge guard) then applies unchanged. Helpers: in_declaration_forward_name, forward_endpoints.
• Completeness (schema.rs): the in-name own-end lookup is removed. An out declaration counts edges from its own end; an in declaration counts only the inbound forward edge.
• Reads (rel_ops.rs): resolve_relationship_name resolves a node's own in declaration as a Reverse traversal of its forward name. Without this, a read through the in name would find nothing once writes are normalized.

No backfill: reset the database to drop any in-name edges already stored.

Tests

New packages/core/tests/relationship_in_name_normalization_test.rs:

• An in-name write stores the same row as the forward write.
• Forward and reverse cardinality are enforced through the in name.
• Update and delete work through the in name, including on an edge written the forward way.
• A read through the in name finds the forward edge.

The existing required_inbound_relationship_written_through_in_name_reports_complete still passes, now on the normalized shape.

Follow-up (not in this PR)

The relationships panel (get_node_relationships) still draws an own in declaration as an outgoing group, and get_inbound_relationships returns it as an incoming group as well. Both groups are now always empty. The forward declaration's incoming group already shows these edges, so nothing regresses, but the panel should render in declarations once.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2951

Review Type: Initial Review

Requirements Check

• ✅ create_relationship(a, <in name>, b) stores the same row as create_relationship(b, <forward name>, a). Covered by write_through_in_name_stores_the_forward_edge, which also checks that writing the forward spelling afterwards is idempotent.
• ✅ Cardinality and reverse_cardinality are enforced for writes through an in name. Both ends are tested. The swap happens before the declaration is resolved, so the forward declaration's replace semantics apply unchanged.
• ✅ Update and delete resolve an in name. Tested. remove_relationship_in_tx (the Play invariant path) is normalized too, but no test covers it.
• ✅ The in-name lookup is removed from check_node_completeness. required_inbound_relationship_written_through_in_name_reports_complete still passes.
• ✅ quality:fix and tests. Clippy is clean, the full core suite passed, and the pre-push gate passed.

Focus areas checked, no finding

• Evictions can't be swapped a second time by mistake. A name can't appear twice on one extends chain: duplicates are rejected per schema, and a subtype can't redeclare a name its ancestor declares. The lookup is keyed on the source's own type, and eviction callers always pass the forward spelling.
• Reverse for own in declarations is handled correctly by rel_ops::get_related_nodes, graph_resolver, and the agent both fan-out.
• No write path skips normalization for declared relationships. The raw store INSERTs are builtin-only.
• CLAUDE.md standards are met. No backfill, no issue numbers in code comments.

Code Review Findings

🟡 Important: the viewer and inbound readers still assume an in declaration describes stored edges

📁 packages/core/src/ops/rel_ops.rs:661-684, packages/core/src/services/node_service/relationship.rs:1788-1822

• Problem: get_node_relationships emits a node's own in declaration as an out group that queries its in name. That group is now permanently empty. The frontend still lists it as addable, and it can't edit edge fields on the inbound group where the edge actually appears.
• Problem: get_inbound_relationships has no direction filter. It reports other schemas' in declarations as inbound forward declarations. The panel shows an empty group for them, and resolve_relationship_name classifies them as InboundForward, which can never return anything. The "unknown relationship" error then suggests them.
• Recommendation:
  • Filter to Out in get_inbound_relationships.
  • Stop emitting own In declarations as outbound groups.
  • Add a viewer-level test.
• Principle: keep one source of truth consistently. Every reader should follow the single storage shape.

🟡 Important: an in declaration without a matching forward declaration can no longer be written, and the error is misleading

📁 packages/core/src/services/node_service/relationship.rs:532-548, :976-980

• Problem: saving a schema doesn't require an in declaration's reverseName to exist as an out declaration on targetType. After the swap, a declaration with no forward partner fails with an error naming a relationship and schema the caller never mentioned. If it is required, it can never be satisfied.
• Problem: if the far type pairs the name with another in declaration, get_relationship ignores direction, so it resolves anyway and the edge is stored under an in name. That is the second storage shape this PR exists to eliminate.
• Recommendation:
  • Validate the pairing when the schema is saved.
  • At minimum, reject a resolved In declaration on create, with an error in the caller's own terms.
  • Add a test for the unpaired case.
• Principle: fail fast, and enforce the invariant the design rests on.

🟢 Suggestion: the normalization block is repeated four times

📁 relationship.rs in create_in_tx, remove_in_tx, delete, update

• Recommendation: fold it into one helper that returns the normalized triple and reuses the already-fetched source node. That also removes the extra node fetches and extends-chain resolutions per write.

🟢 Suggestion: the tests only use a self-referential schema

📁 packages/core/tests/relationship_in_name_normalization_test.rs

• Recommendation: add a pair across two types and cover:
  • wrong-type rejection through the in name;
  • rel_ops narrowing through the in name;
  • a Play remove_relationship through the in name.

🟢 Nit: errors describe the rewritten call, not the caller's

📁 relationship.rs:1646-1650, :998-1001

• Problem: a failed update through superseded_by reports Relationship 'supersedes' from 'new' to 'old'….
• Recommendation: keep the caller's original spelling in error text.

Recommendation: REQUEST CHANGES

• The core swap is correct in all four write paths.
• Fix the two 🟡 findings in this PR: the leftover readers and the unvalidated in→out pairing.

[malibio]

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

[malibio]

Address-review summary

Addressed (4 of 5):

• 🟡 Leftover readers. get_inbound_relationships now returns only out declarations. This fixes the panel's always-empty inbound group, the name lookup returning InboundForward for a spelling that could never return anything, and the unknown-relationship error suggesting that spelling. get_node_relationships no longer draws a node's own in declarations as outgoing groups. The edge now shows once, in the forward declaration's incoming group.
• 🟡 Unpaired / wrongly paired in declarations. Handled at both levels:
  • Save time (folded in Validate in/out relationship pairing at schema save #3026). create_schema and update_schema add_relationships reject an in declaration unless it exactly mirrors an out declaration on its targetType chain: it names that declaration back, targets this type or an ancestor, mirrors its cardinalities, and declares no edgeFields. A self-referential pair declared in one payload still validates.
  • Write time. A write through an in name whose forward half was removed after saving is rejected with an error in the caller's terms.
• 🟢 Cross-type tests. doc.approved_by (in) ↔ person.approves (out), plus a memo that uses the same forward name. They cover forward storage, rejection of wrong types, narrowing reads to the declared type, panel rendering, save-time rejection cases, and the removed-forward case.
• 🟢 Error spelling. Update's missing-edge error and the unpaired-write error use the name the caller wrote.

Skipped (1):

• 🟢 Folding the four normalization call sites into one helper. Each site looks up the source node differently: a transaction read, a transaction read that also accepts not-yet-created date pages, and a plain read. A shared helper would need the node passed in, and would save only a few lines per site. Worth revisiting if a fifth write path appears.

Not covered by a test: a Play remove_relationship through an in name. It goes through remove_relationship_in_tx, which is crate-private, so integration tests can't reach it. It uses the same normalization as the other paths.

Verification: core suite and clippy clean; quality:fix clean; pre-push gate passed (test:all + e2e).

Re-review needed: yes. Reader behavior changed and schema-save validation is new.

[malibio]

Pragmatic Code Review: Re-Review (commits 233c2896, 874fb134 vs dd2442c4)

Previous Review Summary

• Previous recommendations: 5. Addressed: 4.
• Not addressed: 1, the DRY helper. The skip rationale holds: each call site reads its source node differently.
• New issues found: 3, all 🟢.

Previous findings

• 🟡 Readers. Fixed. get_inbound_relationships is filtered to out, and get_node_relationships skips a node's own in declarations. Every consumer was checked (validation.rs reverse-segment lookup, graph_resolver::is_declared_many_relationship, rel_ops name resolution and its error suggestions). None breaks, and several become more correct.
• 🟡 Pairing. Fixed both at write time, with an error in the caller's terms, and at save time, via validate_in_declarations_paired.
• 🟢 Cross-type tests and 🟢 error spelling: addressed.

Also checked, no finding

• No bypass path. update_schema has only add/remove/extends. Methodology install goes through handle_create_schema. No seeded or core schema declares in.
• Inheritance. A forward declaration on an ancestor of targetType is found. A forward that targets an ancestor of the schema is accepted.

Requirements

• create_relationship stores writes through an in-direction name with reversed endpoints, giving one edge two storage shapes #2941: all criteria ✅.
• Validate in/out relationship pairing at schema save #3026: all criteria ✅. edgeFields on in declarations are also rejected.

New findings

🟢 1. Editing the forward side can silently break an accepted pair
📁 packages/core/src/schema/mod.rs (the check sees only add_rels); relationship.rs (the runtime check sees only direction == Out)

• Scenario: remove approves and re-add it with a different reverseName or cardinality. The update passes because it adds an Out declaration. A write through approved_by still resolves Out at runtime. The edge is then stored under the forward side's new cardinality while the doc's in declaration says otherwise.
• Recommendation: re-validate the in declarations that mirror an Out declaration when that declaration changes. At minimum, add a runtime check that the forward's reverse_name equals the name the caller wrote.

🟢 2. A self-referential in declaration misses a forward inherited from an ancestor
📁 validate_in_declarations_paired, the far_type == schema_id branch

• Scenario: sub extends base, and base declares supersedes. Declaring superseded_by (in, targetType sub) on sub is rejected, even though sub inherits supersedes and the runtime write path would accept it.
• Recommendation: after checking this schema's own declarations, also check its ancestors' relationships.

🟢 3. update_schema builds own_chain before a same-call extends re-target

• Scenario: a combined extends + add_relationships call validates against the old parent chain.
• Recommendation: run the pairing check after the extends re-target.

Test notes

• The forward cardinality vs in reverseCardinality mirror check has no test.
• The wrong-type test asserts only is_err().

Recommendation: APPROVE

[malibio]

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

[malibio]

Address-review summary (re-review round), commit e65d7a3d

Addressed: all 3 🟢 findings and both test notes

1. Editing the forward side. When update_schema changes a schema's relationships or extends, every other schema's in declaration whose forward resolves through that schema is re-checked against the result. Removing a mirrored forward, or re-adding it with a different reverseName or cardinality, is rejected with an error naming the pair it would break. There is also a runtime check: a write through an in name requires the forward to name it back. That check matters because set_schema_relationships (called by Play actions) writes declarations without running schema-save validation.
2. Inherited forward. The forward lookup walks the far type's whole extends chain. The schema being saved is read from this call's values wherever it appears on that chain, so a subtype's in declaration can mirror a forward it inherits (ADR-078).
3. extends ordering. update_schema checks pairing after a same-call extends re-target, against the resulting chain.

• Test notes: added the forward-cardinality ↔ in reverseCardinality mirror case. The wrong-type test now asserts the specific error messages.

New tests: rejecting an edit to a mirrored forward; accepting an inherited forward; validating after an extends re-target; the runtime path when a declaration change bypasses schema save. The suite has 16 tests.

Verification: core suite and clippy clean; quality:fix clean; pre-push gate passed.

Re-review needed: yes. The validator changed shape (it now re-checks other schemas).

[malibio]

Pragmatic Code Review: Re-Review (commit e65d7a3d)

Previous Review Summary: 3 recommendations and 2 test notes. All were addressed, none were left open. 2 new issues were found.

Checked, no finding

• The update check is gated on relationships_added, and an extends re-target bumps that counter. Add, remove and extends are the only ways to change relationships, and all three bump a counter.
• No seed or methodology data declares an in relationship, so installs are unaffected.
• The cost is acceptable, because the check only runs on updates that change relationships.

Requirements: ✅ #2941 · ✅ #3026

🟡 type_chain substitutes the saved-state chain only at the pending schema's own id, not for its descendants

📁 packages/core/src/schema/mod.rs:443-456

• Setup:
  • A declares reviews (out, targetType D). C extends A, and E extends C.
  • D declares reviewed_by (in, targetType E, reverseName reviews).
• Scenario: update_schema(C, extends: B) is accepted, because E's stored chain [E, C, A] still reaches A's reviews.
• Result: after the commit E's chain is [E, C, B], so D's reviewed_by no longer has a forward declaration. The owner chain has the same gap.
• Fix: splice. Take the stored chain up to pending.id, then append pending.chain.

🟢 Mirror-error wrapping double-prefixes and reclassifies internal errors

📁 packages/core/src/schema/mod.rs:635-641

• Problem 1: the message reads invalid params: … which mirrors it: invalid params: …, with the prefix repeated.
• Problem 2: a storage fault inside the check is turned into InvalidParams, so the caller is told their input is wrong when it isn't.
• Fix: wrap only InvalidParams, and pass other errors through unchanged.

Nits

• Skip get_all_schemas when no schema declares an in relationship.
• Add a comment on the gate noting that an extends re-target bumps relationships_added.

Recommendation: APPROVE (fix the 🟡 before merge; small and local)

[malibio]

Automated pragmatic-code-review (re-review 2): APPROVE with a 🟡 to fix before merge (posted as a comment — GitHub blocks self-approval). Findings in the comment above.

[malibio]

Address-review summary (round 3), commit 3b18f8a0

Addressed

• 🟡 Splice. type_chain now takes a descendant's stored chain up to the schema being saved, then appends the chain this call leaves that schema with. This fixes both the forward lookup and the owner chain. New test reparenting_middle_schema_that_drops_mirrored_forward_is_rejected covers grandchild → middle → re-parent.
• 🟢 Error wrapping. Only InvalidParams is re-framed, and its inner message is used, so the kind prefix isn't repeated (the same test asserts this). Storage errors pass through unchanged.
• Nit (gate comment). Added a comment noting that an extends re-target counts as a declaration change.

Skipped

• Nit (early return in the mirror scan). Knowing that no schema declares an in relationship takes the same get_all_schemas read, so returning early saves nothing.

Verification: 17/17 normalization tests pass, the core suite passes, clippy is clean, and the pre-push gate passed. One unrelated flake during the local run (SqliteStore init failed at test setup) passes on rerun.

Re-review needed: a quick check of the splice fix only, as the reviewer asked.

[malibio]

Pragmatic Code Review: Re-Review (commit 3b18f8a0)

Previous Review Summary: 3 addressed, 0 not addressed, 0 new.

• 🟡 type_chain splice: fixed correctly. The stored chain and the pending chain are both nearest-first and start with the type itself. Truncating at the saved schema and appending the chain this call leaves it with gives [descendant…, pending, new ancestors…], with no duplicates and none of the old ancestors left over. This fixes the forward lookup, the mirror-scope check and the mirror owner_chain. A type that doesn't descend from the saved schema is left unchanged.
• 🟢 Error wrapping: fixed correctly. Only InvalidParams is re-framed, with a single prefix. Other kinds pass through unchanged.
• Nit: gate comment: addressed.

Test: traced by hand. reparenting_middle_schema_that_drops_mirrored_forward_is_rejected would fail without the splice: the forward would be found on the old grandparent and the save would return Ok. Its prefix-count assertion also guards the wrapping fix.

Code Review Findings: None. The splice runs after the cycle check, so it cannot introduce a loop.

Recommendation: APPROVE. No further re-review needed.

[malibio]

Automated pragmatic-code-review (re-review 3): APPROVE, no findings (posted as a comment — GitHub blocks self-approval).

[malibio]

Address-review (final)

The re-review of 3b18f8a0 returned no findings, so nothing changed after it. No re-review needed. The PR is ready to merge.