From 48fd5deafe5b77a034a30a2ec49d75cd4f5cf368 Mon Sep 17 00:00:00 2001 From: Michael Libio Date: Mon, 7 Sep 2026 23:54:07 +0200 Subject: [PATCH] Require reverseName and reverseCardinality on every relationship (closes #2433) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A schema relationship is declared once — one `relationship` row between the two schema nodes — but read from both ends. The reverse half of that declaration was optional and nothing derived it, so one stored edge could be only half-named: it read well from the end that declared it and poorly or not at all from the other. The target's side fell back to a synthesized "{SourceType} ({Relationship Name})" label — "Invoice (Customer)" where the modeled answer is "Invoices" — and the inbound group carried no cardinality, so nothing downstream could say how many sources may point at a node. Naming the inverse is a modeling decision only the author can make, so both fields are now required rather than derived: - `SchemaRelationship::reverse_name`/`reverse_cardinality` are non-`Option`, so the type carries the invariant instead of a validator alone. - `create_schema` and `update_schema` check the raw payload before serde and reject a relationship missing either field, naming the field and the offending entry by position and showing a corrected example built from the caller's own forward declaration. Serde's bare `missing field` message names the key but not what to put in it; this follows the existing `describe_missing_top_level_keys` pattern for the same reason. - `RelationshipGroup`'s `reverse_name` and `cardinality` are non-optional too, known on both sides now that a declaration must supply both. - The frontend's synthesized-label fallback is deleted rather than kept as a defensive branch: it would only hide a schema that failed validation. - `cli.md` and the `skill_rules.rs` source it is generated from document both fields as required, and every relationship example in the reference carries them. Breaking change to the schema API. Per CLAUDE.md this is greenfield with zero users: affected declarations are fixed in place and no migration or backfill is added. The one built-in schema declaring a relationship (`project.tasks`) already satisfied the rule. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KHdtH4eZHiU7RzhCuHArQX --- packages/agent/src/local_agent/tools.rs | 10 +- packages/agent/src/skill_rules.rs | 8 +- .../stage2_candidate_block.golden | 8 +- .../stage2_tool_surface.golden | 32 +- packages/cli/tests/cli_integration.rs | 8 +- packages/core/benches/performance.rs | 6 +- packages/core/src/models/core_schemas.rs | 8 +- packages/core/src/models/schema_node.rs | 4 +- packages/core/src/ops/context_ops.rs | 4 +- packages/core/src/ops/rel_ops.rs | 28 +- packages/core/src/playbook/graph_resolver.rs | 66 ++-- packages/core/src/playbook/validation.rs | 20 +- packages/core/src/schema/mod.rs | 161 ++++++++- packages/core/src/schema/schema_test.rs | 329 ++++++++++++++++-- .../core/src/services/node_service/mod.rs | 7 +- .../core/src/services/node_service/schema.rs | 21 +- .../core/tests/relationship_editing_test.rs | 4 +- ...elationship_reverse_name_traversal_test.rs | 35 +- .../schema_relationship_declarations_test.rs | 103 +++++- .../src/lib/services/relationship-grouping.ts | 29 +- .../tests/unit/relationship-grouping.test.ts | 17 +- packages/nodespace-types/src/schema.rs | 113 +++++- packages/skill/references/cli.md | 10 +- 23 files changed, 854 insertions(+), 177 deletions(-) diff --git a/packages/agent/src/local_agent/tools.rs b/packages/agent/src/local_agent/tools.rs index e3b624ee6..07799428e 100644 --- a/packages/agent/src/local_agent/tools.rs +++ b/packages/agent/src/local_agent/tools.rs @@ -1240,9 +1240,11 @@ fn def_create_schema() -> ToolDefinition { "targetType": { "type": "string", "description": "Target node type ID — MUST be an existing type from the EXISTING SCHEMAS list (e.g., 'task', 'ticket', 'adr'). Do NOT invent types that don't exist yet." }, "direction": { "type": "string", "enum": ["out", "in"], "description": "Direction: 'out' (this→target, default) or 'in' (target→this)" }, "cardinality": { "type": "string", "enum": ["one", "many"], "description": "Cardinality: 'one' or 'many' (default)" }, + "reverseName": { "type": "string", "description": "REQUIRED. What this edge is called read from the target's end — one edge is stored and read from both sides, so it needs a name from each. Choose it: plural where the target may hold many (an invoice billed_to a customer reads back as 'invoices', NOT 'Invoice (Customer)')." }, + "reverseCardinality": { "type": "string", "enum": ["one", "many"], "description": "REQUIRED. How many sources may point at one target: 'one' or 'many'. An invoice billed_to one customer, where a customer may have many invoices, is cardinality 'one' with reverseCardinality 'many'." }, "description": { "type": "string", "description": "What this relationship represents" } }, - "required": ["name", "targetType", "direction", "cardinality"] + "required": ["name", "targetType", "direction", "cardinality", "reverseName", "reverseCardinality"] } } }, @@ -1319,9 +1321,11 @@ fn def_update_schema() -> ToolDefinition { "name": { "type": "string" }, "targetType": { "type": "string" }, "direction": { "type": "string", "enum": ["out", "in"] }, - "cardinality": { "type": "string", "enum": ["one", "many"] } + "cardinality": { "type": "string", "enum": ["one", "many"] }, + "reverseName": { "type": "string", "description": "REQUIRED. What this edge is called read from the target's end — one edge is stored and read from both sides, so it needs a name from each. Choose it: plural where the target may hold many (an invoice billed_to a customer reads back as 'invoices', NOT 'Invoice (Customer)')." }, + "reverseCardinality": { "type": "string", "enum": ["one", "many"], "description": "REQUIRED. How many sources may point at one target: 'one' or 'many'. An invoice billed_to one customer, where a customer may have many invoices, is cardinality 'one' with reverseCardinality 'many'." } }, - "required": ["name", "targetType", "direction", "cardinality"] + "required": ["name", "targetType", "direction", "cardinality", "reverseName", "reverseCardinality"] } }, "remove_relationships": { diff --git a/packages/agent/src/skill_rules.rs b/packages/agent/src/skill_rules.rs index e547916b8..e1de7fe16 100644 --- a/packages/agent/src/skill_rules.rs +++ b/packages/agent/src/skill_rules.rs @@ -135,8 +135,8 @@ pub const RELATIONSHIP_VS_FIELD: SchemaRule = SchemaRule { pub const TARGET_TYPE_MUST_EXIST: SchemaRule = SchemaRule { id: "target-type-must-exist", - imperative: "The targetType MUST be an existing schema ID from the EXISTING SCHEMAS list in the system prompt, or the schema ID of the type you are creating in this same call — do NOT invent types that aren't listed. If the target type doesn't exist yet, omit the relationship entirely. Set reverseName when the edge has a natural name read from the target's end — it makes the reverse query directly callable. Examples:\n- ADR supersedes adr (one): `{\"name\": \"supersedes\", \"targetType\": \"adr\", \"direction\": \"out\", \"cardinality\": \"one\"}`\n- Ticket has_task task (many): `{\"name\": \"has_task\", \"targetType\": \"task\", \"direction\": \"out\", \"cardinality\": \"many\"}`\n- ADR decided_by person, readable back as the person's decisions: `{\"name\": \"decided_by\", \"targetType\": \"person\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"decisions\", \"reverseCardinality\": \"many\"}`\n\nSELF-REFERENCE: a type may point at itself in the same schema create call — use its own schema ID (the snake_case form of the name), no second call needed. Give the reverse direction a reverseName rather than declaring a second relationship: one stored edge, readable from both ends. Example, on `schema create` for ADR: `{\"name\": \"supersedes\", \"targetType\": \"adr\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"superseded_by\", \"reverseCardinality\": \"one\"}`. Same for `blocks`/`blocked_by` on a task or `parent`/`child` on a category.", - prose: "`targetType` must be an existing schema ID, or the schema ID of the type being created in the same call. Set `reverseName` when the edge has a natural name read from the target's end — that name becomes directly queryable from that side. Examples: `{\"name\":\"supersedes\",\"targetType\":\"adr\",\"direction\":\"out\",\"cardinality\":\"one\"}`, `{\"name\":\"has_task\",\"targetType\":\"task\",\"direction\":\"out\",\"cardinality\":\"many\"}`, `{\"name\":\"decided_by\",\"targetType\":\"person\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"decisions\",\"reverseCardinality\":\"many\"}`.\n\n**Self-referential relationships:** a type may point at itself in the same `schema create` call — give its own schema ID (the snake_case form of the name); no follow-up `schema update` is needed. Name the reverse direction with `reverseName` rather than declaring a second relationship, so one stored edge is readable from both ends: `{\"name\":\"supersedes\",\"targetType\":\"adr\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"superseded_by\",\"reverseCardinality\":\"one\"}`. The same shape covers `blocks`/`blocked_by` on a task and `parent`/`child` on a category.", + imperative: "The targetType MUST be an existing schema ID from the EXISTING SCHEMAS list in the system prompt, or the schema ID of the type you are creating in this same call — do NOT invent types that aren't listed. If the target type doesn't exist yet, omit the relationship entirely. reverseName and reverseCardinality are REQUIRED on every relationship — a declaration missing either is rejected. One edge is stored and read from BOTH ends, so name it from both: reverseName is what the edge is called read from the target (plural where that end may hold many — \"invoices\", not \"Invoice (Customer)\"), and reverseCardinality is \"one\" or \"many\", saying how many sources may point at one target. Examples:\n- ADR supersedes adr (one), read back as superseded_by: `{\"name\": \"supersedes\", \"targetType\": \"adr\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"superseded_by\", \"reverseCardinality\": \"one\"}`\n- Ticket has_task task (many), read back as ticket: `{\"name\": \"has_task\", \"targetType\": \"task\", \"direction\": \"out\", \"cardinality\": \"many\", \"reverseName\": \"ticket\", \"reverseCardinality\": \"one\"}`\n- ADR decided_by person, readable back as the person's decisions: `{\"name\": \"decided_by\", \"targetType\": \"person\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"decisions\", \"reverseCardinality\": \"many\"}`\n\nSELF-REFERENCE: a type may point at itself in the same schema create call — use its own schema ID (the snake_case form of the name), no second call needed. The required reverseName is what names the other direction, so never declare a second relationship for it: one stored edge, readable from both ends. Example, on `schema create` for ADR: `{\"name\": \"supersedes\", \"targetType\": \"adr\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"superseded_by\", \"reverseCardinality\": \"one\"}`. Same for `blocks`/`blocked_by` on a task or `parent`/`child` on a category.", + prose: "`targetType` must be an existing schema ID, or the schema ID of the type being created in the same call. `reverseName` and `reverseCardinality` are **required** on every relationship — a declaration missing either is rejected. One edge is stored and read from both ends, so name it from both: `reverseName` is what the edge is called read from the target (plural where that end may hold many — `invoices`, not `Invoice (Customer)`), and `reverseCardinality` is `one` or `many`, saying how many sources may point at one target. Examples: `{\"name\":\"supersedes\",\"targetType\":\"adr\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"superseded_by\",\"reverseCardinality\":\"one\"}`, `{\"name\":\"has_task\",\"targetType\":\"task\",\"direction\":\"out\",\"cardinality\":\"many\",\"reverseName\":\"ticket\",\"reverseCardinality\":\"one\"}`, `{\"name\":\"decided_by\",\"targetType\":\"person\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"decisions\",\"reverseCardinality\":\"many\"}`.\n\n**Self-referential relationships:** a type may point at itself in the same `schema create` call — give its own schema ID (the snake_case form of the name); no follow-up `schema update` is needed. The required `reverseName` is what names the other direction, so never declare a second relationship for it — one stored edge, readable from both ends: `{\"name\":\"supersedes\",\"targetType\":\"adr\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"superseded_by\",\"reverseCardinality\":\"one\"}`. The same shape covers `blocks`/`blocked_by` on a task and `parent`/`child` on a category.", }; pub const ENUM_EDGE_FIELDS: SchemaRule = SchemaRule { @@ -233,8 +233,8 @@ pub const ORG_NEEDS_EXISTING_COLLECTION: InteractionRule = InteractionRule { /// spellings, but the mapping is cheap to state up front. pub const RELATIONSHIP_REVERSE_TRAVERSAL: InteractionRule = InteractionRule { id: "relationship-reverse-traversal", - imperative: "REVERSE TRAVERSAL: A relationship declared as {\"name\": \"decided_by\", \"targetType\": \"person\", \"reverseName\": \"decisions\"} on the adr schema is readable from BOTH ends. From an adr, call get_related_nodes with relationship_type 'decided_by' and direction 'out'. From the person, use the declared reverseName — relationship_type 'decisions' — or equivalently 'decided_by' with direction 'in'; both return the same ADRs. An empty result means no edges exist, NOT that the reverse direction is unsupported. A relationship name that is declared in neither direction is now an error naming the spellings that do work — read it and retry rather than reporting the capability as missing.", - prose: "**Traversing the reverse direction.** A relationship is declared once, on the source type, but reads from both ends. Given `{\"name\":\"decided_by\",\"targetType\":\"person\",\"reverseName\":\"decisions\"}` on `adr`: from the ADR, `nodespace relationship get --type decided_by --direction out`; from the person, use the declared `reverseName` — `nodespace relationship get --type decisions` — or the equivalent `--type decided_by --direction in`. Both spellings return the same ADRs, and the output line's arrow shows the direction actually traversed. An empty result means no edges exist, not that reverse traversal is unsupported. A name declared in neither direction is rejected with an error naming the spellings that do work — read it and retry rather than concluding the capability is missing.", + imperative: "REVERSE TRAVERSAL: A relationship declared as {\"name\": \"decided_by\", \"targetType\": \"person\", \"direction\": \"out\", \"cardinality\": \"one\", \"reverseName\": \"decisions\", \"reverseCardinality\": \"many\"} on the adr schema is readable from BOTH ends. From an adr, call get_related_nodes with relationship_type 'decided_by' and direction 'out'. From the person, use the declared reverseName — relationship_type 'decisions' — or equivalently 'decided_by' with direction 'in'; both return the same ADRs. An empty result means no edges exist, NOT that the reverse direction is unsupported. A relationship name that is declared in neither direction is now an error naming the spellings that do work — read it and retry rather than reporting the capability as missing.", + prose: "**Traversing the reverse direction.** A relationship is declared once, on the source type, but reads from both ends. Given `{\"name\":\"decided_by\",\"targetType\":\"person\",\"direction\":\"out\",\"cardinality\":\"one\",\"reverseName\":\"decisions\",\"reverseCardinality\":\"many\"}` on `adr`: from the ADR, `nodespace relationship get --type decided_by --direction out`; from the person, use the declared `reverseName` — `nodespace relationship get --type decisions` — or the equivalent `--type decided_by --direction in`. Both spellings return the same ADRs, and the output line's arrow shows the direction actually traversed. An empty result means no edges exist, not that reverse traversal is unsupported. A name declared in neither direction is rejected with an error naming the spellings that do work — read it and retry rather than concluding the capability is missing.", skill_md_key_phrase: "use the declared `reverseName`", }; diff --git a/packages/agent/tests/golden/prompt_assembly/stage2_candidate_block.golden b/packages/agent/tests/golden/prompt_assembly/stage2_candidate_block.golden index 9113a7c02..16a3fb6fe 100644 --- a/packages/agent/tests/golden/prompt_assembly/stage2_candidate_block.golden +++ b/packages/agent/tests/golden/prompt_assembly/stage2_candidate_block.golden @@ -44,15 +44,15 @@ RENAME VS RELABEL: rename_fields can rename a field's storage key OR relabel its DELETING A SCHEMA — the capability exists, use it: When the user asks to remove, drop, undo or clean up a node type — including a throwaway type you created yourself this session — delete it. Do NOT report deletion as unsupported and do NOT propose stripping the schema to an empty shell as a workaround. PREREQUISITE: a schema carrying relationship declarations cannot be deleted; call update_schema with remove_relationships first, on THIS type for the relationships it declares AND on every other type that declares a relationship targeting it, then delete the schema node. This is the mirror of the targetType rule: a relationship's target must EXIST to declare it, and must be ABSENT to delete the type it points at. The rejection names the remaining declaration count — act on it rather than giving up. Only declarations BETWEEN SCHEMAS block the delete: edges between ordinary nodes are instance data and do not count, so do not go hunting for those. Deleting the type does NOT delete its existing instances — they remain as nodes; delete those separately with delete_node if the user wants them gone too. -RELATIONSHIPS: Use relationships (not fields) when a field references another node type. The targetType MUST be an existing schema ID from the EXISTING SCHEMAS list in the system prompt, or the schema ID of the type you are creating in this same call — do NOT invent types that aren't listed. If the target type doesn't exist yet, omit the relationship entirely. Set reverseName when the edge has a natural name read from the target's end — it makes the reverse query directly callable. Examples: +RELATIONSHIPS: Use relationships (not fields) when a field references another node type. The targetType MUST be an existing schema ID from the EXISTING SCHEMAS list in the system prompt, or the schema ID of the type you are creating in this same call — do NOT invent types that aren't listed. If the target type doesn't exist yet, omit the relationship entirely. reverseName and reverseCardinality are REQUIRED on every relationship — a declaration missing either is rejected. One edge is stored and read from BOTH ends, so name it from both: reverseName is what the edge is called read from the target (plural where that end may hold many — "invoices", not "Invoice (Customer)"), and reverseCardinality is "one" or "many", saying how many sources may point at one target. Examples: -ADR supersedes adr (one): `{"name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one"}` +ADR supersedes adr (one), read back as superseded_by: `{"name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one"}` -Ticket has_task task (many): `{"name": "has_task", "targetType": "task", "direction": "out", "cardinality": "many"}` +Ticket has_task task (many), read back as ticket: `{"name": "has_task", "targetType": "task", "direction": "out", "cardinality": "many", "reverseName": "ticket", "reverseCardinality": "one"}` ADR decided_by person, readable back as the person's decisions: `{"name": "decided_by", "targetType": "person", "direction": "out", "cardinality": "one", "reverseName": "decisions", "reverseCardinality": "many"}` -SELF-REFERENCE: a type may point at itself in the same schema create call — use its own schema ID (the snake_case form of the name), no second call needed. Give the reverse direction a reverseName rather than declaring a second relationship: one stored edge, readable from both ends. Example, on `schema create` for ADR: `{"name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one"}`. Same for `blocks`/`blocked_by` on a task or `parent`/`child` on a category. +SELF-REFERENCE: a type may point at itself in the same schema create call — use its own schema ID (the snake_case form of the name), no second call needed. The required reverseName is what names the other direction, so never declare a second relationship for it: one stored edge, readable from both ends. Example, on `schema create` for ADR: `{"name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one"}`. Same for `blocks`/`blocked_by` on a task or `parent`/`child` on a category. TITLE TEMPLATE: Set title_template when a node's identity comes from its fields rather than free-form content. Use {field_name} placeholders. CRITICAL: every placeholder in title_template MUST be defined as a field in the fields array. Omit title_template if the content/title field alone identifies the node. diff --git a/packages/agent/tests/golden/prompt_assembly/stage2_tool_surface.golden b/packages/agent/tests/golden/prompt_assembly/stage2_tool_surface.golden index 0cc10e264..f7005c265 100644 --- a/packages/agent/tests/golden/prompt_assembly/stage2_tool_surface.golden +++ b/packages/agent/tests/golden/prompt_assembly/stage2_tool_surface.golden @@ -417,6 +417,18 @@ parameters_schema: "description": "Relationship name, lowercase snake_case (e.g. 'supersedes', 'blocks', 'assigned_to')", "type": "string" }, + "reverseCardinality": { + "description": "REQUIRED. How many sources may point at one target: 'one' or 'many'. An invoice billed_to one customer, where a customer may have many invoices, is cardinality 'one' with reverseCardinality 'many'.", + "enum": [ + "one", + "many" + ], + "type": "string" + }, + "reverseName": { + "description": "REQUIRED. What this edge is called read from the target's end — one edge is stored and read from both sides, so it needs a name from each. Choose it: plural where the target may hold many (an invoice billed_to a customer reads back as 'invoices', NOT 'Invoice (Customer)').", + "type": "string" + }, "targetType": { "description": "Target node type ID — MUST be an existing type from the EXISTING SCHEMAS list (e.g., 'task', 'ticket', 'adr'). Do NOT invent types that don't exist yet.", "type": "string" @@ -426,7 +438,9 @@ parameters_schema: "name", "targetType", "direction", - "cardinality" + "cardinality", + "reverseName", + "reverseCardinality" ], "type": "object" }, @@ -522,6 +536,18 @@ parameters_schema: "name": { "type": "string" }, + "reverseCardinality": { + "description": "REQUIRED. How many sources may point at one target: 'one' or 'many'. An invoice billed_to one customer, where a customer may have many invoices, is cardinality 'one' with reverseCardinality 'many'.", + "enum": [ + "one", + "many" + ], + "type": "string" + }, + "reverseName": { + "description": "REQUIRED. What this edge is called read from the target's end — one edge is stored and read from both sides, so it needs a name from each. Choose it: plural where the target may hold many (an invoice billed_to a customer reads back as 'invoices', NOT 'Invoice (Customer)').", + "type": "string" + }, "targetType": { "type": "string" } @@ -530,7 +556,9 @@ parameters_schema: "name", "targetType", "direction", - "cardinality" + "cardinality", + "reverseName", + "reverseCardinality" ], "type": "object" }, diff --git a/packages/cli/tests/cli_integration.rs b/packages/cli/tests/cli_integration.rs index 28620b46b..30382aa8b 100644 --- a/packages/cli/tests/cli_integration.rs +++ b/packages/cli/tests/cli_integration.rs @@ -844,7 +844,9 @@ async fn schema_delete_requires_relationship_declarations_removed_first() { "name": "supersedes", "targetType": "memo", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "superseded_by", + "reverseCardinality": "one" }] }) .to_string(), @@ -1032,7 +1034,7 @@ async fn relationship_create_and_get() { "name": "Ticket", "fields": [{"name": "title", "type": "string"}], "relationships": [ - {"name": "blocks", "target_type": "ticket", "direction": "out", "cardinality": "many"} + {"name": "blocks", "targetType": "ticket", "direction": "out", "cardinality": "many", "reverseName": "blocked_by", "reverseCardinality": "many"} ] }) .to_string(), @@ -1126,7 +1128,7 @@ async fn relationship_get_emits_flat_properties() { "name": "Ticket", "fields": [{"name": "severity", "type": "string"}], "relationships": [ - {"name": "blocks", "target_type": "ticket", "direction": "out", "cardinality": "many"} + {"name": "blocks", "targetType": "ticket", "direction": "out", "cardinality": "many", "reverseName": "blocked_by", "reverseCardinality": "many"} ] }) .to_string(), diff --git a/packages/core/benches/performance.rs b/packages/core/benches/performance.rs index 3098e0d54..846c4b566 100644 --- a/packages/core/benches/performance.rs +++ b/packages/core/benches/performance.rs @@ -794,9 +794,11 @@ async fn create_schema_chain(svc: &Arc, depth: usize) -> Vec Vec { direction: RelationshipDirection::Out, cardinality: RelationshipCardinality::Many, required: None, - reverse_name: Some("project".to_string()), - reverse_cardinality: Some(RelationshipCardinality::One), + reverse_name: "project".to_string(), + reverse_cardinality: RelationshipCardinality::One, edge_fields: None, description: Some("Tasks belonging to this project".to_string()), }], @@ -1602,8 +1602,8 @@ mod tests { assert_eq!(rel.name, "tasks"); assert_eq!(rel.target_type.as_deref(), Some("task")); assert_eq!(rel.cardinality, RelationshipCardinality::Many); - assert_eq!(rel.reverse_name.as_deref(), Some("project")); - assert_eq!(rel.reverse_cardinality, Some(RelationshipCardinality::One)); + assert_eq!(rel.reverse_name, "project"); + assert_eq!(rel.reverse_cardinality, RelationshipCardinality::One); let task = schemas.iter().find(|s| s.id == "task").unwrap(); assert!( diff --git a/packages/core/src/models/schema_node.rs b/packages/core/src/models/schema_node.rs index 9209d9423..cfd22d6f7 100644 --- a/packages/core/src/models/schema_node.rs +++ b/packages/core/src/models/schema_node.rs @@ -426,7 +426,9 @@ mod tests { "name": "widgets", "targetType": "widget", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "gadget", + "reverseCardinality": "one" })) .unwrap()]; diff --git a/packages/core/src/ops/context_ops.rs b/packages/core/src/ops/context_ops.rs index 0a741e1e5..a92989580 100644 --- a/packages/core/src/ops/context_ops.rs +++ b/packages/core/src/ops/context_ops.rs @@ -890,8 +890,8 @@ mod tests { direction: RelationshipDirection::Out, cardinality: RelationshipCardinality::Many, required: None, - reverse_name: None, - reverse_cardinality: None, + reverse_name: format!("reverse_{name}"), + reverse_cardinality: RelationshipCardinality::Many, edge_fields: None, description: None, } diff --git a/packages/core/src/ops/rel_ops.rs b/packages/core/src/ops/rel_ops.rs index a7a401dd2..53b194f41 100644 --- a/packages/core/src/ops/rel_ops.rs +++ b/packages/core/src/ops/rel_ops.rs @@ -231,7 +231,7 @@ async fn resolve_relationship_name( } for (source_type, rel) in &inbound { - if rel.reverse_name.as_deref() == Some(relationship_name) { + if rel.reverse_name == relationship_name { return Ok(ResolvedRelName::Reverse { forward_name: rel.name.clone(), source_type: source_type.clone(), @@ -266,16 +266,13 @@ async fn resolve_relationship_name( if BUILTIN_RELATIONSHIP_NAMES.contains(&rel.name.as_str()) { continue; } - // The inbound side is ALWAYS reachable by the forward name read inbound, - // whether or not a reverse_name exists — so that spelling is listed - // either way. A declared reverse_name is an ADDITIONAL, more natural + // The inbound side is ALWAYS reachable by the forward name read + // inbound. The declared reverse_name is an ADDITIONAL, more natural // spelling for the same traversal that needs no --direction flag; it // does not replace the forward one. Listing only the reverse name would // omit a spelling that demonstrably works, which is the same // under-reporting this error exists to prevent. - if let Some(reverse) = rel.reverse_name.as_deref() { - direct.push(reverse.to_string()); - } + direct.push(rel.reverse_name.clone()); needs_direction_in.push(rel.name.clone()); } for list in [&mut direct, &mut needs_direction_in] { @@ -436,15 +433,16 @@ pub struct RelationshipGroup { /// The node type on the FAR end of the edge (outbound: the declared /// `target_type`; inbound: the declaring `source_type`). pub target_type: Option, - /// Reverse label supplied by the source schema; the inbound side displays - /// this when present (falling back to a derived label in the viewer). - pub reverse_name: Option, - /// The schema type that DECLARES this relationship (used to build the inbound - /// fallback label when `reverse_name` is absent). + /// Reverse label supplied by the source schema. Always present — every + /// declaration names the edge from both ends — so the inbound side displays + /// it directly rather than deriving a label. + pub reverse_name: String, + /// The schema type that DECLARES this relationship. pub source_type: String, /// Effective cardinality for THIS side: the forward `cardinality` outbound, - /// the `reverse_cardinality` inbound (may be absent inbound). - pub cardinality: Option, + /// the `reverse_cardinality` inbound. Known on both sides, since a + /// declaration must supply both. + pub cardinality: RelationshipCardinality, /// Whether the forward relationship is required (outbound only). The viewer /// uses this to confirm-on-delete; last-edge removal is enforced server-side /// in `NodeService::delete_relationship`. @@ -565,7 +563,7 @@ pub async fn get_node_relationships( target_type: rel.target_type.clone(), reverse_name: rel.reverse_name.clone(), source_type: node_type.clone(), - cardinality: Some(rel.cardinality.clone()), + cardinality: rel.cardinality.clone(), required: rel.required, edge_fields: rel.edge_fields.clone(), description: rel.description.clone(), diff --git a/packages/core/src/playbook/graph_resolver.rs b/packages/core/src/playbook/graph_resolver.rs index 53b56e0ce..12da2617d 100644 --- a/packages/core/src/playbook/graph_resolver.rs +++ b/packages/core/src/playbook/graph_resolver.rs @@ -703,9 +703,11 @@ mod tests { "gr_issue", json!([{ "name": "story", - "target_type": "gr_story", + "targetType": "gr_story", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -743,9 +745,11 @@ mod tests { "gr_issue_ct", json!([{ "name": "story", - "target_type": "gr_story_ct", + "targetType": "gr_story_ct", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -799,9 +803,11 @@ mod tests { "gr_story3", json!([{ "name": "epic", - "target_type": "gr_epic", + "targetType": "gr_epic", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "stories", + "reverseCardinality": "many" }]), ) .await; @@ -810,9 +816,11 @@ mod tests { "gr_task3", json!([{ "name": "story", - "target_type": "gr_story3", + "targetType": "gr_story3", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -871,9 +879,11 @@ mod tests { "gr_task4", json!([{ "name": "story", - "target_type": "gr_story4", + "targetType": "gr_story4", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -911,9 +921,11 @@ mod tests { "gr_parent", json!([{ "name": "subtasks", - "target_type": "gr_subtask", + "targetType": "gr_subtask", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "parent_task", + "reverseCardinality": "one" }]), ) .await; @@ -958,9 +970,11 @@ mod tests { "gr_source5", json!([{ "name": "target", - "target_type": "gr_target5", + "targetType": "gr_target5", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "sources", + "reverseCardinality": "many" }]), ) .await; @@ -1041,9 +1055,11 @@ mod tests { "gr_story8", json!([{ "name": "epic", - "target_type": "gr_epic8", + "targetType": "gr_epic8", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "stories", + "reverseCardinality": "many" }]), ) .await; @@ -1052,9 +1068,11 @@ mod tests { "gr_task8", json!([{ "name": "story", - "target_type": "gr_story8", + "targetType": "gr_story8", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -1117,9 +1135,11 @@ mod tests { "gr_parent9", json!([{ "name": "items", - "target_type": "gr_item9", + "targetType": "gr_item9", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "collection", + "reverseCardinality": "one" }]), ) .await; @@ -1166,9 +1186,11 @@ mod tests { "gr_parent10", json!([{ "name": "tasks", - "target_type": "gr_sub10", + "targetType": "gr_sub10", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "parent_task", + "reverseCardinality": "one" }]), ) .await; diff --git a/packages/core/src/playbook/validation.rs b/packages/core/src/playbook/validation.rs index 2f4972a51..f3b782839 100644 --- a/packages/core/src/playbook/validation.rs +++ b/packages/core/src/playbook/validation.rs @@ -1332,7 +1332,9 @@ mod tests { { "name": "owned_by", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "owns", + "reverseCardinality": "many" } ]), ) @@ -1366,7 +1368,9 @@ mod tests { { "name": "linked_to", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "linked_from", + "reverseCardinality": "many" } ]), ) @@ -1537,7 +1541,9 @@ mod tests { "name": "story", "targetType": "vp_story", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": "issues", + "reverseCardinality": "many" }]), ) .await; @@ -1630,7 +1636,9 @@ mod tests { json!([{ "name": "linked", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "linked_from", + "reverseCardinality": "many" // no target_type }]), ) @@ -1753,7 +1761,9 @@ mod tests { "name": "vi_epic", "direction": "out", "cardinality": "one", - "targetType": "vi_epic" + "targetType": "vi_epic", + "reverseName": "vi_children", + "reverseCardinality": "many" }]), ) .await; diff --git a/packages/core/src/schema/mod.rs b/packages/core/src/schema/mod.rs index d19d46406..568b31fb5 100644 --- a/packages/core/src/schema/mod.rs +++ b/packages/core/src/schema/mod.rs @@ -423,25 +423,42 @@ async fn validate_relationship_targets_exist( } /// Reject relationship declarations named after a built-in structural -/// relationship (`has_child`, `mentions`, `member_of`, `has_role`). +/// relationship (`has_child`, `mentions`, `member_of`, `has_role`) — in either +/// direction. /// -/// Declarations and the built-in primitives share the one `relationship` -/// table's `relationship_type` column, so a name collision would make every -/// type-keyed relationship query ambiguous — a correctness hazard, not just a -/// display glitch. Checked here so the error surfaces before any write, and -/// re-checked in `NodeService::set_schema_relationships` (the write path) via -/// the same shared predicate. +/// **Forward `name`.** Declarations and the built-in primitives share the one +/// `relationship` table's `relationship_type` column, so a name collision would +/// make every type-keyed relationship query ambiguous — a correctness hazard, +/// not just a display glitch. Checked here so the error surfaces before any +/// write, and re-checked in `NodeService::set_schema_relationships` (the write +/// path) via the same shared predicate. +/// +/// **`reverse_name`.** A different failure, so worth stating separately: a +/// reverse name is never written to `relationship_type` — it is a resolution +/// alias, matched by [`resolve_relationship_name`] to reach the inbound side of +/// an edge stored under the forward name. It therefore cannot make stored data +/// ambiguous. What it can do is nothing at all: that resolver short-circuits on +/// `BUILTIN_RELATIONSHIP_NAMES` before it ever consults a declaration, so +/// `reverseName: "has_child"` is unreachable — the built-in always wins, and +/// the reverse spelling the author chose silently resolves to something else. +/// Rejecting it keeps a declaration from being accepted as inert. +/// +/// Both halves are checked because a relationship must name its edge from both +/// ends, so both names land in a namespace a caller can traverse by. fn reject_reserved_relationship_names( relationships: &[crate::models::schema::SchemaRelationship], ) -> Result<(), MarkdownError> { for rel in relationships { - if crate::models::schema::is_builtin_relationship(&rel.name) { - return Err(MarkdownError::invalid_params(format!( - "Relationship name '{}' is reserved for a built-in structural relationship \ - ({}). Choose a different name.", - rel.name, - crate::models::schema::BUILTIN_RELATIONSHIP_NAMES.join(", ") - ))); + for (which, name) in [("name", &rel.name), ("reverseName", &rel.reverse_name)] { + if crate::models::schema::is_builtin_relationship(name) { + return Err(MarkdownError::invalid_params(format!( + "Relationship {} '{}' is reserved for a built-in structural relationship \ + ({}). Choose a different name.", + which, + name, + crate::models::schema::BUILTIN_RELATIONSHIP_NAMES.join(", ") + ))); + } } } Ok(()) @@ -541,6 +558,103 @@ fn validate_edge_field_declarations( Ok(()) } +/// Reject a relationship declaration that omits `reverseName` or +/// `reverseCardinality`, before serde reports it as a bare `missing field`. +/// +/// A relationship is declared once — one `relationship` row between the two +/// schema nodes — but read from both ends, so a declaration that names only +/// the forward half leaves the stored edge half-declared: the target's side +/// had to synthesize a label (`"Invoice (Customer)"` rather than `"Invoices"`) +/// and had no cardinality at all, so nothing downstream could say how many +/// sources may point at a node. Naming the inverse is a modeling decision only +/// the author can make, so it is required rather than derived. +/// +/// [`SchemaRelationship`](crate::models::schema::SchemaRelationship) carries +/// both as non-`Option` fields, which is what makes this an invariant every +/// reader can rely on. This check exists because that type-level guarantee +/// alone produces `missing field reverseName` — a message naming the key but +/// not what to put in it. Sibling of [`describe_missing_top_level_keys`]: same +/// reason, one level down. Reported per relationship (by name and array +/// position) so a caller repairing a multi-relationship payload knows which +/// entry to fix. +fn describe_missing_reverse_fields(relationships: &Value) -> Result<(), MarkdownError> { + let Some(entries) = relationships.as_array() else { + return Ok(()); + }; + + for (index, entry) in entries.iter().enumerate() { + let Some(obj) = entry.as_object() else { + // A non-object entry is a different mistake; serde reports it with + // a clearer type error than this function could add. + continue; + }; + + // Absent, null, and (for the name) blank are the same defect from the + // caller's side: the reverse half was not supplied. Treat them alike so + // the repair instruction is the same in every case. + let mut missing: Vec<&str> = Vec::new(); + match obj.get("reverseName") { + Some(Value::String(s)) if !s.trim().is_empty() => {} + _ => missing.push("reverseName"), + } + match obj.get("reverseCardinality") { + Some(Value::String(s)) if !s.trim().is_empty() => {} + _ => missing.push("reverseCardinality"), + } + if missing.is_empty() { + continue; + } + + // Reflect the caller's own forward declaration back in the example, so + // the fix is a two-key addition to the entry they already sent rather + // than a rewrite of it. + let name = obj + .get("name") + .and_then(Value::as_str) + .unwrap_or(""); + let direction = obj + .get("direction") + .and_then(Value::as_str) + .unwrap_or("out"); + let cardinality = obj + .get("cardinality") + .and_then(Value::as_str) + .unwrap_or("many"); + // `targetType` is legitimately omitted (an untyped relationship — the + // documented escape hatch for "the target type doesn't exist yet"), so + // the message has to read correctly without one rather than printing a + // placeholder where a type name belongs. + let target_type = obj.get("targetType").and_then(Value::as_str); + let target_phrase = match target_type { + Some(tt) => format!("a '{tt}'"), + None => "the target".to_string(), + }; + let target_type_key = match target_type { + Some(tt) => format!("\"targetType\":\"{tt}\","), + None => String::new(), + }; + let missing_phrase = match missing.as_slice() { + [one] => format!("\"{one}\""), + _ => "\"reverseName\" and \"reverseCardinality\"".to_string(), + }; + + return Err(MarkdownError::invalid_params(format!( + "Relationship '{name}' (entry {index}) is missing {missing_phrase}. Every relationship \ + must name the edge from BOTH ends: it is stored once and read from either side, \ + so the target's end needs its own name and cardinality. \"reverseName\" is what \ + this edge is called read from {target_phrase} — a name you choose, plural where \ + that end may hold many (\"invoices\", not \"Invoice (Customer)\") — and \ + \"reverseCardinality\" is \"one\" or \"many\", saying how many '{name}' sources \ + may point at one target. Corrected: \ + {{\"name\":\"{name}\",{target_type_key}\"direction\":\"{direction}\",\ + \"cardinality\":\"{cardinality}\",\"reverseName\":\"...\",\ + \"reverseCardinality\":\"many\"}}." + ))); + } + + Ok(()) +} + /// Name of a JSON value's type, for error messages. fn json_type_name(v: &Value) -> &'static str { match v { @@ -641,6 +755,13 @@ pub async fn handle_create_schema( // a missing `name` is a one-key addition to an otherwise-correct call. describe_missing_top_level_keys(¶ms)?; + // Before serde: a relationship missing its reverse half deserializes to a + // bare "missing field" naming the key but not what the reverse half is FOR + // or what value to choose. + if let Some(relationships) = params.get("relationships") { + describe_missing_reverse_fields(relationships)?; + } + // Runs AFTER both checks, not before: an entry whose every value is null // carries no information to validate, but removing it first renumbers the // array and makes the positions those checks report point at the wrong @@ -909,6 +1030,14 @@ pub async fn handle_update_schema( // only the absent key with no position. describe_malformed_fields(¶ms, "add_fields")?; + // Same reverse-half check `handle_create_schema` runs, on the key that + // carries new declarations here. A relationship added by an update is a new + // stored edge and must be named from both ends exactly as one declared at + // create time. + if let Some(relationships) = params.get("add_relationships") { + describe_missing_reverse_fields(relationships)?; + } + // `add_fields` is `fields` under another name and takes the same treatment: // the model appends an informationless `{"description":null,"name":null}` // here too, and failing an otherwise-correct batch over it drives the same @@ -1552,8 +1681,8 @@ mod tests { direction: RelationshipDirection::Out, cardinality: RelationshipCardinality::Many, required: None, - reverse_name: None, - reverse_cardinality: None, + reverse_name: "orgs".to_string(), + reverse_cardinality: RelationshipCardinality::Many, edge_fields: Some(edge_fields), description: None, } diff --git a/packages/core/src/schema/schema_test.rs b/packages/core/src/schema/schema_test.rs index 76d1a18d8..fee9c0fa4 100644 --- a/packages/core/src/schema/schema_test.rs +++ b/packages/core/src/schema/schema_test.rs @@ -2725,7 +2725,7 @@ async fn test_create_schema_rejects_relationship_to_nonexistent_target_type() { { "name": "amount", "type": "number", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one" } + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } ] }), ) @@ -2755,7 +2755,7 @@ async fn test_create_schema_with_relationship_to_existing_target_type_succeeds() { "name": "amount", "type": "number", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one" } + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } ] }), ) @@ -2783,7 +2783,7 @@ async fn test_create_schema_relationship_with_no_target_type_succeeds() { { "name": "amount", "type": "number", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "billed_to", "direction": "out", "cardinality": "one" } + { "name": "billed_to", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } ] }), ) @@ -2807,7 +2807,7 @@ async fn test_update_schema_add_relationships_rejects_nonexistent_target_type() json!({ "schema_id": invoice_id, "add_relationships": [ - { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one" } + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } ] }), ) @@ -2835,7 +2835,7 @@ async fn test_update_schema_add_relationships_to_existing_target_type_succeeds() json!({ "schema_id": invoice_id, "add_relationships": [ - { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one" } + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } ] }), ) @@ -2848,6 +2848,281 @@ async fn test_update_schema_add_relationships_to_existing_target_type_succeeds() ); } +// ============================================================================ +// required reverse fields +// ============================================================================ +// +// A relationship is stored once — one row between the two schema nodes — but +// read from both ends, so a declaration naming only the forward half leaves +// the stored edge half-declared: the target's side had to synthesize a label +// ("Invoice (Customer)" where the modeled answer is "Invoices") and carried no +// cardinality at all. Naming the inverse is a modeling decision only the +// author can make, so both halves are required rather than derived. + +/// The corrected example matters as much as the rejection: an agent repairing +/// the call has only the error text to work from, so it must carry a payload +/// that would be accepted. +fn assert_reverse_field_error(msg: &str, expect_named: &[&str]) { + for field in expect_named { + assert!( + msg.contains(field), + "error must name the missing field '{field}': {msg}" + ); + } + assert!( + msg.contains("Corrected:"), + "error must show a corrected example: {msg}" + ); + assert!( + msg.contains("\"reverseName\":\"...\"") && msg.contains("\"reverseCardinality\":\"many\""), + "the corrected example must include both reverse keys: {msg}" + ); +} + +#[tokio::test] +async fn create_schema_rejects_a_relationship_without_reverse_name() { + let (svc, _tmp) = create_test_service().await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + + let err = handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [ + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseCardinality": "many" } + ] + }), + ) + .await + .expect_err("a relationship without reverseName must be rejected"); + + let msg = err.to_string(); + assert_reverse_field_error(&msg, &["reverseName", "billed_to"]); + assert!( + !msg.contains("\"reverseCardinality\" and"), + "only the genuinely missing field should be reported: {msg}" + ); + // The rejection must not leave a half-created schema behind. + assert!( + svc.get_schema_node("invoice").await.unwrap().is_none(), + "a rejected create must persist no schema node" + ); +} + +#[tokio::test] +async fn create_schema_rejects_a_relationship_without_reverse_cardinality() { + let (svc, _tmp) = create_test_service().await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + + let err = handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [ + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices" } + ] + }), + ) + .await + .expect_err("a relationship without reverseCardinality must be rejected"); + + assert_reverse_field_error(&err.to_string(), &["reverseCardinality", "billed_to"]); + assert!(svc.get_schema_node("invoice").await.unwrap().is_none()); +} + +/// A null or blank value is the same defect as an absent key from the caller's +/// side, and must produce the same actionable error rather than being stored +/// as an empty reverse name. +#[tokio::test] +async fn create_schema_rejects_null_or_blank_reverse_fields() { + let (svc, _tmp) = create_test_service().await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + + for reverse_name in [json!(null), json!(" ")] { + let err = handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [{ + "name": "billed_to", + "targetType": "customer", + "direction": "out", + "cardinality": "one", + "reverseName": reverse_name, + "reverseCardinality": "many" + }] + }), + ) + .await + .expect_err("a null or blank reverseName must be rejected"); + + assert_reverse_field_error(&err.to_string(), &["reverseName"]); + } +} + +/// The error names the offending entry by position, so a caller repairing a +/// multi-relationship payload knows which one to fix. +#[tokio::test] +async fn create_schema_names_the_offending_relationship_entry() { + let (svc, _tmp) = create_test_service().await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + create_base_schema(&svc, "Vendor", &["name"]).await; + + let err = handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [ + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" }, + { "name": "supplied_by", "targetType": "vendor", "direction": "out", "cardinality": "one" } + ] + }), + ) + .await + .expect_err("the second relationship is missing both reverse fields"); + + let msg = err.to_string(); + assert!( + msg.contains("supplied_by") && msg.contains("entry 1"), + "error must identify the offending entry by name and position: {msg}" + ); + assert!( + !msg.contains("billed_to"), + "the well-formed relationship must not be implicated: {msg}" + ); +} + +/// An untyped relationship (no `targetType`) is a documented shape, so the +/// message must read correctly without a target type name rather than printing +/// a placeholder where one belongs. +#[tokio::test] +async fn create_schema_reverse_error_reads_correctly_without_a_target_type() { + let (svc, _tmp) = create_test_service().await; + + let err = handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [ + { "name": "related", "direction": "out", "cardinality": "many" } + ] + }), + ) + .await + .expect_err("a relationship without reverse fields must be rejected"); + + let msg = err.to_string(); + assert_reverse_field_error(&msg, &["reverseName", "reverseCardinality"]); + assert!( + !msg.contains(""), + "the message must not print a placeholder where a type name belongs: {msg}" + ); + assert!( + !msg.contains("\"targetType\""), + "the corrected example must omit targetType when the caller sent none: {msg}" + ); +} + +#[tokio::test] +async fn update_schema_rejects_an_added_relationship_without_reverse_fields() { + let (svc, _tmp) = create_test_service().await; + let invoice_id = create_base_schema(&svc, "Invoice", &["amount"]).await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + + let err = handle_update_schema( + &svc, + json!({ + "schema_id": invoice_id, + "add_relationships": [ + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one" } + ] + }), + ) + .await + .expect_err("update_schema must apply the same reverse-field rule as create"); + + assert_reverse_field_error(&err.to_string(), &["reverseName", "reverseCardinality"]); + + // The rejection leaves the schema untouched — no partial declaration. + let schema = svc + .get_schema_node("invoice") + .await + .unwrap() + .expect("invoice schema"); + assert!( + schema.relationships.is_empty(), + "a rejected add_relationships must declare nothing" + ); +} + +/// The end-to-end property the requirement exists for: a relationship declared +/// with both fields is readable from the TARGET's end by the declared +/// `reverseName`, with the declared `reverseCardinality` reported as the +/// inbound group's cardinality. +#[tokio::test] +async fn declared_reverse_fields_govern_the_inbound_group() { + let (svc, _tmp) = create_test_service().await; + create_base_schema(&svc, "Customer", &["first_name"]).await; + + handle_create_schema( + &svc, + json!({ + "name": "Invoice", + "fields": [{ "name": "amount", "type": "number" }], + "relationships": [ + { "name": "billed_to", "targetType": "customer", "direction": "out", "cardinality": "one", "reverseName": "invoices", "reverseCardinality": "many" } + ] + }), + ) + .await + .expect("a fully declared relationship must be accepted"); + + for (id, node_type) in [("c1", "customer"), ("i1", "invoice")] { + svc.store() + .create_node( + crate::models::Node::new_with_id( + id.to_string(), + node_type.to_string(), + id.to_string(), + json!({}), + ), + None, + None, + ) + .await + .unwrap(); + } + svc.create_relationship("i1", "billed_to", "c1", json!({})) + .await + .unwrap(); + + let inbound = crate::ops::rel_ops::get_node_relationships(&svc, "c1") + .await + .unwrap(); + let group = inbound + .groups + .iter() + .find(|g| g.relationship_name == "billed_to" && g.direction == "in") + .expect("the customer must see the inbound group"); + + assert_eq!( + group.reverse_name, "invoices", + "the inbound group is labeled by the declared reverseName, not a synthesized name" + ); + assert_eq!( + group.cardinality, + crate::models::schema::RelationshipCardinality::Many, + "the declared reverseCardinality governs the inbound side" + ); + assert_eq!(group.count, 1); + assert_eq!(group.related[0].id, "i1"); +} + // ============================================================================ // self-referential relationships // ============================================================================ @@ -2871,7 +3146,7 @@ async fn test_create_schema_accepts_self_referential_relationship() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -2908,8 +3183,8 @@ async fn test_create_schema_accepts_self_reference_alongside_existing_target() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" }, - { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "many" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" }, + { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "many", "reverseName": "decided_by", "reverseCardinality": "one" } ] }), ) @@ -2941,8 +3216,8 @@ async fn test_create_schema_self_reference_reverse_edge_resolves() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" }, - { "name": "superseded_by", "targetType": "adr", "direction": "in", "cardinality": "one" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" }, + { "name": "superseded_by", "targetType": "adr", "direction": "in", "cardinality": "one", "reverseName": "supersedes", "reverseCardinality": "one" } ] }), ) @@ -2985,7 +3260,7 @@ async fn test_create_schema_self_reference_uses_normalized_schema_id() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "design_decision", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "design_decision", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -3022,7 +3297,7 @@ async fn test_create_schema_self_reference_by_display_name_is_rejected() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "Design Decision", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "Design Decision", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -3058,7 +3333,7 @@ async fn test_punctuation_only_name_does_not_make_the_exemption_vacuous() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -3092,8 +3367,8 @@ async fn test_self_referential_declaration_writes_a_self_loop_row() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" }, - { "name": "superseded_by", "targetType": "adr", "direction": "in", "cardinality": "one" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" }, + { "name": "superseded_by", "targetType": "adr", "direction": "in", "cardinality": "one", "reverseName": "supersedes", "reverseCardinality": "one" } ] }), ) @@ -3158,7 +3433,7 @@ async fn test_self_reference_does_not_bypass_the_already_exists_check() { { "name": "status", "type": "string", "protection": "user", "indexed": false } ], "relationships": [ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -3182,7 +3457,7 @@ async fn test_update_schema_accepts_self_referential_relationship() { json!({ "schema_id": adr_id, "add_relationships": [ - { "name": "supersedes", "targetType": adr_id, "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": adr_id, "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ] }), ) @@ -3222,29 +3497,29 @@ async fn test_rejected_create_persists_no_schema_node() { let cases: Vec<(serde_json::Value, &str, &str)> = vec![ ( json!([ - { "name": "has_child", "targetType": "adr", "direction": "out", "cardinality": "many" } + { "name": "has_child", "targetType": "adr", "direction": "out", "cardinality": "many", "reverseName": "parent", "reverseCardinality": "one" } ]), "reserved built-in relationship name", "reserved for a built-in structural relationship", ), ( json!([ - { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "one" } + { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "one", "reverseName": "decided_by", "reverseCardinality": "one" } ]), "target type that does not exist", "targets 'ticket', which is not an existing schema", ), ( json!([ - { "name": "supersedes", "targetType": "adrr", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "adrr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" } ]), "typo'd targetType that is neither the pending schema nor an existing one", "targets 'adrr', which is not an existing schema", ), ( json!([ - { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one" }, - { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "one" } + { "name": "supersedes", "targetType": "adr", "direction": "out", "cardinality": "one", "reverseName": "superseded_by", "reverseCardinality": "one" }, + { "name": "decides", "targetType": "ticket", "direction": "out", "cardinality": "one", "reverseName": "decided_by", "reverseCardinality": "one" } ]), "a valid self-reference batched with an invalid target", "targets 'ticket', which is not an existing schema", @@ -3559,6 +3834,8 @@ fn rel_with_enum_edge_field(core_values: serde_json::Value) -> serde_json::Value "targetType": "widget", "direction": "out", "cardinality": "many", + "reverseName": "grants", + "reverseCardinality": "many", "edgeFields": [{ "name": "access", "type": "enum", "coreValues": core_values }] }) } @@ -3602,6 +3879,8 @@ async fn create_schema_rejects_an_enum_edge_field_without_core_values() { "targetType": "widget", "direction": "out", "cardinality": "many", + "reverseName": "grants", + "reverseCardinality": "many", "edgeFields": [{ "name": "access", "type": "enum" }] }] }), @@ -3631,6 +3910,8 @@ async fn create_schema_rejects_an_enum_edge_field_default_outside_the_set() { "targetType": "widget", "direction": "out", "cardinality": "many", + "reverseName": "grants", + "reverseCardinality": "many", "edgeFields": [{ "name": "access", "type": "enum", @@ -3663,6 +3944,8 @@ async fn update_schema_rejects_an_enum_edge_field_without_core_values() { "targetType": "widget", "direction": "out", "cardinality": "many", + "reverseName": "grants", + "reverseCardinality": "many", "edgeFields": [{ "name": "access", "type": "enum" }] }] }), @@ -3719,6 +4002,8 @@ async fn schema_declaring_an_edge_field_on_a_builtin_relationship_is_rejected() "targetType": "collection", "direction": "out", "cardinality": "many", + "reverseName": "members", + "reverseCardinality": "many", "edgeFields": [{ "name": "access", "type": "enum", diff --git a/packages/core/src/services/node_service/mod.rs b/packages/core/src/services/node_service/mod.rs index 4f157f5e5..85646246a 100644 --- a/packages/core/src/services/node_service/mod.rs +++ b/packages/core/src/services/node_service/mod.rs @@ -4928,7 +4928,7 @@ mod tests { .iter() .find(|g| g.relationship_name == "assigned_to" && g.direction == "in") .expect("inbound assigned_to group present"); - assert_eq!(group.reverse_name.as_deref(), Some("gadgets")); + assert_eq!(group.reverse_name, "gadgets"); assert_eq!(group.source_type, "gadget"); assert_eq!(group.target_type.as_deref(), Some("gadget")); assert_eq!(group.count, 1); @@ -4987,7 +4987,8 @@ mod tests { "targetType": "widget", "direction": "out", "cardinality": "many", - "reverseName": "gadgets" + "reverseName": "gadgets", + "reverseCardinality": "many" }])) .unwrap(); service @@ -6133,6 +6134,8 @@ mod tests { "targetType": "widget", "direction": "out", "cardinality": "many", + "reverseName": "gadgets", + "reverseCardinality": "many", "edgeFields": [ { "name": "role", diff --git a/packages/core/src/services/node_service/schema.rs b/packages/core/src/services/node_service/schema.rs index f383fd8d8..f28ec1729 100644 --- a/packages/core/src/services/node_service/schema.rs +++ b/packages/core/src/services/node_service/schema.rs @@ -779,13 +779,20 @@ impl NodeService { relationships: &[crate::models::schema::SchemaRelationship], ) -> Result<(), NodeServiceError> { for rel in relationships { - if crate::models::schema::is_builtin_relationship(&rel.name) { - return Err(NodeServiceError::invalid_update(format!( - "Relationship name '{}' is reserved for a built-in structural relationship \ - ({}); choose a different name", - rel.name, - crate::models::schema::BUILTIN_RELATIONSHIP_NAMES.join(", ") - ))); + // Both names, for the two distinct reasons spelled out on + // `reject_reserved_relationship_names`: a reserved forward name + // makes stored edges ambiguous, a reserved reverse name makes the + // declaration silently unreachable. + for (which, name) in [("name", &rel.name), ("reverseName", &rel.reverse_name)] { + if crate::models::schema::is_builtin_relationship(name) { + return Err(NodeServiceError::invalid_update(format!( + "Relationship {} '{}' is reserved for a built-in structural relationship \ + ({}); choose a different name", + which, + name, + crate::models::schema::BUILTIN_RELATIONSHIP_NAMES.join(", ") + ))); + } } } diff --git a/packages/core/tests/relationship_editing_test.rs b/packages/core/tests/relationship_editing_test.rs index 984d1ca83..27e513117 100644 --- a/packages/core/tests/relationship_editing_test.rs +++ b/packages/core/tests/relationship_editing_test.rs @@ -47,7 +47,9 @@ async fn service_with_gizmo_schema( "targetType": "person", "direction": "out", "cardinality": cardinality, - "required": required + "required": required, + "reverseName": "gizmos", + "reverseCardinality": "many" } ] }), diff --git a/packages/core/tests/relationship_reverse_name_traversal_test.rs b/packages/core/tests/relationship_reverse_name_traversal_test.rs index bc7f18f6b..e6bf1f6aa 100644 --- a/packages/core/tests/relationship_reverse_name_traversal_test.rs +++ b/packages/core/tests/relationship_reverse_name_traversal_test.rs @@ -347,9 +347,9 @@ async fn untyped_reverse_name_resolves_only_where_edges_reach() -> Result<()> { } /// Two schemas may declare the same forward name toward one type. The store's -/// "in" query keys on the name alone, so a reverse name — declared by exactly -/// one of them — must be narrowed to its own declaring type rather than -/// sweeping in the other schema's edges and over-reporting the count. +/// "in" query keys on the name alone, so each schema's reverse name must be +/// narrowed to its own declaring type rather than sweeping in the other +/// schema's edges and over-reporting the count. #[tokio::test] async fn reverse_name_is_scoped_to_its_declaring_type() -> Result<()> { let (svc, _t) = create_test_service().await?; @@ -363,19 +363,19 @@ async fn reverse_name_is_scoped_to_its_declaring_type() -> Result<()> { .await .map_err(|e| anyhow::anyhow!("reviewer schema: {e}"))?; - // Both `adr` and `memo` declare `decided_by → reviewer`; only `adr` names the - // reverse `decisions`. - for (name, reverse) in [("Adr", Some("decisions")), ("Memo", None)] { - let mut rel = json!({ + // Both `adr` and `memo` declare `decided_by → reviewer`, each naming its own + // reverse. Since every declaration must name the edge from both ends, the + // collision this test guards is now between two live reverse names sharing + // one forward name — not between a named and an unnamed one. + for (name, reverse) in [("Adr", "decisions"), ("Memo", "memos")] { + let rel = json!({ "name": "decided_by", "targetType": "reviewer", "direction": "out", - "cardinality": "one" + "cardinality": "one", + "reverseName": reverse, + "reverseCardinality": "many" }); - if let Some(reverse) = reverse { - rel["reverseName"] = json!(reverse); - rel["reverseCardinality"] = json!("many"); - } handle_create_schema( &svc, json!({ @@ -405,6 +405,17 @@ async fn reverse_name_is_scoped_to_its_declaring_type() -> Result<()> { by_reverse.related_nodes[0]["id"], "adr1", "wrong declaring type surfaced under the reverse name" ); + + // The symmetric case: memo's own reverse name resolves only to memo's edge. + let by_memo_reverse = rel_ops::get_related_nodes(&svc, get("p1", "memos", "out")).await?; + assert_eq!( + by_memo_reverse.count, 1, + "`memos` is memo's reverse name — it must not also collect adr's edges" + ); + assert_eq!( + by_memo_reverse.related_nodes[0]["id"], "memo1", + "wrong declaring type surfaced under the reverse name" + ); Ok(()) } diff --git a/packages/core/tests/schema_relationship_declarations_test.rs b/packages/core/tests/schema_relationship_declarations_test.rs index db96dd59a..e0320f9c9 100644 --- a/packages/core/tests/schema_relationship_declarations_test.rs +++ b/packages/core/tests/schema_relationship_declarations_test.rs @@ -106,7 +106,7 @@ async fn declarations_are_relationship_rows_not_properties_json() -> Result<()> let rel = &declarations[0]; assert_eq!(rel.name, "widgets"); assert_eq!(rel.target_type.as_deref(), Some("widget")); - assert_eq!(rel.reverse_name.as_deref(), Some("assemblies")); + assert_eq!(rel.reverse_name, "assemblies"); Ok(()) } @@ -118,7 +118,7 @@ async fn untyped_declaration_stores_as_self_edge_and_round_trips() -> Result<()> json!({ "name": "Notebook", "fields": [], - "relationships": [{ "name": "related", "direction": "out", "cardinality": "many" }] + "relationships": [{ "name": "related", "direction": "out", "cardinality": "many", "reverseName": "related_from", "reverseCardinality": "many" }] }), ) .await @@ -251,7 +251,7 @@ async fn reserved_builtin_names_are_rejected_at_declaration_time() -> Result<()> json!({ "name": "Bad", "fields": [], - "relationships": [{ "name": "has_child", "direction": "out", "cardinality": "many" }] + "relationships": [{ "name": "has_child", "direction": "out", "cardinality": "many", "reverseName": "parent", "reverseCardinality": "one" }] }), ) .await @@ -271,7 +271,7 @@ async fn reserved_builtin_names_are_rejected_at_declaration_time() -> Result<()> &svc, json!({ "schema_id": "good", - "add_relationships": [{ "name": "mentions", "direction": "out", "cardinality": "many" }] + "add_relationships": [{ "name": "mentions", "direction": "out", "cardinality": "many", "reverseName": "mentioned_by", "reverseCardinality": "many" }] }), ) .await @@ -280,6 +280,95 @@ async fn reserved_builtin_names_are_rejected_at_declaration_time() -> Result<()> Ok(()) } +/// A reserved name is rejected as a `reverseName` too, not only as the forward +/// `name`. +/// +/// The failure differs from the forward case and is easy to wave off as +/// harmless: a reverse name is never written to `relationship_type` — it is a +/// resolution alias — so it cannot make stored edges ambiguous the way a +/// reserved forward name does. What it does instead is nothing at all. +/// `resolve_relationship_name` short-circuits on the built-in names before it +/// consults any declaration, so a `reverseName` of `has_child` can never +/// resolve to this relationship; the built-in wins and the author's chosen +/// reverse spelling is silently inert. +/// +/// This matters more now that every relationship must carry a reverse name: +/// what used to be a sparsely-populated opt-in namespace now gains an entry per +/// declaration. +#[tokio::test] +async fn reserved_builtin_names_are_rejected_as_reverse_names() -> Result<()> { + let (svc, _t) = create_test_service().await?; + + let err = handle_create_schema( + &svc, + json!({ + "name": "Folder", + "fields": [], + "relationships": [{ + "name": "contains", + "direction": "out", + "cardinality": "many", + "reverseName": "has_child", + "reverseCardinality": "one" + }] + }), + ) + .await + .expect_err("a reserved reverseName must be rejected"); + assert!( + err.to_string().contains("reverseName"), + "the error must say WHICH end is at fault, so the fix is unambiguous: {err}" + ); + assert!( + err.to_string().contains("has_child"), + "error should name the reserved relationship: {err}" + ); + // The rejection must not leave a half-created schema behind. + assert!(svc.get_schema_node("folder").await?.is_none()); + + // Same via update_schema on an existing schema. + handle_create_schema(&svc, json!({ "name": "Tray", "fields": [] })) + .await + .map_err(|e| anyhow::anyhow!("{e}"))?; + let err = handle_update_schema( + &svc, + json!({ + "schema_id": "tray", + "add_relationships": [{ + "name": "holds", + "direction": "out", + "cardinality": "many", + "reverseName": "member_of", + "reverseCardinality": "one" + }] + }), + ) + .await + .expect_err("update_schema must reject a reserved reverseName"); + assert!(err.to_string().contains("member_of"), "got: {err}"); + + // A non-reserved reverse name on the same shape is still accepted — the + // guard must reject the reserved word, not the direction. + handle_create_schema( + &svc, + json!({ + "name": "Crate", + "fields": [], + "relationships": [{ + "name": "contains", + "direction": "out", + "cardinality": "many", + "reverseName": "contained_by", + "reverseCardinality": "one" + }] + }), + ) + .await + .map_err(|e| anyhow::anyhow!("a non-reserved reverseName must still be accepted: {e}"))?; + + Ok(()) +} + // --------------------------------------------------------------------------- // Block-by-default protection // --------------------------------------------------------------------------- @@ -343,7 +432,9 @@ async fn retargeting_a_declaration_with_live_edges_is_blocked() -> Result<()> { "name": "widgets", "targetType": "gear", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "assemblies", + "reverseCardinality": "one" }] }), ) @@ -423,7 +514,7 @@ async fn create_relationship_refuses_schema_node_endpoints() -> Result<()> { json!({ "name": "Board", "fields": [], - "relationships": [{ "name": "pins", "direction": "out", "cardinality": "many" }] + "relationships": [{ "name": "pins", "direction": "out", "cardinality": "many", "reverseName": "pinned_on", "reverseCardinality": "many" }] }), ) .await diff --git a/packages/desktop-app/src/lib/services/relationship-grouping.ts b/packages/desktop-app/src/lib/services/relationship-grouping.ts index eb4c2f3ca..b4701a52e 100644 --- a/packages/desktop-app/src/lib/services/relationship-grouping.ts +++ b/packages/desktop-app/src/lib/services/relationship-grouping.ts @@ -9,9 +9,8 @@ * Responsibilities: * - Keep outbound and inbound as SEPARATE groups (never flatten different * directions / target types together). - * - Derive each group's display label — inbound uses `reverseName`, falling back - * to `"{SourceType} ({relationship_name})"` when it is absent, rather than - * hiding the group. + * - Derive each group's display label — outbound uses the relationship name, + * inbound uses the schema-declared `reverseName`. * - Classify each group as a `table` (carries edge attributes) or `chips` * (bare edge with no edge data) layout. * - Compute the ordered edge-attribute column set for the table layout. @@ -58,9 +57,9 @@ export interface RawRelationshipGroup { relationshipName: string; direction: RelationshipDirection; targetType: string | null; - reverseName: string | null; + reverseName: string; sourceType: string; - cardinality: RelationshipCardinality | null; + cardinality: RelationshipCardinality; required: boolean | null; edgeFields: RawEdgeField[] | null; description: string | null; @@ -138,18 +137,18 @@ export function humanizeName(name: string): string { /** * Display label for a group. * - Outbound: the humanized relationship name. - * - Inbound: the humanized `reverseName` when present, else a derived - * `"{SourceType} ({Relationship Name})"` fallback — never hide the group, - * since that would make the edge invisible from one side. + * - Inbound: the humanized `reverseName`. + * + * There is no synthesized `"{SourceType} ({Relationship Name})"` fallback: a + * schema relationship must declare `reverseName`, so every inbound group + * carries a name its author chose. The fallback was what produced + * "Invoice (Customer)" where the modeled answer was "Invoices", and keeping it + * as a defensive branch would only hide a schema that failed validation. */ export function groupDisplayLabel(group: RawRelationshipGroup): string { - if (group.direction === 'out') { - return humanizeName(group.relationshipName); - } - if (group.reverseName && group.reverseName.trim().length > 0) { - return humanizeName(group.reverseName); - } - return `${humanizeName(group.sourceType)} (${humanizeName(group.relationshipName)})`; + return humanizeName( + group.direction === 'out' ? group.relationshipName : group.reverseName + ); } /** diff --git a/packages/desktop-app/src/tests/unit/relationship-grouping.test.ts b/packages/desktop-app/src/tests/unit/relationship-grouping.test.ts index 7cffadcb8..457bf642a 100644 --- a/packages/desktop-app/src/tests/unit/relationship-grouping.test.ts +++ b/packages/desktop-app/src/tests/unit/relationship-grouping.test.ts @@ -18,7 +18,7 @@ function makeGroup(overrides: Partial = {}): RawRelationsh relationshipName: 'assigned_to', direction: 'out', targetType: 'person', - reverseName: null, + reverseName: 'tasks', sourceType: 'task', cardinality: 'many', required: null, @@ -49,7 +49,7 @@ describe('relationship-grouping: groupDisplayLabel', () => { ); }); - it('uses reverseName for inbound groups when present', () => { + it('uses reverseName for inbound groups', () => { const group = makeGroup({ direction: 'in', relationshipName: 'assigned_to', @@ -59,14 +59,17 @@ describe('relationship-grouping: groupDisplayLabel', () => { expect(groupDisplayLabel(group)).toBe('Tasks'); }); - it('falls back to "{SourceType} ({Relationship Name})" when reverseName is absent', () => { + it('never synthesizes a label from the source type, since reverseName is declared', () => { + // The schema layer requires reverseName on every relationship, so the + // inbound label is always the author's chosen name — "Invoices", not the + // old synthesized "Invoice (Customer)". const group = makeGroup({ direction: 'in', - relationshipName: 'assigned_to', - reverseName: null, - sourceType: 'task' + relationshipName: 'billed_to', + reverseName: 'invoices', + sourceType: 'invoice' }); - expect(groupDisplayLabel(group)).toBe('Task (Assigned To)'); + expect(groupDisplayLabel(group)).toBe('Invoices'); }); }); diff --git a/packages/nodespace-types/src/schema.rs b/packages/nodespace-types/src/schema.rs index 433d98701..7c16765da 100644 --- a/packages/nodespace-types/src/schema.rs +++ b/packages/nodespace-types/src/schema.rs @@ -245,10 +245,36 @@ pub struct SchemaRelationship { pub cardinality: RelationshipCardinality, #[serde(skip_serializing_if = "Option::is_none")] pub required: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub reverse_name: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub reverse_cardinality: Option, + /// The name this edge reads by from the target's end. REQUIRED. + /// + /// A relationship is declared once — the storage model keeps a single + /// `relationship` row between the two schema nodes — but it is read from + /// both ends. Leaving this unset left that one stored edge only + /// half-declared: the target's side fell back to a synthesized + /// `"{SourceType} ({Relationship Name})"` label, so an invoice declaring + /// `billed_to → customer` surfaced on the customer as + /// "Invoice (Customer)" rather than "Invoices". + /// + /// Naming the inverse is a modeling decision only the author can make, so + /// it is required rather than derived. See the type-level note on + /// [`SchemaRelationship::reverse_cardinality`] for why both live in the + /// type rather than in a validator alone. + pub reverse_name: String, + /// The cardinality governing the target's end — how many sources may point + /// at one target. REQUIRED, and the counterpart to + /// [`SchemaRelationship::reverse_name`]. + /// + /// Unset, the inbound group carried no cardinality at all, so nothing + /// downstream could reason about how many sources may point at a node. + /// + /// Carrying both halves in the type — not merely in a validator — is what + /// makes "every stored edge is named from both ends" an invariant every + /// reader can rely on instead of a convention that holds only on the paths + /// which happen to route through validation. `handle_create_schema` and + /// `handle_update_schema` reject a payload missing either field before + /// serde sees it, so a caller gets an actionable error naming the field + /// rather than a bare `missing field` message. + pub reverse_cardinality: RelationshipCardinality, #[serde(skip_serializing_if = "Option::is_none")] pub edge_fields: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -966,8 +992,8 @@ mod tests { direction: RelationshipDirection::Out, cardinality: RelationshipCardinality::One, required: Some(true), - reverse_name: Some("invoices".to_string()), - reverse_cardinality: Some(RelationshipCardinality::Many), + reverse_name: "invoices".to_string(), + reverse_cardinality: RelationshipCardinality::Many, edge_fields: Some(vec![ EdgeField { name: "billing_date".to_string(), @@ -1036,10 +1062,10 @@ mod tests { assert_eq!(relationship.target_type, Some("person".to_string())); assert_eq!(relationship.direction, RelationshipDirection::Out); assert_eq!(relationship.cardinality, RelationshipCardinality::Many); - assert_eq!(relationship.reverse_name, Some("tasks".to_string())); + assert_eq!(relationship.reverse_name, "tasks"); assert_eq!( relationship.reverse_cardinality, - Some(RelationshipCardinality::Many) + RelationshipCardinality::Many ); assert!(relationship.required.is_none()); @@ -1051,12 +1077,15 @@ mod tests { #[test] fn test_schema_relationship_minimal() { - // Test minimal relationship (only required fields) + // The smallest legal declaration: the reverse half is part of it, so a + // "minimal" relationship still names the edge from both ends. let json = json!({ "name": "parent_of", "targetType": "document", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "children_of", + "reverseCardinality": "one" }); let relationship: SchemaRelationship = serde_json::from_value(json).unwrap(); @@ -1065,13 +1094,53 @@ mod tests { assert_eq!(relationship.target_type, Some("document".to_string())); assert_eq!(relationship.direction, RelationshipDirection::Out); assert_eq!(relationship.cardinality, RelationshipCardinality::Many); + assert_eq!(relationship.reverse_name, "children_of"); + assert_eq!( + relationship.reverse_cardinality, + RelationshipCardinality::One + ); assert!(relationship.required.is_none()); - assert!(relationship.reverse_name.is_none()); - assert!(relationship.reverse_cardinality.is_none()); assert!(relationship.edge_fields.is_none()); assert!(relationship.description.is_none()); } + /// The reverse half is carried by the type, not merely by a validator. + /// + /// The schema-op validators produce the actionable, example-bearing error a + /// caller acts on; this asserts the floor beneath them — a payload missing + /// either field cannot become a `SchemaRelationship` at all, so no other + /// deserialization path can smuggle in a half-named edge. + #[test] + fn test_schema_relationship_requires_both_reverse_fields() { + let missing_name = json!({ + "name": "parent_of", + "targetType": "document", + "direction": "out", + "cardinality": "many", + "reverseCardinality": "one" + }); + let err = serde_json::from_value::(missing_name) + .expect_err("reverseName must be required"); + assert!( + err.to_string().contains("reverseName"), + "error should name the missing field, got: {err}" + ); + + let missing_cardinality = json!({ + "name": "parent_of", + "targetType": "document", + "direction": "out", + "cardinality": "many", + "reverseName": "children_of" + }); + let err = serde_json::from_value::(missing_cardinality) + .expect_err("reverseCardinality must be required"); + assert!( + err.to_string().contains("reverseCardinality"), + "error should name the missing field, got: {err}" + ); + } + #[test] fn test_schema_relationship_incoming_direction() { // Test "in" direction (less common but valid) @@ -1079,7 +1148,9 @@ mod tests { "name": "owned_by", "targetType": "organization", "direction": "in", - "cardinality": "one" + "cardinality": "one", + "reverseName": "owns", + "reverseCardinality": "many" }); let relationship: SchemaRelationship = serde_json::from_value(json).unwrap(); @@ -1092,7 +1163,9 @@ mod tests { let json = json!({ "name": "related", "direction": "out", - "cardinality": "many" + "cardinality": "many", + "reverseName": "related_from", + "reverseCardinality": "many" }); let relationship: SchemaRelationship = serde_json::from_value(json).unwrap(); @@ -1108,8 +1181,8 @@ mod tests { direction: RelationshipDirection::Out, cardinality: RelationshipCardinality::Many, required: None, - reverse_name: None, - reverse_cardinality: None, + reverse_name: "related_from".to_string(), + reverse_cardinality: RelationshipCardinality::Many, edge_fields: None, description: None, }; @@ -1240,7 +1313,13 @@ mod tests { #[test] fn test_parse_relationships_valid_json_no_diagnostic() { let json = json!({ - "relationships": [{ "name": "assigned_to", "direction": "out", "cardinality": "one" }] + "relationships": [{ + "name": "assigned_to", + "direction": "out", + "cardinality": "one", + "reverseName": "tasks", + "reverseCardinality": "many" + }] }); let (relationships, diagnostic) = parse_relationships(&json, "test-schema-id"); diff --git a/packages/skill/references/cli.md b/packages/skill/references/cli.md index 07f9d99f3..3ae2a599d 100644 --- a/packages/skill/references/cli.md +++ b/packages/skill/references/cli.md @@ -255,7 +255,7 @@ nodespace relationship get --type has_task --direction out nodespace relationship get --type billed_to --direction in # Traverse the reverse direction by the schema's declared reverseName -# (adr declares: name decided_by, targetType person, reverseName decisions) +# (adr declares: name decided_by, targetType person, reverseName decisions, reverseCardinality many) nodespace relationship get --type decisions nodespace relationship get --type decided_by --direction in # equivalent ``` @@ -271,7 +271,7 @@ nodespace relationship get --type decided_by --direction in # equi - `--type ` — relationship name to traverse: the forward `name` from the source's end, or the declared `reverseName` from the target's end - `--direction ` — traversal direction (default: `out`), relative to the name given -**Traversing the reverse direction.** A relationship is declared once, on the source type, but reads from both ends. Given `{"name":"decided_by","targetType":"person","reverseName":"decisions"}` on `adr`: from the ADR, `nodespace relationship get --type decided_by --direction out`; from the person, use the declared `reverseName` — `nodespace relationship get --type decisions` — or the equivalent `--type decided_by --direction in`. Both spellings return the same ADRs, and the output line's arrow shows the direction actually traversed (`<--decided_by--` for an inbound resolution). An empty result means no edges exist, not that reverse traversal is unsupported. A name declared in neither direction is rejected with an error naming the spellings that do work — read it and retry rather than concluding the capability is missing. +**Traversing the reverse direction.** A relationship is declared once, on the source type, but reads from both ends. Given `{"name":"decided_by","targetType":"person","direction":"out","cardinality":"one","reverseName":"decisions","reverseCardinality":"many"}` on `adr`: from the ADR, `nodespace relationship get --type decided_by --direction out`; from the person, use the declared `reverseName` — `nodespace relationship get --type decisions` — or the equivalent `--type decided_by --direction in`. Both spellings return the same ADRs, and the output line's arrow shows the direction actually traversed (`<--decided_by--` for an inbound resolution). An empty result means no edges exist, not that reverse traversal is unsupported. A name declared in neither direction is rejected with an error naming the spellings that do work — read it and retry rather than concluding the capability is missing. Reverse names are for *traversal*, not for `relationship create`: an edge is always created under its forward name, from the source node. They are also not usable in `node query --filters`, whose `relationship` filters cover only the structural graph (`parent`, `children`, `mentions`, `mentioned_by`) — use `relationship get` to traverse a schema-declared name. @@ -298,7 +298,7 @@ nodespace schema get task nodespace schema get person # Create a new schema -nodespace schema create --params '{"name":"Ticket","description":"A tracked unit of engineering work","fields":[{"name":"status","type":"enum","required":true,"coreValues":[{"value":"ready_for_dev","label":"Ready for Dev"},{"value":"in_dev","label":"In Dev"},{"value":"done","label":"Done"}]},{"name":"assignee","type":"text"}],"relationships":[{"name":"belongs_to_sprint","targetType":"sprint","direction":"out","cardinality":"one"}]}' +nodespace schema create --params '{"name":"Ticket","description":"A tracked unit of engineering work","fields":[{"name":"status","type":"enum","required":true,"coreValues":[{"value":"ready_for_dev","label":"Ready for Dev"},{"value":"in_dev","label":"In Dev"},{"value":"done","label":"Done"}]},{"name":"assignee","type":"text"}],"relationships":[{"name":"belongs_to_sprint","targetType":"sprint","direction":"out","cardinality":"one","reverseName":"tickets","reverseCardinality":"many"}]}' # Create a schema with a unique field — key flagged unique_case_insensitive nodespace schema create --params '{"name":"ADR","description":"An architecture decision record","fields":[{"name":"key","type":"text","required":true,"unique_case_insensitive":true},{"name":"status","type":"enum","required":true,"coreValues":[{"value":"proposed","label":"Proposed"},{"value":"accepted","label":"Accepted"},{"value":"superseded","label":"Superseded"}]}]}' @@ -347,9 +347,9 @@ Two scoping notes. Only declarations *between schemas* block the delete — rela **Enums:** lowercase values with readable labels — `{"value":"in_progress","label":"In Progress"}`. -**Relationships vs. fields:** use a relationship (not a field) when a value references another node type. `targetType` must be an existing schema ID, or the schema ID of the type being created in the same call. Set `reverseName` when the edge has a natural name read from the target's end — that name becomes directly queryable from that side. Examples: `{"name":"supersedes","targetType":"adr","direction":"out","cardinality":"one"}`, `{"name":"has_task","targetType":"task","direction":"out","cardinality":"many"}`, `{"name":"decided_by","targetType":"person","direction":"out","cardinality":"one","reverseName":"decisions","reverseCardinality":"many"}`. +**Relationships vs. fields:** use a relationship (not a field) when a value references another node type. `targetType` must be an existing schema ID, or the schema ID of the type being created in the same call. `reverseName` and `reverseCardinality` are **required** on every relationship — a declaration missing either is rejected. One edge is stored and read from both ends, so name it from both: `reverseName` is what the edge is called read from the target (plural where that end may hold many — `invoices`, not `Invoice (Customer)`), and `reverseCardinality` is `one` or `many`, saying how many sources may point at one target. Examples: `{"name":"supersedes","targetType":"adr","direction":"out","cardinality":"one","reverseName":"superseded_by","reverseCardinality":"one"}`, `{"name":"has_task","targetType":"task","direction":"out","cardinality":"many","reverseName":"ticket","reverseCardinality":"one"}`, `{"name":"decided_by","targetType":"person","direction":"out","cardinality":"one","reverseName":"decisions","reverseCardinality":"many"}`. -**Self-referential relationships:** a type may point at itself in the same `schema create` call — give its own schema ID (the snake_case form of the name); no follow-up `schema update` is needed. Name the reverse direction with `reverseName` rather than declaring a second relationship, so one stored edge is readable from both ends: `{"name":"supersedes","targetType":"adr","direction":"out","cardinality":"one","reverseName":"superseded_by","reverseCardinality":"one"}`. The same shape covers `blocks`/`blocked_by` on a task and `parent`/`child` on a category. +**Self-referential relationships:** a type may point at itself in the same `schema create` call — give its own schema ID (the snake_case form of the name); no follow-up `schema update` is needed. The required `reverseName` is what names the other direction, so never declare a second relationship for it — one stored edge, readable from both ends: `{"name":"supersedes","targetType":"adr","direction":"out","cardinality":"one","reverseName":"superseded_by","reverseCardinality":"one"}`. The same shape covers `blocks`/`blocked_by` on a task and `parent`/`child` on a category. **Edge fields.** A relationship can carry attributes on the edge itself via `edgeFields` — facts about the *connection*, not about either node (an access level on a membership, a billing date on an invoice link). Give an edge field a fixed vocabulary by declaring it as an enum with `coreValues`, the same shape a node field uses: