diff --git a/packages/core/src/services/node_service/crud.rs b/packages/core/src/services/node_service/crud.rs index e5d5f77b3..64facc435 100644 --- a/packages/core/src/services/node_service/crud.rs +++ b/packages/core/src/services/node_service/crud.rs @@ -441,29 +441,16 @@ impl NodeService { "create_node_with_parent: START" ); - // Step 1: Auto-create date container if parent is a date ID + // Step 1: Reject a node_type that is neither a registered core type + // nor an existing schema id — before any write, so a rejected call + // leaves no auto-created date container behind. + self.ensure_known_node_type(¶ms.node_type).await?; + + // Step 2: Auto-create date container if parent is a date ID if let Some(ref parent_id) = params.parent_id { self.ensure_date_exists(parent_id).await?; } - // Step 2: Reject a node_type that is neither a registered core type - // nor an existing schema id. Without this, an invented id (a display - // name, a paraphrase) falls through to CustomNodeBehavior and the node - // is stored as a bare shell: no schema means nothing to validate - // supplied properties against, so every one of them is silently - // dropped and the caller is told the write succeeded. - if self.behaviors.get(¶ms.node_type).is_none() { - let schema_exists = self - .store - .get_schema(¶ms.node_type) - .await - .map_err(|e| NodeServiceError::query_failed(e.to_string()))? - .is_some(); - if !schema_exists { - return Err(NodeServiceError::unknown_node_type(¶ms.node_type)); - } - } - // Step 3: Validate parent exists and is a container (if provided) if let Some(ref parent_id) = params.parent_id { if params.node_type == "collection" { @@ -518,11 +505,8 @@ impl NodeService { // Step 5: Generate or validate node ID let node_id = if let Some(provided_id) = params.id { // Validate ID format based on node type - if params.node_type == "date" - || params.node_type == "schema" - || provided_id.starts_with("test-") - { - // Date, schema, and test nodes can use their own ID format + if params.node_type == "date" || params.node_type == "schema" { + // Date and schema nodes use their own ID format provided_id } else { // Production nodes must use UUID format @@ -2071,7 +2055,7 @@ impl NodeService { /// Upsert a node with automatic parent creation - single transaction /// /// Creates parent node if it doesn't exist, then upserts the child node. - /// All operations happen in a single transaction to prevent database locking. + /// The steps are separate store writes, not one transaction. /// /// # Arguments /// * `node_id` - ID of the node to upsert @@ -2081,7 +2065,7 @@ impl NodeService { /// /// # Returns /// * `Ok(())` - Operation successful - /// * `Err(NodeServiceError)` - If transaction fails + /// * `Err(NodeServiceError)` - If validation or a store write fails pub async fn upsert_node_with_parent( &self, node_id: &str, @@ -2091,6 +2075,16 @@ impl NodeService { _root_id: &str, // Deprecated: hierarchy now managed via relationships before_sibling_id: Option<&str>, ) -> Result<(), NodeServiceError> { + let existing = self.store.get_node(node_id).await.map_err(|e| { + NodeServiceError::query_failed(format!("Failed to check node existence: {}", e)) + })?; + + // A node about to be created must have a known type — checked before + // the parent is auto-created so a rejected call leaves nothing behind. + if existing.is_none() { + self.ensure_known_node_type(node_type).await?; + } + // Ensure parent exists (create if missing) if self .store @@ -2143,9 +2137,7 @@ impl NodeService { } // Upsert the node (update if exists, create if not) - if let Some(existing) = self.store.get_node(node_id).await.map_err(|e| { - NodeServiceError::query_failed(format!("Failed to check node existence: {}", e)) - })? { + if let Some(existing) = existing { // Update existing node // Recompute title from the new content — this node is always // attached to a parent by the end of this call, so it's never @@ -2190,7 +2182,7 @@ impl NodeService { }); } else { // Create new node - let node = Node { + let mut node = Node { id: node_id.to_string(), node_type: node_type.to_string(), content: content.to_string(), @@ -2200,9 +2192,11 @@ impl NodeService { mentioned_in: vec![], created_at: chrono::Utc::now(), modified_at: chrono::Utc::now(), - title: None, // Title managed by NodeService for root/task nodes + title: None, lifecycle_status: "active".to_string(), }; + // Always attached to a parent by the end of this call — never root. + node.title = self.compute_title(&node, Some(false)).await?; self.store .create_node(node, self.client_id.clone(), self.execution_context.clone()) .await @@ -2240,6 +2234,29 @@ impl NodeService { // Private CRUD helpers // ========================================================================= + /// Reject a node_type that is neither a registered core type nor an + /// existing schema id. Without this, an invented id (a display name, a + /// paraphrase) falls through to CustomNodeBehavior and the node is stored + /// as a bare shell: no schema means nothing to validate supplied + /// properties against, so every one of them is silently dropped and the + /// caller is told the write succeeded. + async fn ensure_known_node_type(&self, node_type: &str) -> Result<(), NodeServiceError> { + if self.behaviors.get(node_type).is_some() { + return Ok(()); + } + let schema_exists = self + .store + .get_schema(node_type) + .await + .map_err(|e| NodeServiceError::query_failed(e.to_string()))? + .is_some(); + if schema_exists { + Ok(()) + } else { + Err(NodeServiceError::unknown_node_type(node_type)) + } + } + /// Validate a node's properties against its schema definition pub(crate) async fn validate_node_against_schema( &self, diff --git a/packages/core/src/services/node_service/mod.rs b/packages/core/src/services/node_service/mod.rs index 05b105fbd..251f12884 100644 --- a/packages/core/src/services/node_service/mod.rs +++ b/packages/core/src/services/node_service/mod.rs @@ -5289,6 +5289,128 @@ mod tests { ); } + /// The create branch of `upsert_node_with_parent` must title the node the + /// same way `create_node_with_parent` does, not leave `title` empty. + #[tokio::test] + async fn test_upsert_node_with_parent_create_branch_computes_title() { + let (service, _temp) = create_test_service().await; + + let parent = Node::new("text".to_string(), "a parent".to_string(), json!({})); + let parent_id = service.create_node(parent).await.unwrap(); + + let task_id = uuid::Uuid::new_v4().to_string(); + service + .upsert_node_with_parent(&task_id, "Buy milk", "task", &parent_id, &task_id, None) + .await + .unwrap(); + + let created = service.get_node(&task_id).await.unwrap().unwrap(); + assert_eq!(created.title.as_deref(), Some("Buy milk")); + + // A text node is titled only as a root; upsert always attaches the + // node to a parent, so a created text child must stay untitled. + let text_id = uuid::Uuid::new_v4().to_string(); + service + .upsert_node_with_parent(&text_id, "a child", "text", &parent_id, &text_id, None) + .await + .unwrap(); + let text = service.get_node(&text_id).await.unwrap().unwrap(); + assert_eq!(text.title, None); + } + + /// `create_node_with_parent` must reject an unknown node type before it + /// auto-creates a missing date parent. + #[tokio::test] + async fn test_create_node_with_parent_unknown_type_leaves_no_date_parent() { + let (service, _temp) = create_test_service().await; + + let result = service + .create_node_with_parent(CreateNodeParams { + id: None, + node_type: "not-a-real-type".to_string(), + content: "content".to_string(), + parent_id: Some("2026-09-25".to_string()), + position: crate::services::InsertPositionOwned::End, + properties: json!({}), + lifecycle_status: None, + }) + .await; + + assert!( + matches!(result, Err(NodeServiceError::UnknownNodeType { .. })), + "expected UnknownNodeType; got: {:?}", + result + ); + assert!( + service + .store + .get_node("2026-09-25") + .await + .unwrap() + .is_none(), + "a rejected create must not leave an auto-created date parent behind" + ); + } + + /// The create branch of `upsert_node_with_parent` must reject an unknown + /// node type — and do so before auto-creating the missing parent. + #[tokio::test] + async fn test_upsert_node_with_parent_create_branch_rejects_unknown_type() { + let (service, _temp) = create_test_service().await; + + let node_id = uuid::Uuid::new_v4().to_string(); + let err = service + .upsert_node_with_parent( + &node_id, + "content", + "not-a-real-type", + "2026-09-25", + &node_id, + None, + ) + .await + .expect_err("unknown node_type must be rejected"); + assert!( + matches!(err, NodeServiceError::UnknownNodeType { .. }), + "expected UnknownNodeType; got: {err:?}" + ); + assert!(service.get_node(&node_id).await.unwrap().is_none()); + assert!( + service + .store + .get_node("2026-09-25") + .await + .unwrap() + .is_none(), + "a rejected upsert must not leave an auto-created parent behind" + ); + } + + /// A caller-supplied id for a non-date/non-schema node must be a UUID — + /// there is no `test-` prefix exemption in the shipped validation path. + #[tokio::test] + async fn test_create_node_with_parent_rejects_test_prefixed_id() { + let (service, _temp) = create_test_service().await; + + let result = service + .create_node_with_parent(CreateNodeParams { + id: Some("test-not-a-uuid".to_string()), + node_type: "text".to_string(), + content: "hello".to_string(), + parent_id: None, + position: crate::services::InsertPositionOwned::End, + properties: json!({}), + lifecycle_status: None, + }) + .await; + + assert!( + matches!(result, Err(NodeServiceError::InvalidUpdate(_))), + "a non-UUID id must be rejected regardless of prefix; got: {:?}", + result + ); + } + #[tokio::test] async fn test_create_node_with_parent_rejects_non_container() { let (service, _temp) = create_test_service().await;