Skip to content

feat(openai-sdk-python): 7-tool parity - #1430

Open
Dhravya wants to merge 1 commit into
fix/openai-sdk-python-v4-apifrom
feat/openai-sdk-python-7-tools
Open

feat(openai-sdk-python): 7-tool parity#1430
Dhravya wants to merge 1 commit into
fix/openai-sdk-python-v4-apifrom
feat/openai-sdk-python-7-tools

Conversation

@Dhravya

@Dhravya Dhravya commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Expand SupermemoryTools from 2 tools to 7 (matches @supermemory/tools)
  • Add memory_forget via shared HTTP helper
  • Add document list/add/delete and get_profile tool surfaces
  • Expand tests for new tools and execution paths

Stacked on #1429

Test plan

  • uv run pytest tests/test_tools.py::TestMemoryOperationsUnit

Made with Cursor

Add document_list, document_add, document_delete, get_profile, and
memory_forget alongside search_memories and add_memory. Includes
forget_memory HTTP helper and expanded tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 8, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
supermemory-mcp 1160c46 Aug 18 2026, 03:22 PM

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @Dhravya's task in 1m 22s —— View job


Review Complete

  • Fetch PR diff and existing review comments
  • Read changed files for full context
  • Analyze for bugs, security issues, and logic errors
  • Submit review findings

Summary: Reviewed the 7-tool expansion for the OpenAI Python SDK. The code structure is clean with good config validation, but I confirmed 3 bugs identified by previous reviewers that will cause runtime failures:

  1. Blocking I/O in async forget_memory_request fallback path
  2. Profile data lost due to isinstance(profile, dict) check rejecting Pydantic models
  3. JSON serialization fails for document_list results containing raw Pydantic objects

Score: 7/10 — Fix the above before merge; see my review comment for details.

@claude claude 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.

Overview: Expands the OpenAI Python SDK from 2 tools to 7 (search, add, get_profile, document_list/add/delete, memory_forget) with a new HTTP helper for the forget endpoint.

Issues found:

  • forget_memory.py:46 — The async function uses synchronous requests in the fallback path, which blocks the event loop. Fix by wrapping in asyncio.to_thread().

Score: 8/10

The code is well-structured with proper error handling, good validation (e.g., rejecting both project_id and container_tags), and comprehensive test coverage. The one issue above should be fixed before merge to avoid blocking async applications when aiohttp isn't installed.

except ImportError:
import requests

response = requests.delete(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Synchronous I/O blocks the event loop

This async function falls back to synchronous requests when aiohttp isn't installed. Since requests.delete() is blocking, it will freeze the entire event loop until the HTTP request completes — potentially causing timeouts or deadlocks in async applications.

Since Python 3.9 is now the minimum version, use asyncio.to_thread() to run the blocking call in a thread pool:

except ImportError:
    import asyncio
    import requests

    def _sync_delete() -> requests.Response:
        return requests.delete(
            f"{base_url}/v4/memories",
            headers={
                "Content-Type": "application/json",
                "Authorization": f"Bearer {api_key}",
            },
            json=payload,
            timeout=30,
        )

    response = await asyncio.to_thread(_sync_delete)
    if not response.ok:
        raise RuntimeError(
            f"Supermemory forget memory failed: {response.status_code} "
            f"{response.reason}. {response.text}"
        )

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1160c46. Configure here.

return ProfileResult(
success=True,
profile=profile if isinstance(profile, dict) else None,
search_results=search_results,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Profile dropped on success

High Severity

get_profile marks success but sets profile to null whenever the SDK returns a Pydantic profile object, because only dict instances are kept. Real client.profile() responses use models, so the tool often returns an empty profile despite a successful API call.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1160c46. Configure here.

Comment thread packages/openai-sdk-python/src/supermemory_openai/tools.py
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 8, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
supermemory-app 1160c46 Commit Preview URL

Branch Preview URL
Aug 18 2026, 03:25 PM

Dhravya commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label Main to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude claude 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.

Overview: Expands the OpenAI Python SDK from 2 tools to 7 (search, add, get_profile, document_list/add/delete, memory_forget) with new result types and a custom HTTP helper for the forget endpoint.

Issues found:

The previous reviews correctly identified 3 bugs that need fixing:

  1. forget_memory.py:46 — Blocking I/O in async function: The synchronous requests.delete() fallback will freeze the event loop. Since Python 3.9 is the minimum version, wrap with asyncio.to_thread().

  2. tools.py:501 — Profile data lost: get_profile filters out Pydantic models with isinstance(profile, dict), but the SDK returns Pydantic objects. Use .model_dump() instead of the type check.

  3. tools.py:531-537 — Pydantic objects not serialized: document_list stores raw documents and pagination objects without calling .model_dump(), which will cause json.dumps() to raise TypeError when execute_tool_call serializes the result.

For consistency, note that search_memories, add_memory, and document_add correctly use .model_dump() — applying the same pattern to get_profile and document_list will fix issues 2 and 3.

Score: 7/10

The code structure is clean, config validation is good (rejecting both project_id and container_tags), and test coverage was expanded. The three issues above will cause runtime failures in production async applications and should be fixed before merge.

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.

2 participants