Skip to content

Conversation

@MaheshtheDev
Copy link
Member

No description provided.

@MaheshtheDev MaheshtheDev marked this pull request as ready for review December 24, 2025 01:10
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 24, 2025

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 failed
View logs
supermemory-app d095bd2 Dec 24 2025, 01:37 AM

Copy link
Member 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.

@claude
Copy link

claude bot commented Dec 24, 2025

Pull Request Review: Vercel AI SDK v5 and v6 Compatibility

Thanks for working on backward compatibility support! This is an important improvement for users on different SDK versions.


✅ Strengths

  1. Good Architectural Approach: Moving from middleware pattern to direct wrapper is cleaner
  2. Proper Peer Dependencies: Using peerDependencies with version ranges (^2.0.0 || ^3.0.0) is correct
  3. Backward Compatibility: Union types allow supporting both SDK versions
  4. Code Organization: Good refactoring with extracted reusable functions

🔴 Critical Issues

1. Broken Test Suite (packages/tools/test/vercel.test.ts)

The refactoring breaks existing tests. Tests import createSupermemoryMiddleware but middleware.ts now exports createSupermemoryContext instead.

Action Required: Update tests to match new architecture - test withSupermemory wrapper, doGenerate/doStream calls, and V2/V3 compatibility.

2. Missing Type Safety (src/vercel/index.ts:97, 133)

Using as any bypasses type checking. Add runtime type guards based on model.specificationVersion.

3. Unsafe Type Assertions (src/vercel/memory-prompt.ts:145-147, 154-158)

Multiple any usages with type assertions could lead to V2/V3 type mismatches.


⚠️ Major Concerns

4. No SDK Version Validation

Code assumes compatible signatures without runtime verification. Add validation for specificationVersion.

5. Fire-and-Forget Memory Saving (src/vercel/index.ts:104-113, 153-162)

saveMemoryAfterResponse is called without await, so errors are silently swallowed. Either await or add explicit error handling.

6. Unsafe Type Coercion (src/vercel/util.ts:87-89)

Content type differs between V2 and V3, making type casts potentially unsafe.


📋 Minor Issues

  • Inconsistent Error Handling: Cannot differentiate memory vs model errors
  • Missing Documentation: No migration guide for v5 to v6
  • Potential Memory Leak: generatedText accumulates for long responses
  • No Input Sanitization: Consider documenting prompt injection risks
  • API Key Exposure: Ensure not logged or included in errors

🧪 Test Coverage

Missing tests for:

  1. V2 and V3 model compatibility
  2. Stream transform error handling
  3. Memory save failures
  4. Type casting edge cases
  5. Large response handling

📝 Must Fix Before Merge

  1. ✅ Fix broken test suite
  2. ✅ Replace as any with proper type guards
  3. ✅ Add error handling for saveMemoryAfterResponse
  4. ✅ Add SDK version validation

Should Fix

  1. Add migration guide to README
  2. Improve error messages
  3. Add V2/V3 compatibility tests
  4. Document streaming considerations

🎬 Conclusion

Valuable feature but needs refinement. Main concerns: broken tests, compromised type safety, and missing async error handling.

Estimated effort: 4-6 hours depending on test rewrites.

@graphite-app
Copy link

graphite-app bot commented Dec 24, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the 12-23-feat_supermemory_tools_vercel_ai_sdk_compatbile_with_v5_and_v6 branch from 176485d to d095bd2 Compare December 24, 2025 01:36
@graphite-app graphite-app bot merged commit d095bd2 into main Dec 24, 2025
3 of 4 checks passed
@claude
Copy link

claude bot commented Dec 24, 2025

Pull Request Review: Vercel AI SDK v5/v6 Compatibility

Overview

This PR refactors @supermemory/tools to support both Vercel AI SDK v5 (LanguageModelV2) and v6 (LanguageModelV3) through runtime detection and union types. The changes are substantial but well-structured.


Strengths

1. Smart Backward Compatibility Strategy

  • Moving @ai-sdk/provider to devDependencies with peerDependencies range ^2.0.0 || ^3.0.0 is the correct approach for dual SDK support
  • Allows consumers to control which version they use without forcing upgrades
  • Version bump to 1.3.62 is appropriate

2. Architecture Improvements

The refactor from middleware-based to wrapper-based approach (packages/tools/src/vercel/index.ts:63-177) improves:

  • Direct control over doGenerate and doStream methods
  • Clearer data flow - easier to understand when memories are injected vs saved
  • Better error handling with try/catch around each method

3. Code Organization

  • Good separation of concerns: middleware.ts now exports reusable functions instead of monolithic middleware
  • New util types LanguageModel, LanguageModelCallOptions, etc. centralize SDK version unions (packages/tools/src/vercel/util.ts:13-22)
  • createSupermemoryContext provides clean context initialization (packages/tools/src/vercel/middleware.ts:177-209)

⚠️ Issues & Concerns

1. Type Safety Issues (Critical)

Problem: Heavy use of any types breaks type safety:

// packages/tools/src/vercel/index.ts:95
const result = await model.doGenerate(transformedParams as any)

// packages/tools/src/vercel/index.ts:104
const { stream, ...rest } = await model.doStream(transformedParams as any)

// packages/tools/src/vercel/memory-prompt.ts:145-149
const newPrompt = params.prompt.map((prompt: any) => ...

Risk: Runtime errors if V2/V3 interfaces diverge beyond current assumptions

Recommendation: Use type guards or conditional types instead:

function isV3Model(model: LanguageModel): model is LanguageModelV3 {
  return 'specificationVersion' in model && model.specificationVersion === 'v3'
}

// Then use it:
if (isV3Model(model)) {
  // TypeScript knows this is V3
  const result = await model.doGenerate(transformedParams as LanguageModelV3CallOptions)
} else {
  const result = await model.doGenerate(transformedParams as LanguageModelV2CallOptions)
}

2. Breaking Changes Not Documented (High Priority)

The PR changes the public API surface:

  • createSupermemoryMiddleware is no longer exported as the primary API
  • Existing code using wrapLanguageModel from AI SDK with createSupermemoryMiddleware will break
  • Return type changed from LanguageModelV2 to generic T extends LanguageModel

Recommendation:

  • Add a CHANGELOG entry or migration guide
  • Consider keeping createSupermemoryMiddleware as deprecated with a proxy to new implementation
  • Update README examples if any reference the old middleware approach

3. Test Coverage Gap (High Priority)

Looking at packages/tools/test/vercel.test.ts, all tests are written against the old createSupermemoryMiddleware API:

  • Line 2-3: Imports both withSupermemory and createSupermemoryMiddleware
  • Line 86-376: All tests use createSupermemoryMiddleware
  • No tests validate V3/V6 SDK compatibility
  • No tests for the new doGenerate/doStream wrapper approach

Critical Missing Tests:

  1. Test withSupermemory with a V3 model mock
  2. Test that doGenerate properly saves memories when addMemory: "always"
  3. Test that doStream properly saves memories after stream completion
  4. Test runtime detection of specificationVersion
  5. Test error handling in the new wrapper methods

Recommendation: Add test file test/vercel-v3.test.ts:

describe('V3 SDK Support', () => {
  it('should work with LanguageModelV3', async () => {
    const mockV3Model: LanguageModelV3 = {
      specificationVersion: 'v3',
      // ... mock implementation
    }
    const wrapped = withSupermemory(mockV3Model, 'test-tag')
    // assertions...
  })
})

4. Potential Memory Leak (Medium)

In packages/tools/src/vercel/index.ts:106-122, the TransformStream captures generatedText in closure:

let generatedText = ""
const transformStream = new TransformStream({
  transform(chunk, controller) {
    if (chunk.type === "text-delta") {
      generatedText += chunk.delta // String concatenation in hot path
    }
    controller.enqueue(chunk)
  },
  // ...
})

Issues:

  • For large streaming responses, repeated string concatenation can be inefficient (creates new string each time)
  • generatedText variable persists in closure until stream completes

Recommendation: Use array and join:

const textChunks: string[] = []
const transformStream = new TransformStream({
  transform(chunk, controller) {
    if (chunk.type === "text-delta") {
      textChunks.push(chunk.delta)
    }
    controller.enqueue(chunk)
  },
  flush: async () => {
    const generatedText = textChunks.join('')
    // ... save memory
  }
})

5. Silent Failure in Memory Save (Medium)

In packages/tools/src/vercel/middleware.ts:95-152, saveMemoryAfterResponse has no error handling - it's fire-and-forget:

export const saveMemoryAfterResponse = async (...) => {
  try {
    // ... API calls
  } catch (error) {
    logger.error("Failed to save memory", { error })
    // Error is logged but not propagated
  }
}

Issue: If memory saving fails, user has no way to know (except checking logs)

Recommendation:

  • Document this behavior in JSDoc
  • Consider adding optional error callback:
interface WrapVercelLanguageModelOptions {
  // ... existing options
  onMemorySaveError?: (error: Error) => void
}

6. Inconsistent Function Naming (Low)

  • transformParamsWithMemory - good, descriptive
  • extractAssistantResponseText - good
  • saveMemoryAfterResponse - good
  • createStreamTransform - defined but never used (packages/tools/src/vercel/middleware.ts:211-244)

Recommendation: Remove unused createStreamTransform export or document why it's kept for future use


🔒 Security Considerations

Good:

  • API key validation remains intact (packages/tools/src/vercel/index.ts:70-74)
  • No injection vulnerabilities introduced
  • Error messages don't leak sensitive data

Improvement Needed:

  • Consider rate limiting on saveMemoryAfterResponse to prevent API abuse if addMemory: "always" is used with high-volume streaming

🎯 Performance Considerations

Positive:

  • Wrapper approach is more efficient than middleware pattern (fewer function calls)
  • Memory search is properly async and non-blocking

Watch Out:

  • String concatenation in stream transform (mentioned above)
  • addSystemPrompt does synchronous array operations on potentially large prompt arrays (packages/tools/src/vercel/memory-prompt.ts:144-151)

📝 Documentation & Maintainability

Good:

  • JSDoc updated to mention V2/V3 support (packages/tools/src/vercel/index.ts:30-31)
  • Comment about union type compatibility: // biome-ignore lint/suspicious/noExplicitAny: Union type compatibility between V2 and V3

Needs Improvement:

  • No README update showing V6 SDK usage examples
  • createSupermemoryContext interface is not documented
  • Migration guide missing for users on the old API

🧪 Testing Recommendations

  1. Add V3 integration tests - Critical for validating the main goal of this PR
  2. Add stream memory save tests - Verify flush() callback works correctly
  3. Add error scenario tests - What happens when API fails during streaming?
  4. Add regression tests - Ensure V2/SDK5 still works exactly as before

Suggestions for Future PRs

  1. Consider adding telemetry to track V2 vs V3 usage
  2. Add performance benchmarks comparing middleware vs wrapper approach
  3. Extract type utilities into a shared types.ts file for clarity
  4. Consider supporting specificationVersion beyond v2/v3 with a warning

📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐ Good refactor, but needs type safety improvements
Test Coverage ⭐⭐ Tests don't cover new functionality
Documentation ⭐⭐⭐ Adequate inline docs, missing migration guide
Security ⭐⭐⭐⭐⭐ No new vulnerabilities introduced
Performance ⭐⭐⭐⭐ Minor string concatenation concern
Breaking Changes ⚠️ Undocumented API changes

🎬 Verdict

Conditional Approval - This is solid architectural work, but needs:

Required Before Merge:

  1. ✅ Add V3/SDK6 test coverage
  2. ✅ Document breaking changes in CHANGELOG
  3. ✅ Fix type safety issues (any casts)

Recommended Before Merge:

  1. Improve stream memory collection performance
  2. Add migration guide to README
  3. Remove unused createStreamTransform function

Nice-to-Have (Can be follow-up PR):

  1. Add error callback for memory save failures
  2. Add telemetry/metrics for V2 vs V3 usage

Great work on the dual SDK support! The architecture is sound, just needs polish on testing and type safety before shipping. 🚀

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.

3 participants