Skip to content

Conversation

MaheshtheDev
Copy link
Member

@MaheshtheDev MaheshtheDev commented Oct 3, 2025

Added AI SDK integration tests for Supermemory tools with environment configuration support.

What changed?

  • Added .env.example file with configuration for Supermemory and OpenAI API keys
  • Added @supermemory/tools dependency to package.json
  • Created new test scripts for different environment configurations
  • Implemented comprehensive AI SDK integration tests in ai-sdk.test.ts
  • Added Vitest configuration file with proper timeout settings

Copy link

cloudflare-workers-and-pages bot commented Oct 3, 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 successful!
View logs
supermemory-app 573c00f Oct 04 2025, 01:24 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 marked this pull request as ready for review October 3, 2025 21:37
@graphite-app graphite-app bot requested a review from Dhravya October 3, 2025 21:37
Copy link

claude bot commented Oct 3, 2025

PR Review: AI SDK Integration Tests

🟢 Strengths

  1. Good Test Coverage: Tests cover both generateText and streamText functionality
  2. Environment Configuration: Adding .env.example is helpful for developers
  3. Multiple Test Scripts: Provides flexibility with different environment files
  4. Proper Async Handling: Tests correctly use async/await patterns

🔴 Critical Issues

1. Circular Dependency (packages/tools/package.json:19)

"@supermemory/tools": "^1.1.12"

❌ The package depends on itself. This will cause installation failures and build issues.

Fix: Remove this dependency entirely.

2. Trailing Comma Syntax Error (packages/tools/package.json:14)

"test:local": "vitest --env-file .env.local",

❌ Trailing comma after last object property will cause JSON parsing errors.

Fix: Remove the comma after .env.local",

3. Invalid .env Format (packages/tools/.env.example:1-9)

SUPERMEMORY_API_KEY="your_supermemory_api_key_here",
TEST_TIMEOUT=30000,
// Test timeouts (in milliseconds)

❌ Issues:

  • Trailing commas are not valid in .env files
  • Comments should use # not //
  • No quotes needed for numeric values

Fix:

# Supermemory Configuration
SUPERMEMORY_API_KEY=your_supermemory_api_key_here
SUPERMEMORY_PROJECT_ID=sm_project_default

# OpenAI Configuration
OPENAI_API_KEY=your_openai_api_key_here

# Test Configuration
TEST_MODEL=gpt-4o-mini

# Test timeouts (in milliseconds)
TEST_TIMEOUT=30000

🟡 Code Quality Issues

4. Console.log in Production Code (test/ai-sdk.test.ts:16)

console.log("SUPERMEMORY_API_KEY", SUPERMEMORY_API_KEY)

⚠️ Logging sensitive API keys is a security risk. This could expose credentials in CI logs.

Fix: Remove this line entirely.

5. Fragile Test Assertions (test/ai-sdk.test.ts:42-46)

expect(
  result.toolResults.some((tool) =>
    JSON.stringify(tool.output).includes("pineapple"),
  ),
).toBe(true)

⚠️ This test assumes specific data ("pineapple") exists in the test environment. This is not deterministic and will fail in clean environments.

Fix: Either set up test data in a beforeAll hook, or make assertions more generic.

6. Incomplete Stream Test (test/ai-sdk.test.ts:71)

The stream test should verify text was actually streamed and tools were invoked.

7. Test Interdependency (test/ai-sdk.test.ts:78-110 & 112-132)

⚠️ Test "should search and retrieve memories" depends on "should handle memory operations correctly" having run first. Tests should be independent.


🟣 Performance & Best Practices

8. Hardcoded Timeout in Test Config (vitest.config.ts:7)

The package.json removed explicit timeouts but vitest.config.ts still has a 100s default. Individual tests use 30s. This inconsistency is confusing.

9. Redundant Type Assertion (test/ai-sdk.test.ts:29)

Already validated in beforeAll, type assertion is redundant.

10. Missing Test Cleanup

⚠️ Tests add memories but do not clean them up. This could cause test pollution, quota issues, and cost accumulation.

Recommendation: Add afterAll or afterEach hooks to clean up test data.


🎯 Summary

Must Fix Before Merge:

  1. ❌ Remove circular dependency (Issue Gemini api #1)
  2. ❌ Fix JSON syntax error (Issue [ImgBot] Optimize images #2)
  3. ❌ Fix .env file format (Issue (new ui) window is not defined in useViewport #3)
  4. ❌ Remove API key logging (Issue New UI #4 - Security)

Should Fix:
5. ⚠️ Improve test reliability (Issues #5, #6, #7)
6. ⚠️ Add test cleanup (Issue #10)

Nice to Have:
7. 📝 Documentation improvements
8. 📝 Config alignment (Issue #8)


Overall, this is a solid addition of integration tests, but the critical issues (especially the circular dependency and syntax errors) will prevent the code from working. Once those are fixed, this will be a valuable addition to the test suite.

🤖 Generated with Claude Code

Copy link

claude bot commented Oct 4, 2025

PR Review: AI SDK Integration Tests

Overview

This PR adds comprehensive integration tests for the AI SDK tools with good environment configuration support. The tests validate both generateText and streamText functionality with Supermemory tools.

✅ Strengths

  1. Good test coverage - Tests cover the main use cases: generateText, streamText, memory operations, and search/retrieval
  2. Environment configuration - Added flexible test scripts for different environments (.env, .env.local)
  3. Proper documentation - .env.example provides clear guidance for required configuration
  4. Timeout handling - Appropriate 30s timeouts for integration tests that make external API calls

🐛 Issues & Concerns

Critical Issues

  1. .env.example has trailing comma (line 9)

    - TEST_TIMEOUT=30000,
    + TEST_TIMEOUT=30000

    This will cause parsing errors.

  2. .env.example has extra space in OPENAI_API_KEY

    - OPENAI_API_KEY= "your_openai_api_key_here"
    + OPENAI_API_KEY="your_openai_api_key_here"
  3. Hard-coded test assertion (line 42-44 of test file)

    expect(
      result.toolResults.some((tool) =>
        JSON.stringify(tool.output).includes("pineapple"),
      ),
    ).toBe(true)

    This expects "pineapple" to exist in the results, which is too specific and will fail for different users/datasets. This makes the test brittle and non-portable.

Code Quality Issues

  1. Inconsistent timeout configuration

    • vitest.config.ts sets testTimeout: 100000 (100s)
    • Individual tests use 30000 (30s)
    • .env.example suggests TEST_TIMEOUT=30000 but it's never used

    Consider using the environment variable consistently:

    const TEST_TIMEOUT = Number(process.env.TEST_TIMEOUT) || 30000
    // Then use: }, TEST_TIMEOUT)
  2. Unnecessary as string type assertion (lines 28, 59, 88, 121)

    supermemoryTools(SUPERMEMORY_API_KEY as string, ...)

    The beforeAll check ensures these are defined. Use non-null assertion instead:

    supermemoryTools(SUPERMEMORY_API_KEY!, ...)

    Or better yet, extract into a helper that returns properly typed values after validation.

  3. Unused variable finalResult (line 70)

    const finalResult = streamResult
    expect(finalResult.text).toBeDefined()

    Just use streamResult directly or remove this variable.

  4. Weak assertion in line 74

    expect(finalResult.toolResults).toBeDefined()

    According to the existing test patterns in tools.test.ts, you should await the full result:

    const finalResult = await streamResult
    expect(finalResult.toolResults).toBeDefined()
  5. Missing dependencies in package.json
    The test imports dotenv at line 2 of vitest.config.ts but this dependency already exists in devDependencies, so this is fine.

Testing Best Practices

  1. Tests depend on external state - All tests assume specific memories exist ("pineapple", programming language preferences). Consider:

    • Adding setup/teardown to create test data
    • Using more generic assertions
    • Or documenting required test data setup in the README
  2. Incomplete assertions in later tests - Tests 3 and 4 (lines 77-109, 111-131) only check that results exist, not their correctness. The existing tools.test.ts shows better patterns with type checking and success field validation.

  3. No error handling tests - Consider adding tests for:

    • Invalid API keys
    • Network failures
    • Malformed responses

Performance Considerations

  1. vitest.config.ts loads .env.local unconditionally (line 4)
    config({ path: ".env.local" })
    This runs at module load time, which may not exist for all developers. Consider using conditional loading or relying on Vitest's --env-file flag instead.

🔒 Security

  • ✅ Properly uses environment variables for sensitive data
  • ✅ Includes .env.example without actual credentials
  • ⚠️ Ensure .env and .env.local are in .gitignore (should verify this)

📝 Recommendations

  1. Fix the critical .env.example syntax errors
  2. Remove or make the "pineapple" assertion more generic
  3. Align timeout configurations across files
  4. Improve assertions to follow patterns from tools.test.ts
  5. Consider adding test data setup/teardown
  6. Add negative test cases (error scenarios)
  7. Document test data requirements if tests depend on specific memories existing

Convention Compliance

✅ Follows Biome formatting (tabs, double quotes, semicolons as needed)
✅ Uses TypeScript properly
✅ Follows existing test structure patterns
⚠️ Could better align with the thorough assertion patterns in tools.test.ts

Copy link

claude bot commented Oct 6, 2025

Pull Request Review: AI SDK Integration Tests

Overall Assessment

Good work adding comprehensive integration tests for the AI SDK! The test suite covers the main functionality well. Below are some suggestions for improvement across code quality, bugs, performance, security, and test coverage.


🟡 Code Quality & Best Practices

1. Environment Configuration Issues

File: .env.example:9

- TEST_TIMEOUT=30000,
+ TEST_TIMEOUT=30000
  • Trailing comma at the end of the file is invalid for .env files

2. Hard-coded Test Assertions

File: test/ai-sdk.test.ts:42-44

expect(
  result.toolResults.some((tool) =>
    JSON.stringify(tool.output).includes("pineapple"),
  ),
).toBe(true)
  • This test assumes specific data ("pineapple") exists in the test environment, which may not be portable
  • Recommendation: Either seed test data explicitly or make assertions more generic (e.g., check that tools were called, not specific content)

3. Package.json Script Cleanup

File: package.json:10-11

The PR removes the --testTimeout flag from base test scripts but vitest.config.ts sets testTimeout: 100000. This is the right approach, but:

  • Consider documenting why such a high timeout (100s) is needed
  • The new test:integration script doesn't inherit this timeout config unless vitest.config.ts is loaded

4. Inconsistent Timeout Values

  • vitest.config.ts: 100000ms (100s)
  • .env.example: 30000ms (30s)
  • Individual tests: 30000ms (30s)
  • Recommendation: Standardize or document the reasoning

🔴 Potential Bugs

1. Missing Await in Stream Test

File: test/ai-sdk.test.ts:69

const finalResult = streamResult
  • streamResult is a promise-like object but you're not awaiting its completion
  • Should be: const finalResult = await streamResult or access the result after stream completes
  • The subsequent assertions on finalResult.text and finalResult.toolResults may fail

2. Weak Assertions

File: test/ai-sdk.test.ts:70-73

expect(streamedText).toBeDefined()
expect(finalResult.text).toBeDefined()
expect(finalResult.toolResults).toBeDefined()
  • These assertions only check existence, not correctness
  • Won't catch if tools returned empty results or errors
  • Recommendation: Add assertions for streamedText.length > 0 and verify tool execution

3. Flaky Test Dependencies

File: test/ai-sdk.test.ts:109-130

Tests depend on previous tests having populated memories:

  • "What programming languages do I like?" assumes the previous test's memory was stored
  • Tests should be independent and use setup/teardown
  • Recommendation: Use beforeEach to seed required test data

⚡ Performance Considerations

1. Timeout Management

  • 30-100 second timeouts suggest API calls may be slow
  • Recommendations:
    • Consider using test mocking for faster unit tests
    • Reserve integration tests with real API calls for CI/CD only
    • Add @slow or @integration tags to separate fast/slow tests

2. Sequential Test Execution

  • All tests hit the live API sequentially
  • Could benefit from test parallelization with isolated test data
  • Use unique containerTags per test to avoid conflicts

🔒 Security Concerns

1. API Key Exposure Risk

File: .env.example:1,4

SUPERMEMORY_API_KEY="your_supermemory_api_key_here"
OPENAI_API_KEY= "your_openai_api_key_here"
  • Good: Using .env.example instead of .env
  • Recommendations:
    • Add .env and .env.local to .gitignore if not already present
    • Document that these are required for tests in the README

2. Test Data Persistence

  • Tests create real memories in the Supermemory backend
  • No cleanup/teardown to remove test data
  • Recommendation: Add afterEach or afterAll hooks to clean up test memories using the API

🧪 Test Coverage

✅ Well Covered

  • Basic tool initialization ✓
  • generateText integration ✓
  • streamText integration ✓
  • Memory addition ✓
  • Memory search ✓

⚠️ Missing Coverage

  1. Error Handling

    • No tests for invalid API keys
    • No tests for network failures
    • No tests for malformed inputs
    • No tests for rate limiting
  2. Edge Cases

    • Empty search results
    • Very long memories
    • Special characters in memory content
    • Invalid containerTags
  3. Configuration Validation

    • Custom baseUrl is set but never verified in tests
    • No test for missing required environment variables during runtime
  4. Tool Result Validation

    • Tests don't verify the structure of toolResults
    • Don't check if search actually returns relevant results

📝 Recommendations Summary

High Priority

  1. Fix the streaming test bug (missing await)
  2. Remove trailing comma from .env.example
  3. Make test assertions more robust
  4. Add test isolation (independent test data)

Medium Priority

  1. Add error handling test cases
  2. Implement test data cleanup
  3. Document timeout requirements
  4. Make assertions check actual values, not just existence

Nice to Have

  1. Add test mocking for faster unit tests
  2. Separate integration tests from unit tests
  3. Add edge case coverage
  4. Verify custom configuration actually works

🎯 Suggested Test Addition

Here's an example of a more robust error handling test:

it("should handle API errors gracefully", async () => {
  const invalidApiKey = "invalid_key"
  
  const result = await generateText({
    model: openai(MODEL),
    messages: [{ role: "user", content: "Test" }],
    tools: {
      ...supermemoryTools(invalidApiKey, {
        containerTags: [PROJECT_ID],
      }),
    },
  })

  expect(result.toolResults.some(
    tool => tool.result?.success === false && tool.result?.error
  )).toBe(true)
})

✅ Conclusion

This PR adds valuable integration test coverage for the AI SDK tools. With the fixes and improvements suggested above, it will be production-ready. The test structure is good, but needs more robust assertions, better isolation, and error case coverage.

Overall: Approve with suggested changes 👍

Copy link

claude bot commented Oct 6, 2025

Pull Request Review: AI SDK Integration Tests

Thank you for adding comprehensive integration tests for the AI SDK! This is a valuable addition to the codebase. Here's my detailed review:

✅ Strengths

  1. Comprehensive Test Coverage: The tests cover both generateText and streamText operations, which are the primary use cases
  2. Good Environment Configuration: The .env.example file provides clear guidance for test setup
  3. Proper Error Handling: Tests validate tool execution and check for expected behavior
  4. Realistic Test Scenarios: Memory operations like remembering preferences are practical test cases

🐛 Issues Found

Critical Issues

  1. Trailing Comma in .env.example (line 9)

    - TEST_TIMEOUT=30000,
    + TEST_TIMEOUT=30000

    This trailing comma will cause parsing errors in many .env parsers.

  2. Hard-Coded Test Data Expectation

    expect(
      result.toolResults.some((tool) =>
        JSON.stringify(tool.output).includes("pineapple"),
      ),
    ).toBe(true)

    This test assumes specific data ("pineapple") exists in the Supermemory instance. This will fail for new/different test accounts and makes tests brittle. Consider:

    • Adding test data setup in beforeAll
    • Testing for the presence of tool calls rather than specific content
    • Using more generic assertions

Configuration Issues

  1. Inconsistent Timeout Configuration

    • vitest.config.ts sets testTimeout: 100000 (100s)
    • Individual tests use 30000 (30s)
    • .env.example suggests TEST_TIMEOUT=30000 but it's never used
    • package.json was changed from 100000 to no timeout, but vitest.config.ts still has it

    Recommendation: Use a single source of truth for timeouts. Either:

    • Read from environment variable: testTimeout: Number(process.env.TEST_TIMEOUT) || 100000
    • Or keep consistent values across the codebase
  2. Dotenv Loading Inconsistency

    • vitest.config.ts manually loads .env.local with dotenv
    • Tests use beforeAll() to validate env vars
    • Vitest has built-in env file support that you're already using in package.json scripts

    The current setup is confusing because:

    // vitest.config.ts - loads .env.local automatically
    config({ path: ".env.local" })
    
    // But package.json has scripts like:
    "test:env": "vitest --env-file .env"

🔧 Code Quality Issues

  1. Unused Environment Variable

    const MODEL = "gpt-4o-mini"  // Hard-coded

    But .env.example includes TEST_MODEL="gpt-4o-mini". Either use the env var or remove it from the example.

  2. Unnecessary Space in .env.example

    - OPENAI_API_KEY= "your_openai_api_key_here"
    + OPENAI_API_KEY="your_openai_api_key_here"
  3. Missing Type Safety
    The test casts SUPERMEMORY_API_KEY unsafely:

    ...supermemoryTools(SUPERMEMORY_API_KEY as string, ...)

    This is unnecessary since you already validate it's not undefined in beforeAll().

  4. Duplicate Assertions
    Multiple tests repeat identical assertion patterns. Consider extracting helper functions:

    const expectValidResult = (result: any) => {
      expect(result).toBeDefined()
      expect(result.content).toBeDefined()
      expect(result.toolResults).toBeDefined()
    }

📊 Test Coverage Gaps

  1. Error Path Testing: No tests for failure scenarios:

    • Invalid API keys
    • Network failures
    • Malformed responses
  2. Edge Cases: Missing tests for:

  • Empty search results
  • Large result sets
  • Special characters in memory content
  • Memory content that exceeds limits

🏗️ Architecture Concerns

  1. Test Isolation: Tests appear to share state (memories added in one test affect others). Consider:
  • Using unique PROJECT_ID per test
  • Cleaning up test data in afterEach()
  • Mocking the Supermemory client for unit tests
  1. Integration vs Unit Tests: These are integration tests requiring live API keys. Consider:
  • Adding unit tests with mocked clients
  • Using CI/CD secrets for integration tests
  • Documenting that these tests require real credentials

🔒 Security Considerations

  1. API Key Exposure Risk: The .env.example has clear placeholder values ✅
  2. No .gitignore check: Ensure .env and .env.local are in .gitignore to prevent accidental commits

🎯 Recommendations

High Priority:

  1. Fix the trailing comma in .env.example
  2. Remove hard-coded "pineapple" assertion
  3. Unify timeout configuration
  4. Use or remove TEST_MODEL environment variable

Medium Priority:
5. Add error scenario tests
6. Improve test isolation with cleanup hooks
7. Extract common assertion patterns
8. Add unit tests with mocked clients

Low Priority:
9. Add JSDoc comments explaining test scenarios
10. Consider adding performance benchmarks
11. Document test prerequisites in README

📝 Suggested Changes

// ai-sdk.test.ts improvements
describe("Supermemory AI SDK Integration Tests", () => {
  const SUPERMEMORY_API_KEY = process.env.SUPERMEMORY_API_KEY
  const OPENAI_API_KEY = process.env.OPENAI_API_KEY
  const PROJECT_ID = process.env.SUPERMEMORY_PROJECT_ID || `test_${Date.now()}`
  const MODEL = process.env.TEST_MODEL || "gpt-4o-mini"
  const TEST_TIMEOUT = Number(process.env.TEST_TIMEOUT) || 30000

  beforeAll(() => {
    if (!SUPERMEMORY_API_KEY) {
      throw new Error("SUPERMEMORY_API_KEY environment variable is required")
    }
    if (!OPENAI_API_KEY) {
      throw new Error("OPENAI_API_KEY environment variable is required")
    }
  })

  it("should generate text with supermemory tools", async () => {
    const result = await generateText({
      model: openai(MODEL),
      messages: [
        { role: "user", content: "What do you remember about my preferences?" },
      ],
      tools: {
        ...supermemoryTools(SUPERMEMORY_API_KEY, {
          containerTags: [PROJECT_ID],
        }),
      },
    })

    expect(result).toBeDefined()
    expect(result.content).toBeDefined()
    expect(result.toolResults).toBeDefined()
    expect(Array.isArray(result.toolResults)).toBe(true)
    
    // Check that tools were actually invoked
    if (result.toolResults.length > 0) {
      expect(result.toolResults[0]).toHaveProperty('toolName')
    }
  }, TEST_TIMEOUT)
})

Overall Assessment

Score: 7/10

This PR adds valuable test coverage, but needs refinement around configuration consistency and test reliability. The tests are well-structured but have some brittleness issues that should be addressed before merging.

Great work on getting integration tests in place! Once the critical issues are addressed, this will be a solid addition to the codebase. 🚀

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