Skip to content

Conversation

MaheshtheDev
Copy link
Member

@MaheshtheDev MaheshtheDev commented Oct 2, 2025

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Oct 2, 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 d12afa1 Oct 08 2025, 01:43 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.

@MaheshtheDev MaheshtheDev changed the title feat: ai sdk language model wrapper feat: ai sdk language model withSupermemory Oct 2, 2025
Copy link

claude bot commented Oct 2, 2025

Pull Request Review: AI SDK Language Model Wrapper

Summary

This PR introduces a middleware wrapper for Vercel AI SDK v5 that integrates Supermemory's semantic search capabilities into language model calls. The implementation allows automatic injection of relevant memories into prompts based on user messages.

Code Quality ✅

Strengths

  1. Clean abstraction: The middleware pattern is well-implemented using AI SDK's LanguageModelV2Middleware interface
  2. Type safety: Proper TypeScript types from @ai-sdk/provider are used throughout
  3. Good separation of concerns: Helper functions (getLastUserMessage, addSystemPrompt) are well-separated
  4. Comprehensive test coverage: 293 lines of tests covering multiple scenarios including edge cases

Areas for Improvement

🐛 Critical Bug in getLastUserMessage (middleware.ts:12)

const lastUserMessage = params.prompt
    .reverse()
    .find((prompt: LanguageModelV2Message) => prompt.role === "user")

Issue: .reverse() mutates the original array! This will:

  • Change the order of messages in the prompt
  • Break multi-turn conversations
  • Cause unexpected behavior in the calling code

Fix: Use a non-mutating approach:

const lastUserMessage = [...params.prompt]
    .reverse()
    .find((prompt: LanguageModelV2Message) => prompt.role === "user")

Or better yet:

const lastUserMessage = params.prompt.findLast((prompt: LanguageModelV2Message) => prompt.role === "user")

🔴 Type Safety Issues (middleware.ts:14-17)

const memories = lastUserMessage?.content
    .filter((content) => content.type === "text")
    .map((content) => content.text)
    .join(" ")

Issues:

  1. content.text assumes type narrowing but TypeScript doesn't guarantee it after filter
  2. Missing null check on lastUserMessage.content
  3. Variable name memories is misleading - it's actually the user's message text

Fix:

const userMessageText = lastUserMessage?.content
    .filter((content): content is { type: 'text'; text: string } => content.type === "text")
    .map((content) => content.text)
    .join(" ") ?? ""

⚠️ Logic Issue in addSystemPrompt (middleware.ts:37-45)

When a system prompt exists, the code modifies it by appending memories. However:

  1. This modifies ALL system prompts, which may not be desired
  2. The concatenation format ${prompt.content} \n ${memories} has inconsistent spacing

Suggested improvement:

if (systemPromptExists) {
    // Find the first system prompt and append memories to it
    let memoriesAdded = false
    return {
        ...params,
        prompt: params.prompt.map((prompt) => {
            if (prompt.role === "system" && !memoriesAdded) {
                memoriesAdded = true
                return { 
                    ...prompt, 
                    content: `${prompt.content}\n\n${memories}` 
                }
            }
            return prompt
        }),
    }
}

🔒 Security Concerns

  1. Environment variable handling (middleware.ts:82-86):

    const SUPERMEMORY_API_KEY = process.env.SUPERMEMORY_API_KEY
    if (!SUPERMEMORY_API_KEY) {
        throw new Error("SUPERMEMORY_API_KEY is not set")
    }
    • ✅ Good: Validates API key presence
    • ⚠️ Consider: Add validation for key format/length to fail fast
  2. No input sanitization: The user message is sent directly to Supermemory API without validation

  3. No error handling: If supermemory.search.execute() fails, the entire LLM call fails. Consider:

    try {
        const memoriesResponse = await supermemory.search.execute({...})
        // ... use memories
    } catch (error) {
        // Log error and continue without memories
        console.error('Failed to fetch memories:', error)
        return params // Return original params
    }

Performance Considerations

Latency Impact

  • Every LLM call now includes a Supermemory API call, adding latency
  • Recommendation:
    • Add timeout for the search API call
    • Consider caching recent searches
    • Add metrics/logging to track performance impact

💾 Memory Efficiency

  • The current implementation joins all chunks from all results with newlines (middleware.ts:34-36)
  • This could create very large system prompts
  • Recommendation:
    const memories = memoriesResponse.results
        .slice(0, 5) // Limit to top 5 results
        .map((result) => result.chunks.slice(0, 3).map((chunk) => chunk.content).join(" "))
        .join("\n")

Test Coverage ⚠️

Issues with Tests (test/vercel.test.ts)

  1. Hardcoded test data (lines 51, 80, 97, 134, etc.):

    { role: "system", content: "My Name is Mahesh" }
    • Tests are testing against hardcoded values that don't match the implementation
    • The implementation calls the Supermemory API, but tests assume static responses
    • This means tests will fail when run!
  2. Missing mocks:

    const middleware = createSupermemoryMiddleware

    Tests call createSupermemoryMiddleware without providing required parameters (supermemory instance, containerTag)

  3. No API mocking: The Supermemory SDK is never mocked, so tests would make real API calls

Recommended test structure:

const mockSupermemory = {
    search: {
        execute: vi.fn().mockResolvedValue({
            results: [{
                chunks: [{ content: "Test memory content" }]
            }]
        })
    }
}

const middleware = createSupermemoryMiddleware(mockSupermemory as any, "test-tag")

Additional Issues

📁 Test File Location (packages/ai-sdk-test.ts)

  • This appears to be a manual test file, not part of the automated test suite
  • Should be moved to packages/tools/test/ or deleted
  • Contains import error: imports from ./tools/src/vercel but this file is at wrong level

📦 Missing Package.json Update

  • The PR adds new files but doesn't update packages/tools/package.json to include new dependencies or exports
  • The packages/tools/src/vercel/index.ts export should be added to package.json exports field

📝 Missing Documentation

  • No README or docs explaining how to use this middleware
  • No JSDoc comments on public functions
  • No examples beyond the test file

Recommendations

High Priority (Must Fix)

  1. ✅ Fix the .reverse() mutation bug
  2. ✅ Fix type safety issues with content filtering
  3. ✅ Fix tests to properly mock Supermemory API
  4. ✅ Add error handling for API failures
  5. ✅ Remove or fix packages/ai-sdk-test.ts

Medium Priority (Should Fix)

  1. Add limits to prevent huge prompts
  2. Add timeout handling for API calls
  3. Only modify first system prompt, not all
  4. Add JSDoc documentation
  5. Update package.json exports

Low Priority (Nice to Have)

  1. Add caching layer for performance
  2. Add telemetry/logging
  3. Add configuration options (max results, timeout, etc.)
  4. Create usage examples in README

Security Assessment ✅

No major security concerns, but consider:

  • Input validation on containerTag parameter
  • Rate limiting awareness for Supermemory API
  • Sensitive data in memories being injected into prompts

Verdict

Request Changes ⚠️

This PR has a solid foundation and good test coverage, but contains critical bugs that would break functionality in production. The .reverse() mutation and broken tests need to be fixed before merging. Once these issues are addressed, this will be a valuable addition to the codebase.

Copy link

claude bot commented Oct 2, 2025

Pull Request Review: AI SDK Language Model withSupermemory

Overview

This PR introduces a middleware wrapper for Vercel AI SDK language models that integrates Supermemory for context-aware AI responses. The implementation allows models to automatically retrieve and inject relevant memories into system prompts based on user queries.

✅ Strengths

Code Quality

  • Well-structured architecture: Clean separation between middleware logic and the wrapper function
  • Comprehensive test coverage: 525 lines of tests covering edge cases, error handling, and integration scenarios
  • Type safety: Proper TypeScript types from @ai-sdk/provider
  • Clear naming conventions: Functions and variables follow clear, descriptive patterns

Implementation Highlights

  • Robust error handling for missing API keys (packages/tools/src/vercel/middleware.ts:84-86)
  • Thorough test suite including unit tests, integration tests, and edge case coverage
  • Good use of dependency injection pattern (Supermemory instance passed to middleware)

⚠️ Issues & Concerns

1. Critical Bug: Array Mutation

Location: packages/tools/src/vercel/middleware.ts:10-13

const lastUserMessage = params.prompt
    .reverse()
    .find((prompt: LanguageModelV2Message) => prompt.role === "user")

Issue: .reverse() mutates the original array. This will reverse the prompt order in the params object, which could break the conversation flow.

Fix: Use a non-mutating approach:

const lastUserMessage = [...params.prompt]
    .reverse()
    .find((prompt: LanguageModelV2Message) => prompt.role === "user")

2. Performance Concern: Unnecessary API Calls

Location: packages/tools/src/vercel/middleware.ts:30-32

Issue: The middleware calls the Supermemory API on every request, even when memories might be empty or irrelevant. For high-frequency applications, this could:

  • Increase latency significantly
  • Generate unnecessary API costs
  • Potentially hit rate limits

Recommendation: Consider:

  • Caching recent search results
  • Adding a configuration option to skip search for certain message types
  • Implementing a threshold for when to fetch memories

3. String Concatenation Without Delimiter

Location: packages/tools/src/vercel/middleware.ts:14-17

const memories = lastUserMessage?.content
    .filter((content) => content.type === "text")
    .map((content) => content.text)
    .join(" ")  // Only space delimiter

Issue: Multiple text contents are joined with just a space. This could create unclear queries if content parts are sentences or paragraphs.

Recommendation: Use " " or consider trimming individual parts first.

4. Memory Injection Strategy

Location: packages/tools/src/vercel/middleware.ts:37-54

Issue: When a system prompt exists, memories are appended with \n. When creating a new system prompt, memories become the entire prompt. This inconsistency could lead to:

  • Poor prompt structure for models sensitive to formatting
  • Lack of context about what the injected content represents

Recommendation:

const memoryPrompt = memories ? `\n\nRelevant context from memory:\n${memories}` : ""

This makes it clear to the model that the content is contextual memory, not direct instructions.

5. Missing Error Handling

Location: packages/tools/src/vercel/middleware.ts:68-74

Issue: If the Supermemory API call fails, the error propagates uncaught (though there's a test for it). For production use, this could break the entire model call.

Recommendation: Add try-catch with graceful degradation:

try {
    const transformedParams = await addSystemPrompt(...)
    return transformedParams
} catch (error) {
    console.error("Failed to fetch memories:", error)
    return params // Fall back to original params
}

6. Lock File in PR

Location: Root bun.lock with 5,939 additions

Issue: The PR adds a new lockfile which wasn't previously tracked. This suggests:

  • Either the repo wasn't tracking lockfiles before (bad practice)
  • Or this is accidentally committed

Recommendation: Verify with team whether lockfiles should be tracked. If not, remove it and add to .gitignore.

7. Test File in Production Code

Location: packages/ai-sdk-test.ts

Issue: This file contains test code but is placed at the package root, not in a test/ or __tests__/ directory. It also uses a non-existent model "gpt-5" (line 5).

Recommendation:

  • Move to test/ directory or remove if it's just for local testing
  • Use an actual model ID like "gpt-4" or make it a proper example

8. Package Version Upgrade

Location: package.json:24

Change: @ai-sdk/openai upgraded from ^1.3.23 to ^2.0.42 (major version)

Issue: Major version upgrades can have breaking changes. No migration notes or changelog review is mentioned.

Recommendation: Document any breaking changes from v1 to v2 or confirm backward compatibility.

🔒 Security Considerations

✅ Good Practices:

  • API key loaded from environment variable
  • No hardcoded credentials
  • Proper validation of environment variables

⚠️ Potential Issues:

  • Container tag injection: The containerTag parameter is passed directly from user code without validation. Malicious values could potentially be used for injection attacks depending on Supermemory's API implementation
  • Memory content trust: Retrieved memories are injected directly into prompts without sanitization. If stored memories contain malicious content, it could lead to prompt injection attacks

Recommendation: Add validation for containerTag and consider sanitizing retrieved memory content.

🎯 Test Coverage Analysis

Excellent Coverage:

  • ✅ Environment validation
  • ✅ Message extraction logic
  • ✅ Multiple message handling
  • ✅ System prompt creation and appending
  • ✅ Empty content edge cases
  • ✅ Non-text content filtering
  • ✅ Multiple memory chunks
  • ✅ Error propagation
  • ✅ Custom container tags

Missing Tests:

  • Actual model invocation: No tests verify that doGenerate or doStream are called correctly
  • Large memory results: No test for performance with large memory sets
  • Concurrent requests: No test for race conditions or concurrent middleware calls
  • Memory content sanitization: No test for handling malicious content in memories

📊 Performance Considerations

  • Network latency: Every model call now includes an additional HTTP request to Supermemory
  • Memory overhead: Storing full prompt arrays in memory
  • Array operations: The reverse/find/filter/map chain could be optimized

🎨 Style & Conventions

✅ Follows CLAUDE.md Guidelines:

  • Uses Biome for linting (aligns with format-lint command)
  • TypeScript throughout
  • Proper test structure with Vitest

Minor Issues:

  • Biome ignore comments are used correctly but could be avoided with better typing
  • Some lines exceed typical line length preferences

📝 Documentation Needs

Missing:

  • README or documentation for the new withSupermemory wrapper
  • JSDoc comments for public API functions
  • Usage examples (beyond the test file)
  • Configuration options documentation

Recommendation: Add a README in packages/tools/src/vercel/ explaining:

  • How to use withSupermemory
  • What container tags are
  • Performance implications
  • Best practices

🚀 Recommendations Summary

Must Fix (Blocking):

  1. ✅ Fix array mutation bug in getLastUserMessage

Should Fix (High Priority):

  1. Add error handling with graceful fallback
  2. Improve memory injection prompt structure
  3. Move or remove ai-sdk-test.ts from production code
  4. Verify @ai-sdk/openai v2 compatibility

Nice to Have (Medium Priority):

  1. Add caching for memory searches
  2. Add JSDoc documentation
  3. Add validation for containerTag
  4. Consider sanitizing retrieved memory content
  5. Add README/usage documentation

For Discussion:

  1. Should bun.lock be tracked in git?
  2. Performance strategy for high-frequency use cases

Overall Assessment

This is a well-implemented feature with strong test coverage and clean architecture. The core functionality is solid, but there are some important bugs and missing production-ready features that should be addressed before merging.

Recommendation: Request changes for critical bug fix, then approve.


Review generated using repository standards from CLAUDE.md

@MaheshtheDev MaheshtheDev force-pushed the 10-02-feat_ai_sdk_language_model_wrapper branch from eace0d4 to 3760a51 Compare October 8, 2025 01:24
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