Validate node type and compute title in upsert create branch; drop test- id exemption

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(&params.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(&params.node_type).is_none() {
-            let schema_exists = self
-                .store
-                .get_schema(&params.node_type)
-                .await
-                .map_err(|e| NodeServiceError::query_failed(e.to_string()))?
-                .is_some();
-            if !schema_exists {
-                return Err(NodeServiceError::unknown_node_type(&params.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,

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;