From 444187c7e9b944f07ab83d6899fffceea8ad98e8 Mon Sep 17 00:00:00 2001 From: shanu Date: Thu, 13 Aug 2026 15:36:27 +0530 Subject: [PATCH 1/4] fix(sync/notion): render database-row properties into the synced document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NOTION_GET_PAGE_MARKDOWN returns page block content only, so a database row's structured property values (select / status / multi_select / date / people / relation / scalars) never appeared in the synced document. The agent therefore received a tracker page with no dropdown text and invented the selections (#5500). Add render_properties(), which walks the already-fetched row (item.raw — the same object notion_title reads, so no extra Composio call) and emits readable 'Name: value' lines under a 'Properties:' header, prepended to the markdown body. The title property is skipped (it is the document title); empty/null values are skipped; lines are sorted for deterministic output. Integration test drives the real fetch->markdown->document path with a row carrying status/select/multi_select/date properties and asserts each selection reaches the document content; it fails on the pre-fix code (which emitted only the markdown body). --- src/memory/sync/composio/providers/notion.rs | 134 ++++++++++++++++++- tests/composio_sync_mock.rs | 81 +++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) diff --git a/src/memory/sync/composio/providers/notion.rs b/src/memory/sync/composio/providers/notion.rs index 33c4038f..6853d03c 100644 --- a/src/memory/sync/composio/providers/notion.rs +++ b/src/memory/sync/composio/providers/notion.rs @@ -136,7 +136,7 @@ impl IncrementalSource for NotionSyncPipeline { state, ) .await?; - let content = [ + let body = [ "/markdown", "/data/markdown", "/data/response_data/markdown", @@ -151,6 +151,17 @@ impl IncrementalSource for NotionSyncPipeline { .filter(|value| !value.trim().is_empty()) .map(str::to_owned) .unwrap_or(serde_json::to_string_pretty(&item.raw)?); + // `NOTION_GET_PAGE_MARKDOWN` returns page *block* content only, so a + // database-row page's structured property values (select / status / + // multi_select / date / …) never appear in `body`. Render them from the + // fetched row and prepend, so the agent reads the real dropdown + // selections instead of inventing them (#5500). + let properties = render_properties(&item.raw); + let content = if properties.is_empty() { + body + } else { + format!("{properties}\n\n{body}") + }; Ok(document( "notion", connection_id, @@ -191,3 +202,124 @@ fn notion_title(page: &Value) -> Option { }) .or_else(|| pick_str(page, &["title", "data.title", "name", "data.name"])) } + +/// Render a Notion row's structured database properties into readable +/// `Name: value` lines under a `Properties:` header. +/// +/// The sync body comes from `NOTION_GET_PAGE_MARKDOWN`, which returns page +/// *block* content only — a database row's property values +/// (`select`/`status`/`multi_select`/`date`/`people`/`relation`/scalars) are +/// **not** in that markdown. Without this, a tracker page reaches the agent with +/// no dropdown text and the model invents the selections (#5500). Values are +/// read from the already-fetched row (`item.raw`, the same object +/// [`notion_title`] reads), so no extra Composio call is needed. The `title` +/// property is skipped here (it is already the document title); empty / null +/// values are skipped. Returns an empty string when nothing renders. +fn render_properties(page: &Value) -> String { + let Some(properties) = page + .get("properties") + .or_else(|| page.pointer("/data/properties")) + .and_then(Value::as_object) + else { + return String::new(); + }; + let mut lines: Vec = properties + .iter() + .filter_map(|(name, property)| { + let kind = property.get("type").and_then(Value::as_str)?; + let value = match kind { + // Already surfaced as the document title. + "title" => return None, + "rich_text" => plain_text(property.get("rich_text")), + "select" | "status" => property + .get(kind) + .and_then(|inner| inner.get("name")) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "multi_select" => named_list(property.get("multi_select")), + "people" => named_list(property.get("people")), + "relation" => property + .get("relation") + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| item.get("id").and_then(Value::as_str)) + .collect::>() + .join(", ") + }) + .unwrap_or_default(), + "date" => property + .get("date") + .and_then(Value::as_object) + .map(|date| { + let start = date + .get("start") + .and_then(Value::as_str) + .unwrap_or_default(); + match date.get("end").and_then(Value::as_str) { + Some(end) if !end.is_empty() => format!("{start} → {end}"), + _ => start.to_string(), + } + }) + .unwrap_or_default(), + "checkbox" => match property.get("checkbox").and_then(Value::as_bool) { + Some(true) => "Yes".to_string(), + Some(false) => "No".to_string(), + None => String::new(), + }, + "number" => property + .get("number") + .filter(|value| value.is_number()) + .map(Value::to_string) + .unwrap_or_default(), + "url" | "email" | "phone_number" => property + .get(kind) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + _ => String::new(), + }; + let value = value.trim(); + (!value.is_empty()).then(|| format!("{name}: {value}")) + }) + .collect(); + if lines.is_empty() { + return String::new(); + } + // Stable ordering so the synced document is deterministic across runs + // (serde_json preserves object insertion order only with the `preserve_order` + // feature, which is not enabled here). + lines.sort(); + format!("Properties:\n{}", lines.join("\n")) +} + +/// Join the `plain_text` runs of a Notion rich-text array into a single string. +fn plain_text(value: Option<&Value>) -> String { + value + .and_then(Value::as_array) + .map(|parts| { + parts + .iter() + .filter_map(|part| part.get("plain_text").and_then(Value::as_str)) + .collect::>() + .join("") + }) + .unwrap_or_default() +} + +/// Comma-join the `name` field of every object in a Notion array property +/// (`multi_select` options, `people` entries). +fn named_list(value: Option<&Value>) -> String { + value + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| item.get("name").and_then(Value::as_str)) + .collect::>() + .join(", ") + }) + .unwrap_or_default() +} diff --git a/tests/composio_sync_mock.rs b/tests/composio_sync_mock.rs index d6496f7b..c790a170 100644 --- a/tests/composio_sync_mock.rs +++ b/tests/composio_sync_mock.rs @@ -518,6 +518,87 @@ async fn notion_fetches_markdown_and_counts_both_requests() { assert_eq!(state.daily_budget.requests_used, 2); } +#[tokio::test] +async fn notion_renders_database_row_properties_into_document() { + // #5500: NOTION_GET_PAGE_MARKDOWN returns page *block* content only, so a + // database row's structured property values (status / select / multi_select + // / date) never appear in the markdown. Before the fix the synced document + // was just the markdown body, so the agent could not read the dropdown + // selections and invented them. The row's `properties` must now be rendered + // into the document text alongside the body. + let server = MockServer::start().await; + Mock::given(path("/tools/execute/NOTION_FETCH_DATA")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "successful": true, + "data": {"results": [{ + "id": "page-1", + "last_edited_time": "2026-03-01T00:00:00Z", + "properties": { + "Name": {"type": "title", "title": [{"plain_text": "Roadmap"}]}, + "Status": {"type": "status", "status": {"name": "In progress"}}, + "Priority": {"type": "select", "select": {"name": "High"}}, + "Tags": {"type": "multi_select", "multi_select": [ + {"name": "infra"}, {"name": "urgent"} + ]}, + "Due": {"type": "date", "date": {"start": "2026-06-01"}}, + // An empty select must be skipped, not rendered blank. + "Owner": {"type": "select", "select": null} + } + }]} + }))) + .mount(&server) + .await; + Mock::given(path("/tools/execute/NOTION_GET_PAGE_MARKDOWN")) + .respond_with(ResponseTemplate::new(200).set_body_json( + serde_json::json!({"successful": true, "data": {"markdown": "# Roadmap\n\nBody"}}), + )) + .mount(&server) + .await; + let (captures, context) = test_context(); + let pipeline = NotionSyncPipeline::new( + ComposioClient::new(direct_config(server.uri(), "key")), + "notion-props-conn", + ); + pipeline.tick(&test_config(), &context).await.unwrap(); + + let documents = captures.documents.lock().unwrap(); + let content = &documents[0].content; + // Title still comes from the `title` property. + assert_eq!(documents[0].title, "Roadmap"); + // Every structured selection reaches the document text … + assert!( + content.contains("Status: In progress"), + "status missing: {content}" + ); + assert!( + content.contains("Priority: High"), + "select missing: {content}" + ); + assert!( + content.contains("Tags: infra, urgent"), + "multi_select missing: {content}" + ); + assert!( + content.contains("Due: 2026-06-01"), + "date missing: {content}" + ); + // … the markdown body is preserved … + assert!( + content.contains("# Roadmap\n\nBody"), + "body missing: {content}" + ); + // … the title property is not duplicated as a property line … + assert!( + !content.contains("Name: Roadmap"), + "title duplicated: {content}" + ); + // … and an empty property is skipped rather than rendered blank. + assert!( + !content.contains("Owner:"), + "empty select rendered: {content}" + ); +} + #[tokio::test] async fn google_docs_fetches_plaintext_and_counts_both_requests() { let server = MockServer::start().await; From 7cf32395462e61d8212d881153d4644d97d588e3 Mon Sep 17 00:00:00 2001 From: shanu Date: Tue, 18 Aug 2026 17:35:26 +0530 Subject: [PATCH 2/4] fix(sync/notion): render fallback property kinds and neutralise text injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property renderer closed the #5500 hallucination gap for select/status/ multi_select/date/scalars but left two holes a reviewer reproduced: - The `_ => String::new()` catch-all silently dropped every kind without an explicit arm — formula, rollup, unique_id, created/last_edited time and user, files — which are exactly the fields a tracker page leans on, so the agent still invented those values, with no signal that anything was skipped. - Property text was emitted without collapsing whitespace, so a rich_text value like "real\nStatus: FAKE" forged a second `Name: value` line that, once the block is sorted, outranked the genuine `Status` — any Notion text field could spoof another property in the agent's context. Add `render_unknown` to render the common unhandled kinds (timestamps, `unique_id` as `PREFIX-n`, `formula`/`rollup` inner value, files, and any bare scalar via `scalar_value`); a kind it still can't read degrades to a `tracing::debug` + skip rather than vanishing. Collapse whitespace in every property name and value at the single emit point, which neutralises the injection for all kinds at once and preserves the one-line contract. Also stop the raw-JSON fallback (used when page markdown is absent) from double-rendering properties: it already contains the `properties` object, so the rendered block is no longer prepended on that path. Tests: the existing case now asserts the exact composed document (header + deterministic sort + body placement), and a new case proves formula / unique_id / timestamp fallbacks render and that an injected newline is collapsed instead of forging a property line. --- src/memory/sync/composio/providers/notion.rs | 110 ++++++++++++++++--- tests/composio_sync_mock.rs | 100 ++++++++++++----- 2 files changed, 169 insertions(+), 41 deletions(-) diff --git a/src/memory/sync/composio/providers/notion.rs b/src/memory/sync/composio/providers/notion.rs index 6853d03c..1473d03a 100644 --- a/src/memory/sync/composio/providers/notion.rs +++ b/src/memory/sync/composio/providers/notion.rs @@ -136,7 +136,7 @@ impl IncrementalSource for NotionSyncPipeline { state, ) .await?; - let body = [ + let markdown = [ "/markdown", "/data/markdown", "/data/response_data/markdown", @@ -149,18 +149,25 @@ impl IncrementalSource for NotionSyncPipeline { .iter() .find_map(|path| response.data.pointer(path).and_then(Value::as_str)) .filter(|value| !value.trim().is_empty()) - .map(str::to_owned) - .unwrap_or(serde_json::to_string_pretty(&item.raw)?); + .map(str::to_owned); // `NOTION_GET_PAGE_MARKDOWN` returns page *block* content only, so a // database-row page's structured property values (select / status / - // multi_select / date / …) never appear in `body`. Render them from the - // fetched row and prepend, so the agent reads the real dropdown + // multi_select / date / …) never appear in the markdown. Render them from + // the fetched row and prepend, so the agent reads the real dropdown // selections instead of inventing them (#5500). - let properties = render_properties(&item.raw); - let content = if properties.is_empty() { - body - } else { - format!("{properties}\n\n{body}") + let content = match markdown { + Some(body) => { + let properties = render_properties(&item.raw); + if properties.is_empty() { + body + } else { + format!("{properties}\n\n{body}") + } + } + // No page markdown available: fall back to the raw row JSON. It + // already contains the `properties` object, so do NOT prepend the + // rendered block as well — that would duplicate the property values. + None => serde_json::to_string_pretty(&item.raw)?, }; Ok(document( "notion", @@ -279,10 +286,15 @@ fn render_properties(page: &Value) -> String { .and_then(Value::as_str) .unwrap_or_default() .to_string(), - _ => String::new(), + _ => render_unknown(kind, property), }; - let value = value.trim(); - (!value.is_empty()).then(|| format!("{name}: {value}")) + // Collapse whitespace (including newlines) in both the property name + // and value so neither can forge extra `Name: value` lines in the + // block — a rich_text value like "real\nStatus: FAKE" would otherwise + // inject a second, higher-sorting property line into the agent's + // context. Also keeps the one-line `Name: value` contract. + let value = collapse_ws(&value); + (!value.is_empty()).then(|| format!("{}: {value}", collapse_ws(name))) }) .collect(); if lines.is_empty() { @@ -323,3 +335,75 @@ fn named_list(value: Option<&Value>) -> String { }) .unwrap_or_default() } + +/// Best-effort render of a Notion property kind not handled explicitly above. +/// +/// Tracker pages routinely carry `formula`, `rollup`, `unique_id`, and the +/// audit timestamps/users; dropping them silently leaves exactly the fields a +/// #5500 page relies on missing, so the agent re-invents them. This reads the +/// concrete shapes and falls back to any bare scalar; a kind it still can't read +/// degrades to "skipped **and** logged" (`tracing::debug`) rather than vanishing, +/// so a newly-introduced Notion property type is visible in telemetry. +fn render_unknown(kind: &str, property: &Value) -> String { + let inner = property.get(kind); + let rendered = match kind { + "created_time" | "last_edited_time" => inner + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "created_by" | "last_edited_by" => inner + .and_then(|user| user.get("name")) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "unique_id" => inner + .and_then(Value::as_object) + .map(|uid| { + let number = uid.get("number").filter(|n| n.is_number()); + match (uid.get("prefix").and_then(Value::as_str), number) { + (Some(prefix), Some(n)) => format!("{prefix}-{n}"), + (_, Some(n)) => n.to_string(), + _ => String::new(), + } + }) + .unwrap_or_default(), + // `formula`/`rollup` wrap their result in `{ "type": T, T: value }`. + "formula" | "rollup" => inner + .and_then(Value::as_object) + .and_then(|obj| obj.get("type").and_then(Value::as_str).map(|t| (obj, t))) + .map(|(obj, t)| scalar_value(obj.get(t))) + .unwrap_or_default(), + // `files` entries expose a `name`; reuse the same object-name join. + "files" => named_list(inner), + _ => scalar_value(inner), + }; + if rendered.trim().is_empty() { + tracing::debug!(kind, "[memory_sync:notion] unrendered property"); + } + rendered +} + +/// Render a bare Notion scalar (`string` / `number` / `bool`) or a `{ name: … }` +/// object into text; empty for a structured shape we don't recognise. The last +/// resort for [`render_unknown`], covering `formula`/`rollup` inner values and +/// any future scalar-typed property. +fn scalar_value(value: Option<&Value>) -> String { + match value { + Some(Value::String(s)) => s.clone(), + Some(Value::Number(n)) => n.to_string(), + Some(Value::Bool(b)) => if *b { "Yes" } else { "No" }.to_string(), + Some(Value::Object(obj)) => obj + .get("name") + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + _ => String::new(), + } +} + +/// Collapse every run of whitespace (spaces, tabs, newlines) to a single space +/// and trim. Applied to each property name and value before it is emitted so a +/// value can't inject additional `Name: value` lines into the rendered block. +fn collapse_ws(text: &str) -> String { + text.split_whitespace().collect::>().join(" ") +} diff --git a/tests/composio_sync_mock.rs b/tests/composio_sync_mock.rs index c790a170..71c9f91e 100644 --- a/tests/composio_sync_mock.rs +++ b/tests/composio_sync_mock.rs @@ -562,40 +562,84 @@ async fn notion_renders_database_row_properties_into_document() { pipeline.tick(&test_config(), &context).await.unwrap(); let documents = captures.documents.lock().unwrap(); - let content = &documents[0].content; // Title still comes from the `title` property. assert_eq!(documents[0].title, "Roadmap"); - // Every structured selection reaches the document text … - assert!( - content.contains("Status: In progress"), - "status missing: {content}" - ); - assert!( - content.contains("Priority: High"), - "select missing: {content}" - ); - assert!( - content.contains("Tags: infra, urgent"), - "multi_select missing: {content}" - ); - assert!( - content.contains("Due: 2026-06-01"), - "date missing: {content}" + // Assert the complete composed document: a `Properties:` header, every + // structured selection rendered, sorted deterministically (Due < Priority < + // Status < Tags), the title property not duplicated, the empty `Owner` select + // skipped, and the markdown body preserved after a blank line. A fragment + // check would pass even if the sort broke or properties landed after the body. + assert_eq!( + documents[0].content, + "Properties:\n\ + Due: 2026-06-01\n\ + Priority: High\n\ + Status: In progress\n\ + Tags: infra, urgent\n\n\ + # Roadmap\n\n\ + Body" ); - // … the markdown body is preserved … - assert!( - content.contains("# Roadmap\n\nBody"), - "body missing: {content}" +} + +#[tokio::test] +async fn notion_renders_fallback_kinds_and_neutralises_injection() { + // #5500 follow-ups: (1) tracker pages lean on `formula` / `unique_id` and the + // audit timestamps, which the explicit match arms don't cover — they must + // still render, not silently vanish; (2) a text value containing a newline + // must not forge a second `Name: value` line in the block (a sorted + // "Status: FAKE-INJECTED" would otherwise outrank the genuine status). + let server = MockServer::start().await; + Mock::given(path("/tools/execute/NOTION_FETCH_DATA")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "successful": true, + "data": {"results": [{ + "id": "page-2", + "last_edited_time": "2026-03-02T00:00:00Z", + "properties": { + "Name": {"type": "title", "title": [{"plain_text": "Tracker"}]}, + "Days left": {"type": "formula", "formula": {"type": "number", "number": 3}}, + "Ticket": {"type": "unique_id", "unique_id": {"prefix": "TASK", "number": 7}}, + "Created": {"type": "created_time", "created_time": "2026-01-02T03:04:00Z"}, + // Injection attempt: a newline that would forge a `Status:` line. + "Notes": {"type": "rich_text", "rich_text": [ + {"plain_text": "real\nStatus: FAKE-INJECTED"} + ]} + } + }]} + }))) + .mount(&server) + .await; + Mock::given(path("/tools/execute/NOTION_GET_PAGE_MARKDOWN")) + .respond_with(ResponseTemplate::new(200).set_body_json( + serde_json::json!({"successful": true, "data": {"markdown": "# Tracker\n\nBody"}}), + )) + .mount(&server) + .await; + let (captures, context) = test_context(); + let pipeline = NotionSyncPipeline::new( + ComposioClient::new(direct_config(server.uri(), "key")), + "notion-fallback-conn", ); - // … the title property is not duplicated as a property line … - assert!( - !content.contains("Name: Roadmap"), - "title duplicated: {content}" + pipeline.tick(&test_config(), &context).await.unwrap(); + + let documents = captures.documents.lock().unwrap(); + // Fallback kinds render (Days left: 3, Ticket: TASK-7, Created timestamp) and + // the injected newline is collapsed to a single line — the forged + // "Status: FAKE-INJECTED" never becomes its own property line. + assert_eq!( + documents[0].content, + "Properties:\n\ + Created: 2026-01-02T03:04:00Z\n\ + Days left: 3\n\ + Notes: real Status: FAKE-INJECTED\n\ + Ticket: TASK-7\n\n\ + # Tracker\n\n\ + Body" ); - // … and an empty property is skipped rather than rendered blank. assert!( - !content.contains("Owner:"), - "empty select rendered: {content}" + !documents[0].content.contains("\nStatus: FAKE-INJECTED"), + "injected newline forged a property line: {}", + documents[0].content ); } From 56c88d98d42e1e90ec9f13b6bb71a40b2368f5a1 Mon Sep 17 00:00:00 2001 From: shanu Date: Tue, 18 Aug 2026 18:11:22 +0530 Subject: [PATCH 3/4] fix(sync/notion): render structured formula and rollup values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit render_unknown handled scalar formula/rollup results but a `date` or `array` result fell through scalar_value to empty output — dropping exactly the rolled-up dates and related-title lists a tracker page carries, the #5500 hallucination class this PR closes. Add render_typed_value for the Notion `{ "type": T, T: }` wrapper used by formula, rollup, and each rollup-array element: it renders date and nested array results and dispatches the common leaf kinds, falling through to scalar_value otherwise. Extract the date formatting into a shared format_date used by both the `date` property arm and these results. Also expand the render_properties doc to state the title/empty omission, whitespace normalization, best-effort unknown handling, sort determinism, and raw-JSON fallback contract (per the repo's documentation guideline). Test: formula date, rollup date, and rollup array all render into the exact composed document. --- src/memory/sync/composio/providers/notion.rs | 102 ++++++++++++++----- tests/composio_sync_mock.rs | 56 ++++++++++ 2 files changed, 133 insertions(+), 25 deletions(-) diff --git a/src/memory/sync/composio/providers/notion.rs b/src/memory/sync/composio/providers/notion.rs index 1473d03a..5736608b 100644 --- a/src/memory/sync/composio/providers/notion.rs +++ b/src/memory/sync/composio/providers/notion.rs @@ -219,9 +219,24 @@ fn notion_title(page: &Value) -> Option { /// **not** in that markdown. Without this, a tracker page reaches the agent with /// no dropdown text and the model invents the selections (#5500). Values are /// read from the already-fetched row (`item.raw`, the same object -/// [`notion_title`] reads), so no extra Composio call is needed. The `title` -/// property is skipped here (it is already the document title); empty / null -/// values are skipped. Returns an empty string when nothing renders. +/// [`notion_title`] reads), so no extra Composio call is needed. +/// +/// Contract for the emitted block: +/// - the `title` property is skipped (it is already the document title), and +/// empty / null values are skipped, so nothing renders as a blank `Name:`; +/// - each name and value is whitespace-collapsed to a single line, so a value +/// can't forge extra property lines (see [`collapse_ws`]); +/// - kinds without an explicit arm (formula/rollup/unique_id/timestamps/…) are +/// handled best-effort by [`render_unknown`], which logs anything it still +/// can't read rather than dropping it silently; +/// - lines are sorted, so the block is deterministic across runs (serde_json +/// only preserves object insertion order under the unused `preserve_order` +/// feature). +/// +/// Returns an empty string when nothing renders (the caller then emits the +/// markdown body alone). Note this is prepended to real page markdown only; when +/// markdown is absent the caller falls back to the raw row JSON instead, which +/// already contains the properties, so the block is not duplicated there. fn render_properties(page: &Value) -> String { let Some(properties) = page .get("properties") @@ -257,20 +272,7 @@ fn render_properties(page: &Value) -> String { .join(", ") }) .unwrap_or_default(), - "date" => property - .get("date") - .and_then(Value::as_object) - .map(|date| { - let start = date - .get("start") - .and_then(Value::as_str) - .unwrap_or_default(); - match date.get("end").and_then(Value::as_str) { - Some(end) if !end.is_empty() => format!("{start} → {end}"), - _ => start.to_string(), - } - }) - .unwrap_or_default(), + "date" => format_date(property.get("date")), "checkbox" => match property.get("checkbox").and_then(Value::as_bool) { Some(true) => "Yes".to_string(), Some(false) => "No".to_string(), @@ -367,12 +369,10 @@ fn render_unknown(kind: &str, property: &Value) -> String { } }) .unwrap_or_default(), - // `formula`/`rollup` wrap their result in `{ "type": T, T: value }`. - "formula" | "rollup" => inner - .and_then(Value::as_object) - .and_then(|obj| obj.get("type").and_then(Value::as_str).map(|t| (obj, t))) - .map(|(obj, t)| scalar_value(obj.get(t))) - .unwrap_or_default(), + // `formula`/`rollup` wrap their result in `{ "type": T, T: value }` — + // render_typed_value handles the date and array shapes a bare scalar + // can't, so a rollup date or array isn't dropped. + "formula" | "rollup" => inner.map(render_typed_value).unwrap_or_default(), // `files` entries expose a `name`; reuse the same object-name join. "files" => named_list(inner), _ => scalar_value(inner), @@ -383,10 +383,62 @@ fn render_unknown(kind: &str, property: &Value) -> String { rendered } +/// Render a Notion "typed value" wrapper `{ "type": T, T: }` — the shape +/// used by `formula`, `rollup`, and each element of a `rollup` array. Handles the +/// `date` and (nested) `array` results that a bare scalar can't, and dispatches +/// the common leaf kinds; anything else falls through to [`scalar_value`]. +fn render_typed_value(wrapper: &Value) -> String { + let Some(kind) = wrapper.get("type").and_then(Value::as_str) else { + return scalar_value(Some(wrapper)); + }; + match kind { + "date" => format_date(wrapper.get("date")), + "array" => wrapper + .get("array") + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .map(render_typed_value) + .filter(|rendered| !rendered.is_empty()) + .collect::>() + .join(", ") + }) + .unwrap_or_default(), + "title" | "rich_text" => plain_text(wrapper.get(kind)), + "select" | "status" => wrapper + .get(kind) + .and_then(|inner| inner.get("name")) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "multi_select" | "people" => named_list(wrapper.get(kind)), + _ => scalar_value(wrapper.get(kind)), + } +} + +/// Format a Notion `date` object (`{ start, end? }`) as `start` or `start → end`. +/// Shared by the `date` property arm and formula/rollup date results. +fn format_date(value: Option<&Value>) -> String { + value + .and_then(Value::as_object) + .map(|date| { + let start = date + .get("start") + .and_then(Value::as_str) + .unwrap_or_default(); + match date.get("end").and_then(Value::as_str) { + Some(end) if !end.is_empty() => format!("{start} → {end}"), + _ => start.to_string(), + } + }) + .unwrap_or_default() +} + /// Render a bare Notion scalar (`string` / `number` / `bool`) or a `{ name: … }` /// object into text; empty for a structured shape we don't recognise. The last -/// resort for [`render_unknown`], covering `formula`/`rollup` inner values and -/// any future scalar-typed property. +/// resort for [`render_unknown`] and [`render_typed_value`], covering any +/// scalar-typed property. fn scalar_value(value: Option<&Value>) -> String { match value { Some(Value::String(s)) => s.clone(), diff --git a/tests/composio_sync_mock.rs b/tests/composio_sync_mock.rs index 71c9f91e..257ff87f 100644 --- a/tests/composio_sync_mock.rs +++ b/tests/composio_sync_mock.rs @@ -643,6 +643,62 @@ async fn notion_renders_fallback_kinds_and_neutralises_injection() { ); } +#[tokio::test] +async fn notion_renders_structured_formula_and_rollup_values() { + // #5500 completeness: formula/rollup results whose inner type is `date` or + // `array` must render, not fall through to an empty scalar — these are common + // on tracker pages (a rolled-up due date, a rollup of related titles). + let server = MockServer::start().await; + Mock::given(path("/tools/execute/NOTION_FETCH_DATA")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "successful": true, + "data": {"results": [{ + "id": "page-3", + "last_edited_time": "2026-03-03T00:00:00Z", + "properties": { + "Name": {"type": "title", "title": [{"plain_text": "Metrics"}]}, + "Due": {"type": "formula", "formula": { + "type": "date", "date": {"start": "2026-08-01", "end": "2026-08-03"} + }}, + "Next": {"type": "rollup", "rollup": { + "type": "date", "date": {"start": "2026-09-01"} + }}, + "Items": {"type": "rollup", "rollup": {"type": "array", "array": [ + {"type": "title", "title": [{"plain_text": "A"}]}, + {"type": "number", "number": 2} + ]}}, + "Score": {"type": "formula", "formula": {"type": "number", "number": 42}} + } + }]} + }))) + .mount(&server) + .await; + Mock::given(path("/tools/execute/NOTION_GET_PAGE_MARKDOWN")) + .respond_with(ResponseTemplate::new(200).set_body_json( + serde_json::json!({"successful": true, "data": {"markdown": "# Metrics\n\nBody"}}), + )) + .mount(&server) + .await; + let (captures, context) = test_context(); + let pipeline = NotionSyncPipeline::new( + ComposioClient::new(direct_config(server.uri(), "key")), + "notion-rollup-conn", + ); + pipeline.tick(&test_config(), &context).await.unwrap(); + + let documents = captures.documents.lock().unwrap(); + assert_eq!( + documents[0].content, + "Properties:\n\ + Due: 2026-08-01 → 2026-08-03\n\ + Items: A, 2\n\ + Next: 2026-09-01\n\ + Score: 42\n\n\ + # Metrics\n\n\ + Body" + ); +} + #[tokio::test] async fn google_docs_fetches_plaintext_and_counts_both_requests() { let server = MockServer::start().await; From 35bb7a46c37d6433090da75850be4fc0c5599fbb Mon Sep 17 00:00:00 2001 From: shanu Date: Tue, 18 Aug 2026 20:42:03 +0530 Subject: [PATCH 4/4] fix(sync/notion): unify property dispatch so rollup-array elements never drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit render_typed_value hand-rolled a narrower subset of the dispatch that render_properties and render_unknown already implemented, and its fallback was a bare scalar — so a rollup `array` element that is itself a formula, rollup, relation, files, or unique_id rendered empty and was dropped. Rollup-of-formula and rollup-of-relation are mainstream tracker configs, so this was #5500's hallucination class surviving one level down (the property absent, the model inventing it). Collapse the three partially-overlapping dispatch tables into one canonical render_property_value(kind, property) covering every kind, and have both render_properties and render_typed_value delegate to it — render_typed_value keeps only the `array` recursion and the bare-scalar (flattened-envelope) fallback local. An array element of any kind now renders exactly as that kind would at property level, making the drop impossible by construction (the same posture as the single-emit-point collapse_ws fix). render_unknown is removed; its concrete shapes live in the unified table, which logs only a genuinely unreadable kind rather than every empty value. Also: log once when a row has no `properties` object at all (the inert-against-real-Composio case), not silently. Tests: a rollup array of {formula, relation} now renders `3, rel-1` (both were dropped), and the flattened `{"formula":3}` / `{"rollup":12}` envelope is pinned. All 5 notion mock tests + sync matrix green; notion.rs 461 lines. --- src/memory/sync/composio/providers/notion.rs | 182 +++++++++---------- tests/composio_sync_mock.rs | 54 ++++++ 2 files changed, 145 insertions(+), 91 deletions(-) diff --git a/src/memory/sync/composio/providers/notion.rs b/src/memory/sync/composio/providers/notion.rs index 5736608b..b9cd6b96 100644 --- a/src/memory/sync/composio/providers/notion.rs +++ b/src/memory/sync/composio/providers/notion.rs @@ -226,9 +226,9 @@ fn notion_title(page: &Value) -> Option { /// empty / null values are skipped, so nothing renders as a blank `Name:`; /// - each name and value is whitespace-collapsed to a single line, so a value /// can't forge extra property lines (see [`collapse_ws`]); -/// - kinds without an explicit arm (formula/rollup/unique_id/timestamps/…) are -/// handled best-effort by [`render_unknown`], which logs anything it still -/// can't read rather than dropping it silently; +/// - every kind is rendered through the single [`render_property_value`] table +/// (formula/rollup/unique_id/timestamps/relation/files/…), which logs anything +/// it still can't read rather than dropping it silently; /// - lines are sorted, so the block is deterministic across runs (serde_json /// only preserves object insertion order under the unused `preserve_order` /// feature). @@ -243,59 +243,25 @@ fn render_properties(page: &Value) -> String { .or_else(|| page.pointer("/data/properties")) .and_then(Value::as_object) else { + // No `properties` object at all — a non-database page, or an envelope + // shaped differently than expected. Log once (the "inert against real + // Composio" case) rather than returning silently. + tracing::debug!("[memory_sync:notion] row has no properties object; rendered nothing"); return String::new(); }; let mut lines: Vec = properties .iter() .filter_map(|(name, property)| { let kind = property.get("type").and_then(Value::as_str)?; - let value = match kind { - // Already surfaced as the document title. - "title" => return None, - "rich_text" => plain_text(property.get("rich_text")), - "select" | "status" => property - .get(kind) - .and_then(|inner| inner.get("name")) - .and_then(Value::as_str) - .unwrap_or_default() - .to_string(), - "multi_select" => named_list(property.get("multi_select")), - "people" => named_list(property.get("people")), - "relation" => property - .get("relation") - .and_then(Value::as_array) - .map(|items| { - items - .iter() - .filter_map(|item| item.get("id").and_then(Value::as_str)) - .collect::>() - .join(", ") - }) - .unwrap_or_default(), - "date" => format_date(property.get("date")), - "checkbox" => match property.get("checkbox").and_then(Value::as_bool) { - Some(true) => "Yes".to_string(), - Some(false) => "No".to_string(), - None => String::new(), - }, - "number" => property - .get("number") - .filter(|value| value.is_number()) - .map(Value::to_string) - .unwrap_or_default(), - "url" | "email" | "phone_number" => property - .get(kind) - .and_then(Value::as_str) - .unwrap_or_default() - .to_string(), - _ => render_unknown(kind, property), - }; + if kind == "title" { + return None; // already surfaced as the document title + } // Collapse whitespace (including newlines) in both the property name // and value so neither can forge extra `Name: value` lines in the // block — a rich_text value like "real\nStatus: FAKE" would otherwise - // inject a second, higher-sorting property line into the agent's - // context. Also keeps the one-line `Name: value` contract. - let value = collapse_ws(&value); + // inject a second, higher-sorting property line. Keeps the one-line + // `Name: value` contract. + let value = collapse_ws(&render_property_value(kind, property)); (!value.is_empty()).then(|| format!("{}: {value}", collapse_ws(name))) }) .collect(); @@ -338,27 +304,65 @@ fn named_list(value: Option<&Value>) -> String { .unwrap_or_default() } -/// Best-effort render of a Notion property kind not handled explicitly above. -/// -/// Tracker pages routinely carry `formula`, `rollup`, `unique_id`, and the -/// audit timestamps/users; dropping them silently leaves exactly the fields a -/// #5500 page relies on missing, so the agent re-invents them. This reads the -/// concrete shapes and falls back to any bare scalar; a kind it still can't read -/// degrades to "skipped **and** logged" (`tracing::debug`) rather than vanishing, -/// so a newly-introduced Notion property type is visible in telemetry. -fn render_unknown(kind: &str, property: &Value) -> String { - let inner = property.get(kind); - let rendered = match kind { - "created_time" | "last_edited_time" => inner +/// The single dispatch for one Notion typed value `{ "type": kind, [kind]: … }` +/// — used for both a top-level database property and each element of a rollup +/// `array`. Every kind lives here, so no caller renders a *narrower* subset that +/// silently drops a shape (the bug class that recurs when a specialization +/// diverges from the general path). Tracker pages lean on `formula`, `rollup`, +/// `unique_id`, `relation`, and the audit timestamps/users; each is rendered +/// concretely. An unrecognised kind falls back to any bare scalar and, if that is +/// empty, is logged (`tracing::debug`) rather than vanishing — a new Notion +/// property type stays visible in telemetry. +fn render_property_value(kind: &str, property: &Value) -> String { + match kind { + "title" | "rich_text" => plain_text(property.get(kind)), + "select" | "status" => property + .get(kind) + .and_then(|inner| inner.get("name")) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "multi_select" | "people" => named_list(property.get(kind)), + "relation" => property + .get("relation") + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| item.get("id").and_then(Value::as_str)) + .collect::>() + .join(", ") + }) + .unwrap_or_default(), + "date" => format_date(property.get("date")), + "checkbox" => match property.get("checkbox").and_then(Value::as_bool) { + Some(true) => "Yes".to_string(), + Some(false) => "No".to_string(), + None => String::new(), + }, + "number" => property + .get("number") + .filter(|value| value.is_number()) + .map(Value::to_string) + .unwrap_or_default(), + "url" | "email" | "phone_number" => property + .get(kind) + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + "created_time" | "last_edited_time" => property + .get(kind) .and_then(Value::as_str) .unwrap_or_default() .to_string(), - "created_by" | "last_edited_by" => inner + "created_by" | "last_edited_by" => property + .get(kind) .and_then(|user| user.get("name")) .and_then(Value::as_str) .unwrap_or_default() .to_string(), - "unique_id" => inner + "unique_id" => property + .get("unique_id") .and_then(Value::as_object) .map(|uid| { let number = uid.get("number").filter(|n| n.is_number()); @@ -369,31 +373,35 @@ fn render_unknown(kind: &str, property: &Value) -> String { } }) .unwrap_or_default(), - // `formula`/`rollup` wrap their result in `{ "type": T, T: value }` — - // render_typed_value handles the date and array shapes a bare scalar - // can't, so a rollup date or array isn't dropped. - "formula" | "rollup" => inner.map(render_typed_value).unwrap_or_default(), - // `files` entries expose a `name`; reuse the same object-name join. - "files" => named_list(inner), - _ => scalar_value(inner), - }; - if rendered.trim().is_empty() { - tracing::debug!(kind, "[memory_sync:notion] unrendered property"); + // `formula`/`rollup` wrap their result in another typed value. + "formula" | "rollup" => property + .get(kind) + .map(render_typed_value) + .unwrap_or_default(), + // `files` entries expose a `name`; reuse the object-name join. + "files" => named_list(property.get(kind)), + _ => { + let rendered = scalar_value(property.get(kind)); + if rendered.trim().is_empty() { + tracing::debug!(kind, "[memory_sync:notion] unrendered property"); + } + rendered + } } - rendered } -/// Render a Notion "typed value" wrapper `{ "type": T, T: }` — the shape -/// used by `formula`, `rollup`, and each element of a `rollup` array. Handles the -/// `date` and (nested) `array` results that a bare scalar can't, and dispatches -/// the common leaf kinds; anything else falls through to [`scalar_value`]. +/// Render a Notion "typed value" wrapper `{ "type": T, T: }` — used by +/// `formula`, `rollup`, and each element of a rollup `array`. `array` recurses; +/// a bare scalar with no `type` renders directly; every other kind delegates to +/// [`render_property_value`], so an array element of any kind renders exactly as +/// the same kind would at property level (no narrower dispatch, so no dropped +/// rollup-of-formula / rollup-of-relation). fn render_typed_value(wrapper: &Value) -> String { let Some(kind) = wrapper.get("type").and_then(Value::as_str) else { return scalar_value(Some(wrapper)); }; - match kind { - "date" => format_date(wrapper.get("date")), - "array" => wrapper + if kind == "array" { + return wrapper .get("array") .and_then(Value::as_array) .map(|items| { @@ -404,17 +412,9 @@ fn render_typed_value(wrapper: &Value) -> String { .collect::>() .join(", ") }) - .unwrap_or_default(), - "title" | "rich_text" => plain_text(wrapper.get(kind)), - "select" | "status" => wrapper - .get(kind) - .and_then(|inner| inner.get("name")) - .and_then(Value::as_str) - .unwrap_or_default() - .to_string(), - "multi_select" | "people" => named_list(wrapper.get(kind)), - _ => scalar_value(wrapper.get(kind)), + .unwrap_or_default(); } + render_property_value(kind, wrapper) } /// Format a Notion `date` object (`{ start, end? }`) as `start` or `start → end`. @@ -437,8 +437,8 @@ fn format_date(value: Option<&Value>) -> String { /// Render a bare Notion scalar (`string` / `number` / `bool`) or a `{ name: … }` /// object into text; empty for a structured shape we don't recognise. The last -/// resort for [`render_unknown`] and [`render_typed_value`], covering any -/// scalar-typed property. +/// resort for [`render_property_value`] and [`render_typed_value`], covering any +/// scalar-typed property and the flattened formula/rollup envelope. fn scalar_value(value: Option<&Value>) -> String { match value { Some(Value::String(s)) => s.clone(), diff --git a/tests/composio_sync_mock.rs b/tests/composio_sync_mock.rs index 257ff87f..b2782eeb 100644 --- a/tests/composio_sync_mock.rs +++ b/tests/composio_sync_mock.rs @@ -699,6 +699,60 @@ async fn notion_renders_structured_formula_and_rollup_values() { ); } +#[tokio::test] +async fn notion_renders_rollup_array_elements_of_every_kind_and_flat_envelope() { + // #5500 completeness, round 3: a rollup `array` element that is itself a + // formula or relation must render — the previous specialization fell through + // to a bare scalar and dropped these mainstream tracker shapes. Also pins the + // flattened `{"formula":3}` / `{"rollup":12}` envelope so it can't regress. + let server = MockServer::start().await; + Mock::given(path("/tools/execute/NOTION_FETCH_DATA")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "successful": true, + "data": {"results": [{ + "id": "page-4", + "last_edited_time": "2026-03-04T00:00:00Z", + "properties": { + "Name": {"type": "title", "title": [{"plain_text": "Deep"}]}, + // A rollup array whose elements are a formula and a relation — + // both dropped before the unified dispatch. + "Nested": {"type": "rollup", "rollup": {"type": "array", "array": [ + {"type": "formula", "formula": {"type": "number", "number": 3}}, + {"type": "relation", "relation": [{"id": "rel-1"}]} + ]}}, + // Flattened envelopes: the value sits directly under the kind. + "FlatF": {"type": "formula", "formula": 3}, + "FlatR": {"type": "rollup", "rollup": 12} + } + }]} + }))) + .mount(&server) + .await; + Mock::given(path("/tools/execute/NOTION_GET_PAGE_MARKDOWN")) + .respond_with(ResponseTemplate::new(200).set_body_json( + serde_json::json!({"successful": true, "data": {"markdown": "# Deep\n\nBody"}}), + )) + .mount(&server) + .await; + let (captures, context) = test_context(); + let pipeline = NotionSyncPipeline::new( + ComposioClient::new(direct_config(server.uri(), "key")), + "notion-nested-conn", + ); + pipeline.tick(&test_config(), &context).await.unwrap(); + + let documents = captures.documents.lock().unwrap(); + assert_eq!( + documents[0].content, + "Properties:\n\ + FlatF: 3\n\ + FlatR: 12\n\ + Nested: 3, rel-1\n\n\ + # Deep\n\n\ + Body" + ); +} + #[tokio::test] async fn google_docs_fetches_plaintext_and_counts_both_requests() { let server = MockServer::start().await;