Skip to content

open-ai-sdk-conversation-id-update#828

Open
sreedharsreeram wants to merge 1 commit intomainfrom
04-03-open-ai-sdk-conversation-id-update
Open

open-ai-sdk-conversation-id-update#828
sreedharsreeram wants to merge 1 commit intomainfrom
04-03-open-ai-sdk-conversation-id-update

Conversation

@sreedharsreeram
Copy link
Copy Markdown
Contributor

No description provided.

@mintlify
Copy link
Copy Markdown
Contributor

mintlify bot commented Apr 3, 2026

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
supermemory 🟢 Ready View Preview Apr 3, 2026, 5:29 PM

Copy link
Copy Markdown
Contributor Author


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.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Apr 3, 2026

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 a73814b Commit Preview URL Apr 06 2026, 05:22 PM

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 2/5 - Changes Needed

Not safe to merge — this PR promotes conversationId/conversation_id from an optional config field to a required positional parameter across both the TypeScript (withSupermemory, createOpenAIMiddleware in packages/tools/src/openai/index.ts and middleware.ts) and Python (with_supermemory, SupermemoryMiddleware in packages/openai-sdk-python/src/supermemory_openai/middleware.py) packages without a migration path for existing consumers. While the intent to enforce conversation identity at the API boundary is sound, removing conversation_id from OpenAIMiddlewareOptions means any caller currently using OpenAIMiddlewareOptions(conversation_id=...) will receive a TypeError at runtime, and any TypeScript consumer who passed options.conversationId will have it silently ignored — a particularly dangerous silent regression. Both issues are breaking API changes with high-impact runtime consequences that require deprecation handling, backward-compatibility shims, or at minimum a major version bump and clear migration guide before merging.

Key Findings:

  • In middleware.py, removing conversation_id from OpenAIMiddlewareOptions is a hard runtime break: existing callers passing OpenAIMiddlewareOptions(conversation_id=...) will receive a TypeError with no warning, since Python does not have compile-time enforcement to catch this before deployment.
  • In packages/tools/src/openai/index.ts, the old options.conversationId path is silently dropped — TypeScript will emit a type error for external consumers, but any compiled or loosely-typed consumer will have their conversation ID silently ignored, leading to data being routed under the wrong or missing conversation context with no observable error.
  • Both packages lack a deprecation shim or version guard that would allow a grace period for consumers to migrate; promoting a field to a required positional parameter in a public library without a major version boundary violates semver contract and can cause production incidents in downstream integrations.
  • The PR does achieve a cleaner, more explicit API surface by making conversation identity a first-class concern rather than an opt-in config field, which improves discoverability and reduces misconfiguration — but this benefit does not offset the unmitigated breaking-change risk.
Files requiring special attention
  • packages/openai-sdk-python/src/supermemory_openai/middleware.py
  • packages/tools/src/openai/index.ts
  • packages/tools/src/openai/middleware.ts

Copy link
Copy Markdown

@entelligence-ai-pr-reviews entelligence-ai-pr-reviews bot left a comment

Choose a reason for hiding this comment

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

🔍 Duplicate Code Detected

Found 1 duplicate function(s) in this PR. Consider consolidating to reduce code duplication.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 3/5 - Review Recommended

Not safe to merge without review — while this PR successfully promotes conversationId/conversation_id to a required positional parameter in both the TypeScript and Python OpenAI middleware packages, the Python change in middleware.py introduces a silent runtime breakage for any existing callers using with_supermemory(client, tag) or with_supermemory(client, tag, options=...), who will now receive a TypeError at runtime since conversation_id is inserted as a new required positional argument between container_tag and options. The TypeScript counterpart appears to handle this breaking change intentionally, but the Python migration path lacks a deprecation shim or clear migration guide, making this a potentially disruptive drop for Python consumers.

Key Findings:

  • In middleware.py, inserting conversation_id: str as a required positional parameter between container_tag and options is a hard runtime break — existing callers using keyword or positional options as the third argument will get TypeError: with_supermemory() missing required positional argument: 'conversation_id' with no graceful fallback.
  • The PR intentionally makes a breaking API change across both SDKs, but only the Python side lacks any compatibility layer (e.g., a DeprecationWarning shim or version bump signaling) that would give downstream consumers time to migrate safely.
  • The TypeScript changes in middleware.ts and index.ts correctly reflect the new signature (client, userId, conversationId, options) and are consistent with each other, which is a positive aspect of the PR's design.
  • No validation is added for the new required conversation_id parameter in either language (e.g., empty string check), meaning callers passing "" would silently produce malformed API calls.
Files requiring special attention
  • packages/openai-sdk-python/src/supermemory_openai/middleware.py
  • packages/tools/src/openai/middleware.ts
  • packages/tools/src/openai/index.ts

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 1/5 - Blocking Issues

Not safe to merge — this PR introduces a breaking API change by promoting conversation_id to a required positional parameter in with_supermemory() and removing it from OpenAIMiddlewareOptions, which will cause TypeError: unexpected keyword argument 'conversation_id' crashes for any existing Python callers who were passing it via OpenAIMiddlewareOptions(conversation_id=...) or using the old positional signature with_supermemory(client, tag, options=...). The PR does accomplish its stated goal of making conversationId/conversation_id a required parameter in both TypeScript and Python integrations, but the migration path for existing callers is unsafe without a deprecation shim or migration guide. A pre-existing unresolved concern about positional argument ordering in middleware.py at L616-L632 remains open, compounding the breakage risk for callers using keyword-only invocation patterns.

Key Findings:

  • Removing conversation_id from OpenAIMiddlewareOptions in middleware.py is a hard runtime break: existing callers passing OpenAIMiddlewareOptions(conversation_id='...') will get an immediate TypeError: unexpected keyword argument 'conversation_id' with no graceful fallback or deprecation warning.
  • The insertion of conversation_id: str as a required positional parameter between container_tag and options in with_supermemory() breaks any caller using with_supermemory(client, tag, options=SomeOptions()) without passing conversation_id, turning a previously valid call into a TypeError at call sites that haven't been updated.
  • No backward-compatibility shim, deprecation warning, or migration guide is present to help existing users transition from the old optional config-based API to the new required positional parameter API, making this a silent runtime bomb for downstream consumers.
  • The version bump alone is insufficient signal for breaking changes of this magnitude — the TypeScript signature change (withSupermemory(openai, userId, conversationId, options)) similarly removes the optional nature of conversationId without guarding against callers who omit it or pass it as part of the options object.
Files requiring special attention
  • packages/openai-sdk-python/src/supermemory_openai/middleware.py
  • packages/tools/src/openai/index.ts
  • packages/tools/src/openai/middleware.ts

@sreedharsreeram sreedharsreeram marked this pull request as ready for review April 3, 2026 21:31
@claude
Copy link
Copy Markdown

claude bot commented Apr 3, 2026

Claude finished @sreedharsreeram's task in 1m 47s —— View job


PR Review Complete

  • Fetch PR diff and understand the changes
  • Review TypeScript SDK changes
  • Review Python SDK changes
  • Check for real bugs, security issues, or breaking changes
  • Submit review

Summary: Reviewed the PR making customId/custom_id a required parameter in both TypeScript and Python OpenAI SDK wrappers. Found no bugs, security issues, or logic errors. The implementation is clean with proper validation, consistent defaults (addMemory = "never"), and solid test coverage. Many automated review comments flagged false positives about the addMemory default — the code clearly shows it remains "never".

Score: 10/10 — Good to ship.

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Makes conversationId a required positional parameter instead of optional config field in both TypeScript and Python OpenAI SDK wrappers.

Issues found: None — this looks good to ship.

The implementation is correct:

  • Both SDKs properly validate that conversationId is non-empty (rejects empty strings and whitespace-only)
  • The parameter is correctly propagated through wrapper classes
  • Documentation and examples are updated to reflect the new API signature
  • Tests cover the new validation behavior and updated signatures
  • Shared test utilities extracted to avoid duplication

The breaking change nature has been flagged by existing review comments. The version bumps (1.4.01→1.5.0 for TS, 1.0.2→1.1.0 for Python) are minor versions rather than major, which technically violates semver for breaking changes. However, the implementation itself has no bugs, security issues, or logic errors.

Score: 9/10

Minor deduction for semver semantics (breaking change should ideally be a major version bump), but the code quality is solid.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 3/5 - Review Recommended

Not safe to merge without review — while this PR correctly promotes conversation_id to a required positional parameter in both the TypeScript and Python SDKs, it introduces a silent breaking change in middleware.py by removing conversation_id from OpenAIMiddlewareOptions without any deprecation path, meaning any existing caller using OpenAIMiddlewareOptions(conversation_id='...') will receive a TypeError at runtime with no warning. The TypeScript side appears well-handled with explicit validation guards and clear error throws, but the Python side needs a migration story — at minimum a deprecation warning or backwards-compatible shim before hard removal.

Key Findings:

  • Removing conversation_id from OpenAIMiddlewareOptions in middleware.py is a hard breaking change with no deprecation path — existing callers passing OpenAIMiddlewareOptions(conversation_id='...') will silently fail at runtime with a TypeError, which is especially dangerous in a SDK wrapper used by downstream consumers who may not pin their dependency version.
  • The TypeScript implementation in packages/tools/src/openai/index.ts and middleware.ts correctly enforces the new required conversationId parameter with explicit validation guards that throw descriptive errors, which is the right pattern but is inconsistently applied across the two SDK implementations.
  • Version bumps are present, but without a changelog entry or migration guide documenting the conversation_id removal from OpenAIMiddlewareOptions, users of the Python SDK will have no guidance on how to update their integration code.
Files requiring special attention
  • packages/openai-sdk-python/src/supermemory_openai/middleware.py

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Makes conversationId a required parameter in both TypeScript and Python OpenAI SDK wrappers, consolidating the API surface.

Issues found: One minor issue already flagged by Sentry — containerTag validation in TypeScript (packages/tools/src/openai/index.ts:75) is missing a .trim() check, allowing whitespace-only strings to pass validation. This is inconsistent with the Python implementation which correctly validates both fields.

The implementation is otherwise solid:

  • Both SDKs properly validate that conversationId is non-empty with type and .trim() checks
  • The parameter is correctly propagated through wrapper classes
  • Documentation and examples are updated to reflect the new API signature
  • Tests cover the new validation behavior and updated signatures
  • Shared test utilities extracted to avoid duplication

The breaking change nature is intentional and well-documented. Version bumps are minor (1.5.0 for TS, 1.1.0 for Python) rather than major, which technically doesn't follow semver for breaking changes — but that's a release process decision.

Score: 9/10

Minor deduction for the containerTag validation inconsistency. Code quality is solid with proper validation, error messages, and test coverage.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 2/5 - Changes Needed

Not safe to merge — the new required conversationId validation in withSupermemory directly contradicts how conversationId is used throughout createOpenAIMiddleware and its callees, where it is treated as fully optional (guarded by conversationId ? ... : undefined checks). This regression means valid stateless use cases — memory lookup without a conversation ID — will throw at runtime despite being explicitly supported by the downstream logic. The PR's goal of unifying to SupermemoryOpenAIOptions and releasing a clean v2.0.0 API is sound, but the mismatch between the required-field guard and the optional-field usage pattern must be resolved before this ships.

Key Findings:

  • In packages/tools/src/openai/index.ts, withSupermemory throws if conversationId is absent, yet createOpenAIMiddleware and all its callees reference conversationId conditionally (e.g., conversationId ? ... : undefined), meaning the validation rejects a usage pattern the rest of the codebase explicitly accommodates — this is a runtime-breaking logic bug introduced by this PR.
  • The test file packages/tools/test/with-supermemory/unit.test.ts places an import statement after the createMockLanguageModel function declaration rather than at the top with other imports, which will fail Biome's enforced import ordering and break bun run format-lint in CI.
  • The rename from OpenAIMiddlewareOptions to SupermemoryOpenAIOptions and the move to a unified options object are architecturally clean and consistent across the TypeScript and Python SDKs, making the API surface easier to version and document.
  • Making conversationId a required field in the options type without auditing every call site for conditional usage is a classic breaking-change pitfall; the field should either remain optional in the type, or all conditional guards on it should be removed — neither was done consistently.
Files requiring special attention
  • packages/tools/src/openai/index.ts
  • packages/tools/test/with-supermemory/unit.test.ts

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Consolidates containerTag and customId (formerly conversationId) into a required options object for both TypeScript and Python OpenAI SDK wrappers.

Issues found: One minor issue already flagged by Sentry — containerTag validation in TypeScript (packages/tools/src/openai/index.ts:75) is missing a .trim() check, allowing whitespace-only strings to pass validation. This is inconsistent with the customId validation which correctly uses .trim().

The implementation is otherwise solid:

  • Both SDKs properly validate that customId is non-empty with type and .trim() checks
  • Documentation and examples are updated to reflect the new API signature
  • Tests cover the new validation behavior and updated signatures
  • Shared test utilities extracted to avoid duplication

Note on the Entelligence review claiming conversationId is treated as optional in createOpenAIMiddleware: This is incorrect. The conditional checks like customId ? ... : undefined in the middleware are for determining behavior (e.g., using conversation endpoint vs regular memory endpoint), not for treating it as optional. The validation in withSupermemory correctly enforces customId as required before calling createOpenAIMiddleware.

Score: 9/10

Minor deduction for the containerTag validation inconsistency with .trim(). Code quality is solid with proper validation, error messages, and test coverage.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 2/5 - Changes Needed

Not safe to merge — this v2.0.0 refactor introduces a breaking validation regression where customId/custom_id is enforced as a hard requirement in both packages/tools/src/openai/index.ts and packages/openai-sdk-python/src/supermemory_openai/middleware.py, despite neither the TypeScript middleware's downstream if (customId) guards nor the Python SDK's add_memory mode requiring it. The PR's unification of the options surface under SupermemoryOpenAIOptions is a clean improvement, but shipping this validation logic means any caller operating without a customId — which the middleware explicitly supports — will be incorrectly rejected at runtime. Both issues are in the critical path of the public API surface and will cause immediate breakage for valid use cases, warranting fixes before merge.

Key Findings:

  • In packages/tools/src/openai/index.ts, customId is validated as a hard requirement, but all downstream logic wraps its usage in if (customId) guards — meaning the middleware was intentionally designed to function without it. This validation contradicts the implementation and will throw errors for currently valid callers.
  • In packages/openai-sdk-python/src/supermemory_openai/middleware.py, custom_id validation unconditionally rejects empty/missing values regardless of the add_memory mode. Since custom_id is only semantically meaningful when add_memory='always', this breaks the default add_memory='never' path for any caller not supplying a custom_id.
  • Both bugs are in the public API entry points of a major version bump, meaning they will affect all adopters of v2.0.0 immediately and constitute a regression from any prior flexible usage pattern.
  • 2 previous unresolved comment(s) likely resolved in latest diff (score-only signal; thread status unchanged)
Files requiring special attention
  • packages/tools/src/openai/index.ts
  • packages/openai-sdk-python/src/supermemory_openai/middleware.py

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Consolidates containerTag and customId into a required options object for both TypeScript and Python OpenAI SDK wrappers, making the API more explicit and consistent.

Issues found: None — this looks good to ship.

After reviewing the code and existing review comments:

  • Validation is correct: Both TypeScript (packages/tools/src/openai/index.ts:74-96) and Python (packages/openai-sdk-python/src/supermemory_openai/middleware.py:690-702) properly validate required fields with type checks and .trim()/.strip() to reject whitespace-only strings.

  • Sentry's containerTag comment was incorrect: The code at line 77 does include !options.containerTag.trim() — the validation is consistent with customId.

  • Responses API behavior is intentional: The addMemoryTool call passing undefined for messages (line 589) is documented and correct — Responses API doesn't have a messages array, so it falls back to regular memory storage rather than the conversation endpoint.

  • The "optional customId" concern is a misread: The conditional checks like customId ? ... : undefined in the middleware determine which endpoint/behavior to use, not whether customId is optional. The validation in withSupermemory correctly enforces it as required before the middleware is ever called.

The breaking API change from positional parameters to an options object is intentional and improves the API surface. Documentation and examples are updated accordingly.

Score: 10/10

Clean implementation with proper validation, consistent error messages, and good test coverage.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 2/5 - Changes Needed

Not safe to merge — this PR introduces a critical silent behavioral regression in packages/tools/src/openai/middleware.ts where the default value of addMemory has quietly shifted from "never" to "always", meaning any caller who omits the addMemory option will now automatically persist all conversation data as memories without consent. This PR does achieve a clean API unification (renaming OpenAIMiddlewareOptions to SupermemoryOpenAIOptions and consolidating positional arguments into a single options object), but the default-behavior flip carries real privacy and cost implications that must be explicitly addressed before merging. Additionally, the test file packages/tools/test/with-supermemory/unit.test.ts has an import placed mid-file after function declarations, which will fail Biome's lint check per the project's CLAUDE.md formatting rules.

Key Findings:

  • In middleware.ts, the addMemory default changed from options?.addMemory ?? "never" to effectively "always" — this is a breaking behavioral change that will silently start persisting every conversation for any caller that doesn't explicitly pass addMemory: "never", introducing both unexpected costs and potential privacy violations.
  • The addMemory default regression is duplicated/confirmed in two separate review comments flagged at score 8.5, indicating high confidence in the finding and that it is not a false positive.
  • In unit.test.ts, an import statement is placed after function declarations mid-file, violating ES module ordering conventions and Biome's enforced linting rules, which will cause bun run format-lint to fail in CI.
Files requiring special attention
  • packages/tools/src/openai/middleware.ts
  • packages/tools/test/with-supermemory/unit.test.ts

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Consolidates containerTag and customId into a required options object for both TypeScript and Python OpenAI SDK wrappers, making conversation identity a first-class required parameter.

Issues found: None — this looks good to ship.

After reviewing the implementation:

  • Validation is correct: Both TypeScript (index.ts:74-96) and Python (middleware.py:695-707) properly validate required fields with type checks and .trim()/.strip() to reject whitespace-only strings
  • Defaults are correct: addMemory = "never" (line 448 in middleware.ts, line 53 in middleware.py) and mode = "profile" — the previous automated reviews incorrectly claimed the default changed to "always", but the code clearly shows "never"
  • Test coverage is comprehensive: Unit tests verify validation, memory injection, and default behavior
  • Breaking change is intentional: The API surface change from positional params to an options object with required customId is documented and improves consistency

Note: Several previous automated review comments flagged false positives about the addMemory default. The code at middleware.ts:448 is addMemory = "never", not "always".

Score: 10/10

Clean implementation with proper validation, consistent error messages across both SDKs, and solid test coverage.

@socket-security
Copy link
Copy Markdown

socket-security bot commented Apr 6, 2026

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 4/5 - Mostly Safe

Likely safe but review recommended — this is a well-scoped v2.0.0 breaking change that consistently renames OpenAIMiddlewareOptions to SupermemoryOpenAIOptions and unifies the userId/containerTag positional parameters into a single options object across both TypeScript and Python SDKs. The only flagged issue is a cosmetic indentation inconsistency in packages/openai-sdk-python/tests/test_middleware.py where arguments to with_supermemory() are not indented one level deeper than the call site, which a formatter like Black or Ruff would auto-fix but has zero runtime impact. The core API refactoring logic appears coherent and the rename is applied consistently across the reviewed files.

Key Findings:

  • The indentation style of mock_openai_client and SupermemoryOpenAIOptions(...) arguments inside with_supermemory() calls in test_middleware.py deviates from the surrounding code's formatting convention, but this is a purely cosmetic/style issue with no effect on test correctness or runtime behavior.
  • The unification of userId/user_id and containerTag into a single SupermemoryOpenAIOptions object is a clean, consistent breaking change applied across both SDKs — no partial or mixed old/new API signatures were detected in the reviewed files.
  • 4 of 17 changed files were not reviewed, which introduces slight uncertainty, but the pattern of changes is consistent and mechanical enough that the risk from unreviewed files is low.
Files requiring special attention
  • packages/openai-sdk-python/tests/test_middleware.py

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Overview: Makes customId/custom_id a required parameter in both TypeScript and Python OpenAI SDK wrappers, consolidating the API surface into a cleaner options-based interface.

Issues found: None — this looks good to ship.

After reviewing the implementation:

  • Validation is correct: Both TypeScript (index.ts:74-96) and Python (middleware.py:810-821) properly validate required fields with type checks and .trim()/.strip() to reject whitespace-only strings
  • Defaults are correct: addMemory = "never" (line 448 in middleware.ts, line 53 in middleware.py) and mode = "profile" — contrary to several automated review comments, the default did NOT change to "always"
  • Test coverage is solid: Unit tests verify validation, memory injection, and default behavior for both SDKs
  • Breaking change is intentional: The API change from optional conversationId to required customId is by design and improves API consistency

Note: Many automated reviews flagged false positives about the addMemory default changing to "always". The code clearly shows addMemory = "never" in both SDKs.

Score: 10/10

Clean implementation with proper validation, consistent error messages across both SDKs, and comprehensive test coverage.

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