From 444187c7e9b944f07ab83d6899fffceea8ad98e8 Mon Sep 17 00:00:00 2001 From: shanu Date: Thu, 13 Aug 2026 15:36:27 +0530 Subject: [PATCH 1/2] 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/2] 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 ); }