Skip to content

fix(notion): parent pages to data_source_id for API 2025-09-03 (RCA-33)#8

Merged
ramirocantu merged 2 commits into
mainfrom
ramirocanturamirez/rca-33-test-notion-integration-backend-functionality
Jun 1, 2026
Merged

fix(notion): parent pages to data_source_id for API 2025-09-03 (RCA-33)#8
ramirocantu merged 2 commits into
mainfrom
ramirocanturamirez/rca-33-test-notion-integration-backend-functionality

Conversation

@ramirocantu

@ramirocantu ramirocantu commented Jun 1, 2026

Copy link
Copy Markdown
Owner

What

RCA-33 asked: does the Notion integration even write on the backend? It did not. Testing it live surfaced a silent break, now fixed.

Root cause

notion-client 3.1.0 pins Notion API 2025-09-03, which split databases → data sources. The write seam (app/services/kb/notion.py) parented page creation with {"database_id": <id>}, but the configured NOTION_WIKI_DB_ID is a data-source id → every pages.create returned ObjectNotFound. Nothing was ever written.

The mocked unit suite passed all along because it never validated the parent shape — the bug only existed against the live API.

Fix

  • app/services/kb/notion.py: parent with {"type": "data_source_id", "data_source_id": notion_wiki_db_id} — keeps the configured env value as-is.
  • tests/test_kb_notion.py: regression test pins the parent shape so a revert to the legacy database_id fails in CI.
  • scripts/notion_smoke.py: reusable 3-stage live connectivity check (token / data-source reachable / real write), --write and --keep flags.

Verification

Live end-to-end through the production sync_node_to_notion path against the real workspace: page created in Courses data source for node "BB: Biological and Biochemical Foundations of Living Systems" + fact bullet, repeatable across runs. Mocked suite: 14 passed, lint clean.

Unblocks RCA-14 child #2 (coarser node-page Notion sync).

Closes RCA-33.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Updates

    • Notion integration now uses an updated parent identifier for creating pages to improve API compatibility.
    • Notion client dependency upgraded for compatibility with the new parent identifier behavior.
  • New Features

    • Added a standalone Notion smoke-test script to validate API connectivity, token identity, and optional write/create workflows.
  • Tests

    • Added a regression test ensuring pages are created with the updated parent identifier.

The Notion write-out seam never actually wrote: notion-client 3.1.0
pins API version 2025-09-03, which split databases into databases +
data sources. `_ensure_node_page` parented page creation with
`{"database_id": <id>}`, but the configured NOTION_WIKI_DB_ID is a
data-source id — so every `pages.create` 404'd with ObjectNotFound
and the integration silently failed.

Parent with `{"type": "data_source_id", "data_source_id": ...}` instead,
which keeps the configured env value as-is. Verified live end-to-end via
the production `sync_node_to_notion` path against the real workspace.

- test_kb_notion.py: pin the parent shape so a regression to the legacy
  database_id parent fails in CI instead of silently at runtime (the
  mocked suite never validated the parent, which is why this slipped).
- scripts/notion_smoke.py: reusable 3-stage live connectivity check
  (token / data-source reachable / real write), --write and --keep flags.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9a3f83c3-f085-4cfa-a485-0916ee861f4e

📥 Commits

Reviewing files that changed from the base of the PR and between 805ff4b and a151314.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • pyproject.toml
  • scripts/notion_smoke.py

📝 Walkthrough

Walkthrough

Switch Notion page creation to use the data_source_id parent field, add a regression test asserting the new parent contract, and add a staged smoke-test script that verifies connectivity and optionally performs a gated write using sync_node_to_notion().

Changes

Notion parent field migration

Layer / File(s) Summary
Notion parent field migration in sync service
app/services/kb/notion.py, pyproject.toml
_ensure_node_page updates the pages.create call to use Notion's data_source_id parent field (configured via notion_wiki_db_id) instead of the legacy database_id; pyproject.toml bumps notion-client to >=3.1 and documents legacy database_id behavior.
Regression test for parent field contract
tests/test_kb_notion.py
test_pages_create_parents_to_data_source syncs a node with a mocked client and asserts pages.create was called with parent: {"type": "data_source_id", "data_source_id": ...} and that no database_id is present.
Notion smoke test script with staged verification
scripts/notion_smoke.py
New script validates NOTION_API_TOKEN and NOTION_WIKI_DB_ID, runs read-only checks (users.me(), data_sources.retrieve) and optionally runs a gated write (via --write) that creates/updates a page with sync_node_to_notion() and either archives or keeps the page based on --keep.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: migrating Notion page parent references from database_id to data_source_id for API 2025-09-03, with ticket reference.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ramirocanturamirez/rca-33-test-notion-integration-backend-functionality

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/notion_smoke.py`:
- Around line 4-8: The docstring's staged diagnostic description is wrong: it
calls out "databases.retrieve(db_id)" for stage 2 but the code actually queries
the data_sources endpoint (data_sources/{id}); update the docstring in
scripts.notion_smoke.py so stage 2 describes the actual call (e.g.,
"data_sources.retrieve(data_source_id)" or "GET data_sources/{id} — data source
reachable & integration shared") to match the request performed on lines that
hit data_sources/{id}.
- Around line 42-44: Replace the brittle raw request call to the Notion SDK with
the typed helper: instead of await
client.request(path=f"data_sources/{settings.NOTION_WIKI_DB_ID}", method="GET")
use await
client.data_sources.retrieve(data_source_id=settings.NOTION_WIKI_DB_ID"); update
the same pattern found at the other occurrence (around line ~117) as well; leave
the existing await client.aclose() in finally as-is since AsyncClient in this
repo's pinned notion-client (3.1.0) supports an awaitable aclose().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 511e662a-42af-4d82-91e6-5538a9465f2a

📥 Commits

Reviewing files that changed from the base of the PR and between 4378dfe and 805ff4b.

📒 Files selected for processing (3)
  • app/services/kb/notion.py
  • scripts/notion_smoke.py
  • tests/test_kb_notion.py

Comment thread scripts/notion_smoke.py
Comment thread scripts/notion_smoke.py Outdated
PR #8 review autofixes:

- pyproject: notion-client>=2.2 -> >=3.1. The data_source_id parent shape
  requires Notion API 2025-09-03, only sent by notion-client 3.x. A 2.x
  resolve would send the legacy database_id parent and 404, reintroducing
  the exact silent ObjectNotFound break PR #8 fixes. Refresh uv.lock.
- scripts/notion_smoke.py: stage 2 now uses the typed
  client.data_sources.retrieve(...) instead of a raw client.request(...),
  fix the docstring (stage 2 was mislabeled databases.retrieve), add a
  docstring to main() (docstring-coverage gate), and drop a redundant
  truthiness guard in the --keep branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ramirocantu
ramirocantu merged commit d58e921 into main Jun 1, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant