diff --git a/packages/agent/src/local_agent/tools.rs b/packages/agent/src/local_agent/tools.rs index 1a438564f..d9381ef32 100644 --- a/packages/agent/src/local_agent/tools.rs +++ b/packages/agent/src/local_agent/tools.rs @@ -6858,6 +6858,53 @@ mod tests { stored.result["properties"] ); } + + /// A collection's description is a declared field, so the agent sets + /// it on create and changes it on update like any other field value, + /// and it lands in the collection bucket. + #[tokio::test(flavor = "multi_thread")] + async fn create_and_update_set_a_collection_description() { + let (ns, _tmp) = make_test_service().await; + let executor = plain_executor(ns.clone()); + + let created = executor + .execute( + "create_node", + json!({ + "content": "Clients", + "node_type": "collection", + "field_values": { "description": "Accounts we bill" }, + }), + ) + .await + .unwrap(); + assert!(!created.is_error, "{created:?}"); + let node_id = strip_node_uri(created.result["id"].as_str().expect("id")).to_string(); + + let stored = ns.get_node(&node_id).await.unwrap().unwrap(); + assert_eq!( + stored.properties, + json!({ "collection": { "description": "Accounts we bill" } }) + ); + + let updated = executor + .execute( + "update_node", + json!({ + "id": node_id, + "field_values": { "description": "Accounts we bill, one page each" }, + }), + ) + .await + .unwrap(); + assert!(!updated.is_error, "{updated:?}"); + + let stored = ns.get_node(&node_id).await.unwrap().unwrap(); + assert_eq!( + stored.properties, + json!({ "collection": { "description": "Accounts we bill, one page each" } }) + ); + } } mod update_node_noop_gate { diff --git a/packages/agent/tests/golden/prompt_assembly/resident_system_prompt.golden b/packages/agent/tests/golden/prompt_assembly/resident_system_prompt.golden index 28ca2928a..631bd9573 100644 --- a/packages/agent/tests/golden/prompt_assembly/resident_system_prompt.golden +++ b/packages/agent/tests/golden/prompt_assembly/resident_system_prompt.golden @@ -4,7 +4,9 @@ Call exactly one tool. Do not answer the user in prose when a tool applies. Current date: 2026-06-15 Active model: gemma-4-E4B-it-Q4_K_M -COLLECTIONS: Engineering, Q3 Planning +COLLECTIONS: +- Engineering — Design notes, ADRs and tickets for the product team +- Q3 Planning MENTIONED ENTITIES (already resolved — use these ids directly, do not ask for one): - "Northwind Trading" (company_sold_to) id=01J8ZQ3K7X2N4P6R8T0V2W4Y6A diff --git a/packages/agent/tests/golden/prompt_assembly/resident_workspace_context.golden b/packages/agent/tests/golden/prompt_assembly/resident_workspace_context.golden index f88d6b8e5..9a046182e 100644 --- a/packages/agent/tests/golden/prompt_assembly/resident_workspace_context.golden +++ b/packages/agent/tests/golden/prompt_assembly/resident_workspace_context.golden @@ -1,4 +1,6 @@ -COLLECTIONS: Engineering, Q3 Planning +COLLECTIONS: +- Engineering — Design notes, ADRs and tickets for the product team +- Q3 Planning MENTIONED ENTITIES (already resolved — use these ids directly, do not ask for one): - "Northwind Trading" (company_sold_to) id=01J8ZQ3K7X2N4P6R8T0V2W4Y6A diff --git a/packages/agent/tests/it/prompt_assembly_snapshot.rs b/packages/agent/tests/it/prompt_assembly_snapshot.rs index c75555a47..e4fc0632b 100644 --- a/packages/agent/tests/it/prompt_assembly_snapshot.rs +++ b/packages/agent/tests/it/prompt_assembly_snapshot.rs @@ -121,7 +121,9 @@ use nodespace_core::db::ResolvedEntity; use nodespace_core::markdown::{prepare_nodes_from_template, NodeTemplate, PreparedNode}; use nodespace_core::models::schema::EnumValue; use nodespace_core::models::{Node, SchemaField, SchemaProtectionLevel, SkillNode}; -use nodespace_core::ops::context_ops::{EntityResolution, PlaybookInfo, WorkspaceContext}; +use nodespace_core::ops::context_ops::{ + CollectionSummary, EntityResolution, PlaybookInfo, WorkspaceContext, +}; use nodespace_core::ops::entity_types_block::EntityTypeDescriptor; use nodespace_core::services::render_subtree_markdown; @@ -287,7 +289,14 @@ fn fixture_schema_release() -> nodespace_core::models::SchemaNode { fn fixture_workspace_context() -> WorkspaceContext { WorkspaceContext { - collections: vec!["Engineering".to_string(), "Q3 Planning".to_string()], + collections: vec![ + CollectionSummary { + name: "Engineering".to_string(), + description: "Design notes, ADRs and tickets for the product team".to_string(), + }, + // Exercises the no-description rendering branch. + CollectionSummary::named("Q3 Planning"), + ], active_playbooks: vec![ PlaybookInfo { name: "Sprint Close-Out".to_string(), diff --git a/packages/cli/tests/it/cli_integration.rs b/packages/cli/tests/it/cli_integration.rs index baa049b31..8144fba76 100644 --- a/packages/cli/tests/it/cli_integration.rs +++ b/packages/cli/tests/it/cli_integration.rs @@ -3759,3 +3759,90 @@ async fn routing_with_no_selection_pins_the_resolved_default_database_id() { let _ = shutdown.send(()); } + +/// `node create --type collection` makes the collection the name identifies: +/// the same id an import or the app's create derives, with the description in +/// the collection bucket. `node update --property description=…` changes it. +#[tokio::test] +async fn node_create_and_update_set_a_collection_description() { + use nodespace_core::services::collection_service::deterministic_collection_id; + + let (sock, shutdown, _tempdir) = spawn_test_daemon().await; + let mut client = connect(&sock, DatabaseIdInterceptor::none()) + .await + .expect("connect"); + let mut raw = connect(&sock, DatabaseIdInterceptor::none()) + .await + .expect("raw connect"); + + let create = |name: &str| { + commands::node::NodeAction::Create(commands::node::CreateArgs { + node_type: "collection".into(), + content: Some(name.into()), + parent: None, + properties: vec![("description".into(), serde_json::json!("Accounts we bill"))], + collections: vec![], + collection_ids: vec![], + }) + }; + commands::node::run(&mut client, create("Clients"), true) + .await + .expect("create collection"); + + let id = deterministic_collection_id("Clients"); + let stored_description = |raw: &mut NodeClient| { + let mut raw = raw.clone(); + let id = id.clone(); + async move { + let node = raw + .get_node(GetNodeRequest { node_id: id }) + .await + .expect("the collection exists at its deterministic id") + .into_inner() + .node_data + .expect("node_data"); + let props: serde_json::Value = + serde_json::from_str(&node.properties).expect("parse properties"); + assert!( + props.get("description").is_none(), + "no flat description key: {props}" + ); + props["collection"]["description"].clone() + } + }; + assert_eq!(stored_description(&mut raw).await, "Accounts we bill"); + + // A second create of the same name is the same collection, so it is + // refused rather than stored as a duplicate under another id. + let err = commands::node::run(&mut client, create("clients"), true) + .await + .expect_err("a duplicate collection name is refused"); + assert!( + format!("{err:#}").contains("Already exists"), + "expected an already-exists error, got: {err:#}" + ); + + commands::node::run( + &mut client, + commands::node::NodeAction::Update(commands::node::UpdateArgs { + id: id.clone(), + content: None, + properties: vec![( + "description".into(), + serde_json::json!("Accounts we bill, one page each"), + )], + collections: vec![], + collection_ids: vec![], + remove_collection_ids: vec![], + }), + true, + ) + .await + .expect("update description"); + assert_eq!( + stored_description(&mut raw).await, + "Accounts we bill, one page each" + ); + + let _ = shutdown.send(()); +} diff --git a/packages/core/src/db/sqlite_store/relationships.rs b/packages/core/src/db/sqlite_store/relationships.rs index f9f474e0e..a040868a1 100644 --- a/packages/core/src/db/sqlite_store/relationships.rs +++ b/packages/core/src/db/sqlite_store/relationships.rs @@ -1142,13 +1142,22 @@ impl SqliteStore { Ok(member_ids) } - pub async fn get_all_collection_names(&self) -> Result> { + /// Every collection's name and description, sorted by name. The + /// description is `None` when the collection has none. + /// + /// Only a text value is read as a description. Nothing type-checks a text + /// field on write, and reading a stored number or boolean as a `String` + /// panics in the driver rather than returning an error. + pub async fn get_all_collection_descriptions(&self) -> Result)>> { let mut rows = self .read() .await? .query( &format!( - "SELECT content FROM node WHERE {} ORDER BY content ASC", + "SELECT content, \ + CASE WHEN json_type(properties, '$.collection.description') = 'text' \ + THEN json_extract(properties, '$.collection.description') END \ + FROM node WHERE {} ORDER BY content ASC", crate::db::schema::is_a_sql( "node_type", &[crate::models::CoreNodeType::Collection] @@ -1157,13 +1166,13 @@ impl SqliteStore { (), ) .await - .context("Failed to get all collection names")?; + .context("Failed to get all collection descriptions")?; - let mut names = Vec::new(); + let mut collections = Vec::new(); while let Some(row) = rows.next().await? { - names.push(row.get(0)?); + collections.push((row.get(0)?, row.get(1)?)); } - Ok(names) + Ok(collections) } pub async fn get_all_collections_with_member_counts( diff --git a/packages/core/src/models/core_schemas.rs b/packages/core/src/models/core_schemas.rs index cdbe93d05..33702895d 100644 --- a/packages/core/src/models/core_schemas.rs +++ b/packages/core/src/models/core_schemas.rs @@ -2220,6 +2220,22 @@ mod tests { } } + #[test] + fn test_collection_declares_only_an_optional_description() { + // A collection is grouping only (ADR-083 §5): its one field says what + // the collection is for, and it carries no access-control fields. + let schemas = get_core_schemas(); + let collection = schemas.iter().find(|s| s.id == "collection").unwrap(); + + let names: Vec<&str> = collection.fields.iter().map(|f| f.name.as_str()).collect(); + assert_eq!(names, ["description"]); + + let description = collection.get_field("description").unwrap(); + assert_eq!(description.field_type, crate::models::SchemaFieldType::Text); + assert_eq!(description.required, Some(false)); + assert_eq!(description.default, None); + } + #[test] fn test_person_email_field_is_optional() { // The description is seeded into every new database and shown to users diff --git a/packages/core/src/ops/collection_ops.rs b/packages/core/src/ops/collection_ops.rs index 15196a4ba..3aba5e414 100644 --- a/packages/core/src/ops/collection_ops.rs +++ b/packages/core/src/ops/collection_ops.rs @@ -290,44 +290,71 @@ pub async fn get_collection_by_name( Ok(GetCollectionByNameOutput { collection }) } +/// The id a new collection named `name` takes, or `AlreadyExists` when a node +/// already holds it. +/// +/// The id is a pure hash of the normalized name (`deterministic_collection_id`), +/// so a collection created here converges with the same-named collection +/// created on another device or by import (`CollectionService::create_collection` +/// derives the same id) instead of minting a random UUID that becomes a +/// duplicate. +/// +/// Two checks, in this order: +/// +/// 1. **The id.** It is independent of lifecycle_status — any existing node at +/// that id, active OR archived, already occupies the row the INSERT needs. +/// A name lookup alone would miss an archived collection +/// (`CollectionService::get_collection_by_name` — correctly, for its own +/// callers — only matches active ones), and the INSERT would then fail with +/// an opaque primary-key-constraint error, since `NodeService::create_node` +/// has no get-or-create/upsert semantics for collections (unlike, e.g., the +/// `database-settings` singleton). +/// 2. **The active name.** A renamed collection keeps the id of its first +/// name, so the id of its new name is free: without this check, a create +/// under that new name would make a second active collection of the name. +/// For the same reason the id of a renamed collection's first name stays +/// taken. +pub(crate) async fn new_collection_id( + node_service: &Arc, + name: &str, +) -> Result { + let deterministic_id = deterministic_collection_id(name); + let in_the_way = match node_service.get_node(&deterministic_id).await? { + Some(holder) => Some(holder), + // Trimmed, as the id is, so a padded name cannot slip past. + None => { + CollectionService::new(node_service.store(), node_service) + .get_collection_by_name(name.trim()) + .await? + } + }; + match in_the_way { + Some(existing) => Err(collection_in_the_way(&existing)), + None => Ok(deterministic_id), + } +} + +/// The refusal for a collection name that `existing` already holds. It names +/// that collection and its id, which is what the caller needs to act on it +/// instead. +fn collection_in_the_way(existing: &Node) -> OpsError { + OpsError::AlreadyExists { + id: format!("collection '{}' (id {})", existing.content, existing.id), + } +} + pub async fn create_collection( node_service: &Arc, input: CreateCollectionInput, ) -> Result { - // The id this create will use is a pure hash of the normalized name - // (`deterministic_collection_id`), independent of lifecycle_status — so - // any existing node at that id, active OR archived, already occupies the - // row this INSERT needs. Check by id, not by - // `CollectionService::get_collection_by_name` (which — correctly, for its - // own callers — only matches `lifecycle_status = 'active'` collections): - // an archived collection would pass that check, then fail the INSERT - // below with an opaque primary-key-constraint error, since - // `NodeService::create_node` has no get-or-create/upsert semantics for - // the `collection` node type (unlike, e.g., the `database-settings` - // singleton). - let deterministic_id = deterministic_collection_id(&input.name); - if node_service - .get_node(&deterministic_id) - .await - .map_err(OpsError::from)? - .is_some() - { - return Err(OpsError::AlreadyExists { - id: input.name.clone(), - }); - } + let deterministic_id = new_collection_id(node_service, &input.name).await?; let properties = if input.description.is_empty() { serde_json::json!({}) } else { - serde_json::json!({ "description": input.description }) + serde_json::json!({ "collection": { "description": input.description } }) }; - // Deterministic id from the (globally-unique) name so a UI-created collection - // converges with the same-named collection created on another device or by import - // (`CollectionService::create_collection` derives the same id) instead of minting a - // random UUID that syncs up as a duplicate. The id-existence check above already - // rejects a local duplicate (including an archived one). let collection_id = node_service .create_node_with_parent(CreateNodeParams { id: Some(deterministic_id), @@ -356,9 +383,7 @@ pub async fn rename_collection( .map_err(OpsError::from)? { if existing.id != input.collection_id { - return Err(OpsError::AlreadyExists { - id: input.new_name.clone(), - }); + return Err(collection_in_the_way(&existing)); } } @@ -391,6 +416,7 @@ pub async fn rename_collection( mod tests { use super::*; use crate::db::SqliteStore; + use crate::ops::node_ops; use crate::services::NodeService; use std::sync::Arc; use tempfile::TempDir; @@ -411,17 +437,23 @@ mod tests { name: "my-collection".to_string(), description: String::new(), }; - create_collection(&svc, input).await.unwrap(); + let created = create_collection(&svc, input).await.unwrap(); let dup = CreateCollectionInput { name: "my-collection".to_string(), description: String::new(), }; let err = create_collection(&svc, dup).await.unwrap_err(); + assert_already_exists(&err, "my-collection", &created.collection_id); + } + + /// The refusal names the collection in the way and its id, so the caller + /// can update that collection instead. + fn assert_already_exists(err: &OpsError, existing_name: &str, existing_id: &str) { + let expected = format!("collection '{existing_name}' (id {existing_id})"); assert!( - matches!(err, OpsError::AlreadyExists { ref id } if id == "my-collection"), - "expected AlreadyExists, got {:?}", - err + matches!(err, OpsError::AlreadyExists { id } if *id == expected), + "expected AlreadyExists for {expected}, got {err:?}" ); } @@ -458,6 +490,332 @@ mod tests { ); } + fn generic_create(name: &str, properties: serde_json::Value) -> node_ops::CreateNodeInput { + node_ops::CreateNodeInput { + id: None, + node_type: "collection".to_string(), + content: name.to_string(), + parent_id: None, + position: InsertPositionOwned::End, + properties, + collections: Vec::new(), + collection_ids: Vec::new(), + lifecycle_status: None, + } + } + + #[tokio::test] + async fn create_collection_stores_the_description_in_the_collection_bucket() { + let (svc, _tmp) = make_service().await; + + let created = create_collection( + &svc, + CreateCollectionInput { + name: "Clients".to_string(), + description: "Accounts we bill".to_string(), + }, + ) + .await + .unwrap(); + + let node = svc.get_node(&created.collection_id).await.unwrap().unwrap(); + assert_eq!( + node.properties, + serde_json::json!({ "collection": { "description": "Accounts we bill" } }), + "the description is in the collection bucket and nowhere else" + ); + } + + #[tokio::test] + async fn create_collection_without_a_description_stores_none() { + let (svc, _tmp) = make_service().await; + + let created = create_collection( + &svc, + CreateCollectionInput { + name: "Clients".to_string(), + description: String::new(), + }, + ) + .await + .unwrap(); + + let node = svc.get_node(&created.collection_id).await.unwrap().unwrap(); + assert_eq!(node.properties, serde_json::json!({ "collection": {} })); + } + + /// The generic create is what the CLI's `node create --type collection` + /// and the agent's `create_node` call. It must make the same node the + /// collection op and an import make, not a second collection of that name + /// under a random id. + #[tokio::test] + async fn generic_create_of_a_collection_uses_the_deterministic_id() { + let (svc, _tmp) = make_service().await; + + let created = node_ops::create_node( + &svc, + generic_create( + "Clients", + serde_json::json!({ "description": "Accounts we bill" }), + ), + ) + .await + .unwrap(); + + assert_eq!(created.node_id, deterministic_collection_id("Clients")); + let node = svc.get_node(&created.node_id).await.unwrap().unwrap(); + assert_eq!( + node.properties, + serde_json::json!({ "collection": { "description": "Accounts we bill" } }) + ); + } + + #[tokio::test] + async fn generic_create_of_an_existing_collection_name_returns_already_exists() { + let (svc, _tmp) = make_service().await; + + let created = create_collection( + &svc, + CreateCollectionInput { + name: "Clients".to_string(), + description: String::new(), + }, + ) + .await + .unwrap(); + + // Names are compared case-insensitively, as the id is. + let err = node_ops::create_node(&svc, generic_create("clients", serde_json::json!({}))) + .await + .unwrap_err(); + assert_already_exists(&err, "Clients", &created.collection_id); + } + + /// A subtype of collection is a collection: it is named the same way, so + /// it takes the name-derived id, and its inherited description lands in + /// the collection bucket. + #[tokio::test] + async fn generic_create_of_a_collection_subtype_uses_the_deterministic_id() { + let (svc, _tmp) = make_service().await; + crate::schema::handle_create_schema( + &svc, + serde_json::json!({ + "name": "Shelf", + "extends": "collection", + "fields": [ + { "name": "room", "type": "text", "protection": "user", "indexed": false } + ] + }), + ) + .await + .unwrap(); + + let mut input = generic_create( + "Cookbooks", + serde_json::json!({ "description": "Recipes we kept", "room": "kitchen" }), + ); + input.node_type = "shelf".to_string(); + let created = node_ops::create_node(&svc, input).await.unwrap(); + + assert_eq!(created.node_id, deterministic_collection_id("Cookbooks")); + let node = svc.get_node(&created.node_id).await.unwrap().unwrap(); + assert_eq!( + node.properties, + serde_json::json!({ + "shelf": { "room": "kitchen" }, + "collection": { "description": "Recipes we kept" } + }) + ); + + // The agent's workspace context lists it like any other collection. + let listed = CollectionService::new(svc.store(), &svc) + .get_all_collection_descriptions() + .await + .unwrap(); + assert_eq!( + listed, + [("Cookbooks".to_string(), Some("Recipes we kept".to_string()))] + ); + } + + /// A renamed collection keeps the id of its first name, so the id check + /// alone would let a second collection take its new name. + #[tokio::test] + async fn create_refuses_the_name_of_a_renamed_collection() { + let (svc, _tmp) = make_service().await; + + let created = create_collection( + &svc, + CreateCollectionInput { + name: "Clients".to_string(), + description: String::new(), + }, + ) + .await + .unwrap(); + let version = svc + .get_node(&created.collection_id) + .await + .unwrap() + .unwrap() + .version; + rename_collection( + &svc, + RenameCollectionInput { + collection_id: created.collection_id.clone(), + new_name: "Accounts".to_string(), + version, + }, + ) + .await + .unwrap(); + + // Padded and differently-cased spellings are the same name. + for name in ["Accounts", "Clients", " accounts "] { + let err = node_ops::create_node(&svc, generic_create(name, serde_json::json!({}))) + .await + .unwrap_err(); + assert_already_exists(&err, "Accounts", &created.collection_id); + + let err = create_collection( + &svc, + CreateCollectionInput { + name: name.to_string(), + description: String::new(), + }, + ) + .await + .unwrap_err(); + assert_already_exists(&err, "Accounts", &created.collection_id); + } + } + + #[tokio::test] + async fn generic_update_sets_changes_and_clears_a_collection_description() { + let (svc, _tmp) = make_service().await; + let created = node_ops::create_node(&svc, generic_create("Clients", serde_json::json!({}))) + .await + .unwrap(); + + let update = |properties: serde_json::Value| node_ops::UpdateNodeInput { + node_id: created.node_id.clone(), + version: None, + node_type: None, + content: None, + properties: Some(properties), + add_to_collections: Vec::new(), + add_to_collection_ids: Vec::new(), + remove_from_collection_ids: Vec::new(), + lifecycle_status: None, + }; + let stored = || async { + svc.get_node(&created.node_id) + .await + .unwrap() + .unwrap() + .properties + }; + + node_ops::update_node( + &svc, + update(serde_json::json!({ "description": "Accounts" })), + ) + .await + .unwrap(); + assert_eq!( + stored().await, + serde_json::json!({ "collection": { "description": "Accounts" } }) + ); + + node_ops::update_node( + &svc, + update(serde_json::json!({ "description": "Accounts we bill" })), + ) + .await + .unwrap(); + assert_eq!( + stored().await, + serde_json::json!({ "collection": { "description": "Accounts we bill" } }) + ); + + // The generic update clears a field by writing null, which reads as + // no description. + node_ops::update_node(&svc, update(serde_json::json!({ "description": null }))) + .await + .unwrap(); + assert_eq!( + stored().await, + serde_json::json!({ "collection": { "description": null } }) + ); + let listed = CollectionService::new(svc.store(), &svc) + .get_all_collection_descriptions() + .await + .unwrap(); + assert_eq!(listed, [("Clients".to_string(), None)]); + } + + /// A text field is not type-checked on write, and the CLI parses + /// `--property description=42` as a number. The listing runs on every + /// agent turn, so a stored non-text value must read as no description + /// rather than fail it. + #[tokio::test] + async fn a_non_text_collection_description_is_listed_as_none() { + let (svc, _tmp) = make_service().await; + for (name, description) in [ + ("Numbered", serde_json::json!(42)), + ("Flagged", serde_json::json!(true)), + ("Nested", serde_json::json!({ "text": "Accounts" })), + ] { + node_ops::create_node( + &svc, + generic_create(name, serde_json::json!({ "description": description })), + ) + .await + .unwrap(); + } + + let listed = CollectionService::new(svc.store(), &svc) + .get_all_collection_descriptions() + .await + .unwrap(); + assert_eq!( + listed, + [ + ("Flagged".to_string(), None), + ("Nested".to_string(), None), + ("Numbered".to_string(), None), + ] + ); + } + + #[tokio::test] + async fn collection_descriptions_are_listed_by_name() { + let (svc, _tmp) = make_service().await; + for (name, description) in [("Research", ""), ("Clients", "Accounts we bill")] { + create_collection( + &svc, + CreateCollectionInput { + name: name.to_string(), + description: description.to_string(), + }, + ) + .await + .unwrap(); + } + + let listed = CollectionService::new(svc.store(), &svc) + .get_all_collection_descriptions() + .await + .unwrap(); + assert_eq!( + listed, + [ + ("Clients".to_string(), Some("Accounts we bill".to_string())), + ("Research".to_string(), None), + ] + ); + } + #[tokio::test] async fn rename_collection_to_same_name_succeeds() { let (svc, _tmp) = make_service().await; @@ -489,7 +847,7 @@ mod tests { async fn rename_collection_to_existing_name_returns_already_exists() { let (svc, _tmp) = make_service().await; - create_collection( + let alpha = create_collection( &svc, CreateCollectionInput { name: "alpha".to_string(), @@ -520,10 +878,6 @@ mod tests { ) .await .unwrap_err(); - assert!( - matches!(err, OpsError::AlreadyExists { ref id } if id == "alpha"), - "expected AlreadyExists, got {:?}", - err - ); + assert_already_exists(&err, "alpha", &alpha.collection_id); } } diff --git a/packages/core/src/ops/context_ops.rs b/packages/core/src/ops/context_ops.rs index c69815325..7e8653887 100644 --- a/packages/core/src/ops/context_ops.rs +++ b/packages/core/src/ops/context_ops.rs @@ -39,7 +39,7 @@ use super::OpsError; /// Assembled workspace context from the database. #[derive(Default)] pub struct WorkspaceContext { - pub collections: Vec, + pub collections: Vec, pub active_playbooks: Vec, /// Schemas semantically relevant to the current query (may be empty), /// already described across their `extends` chains. Held as descriptors @@ -116,6 +116,97 @@ pub struct PlaybookInfo { pub description: String, } +/// A collection, as the context lists it. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct CollectionSummary { + pub name: String, + /// What the collection is for. Empty when it has no description. + pub description: String, +} + +impl CollectionSummary { + /// A collection with no description. + pub fn named(name: impl Into) -> Self { + Self { + name: name.into(), + description: String::new(), + } + } +} + +/// Longest description shown for one collection, in characters. +/// +/// A description says what a collection is for, which a sentence covers. The +/// cap keeps one long description from crowding out the rest of the list. +const MAX_CHARS_PER_COLLECTION_DESCRIPTION: usize = 80; + +/// Share of the context budget that collection descriptions may take, as a +/// divisor of `max_chars`. +/// +/// The collections section is rendered first, so without a cap a workspace +/// with many described collections would spend the budget the entity and +/// schema tiers below it need. Collections past the cap are listed by name. +/// +/// Like the rest of the `max_chars` budget, this share is counted in bytes, +/// so descriptions in a multi-byte script reach it sooner. +const COLLECTION_DESCRIPTIONS_BUDGET_DIVISOR: usize = 4; + +/// A collection description as one line of at most +/// [`MAX_CHARS_PER_COLLECTION_DESCRIPTION`] characters, ending in `…` when cut. +fn collection_description_line(description: &str) -> String { + let one_line = description.split_whitespace().collect::>().join(" "); + let kept = leading_chars(&one_line, MAX_CHARS_PER_COLLECTION_DESCRIPTION); + if kept.len() < one_line.len() { + format!("{}…", kept.trim_end()) + } else { + one_line + } +} + +/// The collections section of the context, or `None` when there is nothing to +/// list or it does not fit in `max_chars`. +/// +/// With no descriptions it is one line of names. Once any collection has a +/// description it is one collection per line, `name — description`, so that a +/// comma inside a description cannot read as the start of another name; a +/// collection without a description stays a bare name. +fn render_collections_section( + collections: &[CollectionSummary], + max_chars: usize, +) -> Option { + if collections.is_empty() { + return None; + } + + let names_only = || { + let names: Vec<&str> = collections.iter().map(|c| c.name.as_str()).collect(); + format!("COLLECTIONS: {}\n", names.join(", ")) + }; + + let mut descriptions_budget = max_chars / COLLECTION_DESCRIPTIONS_BUDGET_DIVISOR; + let mut any_described = false; + let mut listed = String::from("COLLECTIONS:\n"); + for collection in collections { + let description = collection_description_line(&collection.description); + if description.is_empty() || description.len() > descriptions_budget { + listed.push_str(&format!("- {}\n", collection.name)); + } else { + descriptions_budget -= description.len(); + any_described = true; + listed.push_str(&format!("- {} — {}\n", collection.name, description)); + } + } + + // Descriptions give way before names do: a list too long to fit with them + // is still worth showing without. + let section = if any_described && listed.len() <= max_chars { + listed + } else { + names_only() + }; + (section.len() <= max_chars).then_some(section) +} + // --------------------------------------------------------------------------- // Builder // --------------------------------------------------------------------------- @@ -629,12 +720,18 @@ pub async fn build_workspace_context( query: Option<&str>, entity_query: Option<&str>, ) -> Result { - // Fetch collection names + // Fetch collection names and descriptions let collection_service = CollectionService::new(node_service.store(), node_service); let collections = collection_service - .get_all_collection_names() + .get_all_collection_descriptions() .await - .unwrap_or_default(); + .unwrap_or_default() + .into_iter() + .map(|(name, description)| CollectionSummary { + name, + description: description.unwrap_or_default(), + }) + .collect(); // Fetch active playbooks let playbook_nodes = node_service @@ -819,11 +916,8 @@ impl WorkspaceContext { let mut out = String::new(); // Collections section - if !self.collections.is_empty() { - let section = format!("COLLECTIONS: {}\n", self.collections.join(", ")); - if out.len() + section.len() <= max_chars { - out.push_str(§ion); - } + if let Some(section) = render_collections_section(&self.collections, max_chars) { + out.push_str(§ion); } // Resolved entities section. @@ -982,7 +1076,9 @@ mod tests { fn sample_context() -> WorkspaceContext { WorkspaceContext { - collections: vec!["Projects".into(), "Clients".into(), "Research".into()], + collections: ["Projects", "Clients", "Research"] + .map(CollectionSummary::named) + .to_vec(), active_playbooks: vec![PlaybookInfo { name: "Task completion".into(), description: "When task.status -> Done, evaluate project progress".into(), @@ -1469,19 +1565,117 @@ mod tests { #[test] fn format_for_prompt_collections_only() { - let ctx = WorkspaceContext { - collections: vec!["Projects".into(), "Clients".into()], - active_playbooks: vec![], - relevant_schemas: vec![], - related_schemas: vec![], - semantic_schema_count: 0, - resolved_entities: EntityResolution::NotRun, - }; + let ctx = collections_context(vec![ + CollectionSummary::named("Projects"), + CollectionSummary::named("Clients"), + ]); let output = ctx.format_for_prompt(4000); - assert!(output.contains("COLLECTIONS:")); - assert!(output.contains("Projects")); - assert!(output.contains("Clients")); - assert!(!output.contains("ACTIVE PLAYBOOKS:")); + assert_eq!(output, "COLLECTIONS: Projects, Clients\n"); + } + + fn collections_context(collections: Vec) -> WorkspaceContext { + WorkspaceContext { + collections, + ..Default::default() + } + } + + fn described_collection(name: &str, description: &str) -> CollectionSummary { + CollectionSummary { + name: name.to_string(), + description: description.to_string(), + } + } + + #[test] + fn a_described_collection_renders_its_description_after_its_name() { + let ctx = collections_context(vec![ + described_collection("Clients", "Accounts we bill, one page per client"), + CollectionSummary::named("Research"), + ]); + + assert_eq!( + ctx.format_for_prompt(4000), + "COLLECTIONS:\n- Clients — Accounts we bill, one page per client\n- Research\n", + "a collection without a description stays a bare name" + ); + } + + #[test] + fn a_long_collection_description_is_cut_to_one_short_line() { + let long = format!("Notes\nfrom every {}", "meeting ".repeat(30)); + let ctx = collections_context(vec![described_collection("Meetings", &long)]); + + let output = ctx.format_for_prompt(4000); + let line = output.lines().nth(1).unwrap(); + let description = line.strip_prefix("- Meetings — ").unwrap(); + + assert!(description.starts_with("Notes from every meeting")); + assert!(description.ends_with('…')); + assert_eq!( + description.chars().count(), + MAX_CHARS_PER_COLLECTION_DESCRIPTION + 1, + "the cap, plus the ellipsis" + ); + assert_eq!( + output.lines().count(), + 2, + "the description stays on one line" + ); + } + + #[test] + fn a_collection_description_is_cut_on_a_character_boundary() { + let ctx = collections_context(vec![described_collection("Café", &"é".repeat(200))]); + + let output = ctx.format_for_prompt(4000); + let expected = format!( + "- Café — {}…", + "é".repeat(MAX_CHARS_PER_COLLECTION_DESCRIPTION) + ); + assert_eq!(output.lines().nth(1), Some(expected.as_str())); + } + + #[test] + fn collection_descriptions_stop_at_their_share_of_the_budget() { + let description = "d".repeat(MAX_CHARS_PER_COLLECTION_DESCRIPTION); + let collections: Vec = (0..40) + .map(|i| described_collection(&format!("c{i:02}"), &description)) + .collect(); + let ctx = collections_context(collections); + + let max_chars = 4000; + let output = ctx.format_for_prompt(max_chars); + let described = output.lines().filter(|l| l.contains(" — ")).count(); + + assert_eq!( + described, + max_chars + / COLLECTION_DESCRIPTIONS_BUDGET_DIVISOR + / MAX_CHARS_PER_COLLECTION_DESCRIPTION + ); + assert_eq!( + output.lines().count(), + 41, + "every collection is still listed by name" + ); + assert!(output.len() <= max_chars); + } + + #[test] + fn collections_fall_back_to_names_when_the_described_list_does_not_fit() { + let ctx = collections_context(vec![ + described_collection("Clients", "Bills"), + described_collection("Research", "Notes"), + CollectionSummary::named("Engineering"), + ]); + + // Room for the names line (44 bytes) but not for the described list (68). + assert_eq!( + ctx.format_for_prompt(48), + "COLLECTIONS: Clients, Research, Engineering\n" + ); + assert_eq!(ctx.format_for_prompt(20), "", "too small for the names too"); } // ----------------------------------------------------------------------- diff --git a/packages/core/src/ops/node_ops.rs b/packages/core/src/ops/node_ops.rs index 5395c5f93..112776eaa 100644 --- a/packages/core/src/ops/node_ops.rs +++ b/packages/core/src/ops/node_ops.rs @@ -4,7 +4,10 @@ //! MCP and local agent tools share the same logic. use crate::db::ChildPlacement; -use crate::models::{FilterOperator, Node, NodeFilter, NodeUpdate, OrderBy, PropertyFilter}; +use crate::models::{ + CoreNodeType, FilterOperator, Node, NodeFilter, NodeUpdate, OrderBy, PropertyFilter, +}; +use crate::ops::collection_ops::new_collection_id; use crate::ops::OpsError; use crate::services::{CollectionService, InsertPositionOwned, NodeService}; use serde::Deserialize; @@ -186,6 +189,23 @@ pub async fn create_node( )); } + // A collection's id is derived from its name, so a collection created + // here is the same node as one created by the collection op or an import. + // A subtype of collection is named the same way (ADR-086 §5). + let id = match input.id { + Some(id) => Some(id), + None => { + if node_service + .type_is_a(&input.node_type, CoreNodeType::Collection) + .await? + { + Some(new_collection_id(node_service, &input.content).await?) + } else { + None + } + } + }; + // ADR-069 §5/F6: lifecycle_status is passed into the create itself // rather than applied via a separate update afterward. The old // two-write shape had a real gap — a failure on the second write left a @@ -195,7 +215,7 @@ pub async fn create_node( // of the one write that creates the row. let (node_id, placement) = node_service .create_placed_node(crate::services::CreateNodeParams { - id: input.id, + id, node_type: input.node_type, content: input.content, parent_id: input.parent_id, diff --git a/packages/core/src/services/collection_service.rs b/packages/core/src/services/collection_service.rs index ca51d35d1..e568e3e15 100644 --- a/packages/core/src/services/collection_service.rs +++ b/packages/core/src/services/collection_service.rs @@ -661,19 +661,17 @@ impl<'a> CollectionService<'a> { .map_err(|e| db_error(e, "Failed to get collection by name")) } - /// Get all collection names + /// Every collection's name and description, sorted by name. /// - /// Returns all collection names in the database, sorted alphabetically. - /// Useful for MCP handlers that need the names for filtering. - /// - /// # Returns - /// - /// Vec of collection names (e.g., ["architecture", "archived", "development"]) - pub async fn get_all_collection_names(&self) -> Result, NodeServiceError> { + /// The description is `None` when the collection has none. This is what + /// the agent's workspace context lists. + pub async fn get_all_collection_descriptions( + &self, + ) -> Result)>, NodeServiceError> { self.store - .get_all_collection_names() + .get_all_collection_descriptions() .await - .map_err(|e| db_error(e, "Failed to get all collection names")) + .map_err(|e| db_error(e, "Failed to get all collection descriptions")) } /// Get all collections with their member counts and parent collection IDs in a single query @@ -1159,8 +1157,13 @@ mod tests { ); // A fresh install seeds no collection, so these three are all of them. - let mut names = svc.get_all_collection_names().await.unwrap(); - names.sort(); + let names: Vec = svc + .get_all_collection_descriptions() + .await + .unwrap() + .into_iter() + .map(|(name, _)| name) + .collect(); assert_eq!( names, ["Alternatives", "Architecture", "Decisions"], diff --git a/packages/nodespace-types/src/convert.rs b/packages/nodespace-types/src/convert.rs index 8155d201a..8b02c238f 100644 --- a/packages/nodespace-types/src/convert.rs +++ b/packages/nodespace-types/src/convert.rs @@ -611,6 +611,23 @@ mod wire_contract { ); } + /// The collection viewer's header reads `properties.description`: the + /// collection bucket's field, flattened for the wire like any other. + #[test] + fn collection_description_reaches_the_wire_from_its_bucket() { + let node = Node::new( + "collection".to_string(), + "Clients".to_string(), + serde_json::json!({ "collection": { "description": "Accounts we bill" } }), + ); + let out = node_to_typed_value(node).unwrap(); + + assert_eq!( + out["properties"], + serde_json::json!({ "description": "Accounts we bill" }) + ); + } + #[test] fn flat_properties_view_leaves_untyped_nodes_alone() { let node = Node::new( diff --git a/packages/skill/SKILL.md b/packages/skill/SKILL.md index f681ea07c..ed229974c 100644 --- a/packages/skill/SKILL.md +++ b/packages/skill/SKILL.md @@ -226,7 +226,7 @@ nodespace relationship create --from --type belongs_to_sprint --to < ### Organize a node into a collection -Collections are how NodeSpace tags and groups things — a flat label and a nested `:`-delimited path (`docs:rust`) are one mechanism at two depths, the same syntax `import` and `search` take. Crucially, **collection membership is an argument to the create call**, not a follow-up write: pass `--collection` and every missing segment is created for you. Never look a collection up first or ask the user to pre-create one. +Collections are how NodeSpace tags and groups things — a flat label and a nested `:`-delimited path (`docs:rust`) are one mechanism at two depths. Crucially, **collection membership is an argument to the create call**, not a follow-up write: pass `--collection` and every missing segment is created for you. Never look a collection up first or ask the user to pre-create one. ```bash # One call: creates the node, creates `docs` and `rust`, files the node under `rust` @@ -237,7 +237,9 @@ nodespace node create --type text --content "Retry budget" --collection docs:rus nodespace node update --collection docs:rust ``` -Prefer a collection for any durable grouping: don't add a `tags`/`categories`/`topics`/`labels` field for something collections already model. Unlike an array value, a collection shows in the UI, is renamed once not per member, nests, and needs no schema change to join — `member_of` is structural, legal between any two nodes undeclared. +Describe a collection so agents know what goes in it: `nodespace node update --property description="…"`. + +Prefer a collection for any durable grouping: don't add a `tags`/`categories`/`topics`/`labels` field for something collections already model. Unlike an array value, a collection shows in the UI, is renamed once not per member, nests, and needs no schema change to join (`member_of` is structural). ### Delete a node, or a whole node type diff --git a/packages/skill/references/cli.md b/packages/skill/references/cli.md index f9e261064..ea72e450e 100644 --- a/packages/skill/references/cli.md +++ b/packages/skill/references/cli.md @@ -72,6 +72,15 @@ nodespace node create --type text --content "Meeting notes" --parent Only include properties the schema actually defines as required, plus any optional ones the user gave a value for. Don't invent fields. +**Describing a collection:** a collection has one optional field, `description`, which says what the collection is for. Agents see it next to the collection's name when they decide where a node belongs. Set it on create, or later with `node update`: + +```bash +nodespace node create --type collection --content "Clients" --property description="Accounts we bill, one page per client" +nodespace node update --property description="Accounts we bill, one page per client" +``` + +A collection is identified by its name (case-insensitive): `node create --type collection` with a name that is taken fails with `Already exists`, naming the collection in the way and its id. Update that collection instead, using the id from the error. + **Success semantics:** once `node create` returns an ID, the node exists — confirm what was created to the user and stop. Don't immediately `node get` the same ID to verify; the create response is the confirmation. **If it has headings, it is a tree, not a node.** NodeSpace is one-node-per-block, with hierarchy as first-class edges — a document with sections (an ADR, a spec, a plan) decomposes into a root node plus one child per section, not one node whose `content` holds the whole document: