Skip to content

feat: new tools package#407

Merged
graphite-app[bot] merged 1 commit intomainfrom
shreyans/09-02-feat_new_tools_package
Sep 2, 2025
Merged

feat: new tools package#407
graphite-app[bot] merged 1 commit intomainfrom
shreyans/09-02-feat_new_tools_package

Conversation

@CodeWithShreyans
Copy link
Contributor

No description provided.

@CodeWithShreyans CodeWithShreyans marked this pull request as ready for review September 2, 2025 21:55
@graphite-app graphite-app bot requested a review from Dhravya September 2, 2025 21:55
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 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 Preview URL Updated (UTC)
✅ Deployment successful!
View logs
supermemory-app cae7051 Commit Preview URL

Branch Preview URL
Sep 02 2025, 11:22 PM

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

@graphite-app
Copy link

graphite-app bot commented Sep 2, 2025

Cartoon gif. With a goofy look on his face, Stimpy from The Ren and Stimpy Show pokes his head around a door and gives a thumbs up. (Added via Giphy)

@claude
Copy link

claude bot commented Sep 2, 2025

PR Review: New Tools Package

Summary

This PR refactors the AI SDK by removing the infinite chat functionality and creating a new focused @supermemory/tools package for memory management with both AI SDK and OpenAI integration.

🟢 Positive Changes

1. Better Package Architecture

  • Clear separation of concerns with dedicated tools package
  • Proper exports structure with multiple entry points (./ai-sdk, ./openai)
  • Clean TypeScript setup with proper type exports

2. Code Quality Improvements

  • Good use of shared constants and utilities in shared.ts
  • Proper error handling with consistent return types
  • Well-structured Zod schemas for validation
  • Type-safe implementations for both AI SDK and OpenAI integrations

3. Configuration Management

  • Flexible configuration supporting both projectId and containerTags
  • Proper default values and parameter descriptions
  • Clean helper functions for container tag generation

🟡 Areas for Improvement

1. Biome Configuration Changes
The semicolon style was changed from "always" to "asNeeded". Consider whether this aligns with the project's coding standards and ensure all existing code is updated accordingly.

2. Missing Validation
The configuration interface allows both projectId and containerTags but doesn't enforce mutual exclusivity. Consider adding runtime validation.

3. Error Handling Consistency
In openai.ts:202, JSON parsing could fail but isn't wrapped in try-catch.

🔴 Issues to Address

1. Package Version Inconsistency

  • package.json shows version 1.0.4
  • bun.lock references version 1.0.0

2. Incomplete Code Removal
The infinite chat functionality was removed from ai-sdk but the package version was bumped to 1.0.8. Ensure this doesn't break existing consumers.

3. Test Coverage
While tests exist, they appear to be primarily integration tests. Consider adding unit tests for configuration validation, error scenarios, and edge cases in tool execution.

🛡️ Security Considerations

1. API Key Handling

  • API keys are properly passed through configuration
  • No hardcoded secrets detected

2. Input Validation

  • Proper Zod schema validation for tool inputs
  • Type safety maintained throughout

📈 Performance Considerations

1. Client Initialization
Consider caching Supermemory client instances if tools are called frequently to avoid repeated initialization overhead.

2. Default Values
Good use of constants for defaults (DEFAULT_VALUES.chunkThreshold: 0.6)

🧪 Testing Notes

  • Tests require environment variables which is appropriate for integration testing
  • Consider adding CI environment variable documentation
  • Test timeout of 100s seems high - ensure tests are optimized

Recommendations

  1. Fix version inconsistencies in package files
  2. Add runtime validation for mutually exclusive config options
  3. Wrap JSON.parse in try-catch for robust error handling
  4. Document breaking changes from infinite chat removal
  5. Consider adding unit tests alongside integration tests
  6. Update project documentation to reflect new package structure

Conclusion

This is a well-architected refactor that improves code organization and provides flexible tooling for memory management. The main concerns are around version consistency and ensuring breaking changes are properly communicated. The code quality is good with proper TypeScript usage and error handling patterns.

Recommendation: Approve with requested changes

@graphite-app
Copy link

graphite-app bot commented Sep 2, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the shreyans/09-02-feat_new_tools_package branch from d95cd11 to cae7051 Compare September 2, 2025 23:11
@graphite-app graphite-app bot merged commit cae7051 into main Sep 2, 2025
2 of 3 checks passed
@claude
Copy link

claude bot commented Sep 2, 2025

Code Review: New Tools Package

Summary

This PR introduces a new unified @supermemory/tools package that consolidates memory functionality for both AI SDK and OpenAI function calling, replacing the previous separate packages. Overall, this is a well-structured refactoring that improves maintainability and user experience.

✅ Positive Aspects

Code Quality & Architecture

  • Excellent separation of concerns: Clear division between AI SDK (ai-sdk.ts) and OpenAI (openai.ts) implementations
  • Shared utilities: Smart use of shared.ts for common constants and helper functions
  • Consistent error handling: Proper try-catch blocks with structured error responses
  • Type safety: Good use of TypeScript interfaces and generic types
  • Clean API design: Intuitive function names and parameter structures

Documentation & DX

  • Comprehensive README: Clear examples for both AI SDK and OpenAI usage patterns
  • Good package.json structure: Proper exports, keywords, and dependencies
  • Version increment: Appropriate version bump from 1.0.7 to 1.0.8

🔍 Issues & Concerns

1. Configuration Validation

Location: packages/tools/src/types.ts:3-9

export interface SupermemoryToolsConfig {
    containerTags?: string[]
    projectId?: string  // Mutually exclusive but not enforced
}

Issue: The interface allows both projectId and containerTags but the comment states they're mutually exclusive. This should be enforced at the type level.

Suggested Fix:

type SupermemoryToolsConfig = 
  | { baseUrl?: string; projectId: string; containerTags?: never }
  | { baseUrl?: string; containerTags: string[]; projectId?: never }
  | { baseUrl?: string; projectId?: never; containerTags?: never }

2. Potential Memory Leak

Location: packages/tools/src/ai-sdk.ts:17-22 & packages/tools/src/openai.ts:75-83
Issue: New Supermemory client instances are created for each tool function call instead of being reused.

Performance Impact: Could lead to unnecessary connection overhead and memory usage.

3. Error Handling Inconsistency

Location: packages/tools/src/openai.ts:202-214

const args = JSON.parse(toolCall.function.arguments) // No try-catch

Issue: JSON parsing can throw but isn't wrapped in try-catch, unlike other operations.

4. Missing Input Validation

Location: Multiple locations
Issue: No validation for required parameters like empty strings or invalid limits.

Example:

// Should validate informationToGet is not empty
if (!informationToGet?.trim()) {
    return { success: false, error: "Search query cannot be empty" }
}

🚀 Performance Considerations

Client Instance Management

Consider implementing a client factory or singleton pattern to reuse connections:

const clientCache = new Map<string, Supermemory>()

function getClient(apiKey: string, config?: SupermemoryToolsConfig) {
    const cacheKey = `${apiKey}-${JSON.stringify(config)}`
    if (!clientCache.has(cacheKey)) {
        clientCache.set(cacheKey, new Supermemory({
            apiKey,
            ...(config?.baseUrl && { baseURL: config.baseUrl })
        }))
    }
    return clientCache.get(cacheKey)!
}

🔒 Security Review

API Key Handling

  • ✅ API keys are properly passed through without logging
  • ✅ No hardcoded credentials found
  • ✅ Proper use of environment variables in tests

Input Sanitization

  • ⚠️ Consider sanitizing memory content before storage
  • ⚠️ Validate containerTags format to prevent injection

🧪 Test Coverage

Strengths

  • ✅ Comprehensive test structure with both AI SDK and OpenAI modules
  • ✅ Good error handling test scenarios
  • ✅ Environment variable validation
  • ✅ Integration tests with actual API calls

Gaps

  • ❌ No unit tests for error cases (invalid JSON, network failures)
  • ❌ Missing tests for edge cases (empty queries, large limits)
  • ❌ No performance/load testing

📋 Recommendations

High Priority

  1. Fix mutual exclusivity validation in SupermemoryToolsConfig
  2. Add JSON.parse error handling in tool call executor
  3. Implement client caching to improve performance

Medium Priority

  1. Add input validation for all user inputs
  2. Extend test coverage for error scenarios
  3. Add rate limiting considerations in documentation

Low Priority

  1. Consider adding retry logic for failed API calls
  2. Add metric/monitoring hooks for production usage

Biome Configuration Changes

The Biome config updates look good - trailing slashes for consistency and switching to "asNeeded" semicolons aligns with modern practices.

Overall Assessment

Score: 8/10 - This is a solid refactoring that significantly improves the package structure and developer experience. The main concerns are around type safety and performance optimization, but the core functionality is well-implemented.

The consolidation into a single package with clear submodule exports is excellent architectural decision that will reduce maintenance overhead.

@MaheshtheDev MaheshtheDev deleted the shreyans/09-02-feat_new_tools_package branch October 29, 2025 01:50
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.

2 participants