Skip to content

fix(mothership): fix tool hidden in ui#4563

Closed
Sg312 wants to merge 0 commit into
stagingfrom
dev
Closed

fix(mothership): fix tool hidden in ui#4563
Sg312 wants to merge 0 commit into
stagingfrom
dev

Conversation

@Sg312
Copy link
Copy Markdown
Collaborator

@Sg312 Sg312 commented May 12, 2026

Summary

Fix tool hidden in ui

Type of Change

  • Bug fix

Testing

Manual

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
Copy Markdown

vercel Bot commented May 12, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 12, 2026 2:12am

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 12, 2026

Greptile Summary

This PR fixes load_custom_tool being visible in the chat UI by adding it to the hidden-tools set, and introduces a larger feature: per-workspace Mothership settings that let super admins configure which MCP tools, custom tools, and skills the Mothership agent can use, along with per-user Mothership environment routing (dev/staging/prod) for super admins.

  • Core fix: load_custom_tool added to HIDDEN_TOOL_NAMES in hidden-tools.ts and mirrored in message-content.tsx; the fix works but creates a duplicated set that should use the shared isToolHiddenInUi function instead.
  • New mothership_settings table and API: per-workspace CRUD with super-admin gating, DB migration included; isEffectiveSuperUser logic is duplicated between runtime.ts and the new route.ts.
  • Dynamic load_skill_${id} tools: used for mothership skill loading but their dynamic names won't be caught by the static hidden-tool sets, so they may surface in the UI the same way load_custom_tool did.

Confidence Score: 4/5

Safe to merge with the understanding that the hidden-tool duplication and dynamic skill tool visibility are follow-up items.

The core bug fix is correct and tested. The new Mothership settings infrastructure is well-structured with proper auth gating. The main concern is that message-content.tsx defines its own copy of HIDDEN_TOOL_NAMES instead of reusing the shared function, and the new load_skill_* tools use dynamic names that the static set will never catch, potentially reintroducing the same UI-visibility issue for skill tools.

apps/sim/app/workspace/[workspaceId]/home/components/message-content/message-content.tsx (duplicated hidden set) and apps/sim/lib/mothership/settings/runtime.ts (dynamic tool names not hidden)

Important Files Changed

Filename Overview
apps/sim/lib/copilot/tools/client/hidden-tools.ts Adds load_custom_tool to the hidden-tools set — the core bug fix of this PR
apps/sim/app/workspace/[workspaceId]/home/components/message-content/message-content.tsx Replaces single-tool name check with a local HIDDEN_TOOL_NAMES set, but duplicates the set from hidden-tools.ts rather than reusing isToolHiddenInUi — maintenance hazard
apps/sim/lib/mothership/settings/runtime.ts New module that builds per-workspace mothership tools for super admins; duplicates isEffectiveSuperUser and dynamic load_skill_* tools won't be hidden in the UI
apps/sim/lib/mothership/settings/operations.ts New CRUD layer for mothership_settings; deduplicates refs and validates them against the DB before persisting
apps/sim/app/api/mothership/settings/route.ts New settings API with super-admin gating; settings local variable shadows the DB table import and isEffectiveSuperUser is duplicated from runtime.ts
apps/sim/lib/copilot/server/agent-url.ts New module for per-user Mothership URL resolution; only effective super admins can route to non-default environments
packages/db/schema.ts Adds mothership_settings table and mothershipEnvironment column to settings; migration is present and consistent
apps/sim/app/workspace/[workspaceId]/settings/components/admin/admin.tsx Adds Mothership environment selector and multi-select tool picker to the super-admin settings UI; logic is straightforward with appropriate loading/disabled states
apps/sim/lib/copilot/tools/server/workflow/edit-workflow/lint.ts New workflow linting utility for detecting orphan blocks, empty ports, and invalid connections; well-tested
apps/sim/tools/index.ts Adds execution path for dynamic load_skill_${id} tool names used by mothership skill tools
apps/sim/lib/copilot/chat/payload.ts Replaces inline MCP tool discovery with mothership tool builder; gated behind includeMothershipTools flag

Sequence Diagram

sequenceDiagram
    participant Admin as Super Admin UI
    participant SettingsAPI as /api/mothership/settings
    participant Runtime as buildMothershipToolsForRequest
    participant AgentURL as getMothershipBaseURL
    participant Mothership as Mothership Agent

    Admin->>SettingsAPI: "PUT {workspaceId, mcpTools, customTools, skills}"
    SettingsAPI->>SettingsAPI: isEffectiveSuperUser check
    SettingsAPI->>SettingsAPI: filterMcpToolRefs / filterCustomToolRefs / filterSkillRefs
    SettingsAPI-->>Admin: saved MothershipSettings

    Admin->>AgentURL: select environment (dev/staging/prod)
    AgentURL->>AgentURL: isEffectiveSuperUser check
    AgentURL-->>Admin: environment URL stored in user settings

    Admin->>Mothership: chat request (via copilot/chat/post)
    Mothership->>Runtime: "buildMothershipToolsForRequest({workspaceId, userId})"
    Runtime->>Runtime: isEffectiveSuperUser check
    Runtime->>Runtime: load mcpTools / customTools / skills from DB
    Runtime-->>Mothership: "{tools: [...], catalogContext}"
    Mothership->>AgentURL: "getMothershipBaseURL({userId})"
    AgentURL-->>Mothership: routed environment URL
    Mothership-->>Admin: "stream with hidden load_custom_tool (fixed) + load_skill_* (still visible)"
Loading

Comments Outside Diff (4)

  1. apps/sim/app/workspace/[workspaceId]/home/components/message-content/message-content.tsx, line 18 (link)

    P1 Duplicated hidden-tool set diverges from hidden-tools.ts

    A local HIDDEN_TOOL_NAMES set is defined here instead of reusing isToolHiddenInUi from lib/copilot/tools/client/hidden-tools.ts. Both sets are identical today, but any future change to the authoritative list in hidden-tools.ts must also be manually mirrored here or the tool will silently appear in the UI. The isToolHiddenInUi export is already designed for exactly this purpose and is importable in 'use client' components.

  2. apps/sim/lib/mothership/settings/runtime.ts, line 38-52 (link)

    P2 isEffectiveSuperUser is duplicated across runtime.ts and route.ts

    An identical isEffectiveSuperUser private function (same DB query, same logic) exists in both apps/sim/lib/mothership/settings/runtime.ts and apps/sim/app/api/mothership/settings/route.ts. If the definition of "effective super user" ever changes (e.g., a new flag is added to the check), both copies must be updated in sync. Extracting it to a shared utility in lib/mothership/ or lib/auth/ would prevent silent drift.

  3. apps/sim/app/api/mothership/settings/route.ts, line 56-57 (link)

    P2 settings local variable shadows the module-level settings DB table import

    import { db, settings, user } from '@sim/db' brings in the Drizzle settings table, which is used correctly in isEffectiveSuperUser. However, in both the GET and PUT handlers the result of getMothershipSettings / updateMothershipSettings is also stored in a local variable named settings, shadowing the table import for the rest of each handler body. A future developer who needs to reference the settings DB table inside either handler will silently get the local object instead, causing a hard-to-debug runtime failure. Renaming the local variables (e.g. mothershipData) would eliminate the ambiguity.

  4. apps/sim/lib/mothership/settings/runtime.ts, line 138-146 (link)

    P2 load_skill_${id} tools are not hidden in the UI

    The new per-workspace mothership skill tools have dynamic names (load_skill_<uuid>). The isToolHiddenInUi function (and the local copy in message-content.tsx) performs a static Set.has lookup, so none of these tools will be filtered. If the Mothership agent emits a tool_call event for load_skill_abc123, it will be rendered visibly in the chat UI for super admins—similar to the load_custom_tool issue this PR fixes. Consider adding a prefix check alongside the existing set check, or using a naming pattern that can be caught by the static set.

Reviews (1): Last reviewed commit: 22b5a1e | Re-trigger Greptile

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