Skip to content

fix: Notion not-connected crashes + test suite reliability - #13

Open
0xSY3 wants to merge 5 commits into
mainfrom
fix/notion-not-connected-and-test-reliability
Open

fix: Notion not-connected crashes + test suite reliability#13
0xSY3 wants to merge 5 commits into
mainfrom
fix/notion-not-connected-and-test-reliability

Conversation

@0xSY3

@0xSY3 0xSY3 commented Aug 23, 2026

Copy link
Copy Markdown
Member

Summary

  • Production bug: _notion_request was raising ValueError when Notion isn't connected, propagating unhandled through all Notion tools (search_notion, create_notion_page, etc.). Now returns {"success": False, "error": "Notion not connected..."} — consistent with every other OAuth tool.
  • Pool race tests: patch(..., side_effect=async_fn) creates a MagicMock with no await_count tracking. Switched to new_callable=AsyncMock so the assertion works correctly.
  • Connect flow tests: _connect_client was a plain function that exited the patch("app.connect.get_pool", ...) context before any HTTP request was made. Converted to @asynccontextmanager so the patch stays alive.
  • publish_page tests: patch.dict("sys.modules", ...) is bypassed when app.services.pages_agent is already imported earlier in the suite (module attribute takes precedence). Switched to patch("app.services.pages_agent.create_page", ...).

Test plan

  • 206 passed, 0 failed locally (pytest tests/ -q)
  • Notion tools now return error dict when not connected (no exception escapes)
  • Verify Notion reconnect flow still works in staging

🤖 Generated with Claude Code

0xSY3 and others added 5 commits August 24, 2026 03:25
- app/tools/notion.py: _notion_request returns {"success": False, "error":
  "Notion not connected..."} when no token instead of raising ValueError,
  matching the graceful-degradation contract every other tool follows
- tests: fix AsyncMock pool-race assertions (MagicMock has no await_count)
- tests: _connect_client made async context manager so patch stays active
  during ASGI requests (was exiting before first DB call)
- tests: publish_page mocks switched from sys.modules patch to direct
  patch("app.services.pages_agent.create_page") — sys.modules bypass fails
  when module already imported earlier in suite

206 passed, 0 failed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Caddy runs in ~/memory-layer stack (production data: 27 users).
~/zynd was a dead-end second copy that previous deploys incorrectly
targeted. All future `make deploy` / `make ship` now hit the live stack.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
POST /context/{user_id} and GET /users/{user_id}/graph were 403ing for
all agent-persona users because current_user() translates a Supabase UUID
to the internal memory-layer ID, but the path param check compared the
raw Supabase UUID against the translated ID — they never matched.

Result: ingest worked (no path param check) but context reads silently
failed, so the agent had no memory on every turn even though data was
being saved.

Fix: query with auth_user (already resolved by current_user) instead of
the path param user_id.
Passing a different user_id in the path now returns the caller's own
graph (200) instead of 403 — the path param is ignored in favor of
auth_user. Assert the returned data matches the direct /users/{uid}/graph
call to verify no cross-user leakage.
Same Supabase UUID → internal ID mismatch as the context/graph fix.
All path-param endpoints that compare user_id == auth_user now use
auth_user directly for queries so callers with mapped Supabase UUIDs
are never blocked with a false 403.
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