diff --git a/packages/agent/src/local_agent/tools.rs b/packages/agent/src/local_agent/tools.rs index aff6dd3f0..cdd6da5bf 100644 --- a/packages/agent/src/local_agent/tools.rs +++ b/packages/agent/src/local_agent/tools.rs @@ -3760,8 +3760,8 @@ impl GraphToolExecutor { // (`core_schemas.rs`), and ADR-076's `add_field_values` lets a // methodology bundle append real values to it at install time // (`backlog`, `in_review`, ...). A literal match here would reject - // exactly those values while the service layer - // (`node_service::schema::validate_task_status`) accepts them — + // exactly those values while the service layer (the update + // pipeline's enum check) accepts them — // leaving the agent unable to write a status the schema declares // valid. // diff --git a/packages/cli/src/commands/node.rs b/packages/cli/src/commands/node.rs index 5e215ec6d..cbb93c9c7 100644 --- a/packages/cli/src/commands/node.rs +++ b/packages/cli/src/commands/node.rs @@ -319,7 +319,7 @@ async fn set_status(client: &mut NodeClient, args: SetStatusArgs, json: bool) -> // not a list baked into this binary — is the only source of truth for // which values are valid. An invalid value comes back from the RPC // below as an InvalidArgument status naming the value and the current - // list (`NodeService::validate_task_status`). + // list (the update pipeline's enum check). let properties = json!({ "status": args.status }).to_string(); let response = client diff --git a/packages/cli/tests/it/cli_integration.rs b/packages/cli/tests/it/cli_integration.rs index 41c9ff5a8..baa049b31 100644 --- a/packages/cli/tests/it/cli_integration.rs +++ b/packages/cli/tests/it/cli_integration.rs @@ -2147,7 +2147,7 @@ async fn node_set_status_rejects_invalid_status() { .node_id; // No CLI-side vocabulary check remains, so this exercises the daemon's - // live-schema validation (`NodeService::validate_task_status`) via the + // live-schema validation (the update pipeline's enum check) via the // RPC — same error-mapping path as `schema_create_rejects_malformed_params`. let err = commands::node::run( &mut client, diff --git a/packages/core/src/behaviors/mod.rs b/packages/core/src/behaviors/mod.rs index 3d5479f70..1a8328b6f 100644 --- a/packages/core/src/behaviors/mod.rs +++ b/packages/core/src/behaviors/mod.rs @@ -704,9 +704,8 @@ impl NodeBehavior for ProjectNodeBehavior { // Typed fields live under `properties.project.*` — `project` is a // schema-typed node type, so NodeService hoists its schema-defined - // fields there on write, the same as `task` under `properties.task.*` - // (see `TaskNode::from_node`, which uses the same nested-first/ - // flat-fallback lookup for the same reason). Falling back to the flat + // fields there on write, the same as `task` under `properties.task.*`. + // Falling back to the flat // top level keeps this correct for a node that was constructed // directly. TYPE checks only — the schema system validates enum // membership and allowed values. @@ -2539,7 +2538,6 @@ impl Default for NodeBehaviorRegistry { #[cfg(test)] mod tests { use super::*; - use crate::models::TaskNode; use serde_json::json; #[test] @@ -3015,8 +3013,8 @@ mod tests { assert!(behavior.validate(&complete_node).is_ok()); // Priority is a string enum. A non-string priority is not a supported - // format: `from_node` reads it with `as_str()`, so it is ignored and the - // task falls back to having no priority rather than failing validation. + // format: the typed conversion reads it as a string, so it is ignored and + // the task falls back to having no priority rather than failing validation. let integer_priority_node = Node::new( "task".to_string(), "Task with non-string priority".to_string(), @@ -3026,9 +3024,9 @@ mod tests { // any task-typed node validates. The assertion below is the load-bearing // one: the value is dropped, not interpreted. assert!(behavior.validate(&integer_priority_node).is_ok()); - let parsed = TaskNode::from_node(integer_priority_node).unwrap(); + let typed = crate::models::node_to_typed_value(integer_priority_node).unwrap(); assert!( - parsed.priority.is_none(), + typed.get("priority").is_none(), "non-string priority should be ignored, not interpreted as a legacy format" ); @@ -3055,8 +3053,8 @@ mod tests { "Task".to_string(), json!({"task": {"status": 123}}), // number instead of string ); - // Note: TaskNode::from_node() ignores invalid status and uses default (Open) - // This is graceful degradation - it passes validation + // The typed conversion ignores an invalid status and uses the default + // (open). This is graceful degradation - it passes validation assert!(behavior.validate(&bad_status_type).is_ok()); // Priority is now a string enum (highest, high, medium, low, lowest) with user-extensibility @@ -4514,95 +4512,6 @@ mod tests { )); } - // ========================================================================= - // TaskNode from_node Format Tests - // ========================================================================= - - #[test] - fn test_task_node_from_node_nested_format() { - use crate::models::{Priority, TaskNode}; - - // Test that TaskNode::from_node handles nested format - // Priority now uses string enum format - let nested_node = Node::new( - "task".to_string(), - "Nested format task".to_string(), - json!({ - "task": { - "status": "in_progress", - "priority": "medium" - } - }), - ); - - let task = TaskNode::from_node(nested_node).unwrap(); - assert_eq!(task.content, "Nested format task"); - assert_eq!(task.status.as_str(), "in_progress"); - assert_eq!(task.priority, Some(Priority::Medium)); - } - - #[test] - fn test_task_node_from_node_flat_format() { - use crate::models::{Priority, TaskNode}; - - // Test flat format (old property structure, but string priority) - let flat_node = Node::new( - "task".to_string(), - "Flat format task".to_string(), - json!({ - "status": "done", - "priority": "low" - }), - ); - - let task = TaskNode::from_node(flat_node).unwrap(); - assert_eq!(task.content, "Flat format task"); - assert_eq!(task.status.as_str(), "done"); - assert_eq!(task.priority, Some(Priority::Low)); - } - - #[test] - fn test_task_node_from_node_string_priority() { - use crate::models::{Priority, TaskNode}; - - // Test string priority - now the canonical format - let string_priority_node = Node::new( - "task".to_string(), - "High priority task".to_string(), - json!({ - "task": { - "status": "open", - "priority": "high" - } - }), - ); - - let task = TaskNode::from_node(string_priority_node).unwrap(); - assert_eq!(task.priority, Some(Priority::High)); - - // Test core string priorities - let low_node = Node::new( - "task".to_string(), - "Low".to_string(), - json!({"priority": "low"}), - ); - let low_task = TaskNode::from_node(low_node).unwrap(); - assert_eq!(low_task.priority, Some(Priority::Low)); - - // Test user-defined priority (schema extensibility) - let urgent_node = Node::new( - "task".to_string(), - "Urgent".to_string(), - json!({"priority": "urgent"}), - ); - let urgent_task = TaskNode::from_node(urgent_node).unwrap(); - assert_eq!( - urgent_task.priority, - Some(Priority::User("urgent".to_string())), - "Unknown priorities become User-defined" - ); - } - #[test] fn test_query_node_behavior_validation() { let behavior = QueryNodeBehavior; diff --git a/packages/core/src/db/sqlite_store/mod.rs b/packages/core/src/db/sqlite_store/mod.rs index 4aeeadff5..c28c579dc 100644 --- a/packages/core/src/db/sqlite_store/mod.rs +++ b/packages/core/src/db/sqlite_store/mod.rs @@ -1,7 +1,7 @@ use crate::db::fractional_ordering::FractionalOrderCalculator; use crate::models::{DeleteResult, Node, NodeQuery, NodeUpdate, OrderBy}; use anyhow::{Context, Result}; -use chrono::{DateTime, NaiveDate, Utc}; +use chrono::{DateTime, Utc}; use connections::{Connections, ReadConn, WriteGuard}; use serde_json::Value; use std::collections::HashMap; @@ -11,22 +11,6 @@ use std::sync::Arc; use tokio::sync::broadcast; use tx::Tx; -/// Normalise a stored date string to YYYY-MM-DD on read. -/// Accepts YYYY-MM-DD (pass-through) or RFC 3339 (extract date portion). -/// Returns the original string for unrecognised values so callers can surface them. -fn normalize_date_field(s: &str) -> String { - if NaiveDate::parse_from_str(s, "%Y-%m-%d").is_ok() { - return s.to_string(); - } - if let Ok(dt) = DateTime::parse_from_rfc3339(s) { - return dt.format("%Y-%m-%d").to_string(); - } - if let Ok(dt) = s.parse::>() { - return dt.format("%Y-%m-%d").to_string(); - } - s.to_string() -} - const DOMAIN_EVENT_CHANNEL_CAPACITY: usize = 128; /// KNN over-fetch factor: vec0 returns top-k *chunks*, but search results are grouped /// by node and many chunks map to one node, so we fetch `limit * this` to cover enough diff --git a/packages/core/src/db/sqlite_store/nodes.rs b/packages/core/src/db/sqlite_store/nodes.rs index 767dbfbb3..7c9ab73ee 100644 --- a/packages/core/src/db/sqlite_store/nodes.rs +++ b/packages/core/src/db/sqlite_store/nodes.rs @@ -3779,167 +3779,6 @@ impl SqliteStore { Ok(id) } - /// Convert a raw [`Node`] into a [`crate::models::TaskNode`], reading its - /// typed fields back out of the namespaced `properties.task.*` storage - /// shape (the inverse of [`crate::models::TaskNodeUpdate::apply_to_properties`]). - /// Returns `None` for a non-task node. Shared by [`Self::get_task_node`] - /// and [`Self::update_task_node_with_version_check_in_tx`] so the two - /// read paths can never parse the stored shape differently. - pub(crate) fn node_to_task_node(n: Node) -> Option { - // The typed task shape is `task`'s own: a subtype is never converted - // to its base's struct (ADR-086 §5). - if !crate::models::CoreNodeType::Task.is_exactly(&n.node_type) { - return None; - } - let props = &n.properties; - let task_props = props.get("task").cloned().unwrap_or(serde_json::json!({})); - Some(crate::models::TaskNode { - id: n.id, - node_type: n.node_type, - content: n.content, - version: n.version, - created_at: n.created_at, - modified_at: n.modified_at, - properties: n.properties, - status: task_props - .get("status") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse().ok()) - .unwrap_or_default(), - priority: task_props - .get("priority") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse().ok()), - due_date: task_props - .get("due_date") - .and_then(|v| v.as_str()) - .map(normalize_date_field), - started_at: task_props - .get("started_at") - .and_then(|v| v.as_str()) - .map(normalize_date_field), - completed_at: task_props - .get("completed_at") - .and_then(|v| v.as_str()) - .map(normalize_date_field), - }) - } - - pub async fn get_task_node(&self, id: &str) -> Result> { - let node = self.get_node(id).await?; - Ok(node.and_then(Self::node_to_task_node)) - } - - /// `_in_tx` counterpart of the removed non-tx `update_task_node` (ADR-060 - /// §2), same read-check-write shape as - /// [`Self::update_node_with_version_check_in_tx`]: the pre-read and the - /// version-gated `UPDATE` both run against `tx`, so the OCC check is - /// sound against this transaction's own view — no TOCTOU window between - /// reading `current` and writing. The `WHERE ... AND version = ?` guard - /// on the `UPDATE` is new relative to the removed method (which checked - /// the version only via a separate, non-atomic pre-read): a strict - /// correctness improvement that comes for free from running inside a - /// real transaction, matching the generic node-update path's own guard. - /// - /// Returns `Ok(Err(actual_version))` on a version mismatch rather than - /// `Err` — an expected, common outcome the caller maps to - /// `NodeServiceError::VersionConflict` itself, exactly like - /// `update_node_with_version_check_in_tx`'s own convention (ADR-069 - /// §2a: a version mismatch is not a transaction failure). `actual_version` - /// is the real persisted version observed by this call, not a placeholder - /// — an improvement over the removed method, which could only ever report - /// `0` for a task-node conflict. - /// - /// Returns the raw [`Node`], not [`crate::models::TaskNode`] — the - /// caller (`NodeService::update_task_node_in_tx`) needs the namespaced - /// `properties.task.*` shape for its own changed-properties diff and - /// invariant-rule dispatch, not `TaskNode::into_node`'s flattened wire - /// shape (that shape exists for the RPC boundary only, never storage — - /// see that method's own doc). - pub(crate) async fn update_task_node_with_version_check_in_tx( - tx: &Tx<'_>, - id: &str, - expected_version: i64, - update: crate::models::TaskNodeUpdate, - title: Option, - ) -> Result> { - let current = match Self::get_node_in_tx(tx, id).await? { - Some(n) => n, - None => return Err(anyhow::anyhow!("Task node not found: {}", id)), - }; - - if current.version != expected_version { - return Ok(Err(current.version)); - } - - // Merge the update's task-property fields into the node's properties via the - // shared helper (also used by the service layer to compute a title_template - // title against the post-merge node — keeping the two in lockstep). The - // helper is a best-effort no-op on malformed shapes; the persist path keeps - // the stricter contract and rejects them loudly, as before this refactor. - let mut props = current.properties.clone(); - let root = props - .as_object() - .ok_or_else(|| anyhow::anyhow!("Invalid properties"))?; - if root.get("task").is_some_and(|task| !task.is_object()) { - return Err(anyhow::anyhow!("Invalid task properties")); - } - update.apply_to_properties(&mut props); - - let props_json = serde_json::to_string(&props).context("Failed to serialize properties")?; - let now = Utc::now().to_rfc3339(); - let new_version = expected_version + 1; - - let mut sql = "UPDATE node SET properties = ?1, version = ?2, modified_at = ?3".to_string(); - let mut sql_params: Vec = vec![ - libsql::Value::Text(props_json), - libsql::Value::Integer(new_version), - libsql::Value::Text(now), - ]; - - if let Some(ref content) = update.content { - sql.push_str(&format!(", content = ?{}", sql_params.len() + 1)); - sql_params.push(libsql::Value::Text(content.clone())); - } - - // Persist the recomputed title (when the caller determined content changed) - // in the same UPDATE as content, so a task's indexed `title` column never - // goes stale relative to its content. - if let Some(ref new_title) = title { - sql.push_str(&format!(", title = ?{}", sql_params.len() + 1)); - sql_params.push(libsql::Value::Text(new_title.clone())); - } - - sql.push_str(&format!( - " WHERE id = ?{} AND version = ?{}", - sql_params.len() + 1, - sql_params.len() + 2 - )); - sql_params.push(libsql::Value::Text(id.to_string())); - sql_params.push(libsql::Value::Integer(expected_version)); - - let rows_affected = tx - .conn() - .execute(&sql, sql_params) - .await - .context("Failed to update task node")?; - - if rows_affected == 0 { - // Lost a race between the read above and this write despite - // running inside the same transaction — vanishingly unlikely - // (nothing else can observe or mutate this row mid-transaction), - // same defensive posture as `update_node_with_version_check_in_tx`'s - // own WHERE clause. - return Ok(Err(current.version)); - } - - let node = Self::get_node_in_tx(tx, id) - .await? - .ok_or_else(|| anyhow::anyhow!("Task node '{}' not found after update", id))?; - - Ok(Ok(node)) - } - /// Fetch a schema with `relationships` hydrated from its declaration edges /// in the `relationship` table. One caller-facing fetch: fields come from /// the node row, relationships from `get_schema_declarations` — no consumer diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index 39596b036..593688aca 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -45,7 +45,7 @@ pub use db::{ }; pub use models::{ FilterOperator, Node, NodeFilter, NodeQuery, NodeUpdate, OrderBy, PropertyFilter, SchemaNode, - TaskNode, TaskNodeUpdate, TaskStatus, ValidationError, + TaskNodeUpdate, TaskStatus, ValidationError, }; pub use playbook::PlaybookEngine; pub use services::{ diff --git a/packages/core/src/markdown/mod.rs b/packages/core/src/markdown/mod.rs index e3a39eb74..d897d4ce3 100644 --- a/packages/core/src/markdown/mod.rs +++ b/packages/core/src/markdown/mod.rs @@ -35,7 +35,7 @@ mod error; pub use error::MarkdownError; -use crate::models::{Node, TaskNode, TaskStatus}; +use crate::models::{CoreNodeType, Node, TaskStatus}; use crate::services::{CollectionService, CreateNodeParams, NodeService, NodeServiceError}; use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; @@ -1916,16 +1916,17 @@ pub async fn handle_get_markdown_from_node_id( /// Format task node checkbox based on task status /// -/// Uses TaskNode::from_node for strongly-typed status extraction. -/// Returns "- [x] " for Done tasks, "- [ ] " for all other states. +/// Returns "- [x] " for a done task and "- [ ] " for every other status. fn format_task_checkbox(node: &Node) -> &'static str { - if let Ok(task) = TaskNode::from_node(node.clone()) { - match task.status() { - TaskStatus::Done => "- [x] ", - _ => "- [ ] ", // Open, InProgress, Cancelled all render as unchecked - } + let status = node + .properties + .get(CoreNodeType::Task.as_str()) + .and_then(|task| task.get("status")) + .and_then(|v| v.as_str()); + if status == Some(TaskStatus::Done.as_str()) { + "- [x] " } else { - "- [ ] " // Default to unchecked if conversion fails + "- [ ] " } } diff --git a/packages/core/src/models/ai_chat_node.rs b/packages/core/src/models/ai_chat_node.rs index 276f0c06a..232b7c0e4 100644 --- a/packages/core/src/models/ai_chat_node.rs +++ b/packages/core/src/models/ai_chat_node.rs @@ -4,8 +4,8 @@ //! raw `serde_json::Value` path lookups that the daemon previously used to read //! and write chat messages, status, model, and provider. //! -//! Mirrors the [`TaskNode`](crate::models::TaskNode) pattern: data is stored in -//! the node's `properties` under a type namespace (`properties["ai-chat"]`), and +//! Data is stored in the node's `properties` under a type namespace +//! (`properties["ai-chat"]`), and //! `from_node` / `into_node` convert between the universal [`Node`] and this //! strongly-typed struct. //! @@ -306,8 +306,7 @@ impl AiChatNode { /// /// Reads ai-chat fields from the `properties["ai-chat"]` namespace, falling /// back to flat `properties` when the namespace is absent (e.g. after - /// `flatten_properties_for_api` has promoted them). Mirrors the dual - /// nested/flat handling in [`TaskNode::from_node`](crate::models::TaskNode). + /// `flatten_properties_for_api` has promoted them). /// /// Reads ONLY the canonical snake_case schema names (`turn_status`, /// `session_status`) — deliberately not dual-cased. An earlier version of diff --git a/packages/core/src/models/core_schemas.rs b/packages/core/src/models/core_schemas.rs index df84cc7b6..707142fed 100644 --- a/packages/core/src/models/core_schemas.rs +++ b/packages/core/src/models/core_schemas.rs @@ -2381,17 +2381,17 @@ mod tests { ); } - // Every core_values entry must round-trip through TaskStatus::from_str + // Every core_values entry must round-trip through TaskStatus::from_value // to a NAMED variant, not fall through to the User(_) catch-all — a // core_values entry with no corresponding variant is exactly the // reverse drift (schema declares a value the type doesn't know as a // first-class variant). for value in &core_value_strings { - let parsed: TaskStatus = value.parse().expect("TaskStatus::from_str is infallible"); + let parsed = TaskStatus::from_value(value); assert!( parsed.is_core(), "task.status's core_values entry '{}' does not parse to a named TaskStatus \ - variant (got TaskStatus::User(_)) — add a matching variant in task_node.rs \ + variant (got TaskStatus::User(_)) — add a matching variant to nodespace-types' TaskStatus \ or remove the stray core_values entry.", value ); @@ -2444,7 +2444,7 @@ mod tests { assert_eq!( core_value_strings, expected, "{}.priority's core_values must be exactly the named Priority variants \ - in rank order — update core_schemas.rs or models/priority.rs so they agree.", + in rank order — update core_schemas.rs or nodespace-types' Priority so they agree.", node_type ); } diff --git a/packages/core/src/models/mod.rs b/packages/core/src/models/mod.rs index f4eb37af5..5de2132cf 100644 --- a/packages/core/src/models/mod.rs +++ b/packages/core/src/models/mod.rs @@ -4,7 +4,7 @@ //! //! - `Node` - Universal node model for all content types //! - `Embedding` - Vector embeddings for semantic search (root-aggregate model) -//! - Type-safe wrappers (TaskNode, SkillNode, AiChatNode, SchemaNode) for ergonomic access +//! - Type-safe wrappers (SkillNode, AiChatNode, SchemaNode) for ergonomic access //! - Core schema definitions for built-in node types //! //! All entities use the Pure JSON schema approach with data stored in the @@ -14,23 +14,17 @@ pub mod conflict; pub mod core_schemas; pub mod embedding; mod node; -mod priority; pub mod schema; pub mod time; // Type-safe node wrappers mod ai_chat_node; mod schema_node; -mod task_node; #[cfg(test)] #[path = "ai_chat_node_test.rs"] mod ai_chat_node_test; -#[cfg(test)] -#[path = "task_node_test.rs"] -mod task_node_test; - pub use ai_chat_node::{ AiChatCompletedWrite, AiChatMessage, AiChatNode, AiChatPendingDeletion, AiChatResolvedEntity, AiChatTurnOutcome, AI_CHAT_NODE_TYPE, AI_CHAT_PROVIDERS, @@ -46,9 +40,7 @@ pub use time::{SystemTimeProvider, TimeProvider}; // Export type-safe wrappers pub use embedding::{ChunkInfo, Embedding, EmbeddingConfig, EmbeddingSearchResult, NewEmbedding}; pub use nodespace_types::{SkillNode, DEFAULT_SKILL_MAX_ITERATIONS, SKILL_NODE_TYPE}; -pub use priority::Priority; pub use schema_node::SchemaNode; -pub use task_node::{TaskNode, TaskNodeUpdate, TaskStatus}; // The core type registry (ADR-086 §3): the one list of the types NodeSpace // ships, and what each records. @@ -64,11 +56,15 @@ pub use nodespace_types::{ }; // Typed update payloads for the core types whose writes route through the -// generic update pipeline (`NodeService::update_person_node` / -// `update_project_node` / `update_query_node`). Defined once in -// nodespace-types so the Tauri command layer deserializes the same struct -// the service consumes. -pub use nodespace_types::{PersonNodeUpdate, ProjectNodeUpdate, QueryNodeUpdate}; +// generic update pipeline (`NodeService::update_task_node` / +// `update_person_node` / `update_project_node` / `update_query_node`). +// Defined once in nodespace-types so the Tauri command layer deserializes the +// same struct the service consumes. +pub use nodespace_types::{PersonNodeUpdate, ProjectNodeUpdate, QueryNodeUpdate, TaskNodeUpdate}; + +// The vocabularies of the task and project fields: `task.status`, and the +// priority scale the two types share. +pub use nodespace_types::{Priority, TaskStatus}; // The stored query's typed fields — the only reader of a query node's // properties (see `QueryDefinition::from_fields` for the execution mapping). diff --git a/packages/core/src/models/task_node.rs b/packages/core/src/models/task_node.rs deleted file mode 100644 index 6a6ca58e9..000000000 --- a/packages/core/src/models/task_node.rs +++ /dev/null @@ -1,875 +0,0 @@ -//! Strongly-Typed TaskNode -//! -//! Provides compile-time type safety for task nodes with strongly-typed status, -//! priority, and date fields. Uses Universal Graph Architecture. -//! -//! # Architecture -//! -//! **Universal Graph Architecture:** -//! ```text -//! DB node table (node.properties.status, node.properties.priority, etc.) -//! → Single query from node table -//! → Properties extracted from node.properties JSON -//! → Deserialize directly to TaskNode struct with typed enums -//! → TaskNode.status is a TaskStatus enum field -//! ``` -//! -//! # Serialization -//! -//! When serialized (for Tauri/HTTP responses), outputs a flat structure with typed fields: -//! ```json -//! { -//! "id": "task-123", -//! "nodeType": "task", -//! "content": "Implement feature", -//! "status": "open", -//! "priority": "medium", -//! "dueDate": null -//! } -//! ``` -//! -//! # Examples -//! -//! ```rust -//! use nodespace_core::models::{TaskNode, TaskStatus, Priority}; -//! -//! // Create with builder (for new tasks) -//! let task = TaskNode::builder("Write tests".to_string()) -//! .with_status(TaskStatus::InProgress) -//! .with_priority(Priority::High) -//! .build(); -//! -//! // Direct field access (no JSON parsing) -//! assert_eq!(task.status, TaskStatus::InProgress); -//! assert_eq!(task.priority, Some(Priority::High)); -//! ``` - -use crate::models::{Node, Priority, ValidationError}; -use chrono::{DateTime, NaiveDate, Utc}; -use serde::{Deserialize, Serialize}; -use serde_json::json; -use std::str::FromStr; - -/// Parse a date string (YYYY-MM-DD or RFC 3339) into a YYYY-MM-DD string. -/// Returns None if the input is empty or unparseable. -pub(crate) fn parse_to_date_string(s: &str) -> Option { - // Already YYYY-MM-DD - if NaiveDate::parse_from_str(s, "%Y-%m-%d").is_ok() { - return Some(s.to_string()); - } - // RFC 3339 / ISO 8601 — extract the date portion - if let Ok(dt) = DateTime::parse_from_rfc3339(s) { - return Some(dt.format("%Y-%m-%d").to_string()); - } - if let Ok(dt) = s.parse::>() { - return Some(dt.format("%Y-%m-%d").to_string()); - } - None -} - -/// Custom deserializer for schema `date` fields. -/// Accepts YYYY-MM-DD or RFC 3339 on input; always stores/returns YYYY-MM-DD. -/// Handles the double-Option pattern: None = field absent (no change), Some(None) = clear. -mod flexible_date { - use serde::{self, Deserialize, Deserializer}; - - pub fn deserialize_with_null<'de, D>( - deserializer: D, - ) -> Result>, D::Error> - where - D: Deserializer<'de>, - { - let opt: Option = Option::deserialize(deserializer)?; - match opt { - None => Ok(Some(None)), - Some(s) => match super::parse_to_date_string(&s) { - Some(date_str) => Ok(Some(Some(date_str))), - None => Err(serde::de::Error::custom(format!( - "Invalid date format: '{}'. Expected YYYY-MM-DD or ISO8601", - s - ))), - }, - } - } -} - -/// Task status enumeration -/// -/// Represents the lifecycle states of a task node. -/// Values use lowercase format for consistency across all layers: -/// - "open" - Not started (default) -/// - "in_progress" - Currently being worked on -/// - "done" - Finished -/// - "cancelled" - Cancelled/abandoned -/// - User-defined statuses via schema extension (e.g., "blocked", "review") -/// -/// Core statuses are strongly typed; user-defined statuses use `User(String)`. -/// This aligns with the schema system's `core_values` / `user_values` model. -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub enum TaskStatus { - /// Task has not been started - #[default] - Open, - /// Task is currently being worked on - InProgress, - /// Task has been finished - Done, - /// Task has been cancelled/abandoned - Cancelled, - /// User-defined status (extended via schema) - User(String), -} - -impl FromStr for TaskStatus { - type Err = String; - - fn from_str(s: &str) -> Result { - match s { - "open" => Ok(Self::Open), - "in_progress" => Ok(Self::InProgress), - "done" => Ok(Self::Done), - "cancelled" => Ok(Self::Cancelled), - // Any other value is treated as user-defined - other => Ok(Self::User(other.to_string())), - } - } -} - -impl TaskStatus { - /// Convert status to string representation - pub fn as_str(&self) -> &str { - match self { - Self::Open => "open", - Self::InProgress => "in_progress", - Self::Done => "done", - Self::Cancelled => "cancelled", - Self::User(s) => s.as_str(), - } - } - - /// Check if this is a core (built-in) status - pub fn is_core(&self) -> bool { - !matches!(self, Self::User(_)) - } - - /// Check if this is a user-defined status - pub fn is_user_defined(&self) -> bool { - matches!(self, Self::User(_)) - } -} - -impl Serialize for TaskStatus { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - serializer.serialize_str(self.as_str()) - } -} - -impl<'de> Deserialize<'de> for TaskStatus { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - Ok(Self::from_str(&s).unwrap()) // from_str never fails now - } -} - -/// Strongly-typed task node with direct field access -/// -/// Uses Universal Graph Architecture - properties stored in node.properties JSON. -/// All fields are strongly typed - TaskStatus and Priority enums. -/// -/// # Query Pattern (Universal Graph Architecture) -/// -/// ```sql -/// SELECT -/// record::id(id) AS id, -/// node_type AS nodeType, -/// properties.status AS status, -/// properties.priority AS priority, -/// properties.due_date AS dueDate, -/// content, -/// version, -/// created_at AS createdAt, -/// modified_at AS modifiedAt -/// FROM node:`some-id`; -/// ``` -/// -/// When serialized (for Tauri/HTTP responses), outputs a flat structure with typed fields: -/// ```json -/// { -/// "id": "task-123", -/// "nodeType": "task", -/// "content": "Fix bug", -/// "status": "done", -/// "priority": "medium" -/// } -/// ``` -/// -/// # Examples -/// -/// ```rust -/// use nodespace_core::models::{TaskNode, TaskStatus}; -/// -/// let task = TaskNode::builder("Fix bug".to_string()) -/// .with_status(TaskStatus::Done) -/// .build(); -/// -/// // Direct field access -/// assert_eq!(task.status, TaskStatus::Done); -/// assert_eq!(task.content, "Fix bug"); -/// ``` -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct TaskNode { - // ======================================================================== - // Node fields (from node table) - // ======================================================================== - /// Unique identifier - pub id: String, - - /// Node type (always "task" for TaskNode) - #[serde(rename = "nodeType")] - pub node_type: String, - - /// Primary content/text of the task - pub content: String, - - /// Optimistic concurrency control version - #[serde(default = "default_version")] - pub version: i64, - - /// Creation timestamp - pub created_at: DateTime, - - /// Last modification timestamp - pub modified_at: DateTime, - - /// Properties object (for schema-driven UI compatibility) - /// Contains task fields for generic Node consumers - #[serde(default)] - pub properties: serde_json::Value, - - // ======================================================================== - // Task-specific fields (from node.properties) - // ======================================================================== - /// Task status (strongly typed enum) - #[serde(default)] - pub status: TaskStatus, - - /// Task priority (strongly typed enum: highest, high, medium, low, lowest) - #[serde(default)] - pub priority: Option, - - /// Due date for the task (YYYY-MM-DD) - #[serde(default)] - pub due_date: Option, - - /// Started at date (YYYY-MM-DD, when task moved to in_progress) - #[serde(default)] - pub started_at: Option, - - /// Completed at date (YYYY-MM-DD, when task moved to done) - #[serde(default)] - pub completed_at: Option, -} - -fn default_version() -> i64 { - 1 -} - -impl TaskNode { - /// Default priority value - pub const DEFAULT_PRIORITY: Priority = Priority::Medium; - - /// Create a TaskNode from an existing Node (for backward compatibility) - /// - /// This converts the JSON properties pattern to strongly-typed fields. - /// Prefer using `get_task_node()` from NodeService for direct deserialization. - /// - /// # Property Formats - /// - /// Supports both property formats: - /// - Nested format: `properties.task.status` - /// - Flat format: `properties.status` (used by the markdown importer) - /// - /// # Errors - /// - /// Returns `ValidationError::InvalidNodeType` if the node type is not "task". - pub fn from_node(node: Node) -> Result { - if !crate::models::CoreNodeType::Task.is_exactly(&node.node_type) { - return Err(ValidationError::InvalidNodeType(format!( - "Expected 'task', got '{}'", - node.node_type - ))); - } - - // Try nested format first, fall back to flat format (used by the markdown importer) - let task_props = node - .properties - .get("task") - .and_then(|v| v.as_object()) - .map(|obj| serde_json::Value::Object(obj.clone())); - let props = task_props.as_ref().unwrap_or(&node.properties); - - // Extract status from properties - let status: TaskStatus = props - .get("status") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse().ok()) - .unwrap_or_default(); - - // Extract priority from properties (string enum format) - let priority = props - .get("priority") - .and_then(|v| v.as_str()) - .map(|s| Priority::from_str(s).unwrap_or_default()); - - // Extract due_date from properties — normalize to YYYY-MM-DD (accept RFC 3339 for migration) - let due_date = props - .get("due_date") - .and_then(|v| v.as_str()) - .and_then(parse_to_date_string); - - // Extract started_at from properties — normalize to YYYY-MM-DD - let started_at = props - .get("started_at") - .and_then(|v| v.as_str()) - .and_then(parse_to_date_string); - - // Extract completed_at from properties — normalize to YYYY-MM-DD - let completed_at = props - .get("completed_at") - .and_then(|v| v.as_str()) - .and_then(parse_to_date_string); - - // Build properties object for schema-driven UI compatibility - // Use camelCase keys per naming conventions (snake_case in DB, camelCase in JSON API) - let mut props = serde_json::Map::new(); - props.insert("status".to_string(), json!(status.as_str())); - if let Some(ref p) = priority { - props.insert("priority".to_string(), json!(p.as_str())); - } - if let Some(ref d) = due_date { - props.insert("dueDate".to_string(), json!(d)); - } - if let Some(ref s) = started_at { - props.insert("startedAt".to_string(), json!(s)); - } - if let Some(ref c) = completed_at { - props.insert("completedAt".to_string(), json!(c)); - } - props.insert("_schemaVersion".to_string(), json!(1)); - - Ok(Self { - id: node.id, - node_type: "task".to_string(), - content: node.content, - version: node.version, - created_at: node.created_at, - modified_at: node.modified_at, - properties: json!(props), - status, - priority, - due_date, - started_at, - completed_at, - }) - } - - /// Create a builder for a new TaskNode with the given content - pub fn builder(content: String) -> TaskNodeBuilder { - TaskNodeBuilder { - content, - status: None, - priority: None, - due_date: None, - } - } - - /// Convert to universal Node (for backward compatibility with existing APIs) - /// - /// This creates a Node with properties populated from the strongly-typed fields. - /// Uses snake_case property keys — the canonical on-disk storage form that the - /// persistence layer writes and that [`TaskNode::from_node`] reads back, so the - /// domain→Node→domain round trip is lossless. - pub fn into_node(self) -> Node { - let mut properties = serde_json::Map::new(); - properties.insert("status".to_string(), json!(self.status.as_str())); - - if let Some(priority) = self.priority { - properties.insert("priority".to_string(), json!(priority)); - } - - if let Some(due_date) = self.due_date { - properties.insert("due_date".to_string(), json!(due_date)); - } - - if let Some(started_at) = self.started_at { - properties.insert("started_at".to_string(), json!(started_at)); - } - - if let Some(completed_at) = self.completed_at { - properties.insert("completed_at".to_string(), json!(completed_at)); - } - - Node { - id: self.id, - node_type: "task".to_string(), - content: self.content.clone(), - version: self.version, - created_at: self.created_at, - modified_at: self.modified_at, - properties: json!(properties), - mentions: Vec::new(), - mentioned_in: Vec::new(), - title: Some(crate::utils::strip_markdown(&self.content)), // Task nodes have indexed titles - lifecycle_status: "active".to_string(), - } - } - - /// Get a reference as Node (creates a temporary Node for compatibility) - /// - /// Note: This is less efficient than direct field access. Prefer using - /// the strongly-typed fields directly when possible. - pub fn as_node(&self) -> Node { - self.clone().into_node() - } - - // ======================================================================== - // Convenience methods for backward compatibility - // ======================================================================== - - /// Get the task's status (for API compatibility) - pub fn status(&self) -> TaskStatus { - self.status.clone() - } - - /// Set the task's status - pub fn set_status(&mut self, status: TaskStatus) { - self.status = status; - self.modified_at = Utc::now(); - } - - /// Get the task's priority as Priority enum - pub fn get_priority(&self) -> Priority { - self.priority.clone().unwrap_or(Self::DEFAULT_PRIORITY) - } - - /// Set the task's priority - pub fn set_priority(&mut self, priority: Priority) { - self.priority = Some(priority); - self.modified_at = Utc::now(); - } - - /// Get the task's due date as a YYYY-MM-DD string - pub fn due_date(&self) -> Option { - self.due_date.clone() - } - - /// Set the task's due date from a YYYY-MM-DD or RFC 3339 string - pub fn set_due_date(&mut self, due_date: Option<&str>) { - self.due_date = due_date.and_then(parse_to_date_string); - self.modified_at = Utc::now(); - } -} - -/// Builder for creating new TaskNode instances -pub struct TaskNodeBuilder { - content: String, - status: Option, - priority: Option, - due_date: Option, -} - -impl TaskNodeBuilder { - /// Set the task status - pub fn with_status(mut self, status: TaskStatus) -> Self { - self.status = Some(status); - self - } - - /// Set the task priority - pub fn with_priority(mut self, priority: Priority) -> Self { - self.priority = Some(priority); - self - } - - /// Set the task due date from a YYYY-MM-DD or RFC 3339 string - pub fn with_due_date(mut self, due_date: &str) -> Self { - self.due_date = parse_to_date_string(due_date); - self - } - - /// Set the task due date from string (alias for with_due_date) - pub fn with_due_date_str(mut self, due_date: &str) -> Self { - self.due_date = parse_to_date_string(due_date); - self - } - - /// Build the TaskNode - pub fn build(self) -> TaskNode { - let now = Utc::now(); - let id = uuid::Uuid::new_v4().to_string(); - - let status = self.status.unwrap_or_default(); - - // Build properties object for schema-driven UI compatibility - // Use camelCase keys per naming conventions (snake_case in DB, camelCase in JSON API) - let mut props = serde_json::Map::new(); - props.insert("status".to_string(), json!(status.as_str())); - if let Some(p) = &self.priority { - props.insert("priority".to_string(), json!(p.as_str())); - } - if let Some(d) = &self.due_date { - props.insert("dueDate".to_string(), json!(d)); - } - props.insert("_schemaVersion".to_string(), json!(1)); - - TaskNode { - id, - node_type: "task".to_string(), - content: self.content, - version: 1, - created_at: now, - modified_at: now, - properties: json!(props), - status, - priority: self.priority, - due_date: self.due_date, - started_at: None, - completed_at: None, - } - } -} - -/// Partial update structure for task nodes -/// -/// Supports updating task-specific fields (status, priority, due_date) -/// as well as content. Uses Option for each field to enable partial updates. -/// -/// # Double-Option Pattern -/// -/// Some fields use double-Option to distinguish between: -/// - `None` - Don't change this field -/// - `Some(None)` - Set the field to NULL -/// - `Some(Some(value))` - Set to specific value -/// -/// # Examples -/// -/// ```rust -/// use nodespace_core::models::{TaskNodeUpdate, TaskStatus}; -/// -/// // Update only status -/// let update = TaskNodeUpdate::new().with_status(TaskStatus::InProgress); -/// -/// // Update status and clear due date -/// let update = TaskNodeUpdate::new() -/// .with_status(TaskStatus::Done) -/// .with_due_date(None); // Clears the due date -/// ``` -#[derive(Debug, Clone, Default, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct TaskNodeUpdate { - /// Update task status (task property) - #[serde(skip_serializing_if = "Option::is_none")] - pub status: Option, - - /// Update task priority (task property) - /// - `None` - Don't change - /// - `Some(None)` - Clear priority - /// - `Some(Some(p))` - Set to priority p (highest, high, medium, low, lowest, or user-defined) - #[serde(skip_serializing_if = "Option::is_none")] - pub priority: Option>, - - /// Update due date (task property) - /// - `None` - Don't change - /// - `Some(None)` - Clear due date - /// - `Some(Some(s))` - Set to YYYY-MM-DD date (also accepts RFC 3339, normalised on write) - #[serde( - default, - skip_serializing_if = "Option::is_none", - deserialize_with = "flexible_date::deserialize_with_null" - )] - pub due_date: Option>, - - /// Update started_at date (task property) - /// - `None` - Don't change - /// - `Some(None)` - Clear started_at - /// - `Some(Some(s))` - Set to YYYY-MM-DD date (also accepts RFC 3339, normalised on write) - #[serde( - default, - skip_serializing_if = "Option::is_none", - deserialize_with = "flexible_date::deserialize_with_null" - )] - pub started_at: Option>, - - /// Update completed_at date (task property) - /// - `None` - Don't change - /// - `Some(None)` - Clear completed_at - /// - `Some(Some(s))` - Set to YYYY-MM-DD date (also accepts RFC 3339, normalised on write) - #[serde( - default, - skip_serializing_if = "Option::is_none", - deserialize_with = "flexible_date::deserialize_with_null" - )] - pub completed_at: Option>, - - /// Update content (node field) - #[serde(skip_serializing_if = "Option::is_none")] - pub content: Option, -} - -impl TaskNodeUpdate { - /// Create a new empty update - pub fn new() -> Self { - Self::default() - } - - /// Set status update - pub fn with_status(mut self, status: TaskStatus) -> Self { - self.status = Some(status); - self - } - - /// Set priority update (Some(value) to set, None to clear) - pub fn with_priority(mut self, priority: Option) -> Self { - self.priority = Some(priority); - self - } - - /// Set due date update (Some(value) to set, None to clear). - /// Accepts YYYY-MM-DD or RFC 3339; normalizes to YYYY-MM-DD. - pub fn with_due_date(mut self, due_date: Option<&str>) -> Self { - self.due_date = Some(due_date.and_then(parse_to_date_string)); - self - } - - /// Set content update - pub fn with_content(mut self, content: String) -> Self { - self.content = Some(content); - self - } - - /// Check if the update contains any changes - pub fn is_empty(&self) -> bool { - self.status.is_none() - && self.priority.is_none() - && self.due_date.is_none() - && self.started_at.is_none() - && self.completed_at.is_none() - && self.content.is_none() - } - - /// Check if this update contains task property fields - pub fn has_property_fields(&self) -> bool { - self.status.is_some() - || self.priority.is_some() - || self.due_date.is_some() - || self.started_at.is_some() - || self.completed_at.is_some() - } - - /// Check if this update contains content field - pub fn has_content_field(&self) -> bool { - self.content.is_some() - } - - /// Merge this update's task-property fields into a node's namespaced - /// `properties` value (the `{ "task": { ... } }` object). - /// - /// Shared by the write path (`SqliteStore::update_task_node`, which persists - /// the result) and the title-computation path (`NodeService::update_task_node`, - /// which needs the post-merge node to recompute a `title_template`-driven - /// title) so the two can never drift. Content is deliberately not handled here - /// — it's a node column, not a task property. No-op if `properties` (or its - /// `task` entry) isn't a JSON object. - pub fn apply_to_properties(&self, properties: &mut serde_json::Value) { - let Some(root) = properties.as_object_mut() else { - return; - }; - let Some(task) = root - .entry("task") - .or_insert_with(|| serde_json::json!({})) - .as_object_mut() - else { - return; - }; - - if let Some(ref status) = self.status { - task.insert("status".to_string(), serde_json::json!(status.as_str())); - } - if let Some(ref priority_opt) = self.priority { - match priority_opt { - Some(p) => { - task.insert("priority".to_string(), serde_json::json!(p.as_str())); - } - None => { - task.remove("priority"); - } - } - } - if let Some(ref due_date_opt) = self.due_date { - match due_date_opt { - Some(s) => { - task.insert("due_date".to_string(), serde_json::json!(s)); - } - None => { - task.remove("due_date"); - } - } - } - if let Some(ref started_at_opt) = self.started_at { - match started_at_opt { - Some(s) => { - task.insert("started_at".to_string(), serde_json::json!(s)); - } - None => { - task.remove("started_at"); - } - } - } - if let Some(ref completed_at_opt) = self.completed_at { - match completed_at_opt { - Some(s) => { - task.insert("completed_at".to_string(), serde_json::json!(s)); - } - None => { - task.remove("completed_at"); - } - } - } - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_task_node_update_null_started_at() { - // This is the JSON sent when user clicks to clear a date - let json = r#"{"startedAt": null}"#; - let update: TaskNodeUpdate = serde_json::from_str(json).unwrap(); - - // started_at should be Some(None) to indicate "clear this field" - // NOT None which means "don't change this field" - assert!( - update.started_at.is_some(), - "started_at should be Some(None) for null value, but got None" - ); - assert!( - update.started_at.clone().unwrap().is_none(), - "Inner value should be None (clear the field)" - ); - - // is_empty() should return false because we're explicitly clearing the field - assert!( - !update.is_empty(), - "Update should NOT be empty when clearing started_at" - ); - } - - #[test] - fn test_task_node_update_absent_started_at() { - // When field is absent, it should be None (don't change) - let json = r#"{}"#; - let update: TaskNodeUpdate = serde_json::from_str(json).unwrap(); - - assert!( - update.started_at.is_none(), - "started_at should be None when absent" - ); - assert!( - update.is_empty(), - "Empty JSON should result in empty update" - ); - } -} - -/// Round-trip property test for the `TaskNode` domain type. -/// -/// `TaskNode` is the authoritative in-memory model for `task` nodes: it owns the -/// `from_node` (parse) and `into_node` (serialize) conversions the daemon relies -/// on. If a field is added to the struct but forgotten in either direction — or -/// the two directions disagree on a property key — the field is silently dropped -/// on the next write. This proptest turns that silent drop into a test failure by -/// asserting `from_node(task.into_node())` recovers every typed field: the -/// domain→storage→domain round trip. -#[cfg(test)] -mod roundtrip_proptests { - use super::*; - use proptest::prelude::*; - - fn task_status() -> impl Strategy { - prop_oneof![ - Just(TaskStatus::Open), - Just(TaskStatus::InProgress), - Just(TaskStatus::Done), - Just(TaskStatus::Cancelled), - "[a-z][a-z_]{0,15}".prop_map(TaskStatus::User), - ] - } - - fn task_priority() -> impl Strategy> { - prop_oneof![ - Just(None), - Just(Some(Priority::Highest)), - Just(Some(Priority::High)), - Just(Some(Priority::Medium)), - Just(Some(Priority::Low)), - Just(Some(Priority::Lowest)), - "[a-z][a-z_]{0,15}".prop_map(|s| Some(Priority::User(s))), - ] - } - - /// Optional `YYYY-MM-DD` date string — the normalized on-disk form. - fn opt_date() -> impl Strategy> { - prop_oneof![ - Just(None), - (2000u32..2100, 1u32..=12, 1u32..=28) - .prop_map(|(y, m, d)| Some(format!("{:04}-{:02}-{:02}", y, m, d))), - ] - } - - proptest! { - /// `from_node(task.into_node())` recovers every typed field. - #[test] - fn task_node_survives_node_round_trip( - content in "[ -~]{0,40}", - status in task_status(), - priority in task_priority(), - due_date in opt_date(), - started_at in opt_date(), - completed_at in opt_date(), - ) { - let original = TaskNode { - id: "task-roundtrip".to_string(), - node_type: "task".to_string(), - content: content.clone(), - version: 1, - created_at: Utc::now(), - modified_at: Utc::now(), - properties: serde_json::json!({}), - status: status.clone(), - priority: priority.clone(), - due_date: due_date.clone(), - started_at: started_at.clone(), - completed_at: completed_at.clone(), - }; - - let recovered = TaskNode::from_node(original.clone().into_node()) - .expect("round trip through Node should re-parse as a task"); - - prop_assert_eq!(&recovered.content, &content); - prop_assert_eq!(&recovered.status, &status); - prop_assert_eq!(&recovered.priority, &priority); - prop_assert_eq!(&recovered.due_date, &due_date); - prop_assert_eq!(&recovered.started_at, &started_at); - prop_assert_eq!(&recovered.completed_at, &completed_at); - } - } -} diff --git a/packages/core/src/models/task_node_test.rs b/packages/core/src/models/task_node_test.rs deleted file mode 100644 index fe436787e..000000000 --- a/packages/core/src/models/task_node_test.rs +++ /dev/null @@ -1,406 +0,0 @@ -//! Tests for TaskNode wrapper - -#[cfg(test)] -mod tests { - use crate::models::{task_node::TaskStatus, Node, Priority, TaskNode}; - use serde_json::json; - - #[test] - fn test_from_node_validates_type() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - assert!(TaskNode::from_node(node).is_ok()); - - let wrong_type = Node::new("text".to_string(), "Test".to_string(), json!({})); - let result = TaskNode::from_node(wrong_type); - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("Expected 'task'")); - } - - #[test] - fn test_status_getter() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"status": "in_progress"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.status(), TaskStatus::InProgress); - } - - #[test] - fn test_status_getter_default() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let task = TaskNode::from_node(node).unwrap(); - // Default status is Open - assert_eq!(task.status(), TaskStatus::Open); - } - - #[test] - fn test_status_getter_user_defined() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"status": "blocked"}), - ); - let task = TaskNode::from_node(node).unwrap(); - // User-defined statuses are now valid (schema extensibility) - assert_eq!(task.status(), TaskStatus::User("blocked".to_string())); - assert!(task.status().is_user_defined()); - assert!(!task.status().is_core()); - } - - #[test] - fn test_status_setter() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let mut task = TaskNode::from_node(node).unwrap(); - - task.set_status(TaskStatus::Done); - - assert_eq!(task.status(), TaskStatus::Done); - // Direct field access now - assert_eq!(task.status, TaskStatus::Done); - // Status value uses lowercase format - assert_eq!(task.as_node().properties["status"], "done"); - } - - #[test] - fn test_priority_getter() { - // Priority as string enum value - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"priority": "high"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.get_priority(), Priority::High); - } - - #[test] - fn test_priority_getter_default() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let task = TaskNode::from_node(node).unwrap(); - // Default priority is Medium - assert_eq!(task.get_priority(), Priority::Medium); - } - - #[test] - fn test_priority_setter() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let mut task = TaskNode::from_node(node).unwrap(); - - task.set_priority(Priority::Low); - - assert_eq!(task.get_priority(), Priority::Low); - assert_eq!(task.as_node().properties["priority"], "low"); - } - - #[test] - fn test_due_date_getter() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"due_date": "2025-01-15T00:00:00Z"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.due_date(), Some("2025-01-15".to_string())); - } - - #[test] - fn test_due_date_getter_none() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.due_date(), None); - } - - #[test] - fn test_due_date_setter() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let mut task = TaskNode::from_node(node).unwrap(); - - task.set_due_date(Some("2025-02-01T00:00:00Z")); - - assert_eq!(task.due_date(), Some("2025-02-01".to_string())); - } - - #[test] - fn test_due_date_clear() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"due_date": "2025-01-15T00:00:00Z"}), - ); - let mut task = TaskNode::from_node(node).unwrap(); - - task.set_due_date(None); - - assert_eq!(task.due_date(), None); - assert!(task.as_node().properties.get("dueDate").is_none()); - } - - #[test] - fn test_into_node_preserves_data() { - // Status and priority use string enum format - let original = Node::new( - "task".to_string(), - "Test task".to_string(), - json!({"status": "open", "priority": "medium"}), - ); - let original_id = original.id.clone(); - - let task = TaskNode::from_node(original).unwrap(); - let converted_back = task.into_node(); - - assert_eq!(converted_back.id, original_id); - assert_eq!(converted_back.node_type, "task"); - assert_eq!(converted_back.content, "Test task"); - assert_eq!(converted_back.properties["status"], "open"); - assert_eq!(converted_back.properties["priority"], "medium"); - } - - #[test] - fn test_as_node_reference() { - // Status uses lowercase format - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"status": "done"}), - ); - let task = TaskNode::from_node(node).unwrap(); - - let node_ref = task.as_node(); - assert_eq!(node_ref.node_type, "task"); - assert_eq!(node_ref.content, "Test"); - } - - #[test] - fn test_direct_field_access() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let mut task = TaskNode::from_node(node).unwrap(); - - // Direct field mutation - task.content = "Updated content".to_string(); - - assert_eq!(task.content, "Updated content"); - assert_eq!(task.as_node().content, "Updated content"); - } - - #[test] - fn test_builder_minimal() { - let task = TaskNode::builder("Implement feature".to_string()).build(); - - assert_eq!(task.content, "Implement feature"); - assert_eq!(task.as_node().node_type, "task"); - // Default status is Open - assert_eq!(task.status(), TaskStatus::Open); - // Default priority is Medium - assert_eq!(task.get_priority(), Priority::Medium); - } - - #[test] - fn test_builder_with_status() { - let task = TaskNode::builder("Write tests".to_string()) - .with_status(TaskStatus::InProgress) - .build(); - - assert_eq!(task.status(), TaskStatus::InProgress); - // Direct field access - assert_eq!(task.status, TaskStatus::InProgress); - } - - #[test] - fn test_builder_with_priority() { - let task = TaskNode::builder("Fix bug".to_string()) - .with_priority(Priority::High) - .build(); - - assert_eq!(task.get_priority(), Priority::High); - // Direct field access - assert_eq!(task.priority, Some(Priority::High)); - } - - #[test] - fn test_builder_full() { - let task = TaskNode::builder("Complete project".to_string()) - .with_status(TaskStatus::InProgress) - .with_priority(Priority::High) - .with_due_date("2025-12-31T00:00:00Z") - .build(); - - assert_eq!(task.status(), TaskStatus::InProgress); - assert_eq!(task.get_priority(), Priority::High); - assert_eq!(task.due_date(), Some("2025-12-31".to_string())); - } - - #[test] - fn test_builder_with_due_date_str() { - let task = TaskNode::builder("Complete project".to_string()) - .with_due_date_str("2025-12-31T00:00:00Z") - .build(); - - assert_eq!(task.due_date(), Some("2025-12-31".to_string())); - } - - #[test] - fn test_task_status_from_str() { - // Status values use lowercase format - assert_eq!("open".parse::().unwrap(), TaskStatus::Open); - assert_eq!( - "in_progress".parse::().unwrap(), - TaskStatus::InProgress - ); - assert_eq!("done".parse::().unwrap(), TaskStatus::Done); - assert_eq!( - "cancelled".parse::().unwrap(), - TaskStatus::Cancelled - ); - } - - #[test] - fn test_task_status_from_str_user_defined() { - // User-defined statuses are now valid (schema extensibility) - let result = "blocked".parse::(); - assert!(result.is_ok()); - assert_eq!(result.unwrap(), TaskStatus::User("blocked".to_string())); - - let result = "review".parse::(); - assert!(result.is_ok()); - assert_eq!(result.unwrap(), TaskStatus::User("review".to_string())); - } - - #[test] - fn test_task_status_as_str() { - // Status values use lowercase format - assert_eq!(TaskStatus::Open.as_str(), "open"); - assert_eq!(TaskStatus::InProgress.as_str(), "in_progress"); - assert_eq!(TaskStatus::Done.as_str(), "done"); - assert_eq!(TaskStatus::Cancelled.as_str(), "cancelled"); - // User-defined statuses preserve their original value - assert_eq!(TaskStatus::User("blocked".to_string()).as_str(), "blocked"); - assert_eq!(TaskStatus::User("review".to_string()).as_str(), "review"); - } - - #[test] - fn test_all_status_values() { - // Updated status values - let statuses = vec![ - TaskStatus::Open, - TaskStatus::InProgress, - TaskStatus::Done, - TaskStatus::Cancelled, - ]; - - for status in statuses { - // Round-trip test - let str_repr = status.as_str(); - let parsed: TaskStatus = str_repr.parse().unwrap(); - assert_eq!(parsed, status); - } - } - - #[test] - fn test_multiple_property_updates() { - let node = Node::new("task".to_string(), "Test".to_string(), json!({})); - let mut task = TaskNode::from_node(node).unwrap(); - - // Update multiple properties - task.set_status(TaskStatus::InProgress); - task.set_priority(Priority::Low); - task.set_due_date(Some("2025-03-15T00:00:00Z")); - - // Verify all updates - assert_eq!(task.status(), TaskStatus::InProgress); - assert_eq!(task.get_priority(), Priority::Low); - assert_eq!(task.due_date(), Some("2025-03-15".to_string())); - } - - #[test] - fn test_serde_serialization() { - let task = TaskNode::builder("Serialize me".to_string()) - .with_status(TaskStatus::Done) - .with_priority(Priority::High) - .build(); - - let json = serde_json::to_value(&task).unwrap(); - assert_eq!(json["content"], "Serialize me"); - assert_eq!(json["status"], "done"); - assert_eq!(json["priority"], "high"); - } - - #[test] - fn test_serde_deserialization() { - // Uses camelCase for JSON (matching Node struct convention) - let json = json!({ - "id": "test-123", - "nodeType": "task", - "content": "Deserialize me", - "version": 1, - "createdAt": "2025-01-01T00:00:00Z", - "modifiedAt": "2025-01-01T00:00:00Z", - "status": "in_progress", - "priority": "medium" - }); - - let task: TaskNode = serde_json::from_value(json).unwrap(); - assert_eq!(task.id, "test-123"); - assert_eq!(task.content, "Deserialize me"); - assert_eq!(task.status, TaskStatus::InProgress); - assert_eq!(task.priority, Some(Priority::Medium)); - } - - #[test] - fn test_priority_user_defined() { - // User-defined priorities are allowed (schema extensibility) - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"priority": "critical"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.get_priority(), Priority::User("critical".to_string())); - assert!(task.get_priority().is_user_defined()); - assert!(!task.get_priority().is_core()); - } - - #[test] - fn test_priority_as_str() { - assert_eq!(Priority::Highest.as_str(), "highest"); - assert_eq!(Priority::High.as_str(), "high"); - assert_eq!(Priority::Medium.as_str(), "medium"); - assert_eq!(Priority::Low.as_str(), "low"); - assert_eq!(Priority::Lowest.as_str(), "lowest"); - assert_eq!(Priority::User("critical".to_string()).as_str(), "critical"); - } - - #[test] - fn test_started_at_rfc3339_normalizes_to_date_only() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"started_at": "2025-03-10T08:00:00Z"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.started_at, Some("2025-03-10".to_string())); - } - - #[test] - fn test_completed_at_rfc3339_normalizes_to_date_only() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"completed_at": "2025-04-20T23:59:59+05:30"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.completed_at, Some("2025-04-20".to_string())); - } - - #[test] - fn test_due_date_date_only_format_passes_through() { - let node = Node::new( - "task".to_string(), - "Test".to_string(), - json!({"due_date": "2025-06-09"}), - ); - let task = TaskNode::from_node(node).unwrap(); - assert_eq!(task.due_date, Some("2025-06-09".to_string())); - } -} diff --git a/packages/core/src/services/node_service/mod.rs b/packages/core/src/services/node_service/mod.rs index 57211ae7a..46c97ba9e 100644 --- a/packages/core/src/services/node_service/mod.rs +++ b/packages/core/src/services/node_service/mod.rs @@ -10664,59 +10664,8 @@ mod tests { ); } - /// Regression test: unlike the generic update path - /// (`update_with_version_check_returning_node`), `NodeService::update_task_node` - /// never recomputed the indexed `title` column when `content` changed — it - /// delegated straight to `SqliteStore::update_task_node`, which only ever wrote - /// `content`/`properties`/`version`/`modified_at`. A task's `title` column could - /// go permanently stale relative to its `content`. Title must now track content, - /// via the same `compute_title` call the generic path uses. - #[tokio::test] - async fn update_task_node_recomputes_title_when_content_changes() { - use crate::models::TaskNodeUpdate; - use crate::services::{CreateNodeParams, InsertPositionOwned}; - - let (service, _temp) = create_test_service().await; - - let id = service - .create_node_with_parent(CreateNodeParams { - id: None, - node_type: "task".to_string(), - content: "Original task content".to_string(), - parent_id: None, - position: InsertPositionOwned::End, - properties: json!({}), - lifecycle_status: None, - }) - .await - .unwrap(); - - let created = service.get_node(&id).await.unwrap().unwrap(); - assert_eq!( - created.title.as_deref(), - Some("Original task content"), - "sanity: title must be set on create" - ); - - let update = TaskNodeUpdate::new().with_content("Updated task content".to_string()); - let task = service - .update_task_node(&id, created.version, update) - .await - .expect("update_task_node should succeed"); - assert_eq!(task.content, "Updated task content"); - - let refetched = service.get_node(&id).await.unwrap().unwrap(); - assert_eq!( - refetched.title.as_deref(), - Some("Updated task content"), - "title must be recomputed from the new content, not left stale at the old value" - ); - } - - /// Companion to the above: a status-only update (no `content` in the - /// `TaskNodeUpdate`) must NOT recompute or touch `title` — mirrors the generic - /// path's change-guard, scoped to what's relevant for tasks (content is the - /// only field that affects a task's title). + /// A status-only typed update leaves `title` alone: without a + /// `title_template`, content is the only thing a task's title depends on. #[tokio::test] async fn update_task_node_status_only_does_not_change_title() { use crate::models::{TaskNodeUpdate, TaskStatus}; @@ -10740,12 +10689,15 @@ mod tests { let created = service.get_node(&id).await.unwrap().unwrap(); assert_eq!(created.title.as_deref(), Some("Stable task content")); - let update = TaskNodeUpdate::new().with_status(TaskStatus::Done); + let update = TaskNodeUpdate { + status: Some(TaskStatus::Done), + ..Default::default() + }; let task = service .update_task_node(&id, created.version, update) .await .expect("update_task_node should succeed"); - assert_eq!(task.status, TaskStatus::Done); + assert_eq!(task.properties["task"]["status"], "done"); let refetched = service.get_node(&id).await.unwrap().unwrap(); assert_eq!( @@ -10756,9 +10708,9 @@ mod tests { } /// ADR-076: `update_task_node` must reject a status string not present in - /// `task.status`'s `core_values` + `user_values`, closing the gap where - /// `TaskStatus::from_str`'s infallible `User(String)` fallback let any - /// string through unvalidated. + /// `task.status`'s `core_values` + `user_values`. `TaskStatus` parses any + /// string (`User(String)`), so the schema check in the shared update + /// pipeline is what rejects an undeclared one. #[tokio::test] async fn update_task_node_rejects_status_not_in_schema_vocabulary() { use crate::models::{TaskNodeUpdate, TaskStatus}; @@ -10780,7 +10732,10 @@ mod tests { .unwrap(); let created = service.get_node(&id).await.unwrap().unwrap(); - let update = TaskNodeUpdate::new().with_status(TaskStatus::User("backlog".to_string())); + let update = TaskNodeUpdate { + status: Some(TaskStatus::User("backlog".to_string())), + ..Default::default() + }; let err = service .update_task_node(&id, created.version, update) .await @@ -10834,12 +10789,15 @@ mod tests { .unwrap(); let created = service.get_node(&id).await.unwrap().unwrap(); - let update = TaskNodeUpdate::new().with_status(TaskStatus::User("backlog".to_string())); + let update = TaskNodeUpdate { + status: Some(TaskStatus::User("backlog".to_string())), + ..Default::default() + }; let task = service .update_task_node(&id, created.version, update) .await .expect("a status added via add_field_values must be accepted"); - assert_eq!(task.status, TaskStatus::User("backlog".to_string())); + assert_eq!(task.properties["task"]["status"], "backlog"); } /// Set a `title_template` on the built-in "task" schema, preserving its other @@ -10952,7 +10910,10 @@ mod tests { let created = service.get_node(&id).await.unwrap().unwrap(); // Priority-only update (no content) — the template now depends on it. - let update = TaskNodeUpdate::new().with_priority(Some(Priority::High)); + let update = TaskNodeUpdate { + priority: Some(Some(Priority::High)), + ..Default::default() + }; service .update_task_node(&id, created.version, update) .await @@ -10967,10 +10928,9 @@ mod tests { } /// A templated task's title is computed from the *post-merge* node — the - /// new property value, not the pre-update one (one write behind) — and the - /// typed task update path rejects content on it like every other write. + /// new property value, not the pre-update one (one write behind). #[tokio::test] - async fn update_task_node_templated_title_uses_post_merge_properties_and_rejects_content() { + async fn update_task_node_templated_title_uses_post_merge_properties() { use crate::models::{Priority, TaskNodeUpdate}; use crate::services::{CreateNodeParams, InsertPositionOwned}; @@ -10996,33 +10956,23 @@ mod tests { .update_task_node( &id, created.version, - TaskNodeUpdate::new().with_priority(Some(Priority::Low)), + TaskNodeUpdate { + priority: Some(Some(Priority::Low)), + ..Default::default() + }, ) .await .unwrap(); let seeded = service.get_node(&id).await.unwrap().unwrap(); - let err = service - .update_task_node( - &id, - seeded.version, - TaskNodeUpdate::new() - .with_content("Ship the spec".to_string()) - .with_priority(Some(Priority::High)), - ) - .await - .expect_err("content on a templated task must be rejected"); - assert!( - err.to_string() - .contains("task takes its name from priority; content is not allowed"), - "{err}" - ); - service .update_task_node( &id, seeded.version, - TaskNodeUpdate::new().with_priority(Some(Priority::High)), + TaskNodeUpdate { + priority: Some(Some(Priority::High)), + ..Default::default() + }, ) .await .expect("update_task_node should succeed"); @@ -11204,7 +11154,10 @@ mod tests { .update_task_node( &task_id, task.version, - TaskNodeUpdate::new().with_priority(Some(Priority::High)), + TaskNodeUpdate { + priority: Some(Some(Priority::High)), + ..Default::default() + }, ) .await .expect("a field-only update must not trip the content rule"); diff --git a/packages/core/src/services/node_service/schema.rs b/packages/core/src/services/node_service/schema.rs index d9681fb58..bc61baa84 100644 --- a/packages/core/src/services/node_service/schema.rs +++ b/packages/core/src/services/node_service/schema.rs @@ -3,18 +3,6 @@ use super::*; use crate::models::schema::RelationshipDirection; -/// Result of [`NodeService::update_task_node_in_tx`] — the task-node twin of -/// `crud.rs`'s `VersionCheckedUpdateOutcome`. See that type's own doc for why -/// a version conflict is `Ok` rather than `Err`. `Updated` is boxed simply -/// because `TaskNode` is a large, non-`Copy` struct worth keeping off the -/// stack when this variant is passed around — unlike `VersionCheckedUpdateOutcome`, -/// there's no zero-size sibling variant here for the boxing to protect from -/// paying `TaskNode`'s size (`VersionConflict(i64)` is already small). -pub(crate) enum TaskVersionCheckedUpdateOutcome { - VersionConflict(i64), - Updated(Box), -} - impl NodeService { /// Query nodes by type with optional lifecycle_status filter. /// @@ -156,314 +144,24 @@ impl NodeService { .map_err(NodeServiceError::from_store) } - /// Get a task node with strong typing - /// - /// Returns strongly-typed `TaskNode` instead of generic `Node`. - /// - /// # Arguments - /// - /// * `id` - The task node ID - /// - /// # Returns - /// - /// * `Ok(Some(TaskNode))` - Task found with strongly-typed fields - /// * `Ok(None)` - Task not found - /// * `Err(_)` - Service error - /// - /// # Examples - /// - /// ```no_run - /// # use nodespace_core::services::NodeService; - /// # use nodespace_core::db::SqliteStore; - /// # use std::path::PathBuf; - /// # use std::sync::Arc; - /// # #[tokio::main] - /// # async fn main() -> Result<(), Box> { - /// # let mut db = Arc::new(SqliteStore::new(PathBuf::from("./test.db")).await?); - /// # let service = NodeService::new(&mut db).await?; - /// if let Some(task) = service.get_task_node("my-task-id").await? { - /// // Direct field access - no JSON parsing - /// println!("Status: {:?}", task.status); - /// println!("Content: {}", task.content); - /// } - /// # Ok(()) - /// # } - /// ``` - pub async fn get_task_node( - &self, - id: &str, - ) -> Result, NodeServiceError> { - self.store.get_task_node(id).await.map_err(|e| { - NodeServiceError::DatabaseError(crate::db::DatabaseError::SqlExecutionError { - context: format!("Failed to get task node '{}': {}", id, e), - }) - }) - } - - /// Update a task node with type-safe field updates - /// - /// Updates task-specific fields (status, priority, due_date). - /// Uses optimistic concurrency control (OCC) to prevent lost updates. - /// - /// # Type Safety - /// - /// This method provides end-to-end type safety for task updates: - /// - Frontend sends strongly-typed `TaskNodeUpdate` (not generic NodeUpdate) - /// - Backend updates task fields directly (not via JSON properties) - /// - Returns strongly-typed `TaskNode` with updated fields - /// - /// # Arguments - /// - /// * `id` - The task node ID - /// * `expected_version` - Version for OCC check (prevents lost updates) - /// * `update` - TaskNodeUpdate with fields to update - /// - /// # Returns - /// - /// * `Ok(TaskNode)` - Updated task with new version - /// * `Err(VersionMismatch)` - Version conflict, refresh and retry - /// * `Err(NodeNotFound)` - Task doesn't exist - /// - /// # Examples - /// - /// ```no_run - /// # use nodespace_core::services::NodeService; - /// # use nodespace_core::models::{TaskNodeUpdate, TaskStatus}; - /// # use nodespace_core::db::SqliteStore; - /// # use std::path::PathBuf; - /// # use std::sync::Arc; - /// # #[tokio::main] - /// # async fn main() -> Result<(), Box> { - /// # let mut db = Arc::new(SqliteStore::new(PathBuf::from("./test.db")).await?); - /// # let service = NodeService::new(&mut db).await?; - /// // Update task status - /// let update = TaskNodeUpdate::new().with_status(TaskStatus::InProgress); - /// let task = service.update_task_node("task-123", 1, update).await?; - /// println!("New status: {:?}", task.status); - /// println!("New version: {}", task.version); - /// # Ok(()) - /// # } - /// ``` - /// Validate a `TaskStatus` against `task.status`'s declared vocabulary - /// (`core_values` + `user_values`), mirroring `validate_node_with_fields`'s - /// enum check (`crud.rs`). `TaskStatus::from_str` is infallible by - /// construction — any unrecognized string becomes `TaskStatus::User(_)` — - /// so this is the only place `update_task_node` actually rejects a value - /// the schema hasn't declared. - async fn validate_task_status( - &self, - status: &crate::models::TaskStatus, - ) -> Result<(), NodeServiceError> { - let schema = self - .get_schema_node("task") - .await? - .ok_or_else(|| NodeServiceError::invalid_update("Schema 'task' not found"))?; - - let valid_values = schema.get_enum_values("status").unwrap_or_default(); - let status_str = status.as_str(); - let is_valid = valid_values.iter().any(|ev| ev.value == status_str); - - if !is_valid { - let valid_labels: Vec<_> = valid_values - .iter() - .map(|ev| format!("{} ({})", ev.label, ev.value)) - .collect(); - return Err(NodeServiceError::invalid_update(format!( - "Invalid value '{}' for enum field 'status'. Valid values: {}", - status_str, - valid_labels.join(", ") - ))); - } - - Ok(()) - } - + /// Update a task node's core fields (`status`, `priority`, `due_date`, + /// `started_at`, `completed_at`) with optimistic concurrency control. See + /// [`Self::update_person_node`] for why this delegates to the generic + /// pipeline; `status`/`priority` are validated there against the + /// schema's declared vocabulary (core + user values). pub async fn update_task_node( &self, id: &str, expected_version: i64, update: crate::models::TaskNodeUpdate, - ) -> Result { + ) -> Result { if update.is_empty() { return Err(NodeServiceError::invalid_update( "TaskNodeUpdate contains no changes", )); } - - // Enforce `status` against the schema's declared vocabulary - // (core_values + user_values) — the same check `validate_node_with_fields` - // already performs for schema-only types (ADR-076). `update_task_node` - // is the sole call path into the store-layer write (confirmed: no other - // caller reaches it directly), and the store layer trusts this having - // already run rather than re-validating itself. Read-only and schema-scoped - // (not node-scoped), so it's safe to run before the transaction opens below — - // same posture as `update_with_version_check_returning_node`'s own pre-tx checks. - if let Some(ref status) = update.status { - self.validate_task_status(status).await?; - } - - let service = self.clone(); - let service_for_tx = service.clone(); - let id_for_tx = id.to_string(); - let outcome = service - .with_transaction(move |tx| { - Box::pin(async move { - service_for_tx - .update_task_node_in_tx(tx, &id_for_tx, expected_version, update) - .await - }) - }) - .await?; - - match outcome { - TaskVersionCheckedUpdateOutcome::Updated(task) => Ok(*task), - TaskVersionCheckedUpdateOutcome::VersionConflict(actual_version) => { - Err(NodeServiceError::VersionConflict { - node_id: id.to_string(), - expected_version, - actual_version, - }) - } - } - } - - /// Tx-scoped twin of [`Self::update_task_node`] (ADR-060 §2) — the - /// `update_task_node` counterpart to `crud.rs`'s - /// `update_with_version_check_returning_node_in_tx`. Same - /// read-existing → compute title → version-checked write → diff → - /// buffer event → synchronous invariant dispatch → re-read final state - /// pipeline as that method; see its own doc for why each step is - /// ordered the way it is (in particular, why the event is buffered - /// *before* dispatch, and why the final state is re-read rather than - /// returning the pre-dispatch snapshot — an invariant rule's action can - /// self-referentially write back to this same node). - /// - /// Reads `existing` and computes `title_update` from it here, inside - /// `tx`, rather than reusing a pre-transaction snapshot — the same - /// posture `update_with_version_check_returning_node_in_tx` takes, so a - /// concurrent write landing between a hypothetical pre-tx read and this - /// tx's write can never leave the title computed against stale content. - pub(crate) async fn update_task_node_in_tx( - &self, - tx: &NodeServiceTx<'_>, - id: &str, - expected_version: i64, - update: crate::models::TaskNodeUpdate, - ) -> Result { - let existing = crate::db::SqliteStore::get_node_in_tx(tx.store_tx(), id) + self.update_typed_fields(id, "task", expected_version, update.to_properties_patch()) .await - .map_err(NodeServiceError::from_store)? - .ok_or_else(|| NodeServiceError::node_not_found(id))?; - - // Sync the indexed `title` column, mirroring the generic update path's guard - // (`content_changed || properties_changed`, see crud.rs). A task-schema - // `title_template` makes the title depend on task properties as well as - // content, so recomputing on content alone would leave the title stale after - // a property-only change, and a combined content+property update must compute - // from the *fully-merged* node (not a pre-update snapshot) or the title lands - // one write behind. We build the post-update node with the same shared merge - // the store performs and compute the title from it. When no template is set - // (the built-in "task" schema today), compute_title falls through to - // `strip_markdown(content)`, so a property-only update recomputes to the same - // value — a harmless no-op write, not a behavior change. - let content_changed = update - .content - .as_ref() - .is_some_and(|new_content| new_content != &existing.content); - - let title_update = if content_changed || update.has_property_fields() { - let mut merged = existing.clone(); - if let Some(ref new_content) = update.content { - merged.content = new_content.clone(); - } - if content_changed { - self.validate_templated_content(&merged).await?; - } - update.apply_to_properties(&mut merged.properties); - self.compute_title(&merged, None).await? - } else { - None - }; - - let result = crate::db::SqliteStore::update_task_node_with_version_check_in_tx( - tx.store_tx(), - id, - expected_version, - update, - title_update, - ) - .await - .map_err(NodeServiceError::from_store)?; - - let updated_node = match result { - Ok(node) => node, - Err(actual_version) => { - return Ok(TaskVersionCheckedUpdateOutcome::VersionConflict( - actual_version, - )) - } - }; - - // Real changed_properties, diffed from the namespaced `properties.task.*` - // storage shape — see `compute_property_changes`'s own doc. Required here - // for the same reason it's required in the generic update path: an - // `_in_tx` store write bypasses the store's own notifier, so this is the - // only source of an accurate diff for property_changed-triggered plays - // (reactive or invariant) and WatchNodes consumers. - let changed_properties = - compute_property_changes(&existing.properties, &updated_node.properties); - - // Buffered on `tx`, not broadcast yet — only - // flushed if this whole transaction commits. - self.emit_event_in_tx( - tx, - DomainEvent::NodeUpdated { - node_id: updated_node.id.clone(), - node_type: updated_node.node_type.clone(), - node: updated_node.clone(), - changed_properties: changed_properties.clone(), - }, - ); - - // ADR-060 §2: synchronous invariant-rule dispatch for property_changed - // triggers, inside this same transaction — the exact gap this issue - // closes (a Task's `status` change is the motivating example). A - // rejecting rule's `Err` propagates out through the `?` below, through - // this whole function, and through the caller's `with_transaction`, - // rolling back everything above — including the buffered event. - self.dispatch_invariant_rules_for_update_in_tx(tx, &updated_node, &changed_properties) - .await?; - - // Re-read the trigger node's final state, tx-consistent, rather than - // converting `updated_node` directly — an invariant rule's own action - // can be a self-referential `update_node` on the SAME node, writing a - // second time inside this same transaction (see - // `update_with_version_check_returning_node_in_tx`'s identical - // re-read for the full rationale). - let final_node = crate::db::SqliteStore::get_node_in_tx(tx.store_tx(), id) - .await - .map_err(NodeServiceError::from_store)? - .ok_or_else(|| NodeServiceError::node_not_found(id))?; - - // An invariant rule's action is a generic `update_node` with no - // guard against changing `node_type` — unlike the rest of this - // pipeline, which is task-shape-preserving by construction. If a - // rule's own self-referential action retypes the trigger node away - // from "task" (a deliberately unusual thing for a rule to do, and - // not the shape any known rule uses today), this conversion fails - // and the whole transaction rolls back via the `?` below — a safe, - // no-partial-write outcome, just surfaced as a generic - // `invalid_update` rather than an invariant-specific error variant. - let task_node = crate::db::SqliteStore::node_to_task_node(final_node).ok_or_else(|| { - NodeServiceError::invalid_update(format!( - "Node '{}' is no longer a task node after update", - id - )) - })?; - - Ok(TaskVersionCheckedUpdateOutcome::Updated(Box::new( - task_node, - ))) } /// Update a person node's core fields (`first_name`, `last_name`, `email`) @@ -1686,7 +1384,9 @@ impl NodeService { mod typed_update_tests { use super::*; use crate::db::SqliteStore; - use crate::models::{PersonNodeUpdate, ProjectNodeUpdate, QueryNodeUpdate}; + use crate::models::{ + PersonNodeUpdate, Priority, ProjectNodeUpdate, QueryNodeUpdate, TaskNodeUpdate, TaskStatus, + }; use crate::services::{CreateNodeParams, InsertPositionOwned}; use serde_json::json; use tempfile::TempDir; @@ -1836,6 +1536,176 @@ mod typed_update_tests { .is_err()); } + #[tokio::test] + async fn task_update_writes_fields_and_leaves_others() { + let (service, _t) = create_test_service().await; + let task = create( + &service, + "task", + json!({ "priority": "low", "custom:estimate": 3 }), + ) + .await; + + let updated = service + .update_task_node( + &task.id, + task.version, + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + due_date: set("2026-03-01"), + ..Default::default() + }, + ) + .await + .expect("typed task update succeeds"); + + assert_eq!(updated.version, task.version + 1); + let typed = crate::models::node_to_typed_value(updated).unwrap(); + assert_eq!(typed["status"], "in_progress"); + assert_eq!(typed["dueDate"], "2026-03-01"); + assert_eq!( + typed["priority"], "low", + "an absent priority is left unchanged" + ); + assert_eq!(typed["properties"], json!({ "custom:estimate": 3 })); + } + + #[tokio::test] + async fn task_update_null_clears_priority_and_dates() { + let (service, _t) = create_test_service().await; + let task = create( + &service, + "task", + json!({ "priority": "high", "due_date": "2026-03-01" }), + ) + .await; + + let updated = service + .update_task_node( + &task.id, + task.version, + TaskNodeUpdate { + priority: Some(None), + due_date: Some(None), + ..Default::default() + }, + ) + .await + .unwrap(); + + let typed = crate::models::node_to_typed_value(updated).unwrap(); + assert!(typed.get("priority").is_none(), "{typed}"); + assert!(typed.get("dueDate").is_none(), "{typed}"); + assert_eq!(typed["status"], "open", "status is untouched"); + } + + /// The typed update returns the node as stored: an archived task stays + /// archived in the response, and its extension field is still there. + #[tokio::test] + async fn task_update_response_keeps_lifecycle_status_and_extension_fields() { + let (service, _t) = create_test_service().await; + let id = service + .create_node_with_parent(CreateNodeParams { + id: None, + node_type: "task".to_string(), + content: "Retired task".to_string(), + parent_id: None, + position: InsertPositionOwned::End, + properties: json!({ "custom:estimate": 3 }), + lifecycle_status: Some("archived".to_string()), + }) + .await + .unwrap(); + let task = service.get_node(&id).await.unwrap().unwrap(); + + let updated = service + .update_task_node( + &id, + task.version, + TaskNodeUpdate { + status: Some(TaskStatus::Done), + ..Default::default() + }, + ) + .await + .unwrap(); + + assert_eq!(updated.lifecycle_status, "archived"); + let typed = crate::models::node_to_typed_value(updated).unwrap(); + assert_eq!(typed["lifecycleStatus"], "archived"); + assert_eq!(typed["status"], "done"); + assert_eq!(typed["properties"], json!({ "custom:estimate": 3 })); + } + + #[tokio::test] + async fn task_update_rejects_a_priority_outside_the_schema_vocabulary() { + let (service, _t) = create_test_service().await; + let task = create(&service, "task", json!({})).await; + + let err = service + .update_task_node( + &task.id, + task.version, + TaskNodeUpdate { + priority: Some(Some(Priority::User("someday".to_string()))), + ..Default::default() + }, + ) + .await + .unwrap_err(); + + assert!(err.to_string().contains("Invalid value 'someday'"), "{err}"); + } + + #[tokio::test] + async fn task_update_on_a_stale_version_conflicts() { + let (service, _t) = create_test_service().await; + let task = create(&service, "task", json!({})).await; + + let err = service + .update_task_node( + &task.id, + task.version + 5, + TaskNodeUpdate { + status: Some(TaskStatus::Done), + ..Default::default() + }, + ) + .await + .unwrap_err(); + + assert!( + matches!(err, NodeServiceError::VersionConflict { .. }), + "{err:?}" + ); + } + + /// The typed task shape is `task`'s own: another type's node, and an empty + /// update, are both refused. + #[tokio::test] + async fn task_update_rejects_another_type_and_an_empty_update() { + let (service, _t) = create_test_service().await; + let project = create(&service, "project", json!({})).await; + let err = service + .update_task_node( + &project.id, + project.version, + TaskNodeUpdate { + status: Some(TaskStatus::Done), + ..Default::default() + }, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("not a task node"), "{err}"); + + let task = create(&service, "task", json!({})).await; + assert!(service + .update_task_node(&task.id, task.version, TaskNodeUpdate::default()) + .await + .is_err()); + } + #[tokio::test] async fn project_update_writes_fields_and_leaves_others() { let (service, _t) = create_test_service().await; diff --git a/packages/core/src/services/query_service/mod.rs b/packages/core/src/services/query_service/mod.rs index f1807e8b7..3b0ffa092 100644 --- a/packages/core/src/services/query_service/mod.rs +++ b/packages/core/src/services/query_service/mod.rs @@ -44,7 +44,6 @@ use crate::db::SqliteStore; use crate::models::{Node, Priority}; use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; -use std::str::FromStr; use std::sync::Arc; // The filter and sort vocabulary is shared with the stored query @@ -419,12 +418,7 @@ impl QueryService { (Priority::ABSENT_RANK as i16, String::new()) } Some(serde_json::Value::String(s)) => { - // from_str is infallible — every unknown string is User(_) - // — but name that fallback rather than letting Default's - // Medium stand in for an unparseable value. - let priority = - Priority::from_str(s).unwrap_or_else(|_| Priority::User(s.clone())); - (priority.rank() as i16, s.clone()) + (Priority::from_value(s).rank() as i16, s.clone()) } Some(other) => (Priority::USER_RANK as i16, other.to_string()), } diff --git a/packages/core/tests/it/playbook_invariant_integration_test.rs b/packages/core/tests/it/playbook_invariant_integration_test.rs index 7909208b5..4f42f9414 100644 --- a/packages/core/tests/it/playbook_invariant_integration_test.rs +++ b/packages/core/tests/it/playbook_invariant_integration_test.rs @@ -26,7 +26,7 @@ use nodespace_core::db::events::{ REPLICATED_APPLY_CLIENT_ID, }; use nodespace_core::db::SqliteStore; -use nodespace_core::models::{Node, NodeUpdate, Priority, TaskNodeUpdate, TaskStatus}; +use nodespace_core::models::{Node, NodeUpdate, TaskNodeUpdate, TaskStatus}; use nodespace_core::playbook::types::MAX_CHAIN_DEPTH; use nodespace_core::services::{NodeService, NodeServiceError}; use nodespace_core::PlaybookEngine; @@ -2136,16 +2136,12 @@ async fn reactive_update_rule_still_fires_asynchronously_post_commit() -> Result } // --------------------------------------------------------------------------- -// `update_task_node` wiring (ADR-060 §2, closing the gap the generic -// `update_node` coverage above doesn't reach): `NodeService::update_task_node` -// is a separate write path from `update_node` — it calls -// `SqliteStore::update_task_node_with_version_check_in_tx` directly rather -// than composing `update_node`'s own `_in_tx` pipeline — so it needs its own -// synchronous-dispatch coverage, not just a generic-`update_node` inference. -// Uses the real, seeded built-in "task" schema (no `create_schema` call, -// unlike the generic tests above) since `update_task_node` requires the -// target node's `node_type` to literally be `"task"` (see -// `NodeService::validate_task_status` and `SqliteStore::node_to_task_node`). +// The typed task update (ADR-060 §2, ADR-086 §4): `NodeService::update_task_node` +// lowers into the same update pipeline as `update_node`, so invariant rules +// dispatch synchronously for it and its events are buffered on the same +// transaction. Uses the real, seeded built-in "task" schema (no +// `create_schema` call, unlike the generic tests above), since the typed +// update is for `task` nodes only. // Triggers are scoped to `task.status` — the exact property ADR-060's // own motivating example turns on ("reject this status change to // done/in_progress while sub-issues are open") — and augmenting actions @@ -2214,18 +2210,21 @@ async fn invariant_task_update_rule_executes_synchronously_in_same_transaction() .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_status(TaskStatus::InProgress), + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + ..Default::default() + }, ) .await?; assert_eq!( - updated.priority, - Some(Priority::High), + user_field(&updated, "task", "priority"), + Some(&json!("high")), "invariant action must have already run by the time update_task_node returned" ); assert_eq!( - updated.status, - TaskStatus::InProgress, + user_field(&updated, "task", "status"), + Some(&json!("in_progress")), "the triggering update itself must still have applied" ); @@ -2275,7 +2274,10 @@ async fn invariant_task_update_rule_reject_prevents_partial_write() -> Result<() .update_task_node( &node_id, before.version, - TaskNodeUpdate::new().with_status(TaskStatus::Done), + TaskNodeUpdate { + status: Some(TaskStatus::Done), + ..Default::default() + }, ) .await .unwrap_err(); @@ -2300,7 +2302,7 @@ async fn invariant_task_update_rule_reject_prevents_partial_write() -> Result<() } /// No `DomainEvent` is broadcast for a rejected `update_task_node` call — the -/// buffered `NodeUpdated` event `update_task_node_in_tx` emits before +/// buffered `NodeUpdated` event the update pipeline emits before /// dispatch is discarded on rollback, never flushed. Mirrors /// `invariant_update_rule_reject_emits_no_domain_event` for the generic path. #[tokio::test] @@ -2343,7 +2345,10 @@ async fn invariant_task_update_rule_reject_emits_no_domain_event() -> Result<()> .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_status(TaskStatus::InProgress), + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + ..Default::default() + }, ) .await; assert!(result.is_err(), "expected the update to be rejected"); @@ -2394,12 +2399,15 @@ async fn invariant_task_update_rule_augmenting_action_commits_and_broadcasts_nor .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_status(TaskStatus::InProgress), + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + ..Default::default() + }, ) .await?; assert_eq!( - updated.priority, - Some(Priority::High), + user_field(&updated, "task", "priority"), + Some(&json!("high")), "augmenting action must have run" ); @@ -2421,10 +2429,9 @@ async fn invariant_task_update_rule_augmenting_action_commits_and_broadcasts_nor } /// A `property_changed` invariant rule scoped to `task.status` must not fire -/// when a DIFFERENT task property changes via `update_task_node` — proves -/// `update_task_node_in_tx` reuses the same real exact/wildcard trigger-key -/// matching as the generic path, not a blanket "any update to this node" -/// match. Mirrors +/// when a DIFFERENT task property changes via `update_task_node` — the typed +/// update gets the same exact/wildcard trigger-key matching as the generic +/// path, not a blanket "any update to this node" match. Mirrors /// `invariant_update_rule_scoped_to_one_property_ignores_a_different_property_change`. #[tokio::test] async fn invariant_task_update_rule_scoped_to_one_property_ignores_a_different_property_change( @@ -2456,17 +2463,21 @@ async fn invariant_task_update_rule_scoped_to_one_property_ignores_a_different_p .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_due_date(Some("2026-01-01")), + TaskNodeUpdate { + due_date: Some(Some("2026-01-01".to_string())), + ..Default::default() + }, ) .await?; assert_eq!( - updated.priority, None, + user_field(&updated, "task", "priority"), + None, "a property-key-scoped invariant rule must not fire for an unrelated property change" ); assert_eq!( - updated.due_date.as_deref(), - Some("2026-01-01"), + user_field(&updated, "task", "due_date"), + Some(&json!("2026-01-01")), "the unrelated update itself must still have applied" ); @@ -2474,8 +2485,8 @@ async fn invariant_task_update_rule_scoped_to_one_property_ignores_a_different_p } /// `RuleClass::Reactive` rules triggered by `update_task_node`'s -/// `property_changed` remain completely unaffected by the new synchronous -/// wiring: still async, post-commit, requiring the real engine loop — unlike +/// `property_changed` are unaffected by the synchronous invariant +/// dispatch: still async, post-commit, requiring the real engine loop — unlike /// the invariant tests above, which never spawn one. Mirrors /// `reactive_update_rule_still_fires_asynchronously_post_commit` for the /// generic path. @@ -2513,12 +2524,16 @@ async fn reactive_task_update_rule_still_fires_asynchronously_post_commit() -> R .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_status(TaskStatus::InProgress), + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + ..Default::default() + }, ) .await?; // Must NOT be synchronous for a reactive rule. assert_eq!( - updated.priority, None, + user_field(&updated, "task", "priority"), + None, "a reactive rule's effect must not be visible synchronously" ); @@ -2594,7 +2609,10 @@ async fn user_edit_to_a_node_stamped_at_max_depth_still_fires_the_reactive_rule( .update_task_node( &node_id, version, - TaskNodeUpdate::new().with_status(TaskStatus::InProgress), + TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + ..Default::default() + }, ) .await?; diff --git a/packages/daemon/src/services/node_service.rs b/packages/daemon/src/services/node_service.rs index f38ca4c5b..8fddccfb6 100644 --- a/packages/daemon/src/services/node_service.rs +++ b/packages/daemon/src/services/node_service.rs @@ -1551,37 +1551,25 @@ impl GrpcNodeService for NodeServiceImpl { let this = self.route(&request).await?; let req = request.into_inner(); - let update = build_task_node_update( - req.status, - req.priority, - req.due_date, - req.started_at, - req.completed_at, - req.content, - ) - .map_err(Status::invalid_argument)?; + let update = TaskNodeUpdate { + status: req.status.as_deref().map(TaskStatus::from_value), + priority: optional_priority_clear(req.priority), + due_date: parse_optional_timestamp(req.due_date, "due_date") + .map_err(Status::invalid_argument)?, + started_at: parse_optional_timestamp(req.started_at, "started_at") + .map_err(Status::invalid_argument)?, + completed_at: parse_optional_timestamp(req.completed_at, "completed_at") + .map_err(Status::invalid_argument)?, + }; - let task = match this + match this .node_service .update_task_node(&req.node_id, req.version, update) .await { - Ok(t) => t, - Err(e) => return Err(typed_update_error_to_status(&this.node_service, e).await), - }; - - // Convert TaskNode back to Node for proto wire shape. Frontend reconstructs - // the typed view via task_node_to_typed_value on the Tauri side. - let node: Node = task.into_node(); - let node_type = node.node_type.clone(); - let node_id = node.id.clone(); - - Ok(Response::new(NodeResponse { - placement: None, - node_id, - node_type, - node_data: Some(node_to_proto(node)), - })) + Ok(node) => Ok(Response::new(node_response(node))), + Err(e) => Err(typed_update_error_to_status(&this.node_service, e).await), + } } async fn update_person_node( @@ -1616,7 +1604,7 @@ impl GrpcNodeService for NodeServiceImpl { let update = ProjectNodeUpdate { status: req.status, - priority: optional_string_clear(req.priority), + priority: optional_priority_clear(req.priority), start_date: parse_optional_timestamp(req.start_date, "start_date") .map_err(Status::invalid_argument)?, end_date: parse_optional_timestamp(req.end_date, "end_date") @@ -3072,51 +3060,9 @@ fn optional_string_clear(wrapper: Option) -> Option> pattern: outer `None` ⇒ field unset on the wire, which we -/// surface as "no change". When the wrapper is present, `clear=true` writes -/// `Some(None)` (clear value) and `clear=false` writes `Some(Some(parsed))`. -fn build_task_node_update( - status: Option, - priority: Option, - due_date: Option, - started_at: Option, - completed_at: Option, - content: Option, -) -> Result { - let status = match status { - None => None, - Some(s) => Some( - serde_json::from_value::(serde_json::Value::String(s.clone())) - .map_err(|e| format!("Invalid task status '{}': {}", s, e))?, - ), - }; - - let priority = match priority { - None => None, - Some(w) if w.clear => Some(None), - Some(w) => Some(Some(parse_task_priority(&w.value)?)), - }; - - let due_date = parse_optional_timestamp(due_date, "due_date")?; - let started_at = parse_optional_timestamp(started_at, "started_at")?; - let completed_at = parse_optional_timestamp(completed_at, "completed_at")?; - - Ok(TaskNodeUpdate { - status, - priority, - due_date, - started_at, - completed_at, - content, - }) -} - -fn parse_task_priority(value: &str) -> Result { - serde_json::from_value::(serde_json::Value::String(value.to_string())) - .map_err(|e| format!("Invalid task priority '{}': {}", value, e)) +/// [`optional_string_clear`] for a `priority` field on the shared scale. +fn optional_priority_clear(wrapper: Option) -> Option> { + optional_string_clear(wrapper).map(|value| value.as_deref().map(Priority::from_value)) } fn parse_optional_timestamp( @@ -4577,8 +4523,6 @@ mod tests { due_date: None, started_at: None, completed_at: None, - content: None, - properties: None, }); let err = svc @@ -4602,11 +4546,8 @@ mod tests { assert_eq!(json["node_id"], task_id); assert_eq!(json["expected"], 1); - // `update_task_node` now runs its version check inside the same - // transaction as the write (ADR-060 §2, wiring invariant-rule - // dispatch), reusing the real persisted version the transaction - // observed instead of a placeholder — the winning writer landed at - // version 2, so that's what's reported here. + // The version check runs inside the write's transaction and reports + // the persisted version it observed: the winning writer landed at 2. assert_eq!(json["actual"], 2); // current_node must be non-null — the handler fetches it before returning. @@ -4630,6 +4571,106 @@ mod tests { ); } + /// UpdateTaskNode answers with the node as stored: an archived task stays + /// archived in the response and keeps its extension field, and a cleared + /// priority is gone. + #[tokio::test] + async fn update_task_node_response_is_the_stored_node() { + let (svc, _tmp) = make_service().await; + let task_id = "d1b2c3d4-e5f6-7890-abcd-ef1234567890"; + svc.create_node(Request::new(crate::nodespace::CreateNodeRequest { + id: Some(task_id.to_string()), + node_type: "task".to_string(), + content: "Retired task".to_string(), + parent_id: None, + collections: Vec::new(), + collection_ids: Vec::new(), + lifecycle_status: Some("archived".to_string()), + properties: r#"{"priority":"high","custom:estimate":3}"#.to_string(), + position: None, + })) + .await + .unwrap(); + + let resp = svc + .update_task_node(Request::new(crate::nodespace::UpdateTaskNodeRequest { + node_id: task_id.to_string(), + version: 1, + status: Some("done".to_string()), + priority: Some(crate::nodespace::OptionalStringClear { + clear: true, + value: String::new(), + }), + due_date: None, + started_at: None, + completed_at: None, + })) + .await + .expect("typed task update succeeds") + .into_inner(); + + let data = resp.node_data.expect("response carries the node"); + assert_eq!(data.lifecycle_status, "archived"); + assert_eq!(data.version, 2); + // The stored shape: the extension field sits beside the core fields + // in the `task` bucket, and a cleared field holds `null`. + let properties: serde_json::Value = serde_json::from_str(&data.properties).unwrap(); + assert_eq!( + properties, + serde_json::json!({ + "task": { "status": "done", "priority": null, "custom:estimate": 3 } + }) + ); + + // What a client sees of it, through the typed conversion. + let node = svc.node_service.get_node(task_id).await.unwrap().unwrap(); + let typed = nodespace_core::models::node_to_typed_value(node).unwrap(); + assert_eq!(typed["lifecycleStatus"], "archived"); + assert_eq!(typed["status"], "done"); + assert!(typed.get("priority").is_none(), "{typed}"); + assert_eq!( + typed["properties"], + serde_json::json!({ "custom:estimate": 3 }) + ); + } + + /// An absent priority is left unchanged. + #[tokio::test] + async fn update_task_node_without_a_priority_keeps_it() { + let (svc, _tmp) = make_service().await; + let task_id = "e1b2c3d4-e5f6-7890-abcd-ef1234567890"; + svc.create_node(Request::new(crate::nodespace::CreateNodeRequest { + id: Some(task_id.to_string()), + node_type: "task".to_string(), + content: "Task".to_string(), + parent_id: None, + collections: Vec::new(), + collection_ids: Vec::new(), + lifecycle_status: None, + properties: r#"{"priority":"high"}"#.to_string(), + position: None, + })) + .await + .unwrap(); + + svc.update_task_node(Request::new(crate::nodespace::UpdateTaskNodeRequest { + node_id: task_id.to_string(), + version: 1, + status: Some("in_progress".to_string()), + priority: None, + due_date: None, + started_at: None, + completed_at: None, + })) + .await + .expect("typed task update succeeds"); + + let node = svc.node_service.get_node(task_id).await.unwrap().unwrap(); + let typed = nodespace_core::models::node_to_typed_value(node).unwrap(); + assert_eq!(typed["priority"], "high"); + assert_eq!(typed["status"], "in_progress"); + } + fn create_person_request(id: &str) -> Request { Request::new(crate::nodespace::CreateNodeRequest { id: Some(id.to_string()), diff --git a/packages/desktop-app/app-lib/src/commands/nodes.rs b/packages/desktop-app/app-lib/src/commands/nodes.rs index fb9a7a2fd..b89813fb6 100644 --- a/packages/desktop-app/app-lib/src/commands/nodes.rs +++ b/packages/desktop-app/app-lib/src/commands/nodes.rs @@ -6,7 +6,7 @@ use crate::types::{ node_to_typed_value as types_node_to_typed_value, nodes_to_typed_values as types_nodes_to_typed_values, DeleteResult, Node, NodeQuery, - NodeReference, NodeUpdate, PersonNodeUpdate, ProjectNodeUpdate, QueryNodeUpdate, + NodeReference, NodeUpdate, PersonNodeUpdate, Priority, ProjectNodeUpdate, QueryNodeUpdate, TaskNodeUpdate, }; use chrono::{DateTime, Utc}; @@ -1115,58 +1115,30 @@ pub async fn get_mentioning_roots( Ok(references) } -/// Build UpdateTaskNodeRequest from TaskNodeUpdate -fn task_update_to_proto(id: &str, version: i64, update: TaskNodeUpdate) -> UpdateTaskNodeRequest { - UpdateTaskNodeRequest { - node_id: id.to_string(), - version, - status: update.status.map(|s| s.as_str().to_string()), - priority: update.priority.map(|opt| match opt { - None => OptionalStringClear { - clear: true, - value: String::new(), - }, - Some(p) => OptionalStringClear { - clear: false, - value: p.as_str().to_string(), - }, - }), - due_date: update.due_date.map(|opt| match opt { - None => OptionalTimestampClear { - clear: true, - value: String::new(), - }, - Some(s) => OptionalTimestampClear { - clear: false, - value: s, - }, - }), - started_at: update.started_at.map(|opt| match opt { - None => OptionalTimestampClear { - clear: true, - value: String::new(), - }, - Some(s) => OptionalTimestampClear { - clear: false, - value: s, - }, - }), - completed_at: update.completed_at.map(|opt| match opt { - None => OptionalTimestampClear { - clear: true, - value: String::new(), - }, - Some(s) => OptionalTimestampClear { - clear: false, - value: s, - }, - }), - content: update.content, - properties: None, - } +/// Encode a tri-state update field for the proto's clearable wrapper: +/// `None` → unset (no change), `Some(None)` → clear, `Some(Some(v))` → set. +fn string_clear(value: Option>) -> Option { + value.map(|opt| OptionalStringClear { + clear: opt.is_none(), + value: opt.unwrap_or_default(), + }) +} + +/// [`string_clear`] for a `priority` field on the shared scale. +fn priority_clear(value: Option>) -> Option { + string_clear(value.map(|opt| opt.map(|priority| priority.as_str().to_string()))) +} + +/// Date variant of [`string_clear`]. +fn timestamp_clear(value: Option>) -> Option { + value.map(|opt| OptionalTimestampClear { + clear: opt.is_none(), + value: opt.unwrap_or_default(), + }) } -/// Update a task node with type-safe property updates +/// Update a task node's core fields (status, priority, due/started/completed +/// dates). #[tauri::command] pub async fn update_task_node( client: State<'_, GrpcClient>, @@ -1175,7 +1147,15 @@ pub async fn update_task_node( update: TaskNodeUpdate, ) -> Result { let mut c = client.echo_suppressed_client().await; - let req = task_update_to_proto(&id, version, update); + let req = UpdateTaskNodeRequest { + node_id: id, + version, + status: update.status.map(|status| status.as_str().to_string()), + priority: priority_clear(update.priority), + due_date: timestamp_clear(update.due_date), + started_at: timestamp_clear(update.started_at), + completed_at: timestamp_clear(update.completed_at), + }; let resp = c .update_task_node(Request::new(req)) .await @@ -1185,23 +1165,6 @@ pub async fn update_task_node( node_to_typed_value(node) } -/// Encode a tri-state update field for the proto's clearable wrapper: -/// `None` → unset (no change), `Some(None)` → clear, `Some(Some(v))` → set. -fn string_clear(value: Option>) -> Option { - value.map(|opt| OptionalStringClear { - clear: opt.is_none(), - value: opt.unwrap_or_default(), - }) -} - -/// Date variant of [`string_clear`]. -fn timestamp_clear(value: Option>) -> Option { - value.map(|opt| OptionalTimestampClear { - clear: opt.is_none(), - value: opt.unwrap_or_default(), - }) -} - /// Update a person node's core fields (first name, last name, email). #[tauri::command] pub async fn update_person_node( @@ -1240,7 +1203,7 @@ pub async fn update_project_node( node_id: id, version, status: update.status, - priority: string_clear(update.priority), + priority: priority_clear(update.priority), start_date: timestamp_clear(update.start_date), end_date: timestamp_clear(update.end_date), }; diff --git a/packages/desktop-app/app-lib/src/types.rs b/packages/desktop-app/app-lib/src/types.rs index b0e6c7f87..43afda2e3 100644 --- a/packages/desktop-app/app-lib/src/types.rs +++ b/packages/desktop-app/app-lib/src/types.rs @@ -9,10 +9,10 @@ pub use nodespace_types::{ node_to_typed_value, nodes_to_typed_values, AiChatMessage, AiChatNode, DeleteResult, EdgeField, - EnumValue, Node, NodeQuery, NodeReference, NodeUpdate, OrderBy, PersonNodeUpdate, + EnumValue, Node, NodeQuery, NodeReference, NodeUpdate, OrderBy, PersonNodeUpdate, Priority, ProjectNodeUpdate, QueryNodeUpdate, RelationshipCardinality, RelationshipDirection, SchemaField, SchemaNode, SchemaProtectionLevel, SchemaRelationship, TaskNode, TaskNodeUpdate, - TaskPriority, TaskStatus, ValidationError, + TaskStatus, ValidationError, }; use serde::{Deserialize, Serialize}; diff --git a/packages/desktop-app/app-lib/tests/it/adapter_contract_test.rs b/packages/desktop-app/app-lib/tests/it/adapter_contract_test.rs index e2a9b8d16..9ab4d9747 100644 --- a/packages/desktop-app/app-lib/tests/it/adapter_contract_test.rs +++ b/packages/desktop-app/app-lib/tests/it/adapter_contract_test.rs @@ -21,7 +21,7 @@ use nodespace_app_lib::commands::nodes::{ update_task_node, CreateNodeInput, InsertPositionInput, }; use nodespace_app_lib::types::{ - PersonNodeUpdate, QueryNodeUpdate, TaskNodeUpdate, TaskPriority, TaskStatus, + PersonNodeUpdate, Priority, QueryNodeUpdate, TaskNodeUpdate, TaskStatus, }; use nodespace_app_test_support::{SpawnedDaemon, TauriTestApp, DAEMON_CONNECT_TIMEOUT}; use serde_json::json; @@ -67,7 +67,7 @@ async fn task_tri_state_update_clear_set_no_change_matches_the_http_adapter_cont id.clone(), 1, TaskNodeUpdate { - priority: Some(Some(TaskPriority::High)), + priority: Some(Some(Priority::High)), status: Some(TaskStatus::InProgress), ..Default::default() }, diff --git a/packages/desktop-app/src/lib/services/adapter-core.ts b/packages/desktop-app/src/lib/services/adapter-core.ts index 0009554a4..1c377e930 100644 --- a/packages/desktop-app/src/lib/services/adapter-core.ts +++ b/packages/desktop-app/src/lib/services/adapter-core.ts @@ -295,7 +295,6 @@ export interface TaskNodeUpdatePatch { dueDate: ClearableField; startedAt: ClearableField; completedAt: ClearableField; - content?: string; } function clearable(value: string | null | undefined): ClearableField { @@ -304,6 +303,53 @@ function clearable(value: string | null | undefined): ClearableField { return { clear: false, value }; } +/** + * The fields each per-field typed update carries: its type's schema fields, by + * wire name. `satisfies Record` makes each list exactly the + * update interface's keys, so a field added to one can't be missed here. + */ +const TYPED_UPDATE_FIELDS = { + task: { + status: true, + priority: true, + dueDate: true, + startedAt: true, + completedAt: true + } satisfies Record, + person: { + firstName: true, + lastName: true, + email: true + } satisfies Record, + project: { + status: true, + priority: true, + startDate: true, + endDate: true + } satisfies Record +} as const; + +/** The core types whose typed update travels as one wire field per schema field. */ +export type PerFieldTypedUpdateType = keyof typeof TYPED_UPDATE_FIELDS; + +/** The fields `nodeType`'s typed update carries, by wire name. */ +export function typedUpdateFieldNames(nodeType: PerFieldTypedUpdateType): string[] { + return Object.keys(TYPED_UPDATE_FIELDS[nodeType]); +} + +/** + * The keys of a typed update request body that are not fields of that type's + * update (`version` travels beside them). `content` and `properties` are the + * ones a caller is likely to send: both belong to the generic node update. + */ +export function unknownTypedUpdateKeys( + nodeType: PerFieldTypedUpdateType, + body: Record +): string[] { + const fields: Record = TYPED_UPDATE_FIELDS[nodeType]; + return Object.keys(body).filter((key) => key !== 'version' && !Object.hasOwn(fields, key)); +} + /** * The single authoritative mapping from the frontend's `TaskNodeUpdate` shape * (plain `null` = clear, `undefined`/absent = no change) to the tri-state wire @@ -317,7 +363,6 @@ export function buildTaskNodeUpdatePatch(update: TaskNodeUpdate): TaskNodeUpdate dueDate: clearable(update.dueDate), startedAt: clearable(update.startedAt), completedAt: clearable(update.completedAt), - content: update.content, }; } diff --git a/packages/desktop-app/src/lib/services/backend-adapter.ts b/packages/desktop-app/src/lib/services/backend-adapter.ts index 7925845f3..7e7d6a50a 100644 --- a/packages/desktop-app/src/lib/services/backend-adapter.ts +++ b/packages/desktop-app/src/lib/services/backend-adapter.ts @@ -116,9 +116,8 @@ class TauriAdapter implements BackendAdapter { // the tri-state clear/set/no-change encoding for this path is done by // the Rust #[tauri::command] handler, not here. That handler and // dev-proxy's buildTaskNodeUpdatePatch call site can't literally share - // code across the language boundary; ADR-048 decision 2 (a Rust-side - // integration test driving the Tauri command layer directly) is the - // planned way to prove the two encodings still agree. + // code across the language boundary; the adapter contract tests (ADR-048 + // decision 2) drive both and hold the two encodings to one result. return withDiagnosticLogging( 'updateTaskNode', () => invoke('update_task_node', { id, version, update }), diff --git a/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts b/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts index 33b5be31a..0bb4395e1 100644 --- a/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts +++ b/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts @@ -807,21 +807,6 @@ interface Subscription { /** Core node types with typed fields and a typed backend update. */ export type TypedNodeType = 'task' | 'person' | 'project' | 'query'; -/** - * The keys `updateTypedNode()` accepts for a type: its writable typed core - * fields (`TYPED_CORE_FIELDS` minus the read-only system fields, which the - * typed update rejects), plus `content` for task, whose typed update also - * carries content. - */ -function typedUpdateKeys(nodeType: TypedNodeType): string[] { - return [...writableTypedCoreKeys(nodeType), ...(TYPED_UPDATE_EXTRA_KEYS[nodeType] ?? [])]; -} - -/** Keys a type's typed update carries beyond its typed core fields. */ -const TYPED_UPDATE_EXTRA_KEYS: Partial> = { - task: ['content'] -}; - /** * Typed fields staged for a node but not yet sent (see `updateTypedNode()`), * with the callbacks of every write that staged them — settled by whichever @@ -3589,7 +3574,7 @@ export class SharedNodeStore { } /** - * Update a task node's typed fields (status, priority, dates) and content. + * Update a task node's typed fields (status, priority, dates). * See `updateTypedNode()` for the write path. */ updateTaskNode( @@ -3678,7 +3663,7 @@ export class SharedNodeStore { return; } - const allowed = typedUpdateKeys(nodeType); + const allowed = writableTypedCoreKeys(nodeType); const fields = Object.keys(update).filter( (key) => allowed.includes(key) && update[key] !== undefined ); diff --git a/packages/desktop-app/src/lib/types/task-node.ts b/packages/desktop-app/src/lib/types/task-node.ts index 6d5429a2e..61c8625dc 100644 --- a/packages/desktop-app/src/lib/types/task-node.ts +++ b/packages/desktop-app/src/lib/types/task-node.ts @@ -178,8 +178,9 @@ export function setTaskDueDate(node: TaskNode, dueDate: string | undefined): Tas /** * Partial update structure for task nodes * - * Supports updating task-specific properties (status, priority, dueDate) - * and node fields (content). All fields are optional - only include fields to update. + * Carries the task schema's fields and nothing else: `content` and extension + * fields (`custom:…`) are written through the generic node update. All fields + * are optional - only include fields to update. * * This interface matches the Rust `TaskNodeUpdate` struct for type-safe CRUD operations. * @@ -193,9 +194,6 @@ export function setTaskDueDate(node: TaskNode, dueDate: string | undefined): Tas * status: 'done', * dueDate: null // Explicitly clear the field * }; - * - * // Update content (hub field) - * const update: TaskNodeUpdate = { content: 'Updated task description' }; * ``` */ export interface TaskNodeUpdate { @@ -213,9 +211,6 @@ export interface TaskNodeUpdate { /** Update completed_at date (type-specific field) - null to clear */ completedAt?: string | null; - - /** Update content (node field) */ - content?: string; } /** diff --git a/packages/desktop-app/src/tests/e2e/adapter-contract.e2e.ts b/packages/desktop-app/src/tests/e2e/adapter-contract.e2e.ts index 9b95efa4c..32f8e2f0b 100644 --- a/packages/desktop-app/src/tests/e2e/adapter-contract.e2e.ts +++ b/packages/desktop-app/src/tests/e2e/adapter-contract.e2e.ts @@ -92,7 +92,6 @@ describe('Adapter contract: shape parity (no daemon required)', () => { dueDate: { clear: true }, startedAt: undefined, completedAt: undefined, - content: undefined, }); }); diff --git a/packages/desktop-app/src/tests/services/shared-node-store-extended.test.ts b/packages/desktop-app/src/tests/services/shared-node-store-extended.test.ts index eab1eadf5..85926a25e 100644 --- a/packages/desktop-app/src/tests/services/shared-node-store-extended.test.ts +++ b/packages/desktop-app/src/tests/services/shared-node-store-extended.test.ts @@ -209,15 +209,6 @@ describe('SharedNodeStore - Extended Coverage', () => { expect(updated?.completedAt).toBe('2025-03-07'); }); - it('should update task content', () => { - store.setNode(taskNode, viewerSource, true); - - store.updateTaskNode('task-1', { content: 'Updated task' }, viewerSource); - - const updated = store.getNode('task-1'); - expect(updated?.content).toBe('Updated task'); - }); - it('should warn when updating non-existent task node', () => { store.updateTaskNode('non-existent', { status: 'done' }, viewerSource); diff --git a/packages/desktop-app/src/tests/unit/adapter-core.test.ts b/packages/desktop-app/src/tests/unit/adapter-core.test.ts index 43472113c..aae0a10f5 100644 --- a/packages/desktop-app/src/tests/unit/adapter-core.test.ts +++ b/packages/desktop-app/src/tests/unit/adapter-core.test.ts @@ -5,7 +5,10 @@ import { encodeInsertPosition, normalizeChildrenTree, insertPosition, + typedUpdateFieldNames, + unknownTypedUpdateKeys, } from '$lib/services/adapter-core'; +import { writableTypedCoreKeys } from '$lib/types/typed-core-fields'; describe('adapter-core: buildCreateNodeFields', () => { it('defaults optional fields for a minimal input', () => { @@ -51,10 +54,59 @@ describe('adapter-core: buildTaskNodeUpdatePatch (tri-state clearable encoding)' expect(patch.dueDate).toEqual({ clear: false, value: '2026-01-01T00:00:00Z' }); }); - it('passes status and content straight through (no clear semantics)', () => { - const patch = buildTaskNodeUpdatePatch({ status: 'in_progress', content: 'updated body' }); + it('passes status straight through (no clear semantics)', () => { + const patch = buildTaskNodeUpdatePatch({ status: 'in_progress' }); expect(patch.status).toBe('in_progress'); - expect(patch.content).toBe('updated body'); + }); + + it('names the keys of a request body that are not fields of the typed update', () => { + expect( + unknownTypedUpdateKeys('task', { + version: 3, + status: 'done', + priority: null, + dueDate: '2026-01-01' + }) + ).toEqual([]); + expect( + unknownTypedUpdateKeys('task', { + version: 3, + content: 'Renamed', + properties: { 'custom:x': 1 } + }) + ).toEqual(['content', 'properties']); + expect(unknownTypedUpdateKeys('person', { version: 1, firstName: 'Ada' })).toEqual([]); + // A task field is not a person field, and an inherited Object key is not a field. + expect(unknownTypedUpdateKeys('person', { dueDate: null, toString: 'x' })).toEqual([ + 'dueDate', + 'toString' + ]); + expect(unknownTypedUpdateKeys('project', { endDate: null, content: 'Renamed' })).toEqual([ + 'content' + ]); + }); + + // The store sends a type's writable typed core keys; the dev-proxy and the + // Tauri command refuse anything outside the update's fields. A key in one + // list and not the other is a write the backend rejects. + it.each(['task', 'person', 'project'] as const)( + 'the %s update fields are the keys the store sends for it', + (nodeType) => { + expect(typedUpdateFieldNames(nodeType).sort()).toEqual( + [...writableTypedCoreKeys(nodeType)].sort() + ); + } + ); + + it('carries the task schema fields only', () => { + const patch = buildTaskNodeUpdatePatch({ status: 'done' }); + expect(Object.keys(patch).sort()).toEqual([ + 'completedAt', + 'dueDate', + 'priority', + 'startedAt', + 'status' + ]); }); it('treats a null priority the same as any other clearable field', () => { diff --git a/packages/dev-tools/src/dev-proxy.ts b/packages/dev-tools/src/dev-proxy.ts index 73bfeaab6..c9070b8f6 100644 --- a/packages/dev-tools/src/dev-proxy.ts +++ b/packages/dev-tools/src/dev-proxy.ts @@ -19,9 +19,11 @@ import { buildTaskNodeUpdatePatch, encodeInsertPosition, HTTP_ROUTE_PATTERNS, + unknownTypedUpdateKeys, type ChildPlacement, type CreatedNode, type InsertPosition, + type PerFieldTypedUpdateType, } from '../../desktop-app/src/lib/services/adapter-core.ts'; import { storageNodeToApiFields } from '../../desktop-app/src/lib/services/node-normalize.ts'; import { createNodeSpaceClients, createRunOnceGuard } from './grpc-client.ts'; @@ -239,6 +241,20 @@ function error(code: string, message: string, status = 500, conflictData?: unkno return json(body, status); } +/** + * A typed update carries its type's schema fields only; `content` and + * `properties` go through PATCH /api/nodes/:id. A body naming anything else + * is refused rather than dropped, as the Tauri command refuses it. + */ +function refuseUnknownTypedUpdateKeys( + nodeType: PerFieldTypedUpdateType, + body: Record +): Response | null { + const unknown = unknownTypedUpdateKeys(nodeType, body); + if (unknown.length === 0) return null; + return error('INVALID_ARGUMENT', `Not a ${nodeType} update field: ${unknown.join(', ')}`, 400); +} + /** * Copy the body's values for the proto's repeated string `fields` onto * `request`. An absent or empty list is left off the request; a value that is @@ -524,6 +540,8 @@ async function handleRequest(req: Request): Promise { const nodeId = decodeURIComponent(taskMatch[1]); try { const body = await req.json() as Record; + const refused = refuseUnknownTypedUpdateKeys('task', body); + if (refused) return refused; // buildTaskNodeUpdatePatch is the single authoritative tri-state // encoder (packages/desktop-app/src/lib/services/adapter-core.ts) — // absent field = no change, null = clear, value = set. Reused here so @@ -535,7 +553,6 @@ async function handleRequest(req: Request): Promise { dueDate?: string | null; startedAt?: string | null; completedAt?: string | null; - content?: string; }); const request = { nodeId, @@ -544,9 +561,7 @@ async function handleRequest(req: Request): Promise { priority: patch.priority ?? null, dueDate: patch.dueDate ?? null, startedAt: patch.startedAt ?? null, - completedAt: patch.completedAt ?? null, - content: patch.content ?? null, - properties: body.properties !== undefined ? JSON.stringify(body.properties) : null + completedAt: patch.completedAt ?? null }; const res = await call( (nodeClient as unknown as Record).updateTaskNode, @@ -565,6 +580,8 @@ async function handleRequest(req: Request): Promise { const nodeId = decodeURIComponent(personMatch[1]); try { const body = await req.json() as Record; + const refused = refuseUnknownTypedUpdateKeys('person', body); + if (refused) return refused; const patch = buildPersonNodeUpdatePatch(body as PersonNodeUpdate); const request = { nodeId, @@ -590,6 +607,8 @@ async function handleRequest(req: Request): Promise { const nodeId = decodeURIComponent(projectMatch[1]); try { const body = await req.json() as Record; + const refused = refuseUnknownTypedUpdateKeys('project', body); + if (refused) return refused; const patch = buildProjectNodeUpdatePatch(body as ProjectNodeUpdate); const request = { nodeId, diff --git a/packages/nodespace-types/src/convert.rs b/packages/nodespace-types/src/convert.rs index 0d1ac9bee..5e885ab90 100644 --- a/packages/nodespace-types/src/convert.rs +++ b/packages/nodespace-types/src/convert.rs @@ -1,14 +1,14 @@ use chrono::{DateTime, NaiveDate, Utc}; -use std::str::FromStr; use crate::ai_chat::{AiChatMessage, AiChatNode}; use crate::core_type::CoreNodeType; use crate::node::{Node, NodeEnvelope}; use crate::person::PersonNode; +use crate::priority::priority_prop; use crate::project::{ProjectNode, DEFAULT_PROJECT_STATUS}; use crate::query::{QueryFields, QueryNode}; use crate::schema::SchemaNode; -use crate::task::{TaskNode, TaskPriority, TaskStatus}; +use crate::task::{TaskNode, TaskStatus}; fn normalize_date_field(s: &str) -> String { if NaiveDate::parse_from_str(s, "%Y-%m-%d").is_ok() { @@ -317,16 +317,13 @@ pub fn flat_properties_view(typed: &serde_json::Value) -> serde_json::Value { fn task_node_to_value(node: Node) -> Result { let props = &node.properties; - let status: TaskStatus = props + let status = props .get("status") .and_then(|v| v.as_str()) - .and_then(|s| s.parse().ok()) + .map(TaskStatus::from_value) .unwrap_or_default(); - let priority = props - .get("priority") - .and_then(|v| v.as_str()) - .map(|s| TaskPriority::from_str(s).unwrap_or_default()); + let priority = priority_prop(props); let due_date = props .get("due_date") @@ -386,7 +383,7 @@ fn person_node_to_value(node: Node) -> Result { fn project_node_to_value(node: Node) -> Result { let props = &node.properties; let status = string_prop(props, "status").unwrap_or_else(|| DEFAULT_PROJECT_STATUS.to_string()); - let priority = string_prop(props, "priority"); + let priority = priority_prop(props); let start_date = props .get("start_date") .and_then(|v| v.as_str()) diff --git a/packages/nodespace-types/src/lib.rs b/packages/nodespace-types/src/lib.rs index 4752b1409..786bf879e 100644 --- a/packages/nodespace-types/src/lib.rs +++ b/packages/nodespace-types/src/lib.rs @@ -19,6 +19,7 @@ mod helpers; mod incompatible_database; mod node; mod person; +mod priority; mod project; mod query; mod schema; @@ -42,6 +43,7 @@ pub use node::{ ValidationError, }; pub use person::{PersonNode, PersonNodeUpdate}; +pub use priority::Priority; pub use project::{ProjectNode, ProjectNodeUpdate, DEFAULT_PROJECT_STATUS}; pub use query::{ FilterOperator, FilterType, QueryFields, QueryFilter, QueryGeneratedBy, QueryNode, @@ -53,4 +55,4 @@ pub use schema::{ SchemaField, SchemaFieldType, SchemaNode, SchemaProtectionLevel, SchemaRelationship, }; pub use skill::{SkillNode, DEFAULT_SKILL_MAX_ITERATIONS, SKILL_NODE_TYPE}; -pub use task::{TaskNode, TaskNodeUpdate, TaskPriority, TaskStatus}; +pub use task::{TaskNode, TaskNodeUpdate, TaskStatus}; diff --git a/packages/nodespace-types/src/person.rs b/packages/nodespace-types/src/person.rs index ded80ce62..e6c0debfd 100644 --- a/packages/nodespace-types/src/person.rs +++ b/packages/nodespace-types/src/person.rs @@ -28,7 +28,7 @@ pub struct PersonNode { /// Each field is tri-state: absent leaves it unchanged, `null` clears it, and /// a string sets it. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct PersonNodeUpdate { #[serde( default, @@ -78,6 +78,13 @@ impl PersonNodeUpdate { mod tests { use super::*; + /// The update carries the person schema's fields only; naming anything + /// else is an error rather than a silently dropped write. + #[test] + fn an_unknown_key_is_rejected() { + assert!(serde_json::from_str::(r#"{"content": "Ada"}"#).is_err()); + } + #[test] fn absent_null_and_value_are_distinct() { let update: PersonNodeUpdate = diff --git a/packages/core/src/models/priority.rs b/packages/nodespace-types/src/priority.rs similarity index 55% rename from packages/core/src/models/priority.rs rename to packages/nodespace-types/src/priority.rs index da5c2cd2c..a8218ff3f 100644 --- a/packages/core/src/models/priority.rs +++ b/packages/nodespace-types/src/priority.rs @@ -1,59 +1,44 @@ //! Priority scale shared by the core node types that carry a `priority` field //! //! `task.priority` and `project.priority` use one vocabulary, so one enum -//! backs both: their `core_values`, the rank the query service sorts them by, -//! and the drift guard in `core_schemas.rs` that keeps the three in step. +//! backs both: their typed wire fields, their `core_values`, the rank the +//! query service sorts them by, and the drift guard in core's +//! `core_schemas.rs` that keeps the three in step. use serde::{Deserialize, Serialize}; -use std::str::FromStr; -/// Priority enumeration -/// +use crate::core_type::CoreNodeType; + /// The relative-urgency scale of the core `task` and `project` types. -/// Values use lowercase format for consistency across all layers: -/// - "highest" - Highest priority -/// - "high" - High priority -/// - "medium" - Medium priority (default) -/// - "low" - Low priority -/// - "lowest" - Lowest priority -/// - User-defined priorities via schema extension (e.g., "critical", "urgent") /// -/// Core priorities are strongly typed; user-defined priorities use `User(String)`. -/// This aligns with the schema system's `core_values` / `user_values` model. -#[derive(Debug, Clone, PartialEq, Eq, Default)] +/// Core priorities are strongly typed; a priority a user added to the schema +/// (`user_values`, e.g. "critical") is `User(String)`. There is no default: +/// a node without a priority has none. +#[derive(Debug, Clone, PartialEq, Eq)] pub enum Priority { - /// Highest priority Highest, - /// High priority High, - /// Medium priority (default) - #[default] Medium, - /// Low priority Low, - /// Lowest priority Lowest, /// User-defined priority (extended via schema) User(String), } -impl FromStr for Priority { - type Err = String; - - fn from_str(s: &str) -> Result { +impl Priority { + /// The priority a stored or wire string names. Every string is one: a + /// value outside the core scale is a user-defined priority. + pub fn from_value(s: &str) -> Self { match s { - "highest" => Ok(Self::Highest), - "high" => Ok(Self::High), - "medium" => Ok(Self::Medium), - "low" => Ok(Self::Low), - "lowest" => Ok(Self::Lowest), - // Any other value is treated as user-defined - other => Ok(Self::User(other.to_string())), + "highest" => Self::Highest, + "high" => Self::High, + "medium" => Self::Medium, + "low" => Self::Low, + "lowest" => Self::Lowest, + other => Self::User(other.to_string()), } } -} -impl Priority { /// The core node types whose `priority` field uses this scale. /// /// The query service ranks `priority` by [`Self::rank`] only when the @@ -61,20 +46,15 @@ impl Priority { /// user-defined type's bare field, say) is its own vocabulary and sorts as /// plain text. Every type listed here must declare `priority` with exactly /// this enum's core values, which - /// `test_priority_variants_match_core_values_bidirectionally` in + /// `test_priority_variants_match_core_values_bidirectionally` in core's /// `core_schemas.rs` checks for each of them. - pub const NODE_TYPES: [crate::models::CoreNodeType; 2] = [ - crate::models::CoreNodeType::Task, - crate::models::CoreNodeType::Project, - ]; + pub const NODE_TYPES: [CoreNodeType; 2] = [CoreNodeType::Task, CoreNodeType::Project]; /// Whether `node_type`'s `priority` field uses this scale. pub fn applies_to(node_type: &str) -> bool { - crate::models::CoreNodeType::from_id(node_type) - .is_some_and(|core| Self::NODE_TYPES.contains(&core)) + CoreNodeType::from_id(node_type).is_some_and(|core| Self::NODE_TYPES.contains(&core)) } - /// Convert priority to string representation pub fn as_str(&self) -> &str { match self { Self::Highest => "highest", @@ -96,10 +76,6 @@ impl Priority { /// separate two user values, callers must break that tie on the value /// string to keep the ordering total; see `QueryService::resolve_order_field` /// and `QueryService::compare_priority_values`, which both do exactly that. - /// - /// Kept in sync with `core_values` by - /// `test_priority_variants_match_core_values_bidirectionally` - /// in `core_schemas.rs`. pub fn rank(&self) -> u8 { match self { Self::Highest => 0, @@ -123,33 +99,21 @@ impl Priority { /// has no `Priority` at all — so it lives here as the shared constant /// both ordering paths rank it by. pub const ABSENT_RANK: i8 = -1; - - /// Check if this is a core (built-in) priority - pub fn is_core(&self) -> bool { - !matches!(self, Self::User(_)) - } - - /// Check if this is a user-defined priority - pub fn is_user_defined(&self) -> bool { - matches!(self, Self::User(_)) - } } impl Serialize for Priority { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - serializer.serialize_str(self.as_str()) + fn serialize(&self, s: S) -> Result { + s.serialize_str(self.as_str()) } } impl<'de> Deserialize<'de> for Priority { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - Ok(Self::from_str(&s).unwrap()) // from_str never fails: unknown strings map to User(_) + fn deserialize>(d: D) -> Result { + String::deserialize(d).map(|s| Self::from_value(&s)) } } + +/// Read a stored `priority` value. Anything but a string is no priority. +pub(crate) fn priority_prop(props: &serde_json::Value) -> Option { + props.get("priority")?.as_str().map(Priority::from_value) +} diff --git a/packages/nodespace-types/src/project.rs b/packages/nodespace-types/src/project.rs index 62f99ad3d..5131f0c7d 100644 --- a/packages/nodespace-types/src/project.rs +++ b/packages/nodespace-types/src/project.rs @@ -2,6 +2,7 @@ use serde::{Deserialize, Serialize}; use crate::helpers::deserialize_clearable; use crate::node::NodeEnvelope; +use crate::priority::Priority; use crate::task::flexible_date; /// Default `status` for a project that has none stored, matching the project @@ -15,9 +16,9 @@ pub const DEFAULT_PROJECT_STATUS: &str = "planning"; /// promoted to the top level; they map directly to the TypeScript /// `ProjectNode` interface. /// -/// `status` and `priority` stay strings rather than enums: both are -/// user-extensible (`user_values`), and the schema, not this struct, owns the -/// vocabulary — the service layer validates writes against it. +/// `status` stays a string: it is user-extensible (`user_values`), and the +/// schema, not this struct, owns the vocabulary — the service layer validates +/// writes against it. `priority` is the scale `task` shares. #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct ProjectNode { @@ -27,7 +28,7 @@ pub struct ProjectNode { pub envelope: NodeEnvelope, pub status: String, #[serde(skip_serializing_if = "Option::is_none")] - pub priority: Option, + pub priority: Option, #[serde(skip_serializing_if = "Option::is_none")] pub start_date: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -41,7 +42,7 @@ pub struct ProjectNode { /// sets it. Dates accept `YYYY-MM-DD` or RFC 3339 and are stored as /// `YYYY-MM-DD`. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct ProjectNodeUpdate { #[serde(default, skip_serializing_if = "Option::is_none")] pub status: Option, @@ -50,7 +51,7 @@ pub struct ProjectNodeUpdate { skip_serializing_if = "Option::is_none", deserialize_with = "deserialize_clearable" )] - pub priority: Option>, + pub priority: Option>, #[serde( default, skip_serializing_if = "Option::is_none", @@ -82,8 +83,10 @@ impl ProjectNodeUpdate { if let Some(status) = &self.status { patch.insert("status".to_string(), serde_json::json!(status)); } + if let Some(priority) = &self.priority { + patch.insert("priority".to_string(), serde_json::json!(priority)); + } for (key, value) in [ - ("priority", &self.priority), ("start_date", &self.start_date), ("end_date", &self.end_date), ] { @@ -124,6 +127,23 @@ mod tests { ); } + /// The update carries the project schema's fields only; naming anything + /// else is an error rather than a silently dropped write. + #[test] + fn an_unknown_key_is_rejected() { + assert!(serde_json::from_str::(r#"{"content": "Apollo"}"#).is_err()); + } + + #[test] + fn priority_is_the_shared_scale() { + let update: ProjectNodeUpdate = serde_json::from_str(r#"{"priority": "highest"}"#).unwrap(); + assert_eq!(update.priority, Some(Some(Priority::Highest))); + assert_eq!( + update.to_properties_patch(), + serde_json::json!({ "priority": "highest" }) + ); + } + #[test] fn invalid_date_is_rejected() { let result: Result = diff --git a/packages/nodespace-types/src/task.rs b/packages/nodespace-types/src/task.rs index 65af179cb..f3a4fbf6e 100644 --- a/packages/nodespace-types/src/task.rs +++ b/packages/nodespace-types/src/task.rs @@ -1,7 +1,8 @@ use serde::{Deserialize, Serialize}; -use std::str::FromStr; +use crate::helpers::deserialize_clearable; use crate::node::NodeEnvelope; +use crate::priority::Priority; #[derive(Debug, Clone, PartialEq, Eq, Default)] pub enum TaskStatus { @@ -13,21 +14,19 @@ pub enum TaskStatus { User(String), } -impl FromStr for TaskStatus { - type Err = String; - - fn from_str(s: &str) -> Result { - Ok(match s { +impl TaskStatus { + /// The status a stored or wire string names. Every string is one: a + /// value outside the core statuses is a user-defined status. + pub fn from_value(s: &str) -> Self { + match s { "open" => Self::Open, "in_progress" => Self::InProgress, "done" => Self::Done, "cancelled" => Self::Cancelled, other => Self::User(other.to_string()), - }) + } } -} -impl TaskStatus { pub fn as_str(&self) -> &str { match self { Self::Open => "open", @@ -37,70 +36,23 @@ impl TaskStatus { Self::User(s) => s.as_str(), } } -} - -impl Serialize for TaskStatus { - fn serialize(&self, s: S) -> Result { - s.serialize_str(self.as_str()) - } -} - -impl<'de> Deserialize<'de> for TaskStatus { - fn deserialize>(d: D) -> Result { - let s = String::deserialize(d)?; - Ok(Self::from_str(&s).unwrap()) - } -} - -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub enum TaskPriority { - Highest, - High, - #[default] - Medium, - Low, - Lowest, - User(String), -} - -impl FromStr for TaskPriority { - type Err = String; - - fn from_str(s: &str) -> Result { - Ok(match s { - "highest" => Self::Highest, - "high" => Self::High, - "medium" => Self::Medium, - "low" => Self::Low, - "lowest" => Self::Lowest, - other => Self::User(other.to_string()), - }) - } -} -impl TaskPriority { - pub fn as_str(&self) -> &str { - match self { - Self::Highest => "highest", - Self::High => "high", - Self::Medium => "medium", - Self::Low => "low", - Self::Lowest => "lowest", - Self::User(s) => s.as_str(), - } + /// Whether this is one of the core statuses rather than a user-defined + /// one. + pub fn is_core(&self) -> bool { + !matches!(self, Self::User(_)) } } -impl Serialize for TaskPriority { +impl Serialize for TaskStatus { fn serialize(&self, s: S) -> Result { s.serialize_str(self.as_str()) } } -impl<'de> Deserialize<'de> for TaskPriority { +impl<'de> Deserialize<'de> for TaskStatus { fn deserialize>(d: D) -> Result { - let s = String::deserialize(d)?; - Ok(Self::from_str(&s).unwrap()) + String::deserialize(d).map(|s| Self::from_value(&s)) } } @@ -117,7 +69,7 @@ pub struct TaskNode { pub envelope: NodeEnvelope, pub status: TaskStatus, #[serde(skip_serializing_if = "Option::is_none")] - pub priority: Option, + pub priority: Option, #[serde(skip_serializing_if = "Option::is_none")] pub due_date: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -126,14 +78,27 @@ pub struct TaskNode { pub completed_at: Option, } -/// Partial update for task-specific properties, received from the frontend. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +/// Partial update for a task's core fields, received from the frontend. +/// +/// `status` has no clear path (the schema requires it); the other fields are +/// tri-state: absent leaves the field unchanged, `null` clears it, and a value +/// sets it. Dates accept `YYYY-MM-DD` or RFC 3339 and are stored as +/// `YYYY-MM-DD`. +/// +/// The update carries the task schema's fields and nothing else. `content` is +/// an envelope field and extension fields (`custom:…`) live in `properties`; +/// both are written through the generic node update. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct TaskNodeUpdate { - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] pub status: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub priority: Option>, + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "deserialize_clearable" + )] + pub priority: Option>, #[serde( default, skip_serializing_if = "Option::is_none", @@ -152,8 +117,40 @@ pub struct TaskNodeUpdate { deserialize_with = "flexible_date::deserialize_with_null" )] pub completed_at: Option>, - #[serde(skip_serializing_if = "Option::is_none")] - pub content: Option, +} + +impl TaskNodeUpdate { + /// True when the update changes nothing. + pub fn is_empty(&self) -> bool { + self.status.is_none() + && self.priority.is_none() + && self.due_date.is_none() + && self.started_at.is_none() + && self.completed_at.is_none() + } + + /// The flat, bare-key properties patch this update writes (`{"status": + /// "done"}`); a cleared field is written as `null`. The service layer + /// moves the keys into the `task` storage bucket. + pub fn to_properties_patch(&self) -> serde_json::Value { + let mut patch = serde_json::Map::new(); + if let Some(status) = &self.status { + patch.insert("status".to_string(), serde_json::json!(status)); + } + if let Some(priority) = &self.priority { + patch.insert("priority".to_string(), serde_json::json!(priority)); + } + for (key, value) in [ + ("due_date", &self.due_date), + ("started_at", &self.started_at), + ("completed_at", &self.completed_at), + ] { + if let Some(value) = value { + patch.insert(key.to_string(), serde_json::json!(value)); + } + } + serde_json::Value::Object(patch) + } } /// Flexible date deserializer: accepts ISO8601 full timestamps, date-only @@ -163,10 +160,6 @@ pub struct TaskNodeUpdate { /// - Field absent from JSON → `None` (don't change) /// - Field present as `null` → `Some(None)` (clear the value) /// - Field present as a string → `Some(Some(dt))` (set to this value) -/// -/// Note: The old `src-tauri/types.rs` mirror used `Option>` as -/// the intermediate which caused JSON `null` to map to `None` (no-op) instead -/// of `Some(None)` (clear). This is the corrected implementation. pub(crate) mod flexible_date { use chrono::{DateTime, NaiveDate, Utc}; use serde::{Deserialize, Deserializer}; @@ -205,46 +198,71 @@ pub(crate) mod flexible_date { mod tests { use super::*; - /// This crate carries its own `TaskPriority` copy because it cannot depend - /// on `nodespace-core`, and it is the one that serializes onto the wire. - /// The core-side bidirectional drift test guards the *core* copy against - /// `task.priority`'s schema seed; nothing links this copy to either, so a - /// value added to the scale but missed here would degrade to - /// `TaskPriority::User(_)` at the frontend boundary while every suite - /// stayed green. Pinning the literals here is the cheapest available - /// stand-in for that missing link. #[test] - fn task_priority_core_scale_parses_to_named_variants() { - let scale = [ - ("highest", TaskPriority::Highest), - ("high", TaskPriority::High), - ("medium", TaskPriority::Medium), - ("low", TaskPriority::Low), - ("lowest", TaskPriority::Lowest), - ]; + fn task_node_update_null_clears_priority() { + let update: TaskNodeUpdate = serde_json::from_str(r#"{"priority": null}"#).unwrap(); + assert_eq!(update.priority, Some(None)); + assert!(!update.is_empty()); + } - for (literal, expected) in &scale { - let parsed: TaskPriority = literal.parse().expect("from_str is infallible"); - assert_eq!( - parsed, *expected, - "'{}' must parse to the named TaskPriority variant, not TaskPriority::User(_) — \ - keep this copy in step with task.priority's core_values in core_schemas.rs.", - literal - ); + #[test] + fn task_node_update_absent_priority_is_unchanged() { + let update: TaskNodeUpdate = serde_json::from_str(r#"{"status": "done"}"#).unwrap(); + assert_eq!(update.priority, None); + assert_eq!(update.status, Some(TaskStatus::Done)); + } + + #[test] + fn task_node_update_sets_core_and_user_priorities() { + let update: TaskNodeUpdate = serde_json::from_str(r#"{"priority": "high"}"#).unwrap(); + assert_eq!(update.priority, Some(Some(Priority::High))); + let update: TaskNodeUpdate = serde_json::from_str(r#"{"priority": "urgent"}"#).unwrap(); + assert_eq!( + update.priority, + Some(Some(Priority::User("urgent".to_string()))) + ); + } + + /// `content` is an envelope field and `properties` holds extension + /// fields; neither is part of the typed update, and naming one is an + /// error rather than a silently dropped write. + #[test] + fn task_node_update_rejects_content_and_properties() { + for json in [ + r#"{"content": "Renamed"}"#, + r#"{"status": "done", "properties": {"custom:estimate": 3}}"#, + ] { assert!( - !matches!(parsed, TaskPriority::User(_)), - "'{}' fell through to the user-defined catch-all", - literal - ); - assert_eq!( - parsed.as_str(), - *literal, - "'{}' must round-trip through as_str()", - literal + serde_json::from_str::(json).is_err(), + "{json} must not deserialize as a TaskNodeUpdate" ); } } + #[test] + fn empty_task_node_update_is_empty() { + let update: TaskNodeUpdate = serde_json::from_str("{}").unwrap(); + assert!(update.is_empty()); + } + + #[test] + fn patch_carries_only_the_fields_the_update_names() { + let update = TaskNodeUpdate { + status: Some(TaskStatus::InProgress), + priority: Some(None), + due_date: Some(Some("2026-03-01".to_string())), + ..Default::default() + }; + assert_eq!( + update.to_properties_patch(), + serde_json::json!({ + "status": "in_progress", + "priority": null, + "due_date": "2026-03-01" + }) + ); + } + #[test] fn task_node_update_null_clears_due_date() { let json = r#"{"dueDate": null}"#; diff --git a/packages/proto/proto/node_service.proto b/packages/proto/proto/node_service.proto index 7d23e43d1..332de833e 100644 --- a/packages/proto/proto/node_service.proto +++ b/packages/proto/proto/node_service.proto @@ -65,11 +65,11 @@ service NodeService { rpc GetIncomingMentions(MentionTargetRequest) returns (MentionIdsResponse); rpc GetMentioningRoots(MentionTargetRequest) returns (NodeReferenceListResponse); - // Type-specific updates + // Typed core-field updates for task, person, project and query: an unset + // field is unchanged, a clearable field can be cleared. The daemon writes + // them through the generic update pipeline; the response carries the stored + // node. rpc UpdateTaskNode(UpdateTaskNodeRequest) returns (NodeResponse); - // Typed core-field updates for person, project and query (the same - // tri-state semantics as UpdateTaskNode). The daemon writes them through the - // generic update pipeline; the response carries the stored node. rpc UpdatePersonNode(UpdatePersonNodeRequest) returns (NodeResponse); rpc UpdateProjectNode(UpdateProjectNodeRequest) returns (NodeResponse); rpc UpdateQueryNode(UpdateQueryNodeRequest) returns (NodeResponse); @@ -936,8 +936,8 @@ message NodeReferenceListResponse { // UpdateTaskNode // --------------------------------------------------------------------------- -// TaskNodeUpdate values use a tri-state encoding to match the -// Option> pattern in packages/core/src/models/task_node.rs: +// Typed update values use a tri-state encoding to match the +// Option> fields of the update structs in nodespace-types: // - field unset → no change (outer None) // - field present, clear=true → clear the value (Some(None)) // - field present, clear=false, @@ -957,25 +957,17 @@ message OptionalTimestampClear { string value = 2; } -// status uses a plain optional string because TaskStatus has no "clear" path -// in TaskNodeUpdate (the field type is Option, not -// Option>). +// The task schema's fields and nothing else: content and extension fields +// (`custom:…`) are written through UpdateNode. status has no clear path (the +// task schema requires it). message UpdateTaskNodeRequest { string node_id = 1; int64 version = 2; - optional string status = 3; // TaskStatus serialized as string + optional string status = 3; optional OptionalStringClear priority = 4; optional OptionalTimestampClear due_date = 5; - reserved 6; - reserved "assignee"; optional OptionalTimestampClear started_at = 7; optional OptionalTimestampClear completed_at = 8; - // Generic content/properties updates also accepted; mirrors the - // catch-all fields on the Rust TaskNodeUpdate when it falls back to a - // NodeUpdate for content changes. - optional string content = 9; - // JSON-encoded properties patch; unset = no change. - optional string properties = 10; } // ---------------------------------------------------------------------------