feat(openai-sdk-python): 7-tool parity - #1430
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-mcp | 348483d | Sep 01 2026, 04:39 AM |
|
Claude finished @ved015's task in 2m 0s —— View job PR Review: feat(openai-sdk-python): 7-tool parity
Summary: All three bugs identified in previous reviews have been fixed in commit 7cf0480. The blocking I/O issue was resolved by removing The code is clean and ready to ship. Score: 9/10 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 synchronousrequestsin the fallback path, which blocks the event loop. Fix by wrapping inasyncio.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( |
There was a problem hiding this comment.
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[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ 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, |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 1160c46. Configure here.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-app | 348483d | Commit Preview URL Branch Preview URL |
Sep 01 2026, 04:42 AM |
0a3ff69 to
1160c46
Compare
00462ad to
03849ce
Compare
There was a problem hiding this comment.
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:
-
forget_memory.py:46— Blocking I/O in async function: The synchronousrequests.delete()fallback will freeze the event loop. Since Python 3.9 is the minimum version, wrap withasyncio.to_thread(). -
tools.py:501— Profile data lost:get_profilefilters out Pydantic models withisinstance(profile, dict), but the SDK returns Pydantic objects. Use.model_dump()instead of the type check. -
tools.py:531-537— Pydantic objects not serialized:document_liststores rawdocumentsandpaginationobjects without calling.model_dump(), which will causejson.dumps()to raiseTypeErrorwhenexecute_tool_callserializes 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.
There was a problem hiding this comment.
Overview: Expands the OpenAI Python SDK from 2 tools to 7 (search, add, get_profile, document_list/add/delete, memory_forget) with proper Pydantic model serialization and SDK-native async methods.
Issues found: None — all previously identified bugs have been fixed in commit 7cf0480:
-
Blocking I/O in async function— Fixed. Theforget_memory.pyfile was removed;memory_forgetnow uses the SDK's nativeclient.memories.forget()which is already async. -
Profile data lost— Fixed.get_profilenow uses the new_model_to_dict()helper instead of the brokenisinstance(profile, dict)check. -
Pydantic objects not serialized— Fixed. Bothget_profileanddocument_listnow properly convert SDK response objects to dicts using_model_to_dict()before returning, ensuringjson.dumps()works correctly.
Additional observations:
- Good defensive validation in
execute_tool_call(lines 422-439) — validates JSON parsing, argument types, and schema conformance - Proper scope enforcement in
document_delete— verifies document tags are within configured scope before deletion - Clean error handling with separate paths for network errors vs other exceptions
- Test coverage expanded for new tools
Score: 9/10
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| response = await self.client.documents.add(**kwargs) | ||
| return DocumentAddResult( | ||
| success=True, | ||
| document=response.model_dump(), |
There was a problem hiding this comment.
Inconsistent response handling in document_add method. All other methods use _model_to_dict() helper to safely convert SDK responses (lines 531, 533, 567-568), but this line directly calls response.model_dump(). If the SDK returns a dict or an object without model_dump(), this will crash with AttributeError.
Fix:
document=_model_to_dict(response),| document=response.model_dump(), | |
| document=_model_to_dict(response), |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Merge activity
|
## 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 - [x] `uv run pytest tests/test_tools.py::TestMemoryOperationsUnit` Made with [Cursor](https://cursor.com)
7eca7c7 to
c5b7e7d
Compare
7cf0480 to
348483d
Compare
The base branch was changed.
2 participants
Summary
SupermemoryToolsfrom 2 tools to 7 (matches@supermemory/tools)memory_forgetvia shared HTTP helperStacked on #1429
Test plan
uv run pytest tests/test_tools.py::TestMemoryOperationsUnitMade with Cursor