Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • add composite index on custom tool names & workspace id

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Nov 28, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Nov 28, 2025 11:24pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 28, 2025

Greptile Overview

Greptile Summary

This PR adds a composite unique index on workspace_id and title for custom tools, allowing different workspaces to have tools with the same title. The migration converts existing function-name-based IDs to UUIDs and simplifies the upsert logic.

Critical Issue Found:

  • The migration changes existing tool IDs from function names (e.g., get_weather) to UUIDs, but the application code still tries to update tools using the old function-name IDs
  • When an update attempt fails to find a tool by its old ID, the code falls through to the duplicate title check and throws an error, blocking legitimate updates to existing tools
  • This will break tool updates for all workspace-scoped tools that existed before the migration

Major Changes:

  • Removed function name validation and enforcement (tools no longer use function name as ID)
  • Changed from checking duplicate function names to checking duplicate titles per workspace
  • New tools now use nanoid() instead of function name as the primary key
  • Removed extensive comments explaining the logic

Recommendation: The ID migration strategy needs revision to handle the transition period where old IDs may still be in use, or ensure all references to old IDs are updated before deploying this change.

Confidence Score: 1/5

  • This PR contains a critical bug that will break updates to existing custom tools after migration
  • The migration changes tool IDs from function names to UUIDs, but the application code continues to use old function-name IDs when attempting updates. This causes legitimate updates to fail with "duplicate title" errors. The issue affects all workspace-scoped custom tools created before this migration runs. While the goal (allowing same title across workspaces) is valid, the implementation has a critical flaw in the migration strategy.
  • Pay close attention to packages/db/migrations/0113_calm_tiger_shark.sql and apps/sim/lib/custom-tools/operations.ts - these contain the critical bug that breaks tool updates

Important Files Changed

File Analysis

Filename Score Overview
packages/db/migrations/0113_calm_tiger_shark.sql 1/5 Migration converts function-name IDs to UUIDs, breaking existing tool updates due to ID mismatch
apps/sim/lib/custom-tools/operations.ts 2/5 Simplified upsert logic but duplicate title check now blocks updates of migrated tools
apps/sim/lib/workflows/custom-tools-persistence.ts 4/5 Passes toolId instead of function name, minor cleanup, mostly safe changes

Sequence Diagram

sequenceDiagram
    participant WF as Workflow State
    participant CPT as custom-tools-persistence.ts
    participant OPS as operations.ts
    participant DB as Database (custom_tools)
    
    Note over DB: Migration 0113 runs:<br/>function-name IDs → UUIDs
    
    WF->>CPT: Save workflow with custom tools
    CPT->>CPT: extractCustomToolsFromWorkflowState()
    CPT->>CPT: Filter tools with function.name
    
    CPT->>OPS: upsertCustomTools({tools, workspaceId})
    Note over OPS: tool.id = toolId (may be old function name)
    
    alt tool.id exists
        OPS->>DB: SELECT by id + workspaceId
        alt Found (UUID matches)
            OPS->>DB: UPDATE tool
            DB-->>OPS: Success
        else Not found (old function name)
            OPS->>DB: SELECT legacy tool (no workspaceId)
            alt Legacy found
                OPS->>DB: UPDATE legacy tool
                DB-->>OPS: Success
            else Not found anywhere
                OPS->>DB: SELECT by workspaceId + title
                alt Duplicate title exists
                    OPS-->>CPT: Error: duplicate title
                    Note over OPS,CPT: ⚠️ BREAKS HERE<br/>Can't update migrated tool!
                else No duplicate
                    OPS->>DB: INSERT new tool (nanoid)
                    DB-->>OPS: Success
                end
            end
        end
    else No tool.id
        OPS->>DB: SELECT by workspaceId + title
        alt Duplicate exists
            OPS-->>CPT: Error: duplicate title
        else No duplicate
            OPS->>DB: INSERT new tool (nanoid)
            DB-->>OPS: Success
        end
    end
    
    OPS->>DB: SELECT all workspace tools
    DB-->>OPS: Return tools
    OPS-->>CPT: Return result
    CPT-->>WF: Persistence complete
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@waleedlatif1 waleedlatif1 merged commit 21a640a into staging Nov 28, 2025
14 of 15 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/custom-tools branch November 28, 2025 23:33
waleedlatif1 added a commit that referenced this pull request Nov 29, 2025
…pylon, intercom, mailchimp, loading optimizations (#2132)

* fix(memory-util): fixed unbounded array of gmail/outlook pollers causing high memory util, added missing db indexes/removed unused ones, auto-disable schedules/webhooks after 10 consecutive failures (#2115)

* fix(memory-util): fixed unbounded array of gmail/outlook pollers causing high memory util, added missing db indexes/removed unused ones, auto-disable schedules/webhooks after 10 consecutive failures

* ack PR comments

* ack

* improvement(teams-plan): seats increase simplification + not triggering checkout session (#2117)

* improvement(teams-plan): seats increase simplification + not triggering checkout session

* cleanup via helper

* feat(tools): added sentry, incidentio, and posthog tools (#2116)

* feat(tools): added sentry, incidentio, and posthog tools

* update docs

* fixed docs to use native fumadocs for llms.txt and copy markdown, fixed tool issues

* cleanup

* enhance error extractor, fixed posthog tools

* docs enhancements, cleanup

* added more incident io ops, remove zustand/shallow in favor of zustand/react/shallow

* fix type errors

* remove unnecessary comments

* added vllm to docs

* feat(i18n): update translations (#2120)

* feat(i18n): update translations

* fix build

---------

Co-authored-by: waleedlatif1 <waleedlatif1@users.noreply.github.com>

* improvement(workflow-execution): perf improvements to passing workflow state + decrypted env vars (#2119)

* improvement(execution): load workflow state once instead of 2-3 times

* decrypt only in get helper

* remove comments

* remove comments

* feat(models): host google gemini models (#2122)

* feat(models): host google gemini models

* remove unused primary key

* feat(i18n): update translations (#2123)

Co-authored-by: waleedlatif1 <waleedlatif1@users.noreply.github.com>

* feat(tools): added zendesk, pylon, intercom, & mailchimp (#2126)

* feat(tools): added zendesk, pylon, intercom, & mailchimp

* finish zendesk and pylon

* updated docs

* feat(i18n): update translations (#2129)

* feat(i18n): update translations

* fixed build

---------

Co-authored-by: waleedlatif1 <waleedlatif1@users.noreply.github.com>

* fix(permissions): add client-side permissions validation to prevent unauthorized actions, upgraded custom tool modal (#2130)

* fix(permissions): add client-side permissions validation to prevent unauthorized actions, upgraded custom tool modal

* fix failing test

* fix test

* cleanup

* fix(custom-tools): add composite index on custom tool names & workspace id (#2131)

---------

Co-authored-by: Vikhyath Mondreti <vikhyathvikku@gmail.com>
Co-authored-by: waleedlatif1 <waleedlatif1@users.noreply.github.com>
DarkShark-RAz pushed a commit to DarkShark-RAz/sim that referenced this pull request Nov 30, 2025
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