Skip to content

Enforce MCP tool name uniqueness per capset #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GOLDKUN wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Enforce MCP tool name uniqueness per capset #513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
573d7ae
Enforce MCP tool name uniqueness per capset
Sep 2, 2026
359e016
Preserve ambiguous MCP names across instances
Sep 2, 2026
5c5b488
Restore capset-wide MCP tool uniqueness
Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions internal/admin/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,11 @@ func (s *Server) handleCapsetPath(w http.ResponseWriter, r *http.Request, capset
}
}
if err := s.Store.AddCapsetMethod(r.Context(), domain.CapsetMethod{CapsetInstanceID: ci.ID, MethodFullName: method.FullName, MCPToolName: toolName, Enabled: true}); err != nil {
writeError(w, http.StatusBadRequest, err.Error())
statusCode := http.StatusBadRequest
if errors.Is(err, store.ErrMCPToolNameConflict) {
statusCode = http.StatusConflict
}
writeError(w, statusCode, err.Error())
return
}
}
Expand Down Expand Up @@ -1440,7 +1444,11 @@ func (s *Server) handleCapsetPath(w http.ResponseWriter, r *http.Request, capset
}
}
if err := s.Store.AddCapsetMethod(r.Context(), domain.CapsetMethod{CapsetInstanceID: ciID, MethodFullName: selected.FullName, MCPToolName: toolName, Enabled: true}); err != nil {
writeError(w, http.StatusBadRequest, err.Error())
statusCode := http.StatusBadRequest
if errors.Is(err, store.ErrMCPToolNameConflict) {
statusCode = http.StatusConflict
}
writeError(w, statusCode, err.Error())
return
}
s.logger().Info("capset_method_selected", "capset_id", capsetID, "instance_id", req.InstanceID, "method", selected.FullName, "mcp_tool", toolName)
Expand Down
87 changes: 87 additions & 0 deletions internal/store/mcp_tool_name_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package store

import (
"context"
"errors"
"strings"
"testing"

"octobus/internal/domain"
)

func TestMigrateReportsExistingMCPToolConflicts(t *testing.T) {
st, err := Open(":memory:")
if err != nil {
t.Fatal(err)
}
defer st.Close()
ctx := context.Background()
if err := st.UpsertService(ctx, domain.Service{ID: "echo", Name: "Echo"}); err != nil {
t.Fatal(err)
}
if err := st.UpsertInstance(ctx, domain.Instance{ID: "echo-instance", ServiceID: "echo", Name: "Echo", Enabled: true}); err != nil {
t.Fatal(err)
}
if err := st.CreateCapset(ctx, domain.Capset{ID: "dev", Name: "Dev", Enabled: true}); err != nil {
t.Fatal(err)
}
if err := st.AddCapsetInstance(ctx, domain.CapsetInstance{ID: "dev:echo-instance", CapsetID: "dev", ServiceID: "echo", InstanceID: "echo-instance", Enabled: true}); err != nil {
t.Fatal(err)
}
if err := st.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-instance", MethodFullName: "echo.Echo/Call", MCPToolName: "shared_tool", Enabled: true}); err != nil {
t.Fatal(err)
}
if _, err := st.DB().ExecContext(ctx, `DROP INDEX uq_capset_methods_mcp_tool_key`); err != nil {
t.Fatal(err)
}
if _, err := st.DB().ExecContext(ctx, `INSERT INTO capset_methods (id, capset_instance_id, method_full_name, mcp_tool_name, mcp_tool_key, created_at, updated_at) VALUES (?, ?, ?, ?, '', ?, ?)`, "duplicate", "dev:echo-instance", "echo.Echo/Other", "shared_tool", "", ""); err != nil {
t.Fatal(err)
}
if err := st.Migrate(ctx); err == nil || !strings.Contains(err.Error(), "conflicting methods") {
t.Fatalf("migration conflict error = %v", err)
}
}

func TestAddCapsetMethodEnforcesToolNameUniquenessWithinCapset(t *testing.T) {
st, err := Open(":memory:")
if err != nil {
t.Fatal(err)
}
defer st.Close()
ctx := context.Background()

if err := st.UpsertService(ctx, domain.Service{ID: "echo", Name: "Echo"}); err != nil {
t.Fatal(err)
}
if err := st.UpsertInstance(ctx, domain.Instance{ID: "echo-instance", ServiceID: "echo", Name: "Echo", Enabled: true}); err != nil {
t.Fatal(err)
}
for _, capsetID := range []string{"dev", "qa"} {
if err := st.CreateCapset(ctx, domain.Capset{ID: capsetID, Name: capsetID, Enabled: true}); err != nil {
t.Fatal(err)
}
if err := st.AddCapsetInstance(ctx, domain.CapsetInstance{ID: capsetID + ":echo-instance", CapsetID: capsetID, ServiceID: "echo", InstanceID: "echo-instance", Enabled: true}); err != nil {
t.Fatal(err)
}
}
if err := st.UpsertInstance(ctx, domain.Instance{ID: "echo-copy", ServiceID: "echo", Name: "Echo Copy", Enabled: true}); err != nil {
t.Fatal(err)
}
if err := st.AddCapsetInstance(ctx, domain.CapsetInstance{ID: "dev:echo-copy", CapsetID: "dev", ServiceID: "echo", InstanceID: "echo-copy", Enabled: true}); err != nil {
t.Fatal(err)
}

first := domain.CapsetMethod{CapsetInstanceID: "dev:echo-instance", MethodFullName: "echo.Echo/Call", MCPToolName: "shared_tool", Enabled: true}
if err := st.AddCapsetMethod(ctx, first); err != nil {
t.Fatal(err)
}
if err := st.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-instance", MethodFullName: "echo.Echo/Other", MCPToolName: "shared_tool", Enabled: true}); !errors.Is(err, ErrMCPToolNameConflict) {
t.Fatalf("duplicate tool error = %v", err)
}
if err := st.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-copy", MethodFullName: "echo.Echo/Call", MCPToolName: "shared_tool", Enabled: true}); !errors.Is(err, ErrMCPToolNameConflict) {
t.Fatalf("cross-instance duplicate tool error = %v", err)
}
if err := st.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "qa:echo-instance", MethodFullName: "echo.Echo/Call", MCPToolName: "shared_tool", Enabled: true}); err != nil {
t.Fatalf("same tool in another capset error = %v", err)
}
}
Comment thread
monkeyscan[bot] marked this conversation as resolved.
58 changes: 56 additions & 2 deletions internal/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ type Store struct {
db *sql.DB
}

var ErrMCPToolNameConflict = errors.New("MCP tool name conflict")

const mcpToolKeySeparator = "\x1f"

type ServiceInUseError struct {
ServiceID string
InstanceID string
Expand Down Expand Up @@ -109,6 +113,27 @@ func (s *Store) Migrate(ctx context.Context) error {
if err := addColumnIfMissing(ctx, s.db, "services", "service_root", "TEXT NOT NULL DEFAULT '.'"); err != nil {
return err
}
if err := addColumnIfMissing(ctx, s.db, "capset_methods", "mcp_tool_key", "TEXT NOT NULL DEFAULT ''"); err != nil {
return err
}
if _, err := s.db.ExecContext(ctx, `UPDATE capset_methods SET mcp_tool_key = (

@monkeyscan monkeyscan Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

迁移在“上一版本已建实例级唯一索引且存在跨实例同名工具”的库上会先在回填 UPDATE 处失败，新增友好冲突检测不可达

本变更的目的之一是在迁移阶段对同 capset 内跨实例重复 mcp_tool_name 的存量数据给出可操作的“remove or rename duplicates”指引（新增 127-133 行重复检测 + 新测试）。但上一版本（base）的迁移已创建实例级唯一索引 uq_capset_methods_mcp_tool_key，且其 key 前缀为 capset_instance_id，因此 base 代码允许同 capset 的不同实例写入同名 MCP 工具，即这类问题库必然同时带有该索引。升级时，新 Migrate 的回填 UPDATE（119-124 行）把两条记录都改写为 capset 级 key（capset_id + \x1f + name），在既有唯一索引仍在的情况下，第二条记录被更新成相同 key 时 UPDATE 语句即报 UNIQUE constraint failed 并整体回滚，返回原始 sqlite 错误，执行不到 127-133 行的友好重复检测。证据：新增测试 TestMigrateReportsExistingMCPToolConflicts 必须先执行 DROP INDEX uq_capset_methods_mcp_tool_key 才能让 Migrate 走到 "conflicting methods" 分支，恰好证明在真实升级路径（索引存在）下该分支不可达。因此历史问题“升级遇重复数据时服务无法启动且仅报原始 UNIQUE 错误、无清理指引”在主要升级场景下仍然残留，且新增的错误消息本身展示的是含 \x1f 控制符的内部 mcp_tool_key（如 %q 输出 capset_id\x1ftool_name），可操作性有限。

Problem code:

Changed code at internal/store/store.go:119

Recommendation:
在回填 UPDATE 之前先删除既有索引，使重复 key 能被 UPDATE 生成、再由新增的重复检测返回友好错误，随后再重建唯一索引，例如在 addColumnIfMissing 之后插入 DROP INDEX IF EXISTS uq_capset_methods_mcp_tool_key，并把 CREATE UNIQUE INDEX IF NOT EXISTS 保留在重复检测之后。同时补充一个“保留旧实例级索引且已存在跨实例重复工具数据”的升级迁移测试，验证得到的是含指引的 "conflicting methods" 错误而非原始 UNIQUE 错误；并考虑让错误消息直接展示 capset 与 mcp_tool_name 而非含 \x1f 的内部 key。

Suggested diff:

 	if err := addColumnIfMissing(ctx, s.db, "capset_methods", "mcp_tool_key", "TEXT NOT NULL DEFAULT ''"); err != nil {
 		return err
 	}
+	if _, err := s.db.ExecContext(ctx, `DROP INDEX IF EXISTS uq_capset_methods_mcp_tool_key`); err != nil {
+		return err
+	}
 	if _, err := s.db.ExecContext(ctx, `UPDATE capset_methods SET mcp_tool_key = (
 		SELECT ci.capset_id || char(31) || capset_methods.mcp_tool_name
 		FROM capset_instances ci
 		WHERE ci.id = capset_methods.capset_instance_id
 	)
 	WHERE mcp_tool_name <> ''`); err != nil {
 		return err
 	}

SELECT ci.capset_id || char(31) || capset_methods.mcp_tool_name
FROM capset_instances ci
WHERE ci.id = capset_methods.capset_instance_id
)
WHERE mcp_tool_name <> ''`); err != nil {
return err
}
var duplicateKey string
var duplicateCount int
if err := s.db.QueryRowContext(ctx, `SELECT mcp_tool_key, COUNT(*) FROM capset_methods WHERE mcp_tool_key <> '' GROUP BY mcp_tool_key HAVING COUNT(*) > 1 LIMIT 1`).Scan(&duplicateKey, &duplicateCount); err != nil && !errors.Is(err, sql.ErrNoRows) {
return err
} else if err == nil {
return fmt.Errorf("MCP tool name %q has %d conflicting methods; remove or rename duplicates before restarting", duplicateKey, duplicateCount)
}
if _, err := s.db.ExecContext(ctx, `CREATE UNIQUE INDEX IF NOT EXISTS uq_capset_methods_mcp_tool_key ON capset_methods(mcp_tool_key) WHERE mcp_tool_key <> ''`); err != nil {
return err
Comment thread
monkeyscan[bot] marked this conversation as resolved.
}
_, err := s.db.ExecContext(ctx, `UPDATE services SET runtime_mode = 'long-running' WHERE runtime_mode = ''`)
return err
}
Expand Down Expand Up @@ -782,8 +807,36 @@ func (s *Store) AddCapsetMethod(ctx context.Context, method domain.CapsetMethod)
now := time.Now().UTC()
method.CreatedAt = now
method.UpdatedAt = now
_, err := s.db.ExecContext(ctx, `INSERT INTO capset_methods (id, capset_instance_id, method_full_name, rest_alias, mcp_tool_name, enabled, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, method.ID, method.CapsetInstanceID, method.MethodFullName, method.RestAlias, method.MCPToolName, boolInt(method.Enabled), formatTime(method.CreatedAt), formatTime(method.UpdatedAt))
return err

tx, err := s.db.BeginTx(ctx, nil)
if err != nil {
return err
}
defer tx.Rollback()

toolKey := ""
if method.MCPToolName != "" {
var capsetID string
if err := tx.QueryRowContext(ctx, `SELECT capset_id FROM capset_instances WHERE id = ?`, method.CapsetInstanceID).Scan(&capsetID); err != nil {
return err
}
toolKey = capsetID + mcpToolKeySeparator + method.MCPToolName
}
_, err = tx.ExecContext(ctx, `INSERT INTO capset_methods (id, capset_instance_id, method_full_name, rest_alias, mcp_tool_name, mcp_tool_key, enabled, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, method.ID, method.CapsetInstanceID, method.MethodFullName, method.RestAlias, method.MCPToolName, toolKey, boolInt(method.Enabled), formatTime(method.CreatedAt), formatTime(method.UpdatedAt))
if err != nil {
var existingID string
if idErr := tx.QueryRowContext(ctx, `SELECT id FROM capset_methods WHERE id = ?`, method.ID).Scan(&existingID); idErr == nil {
return err
}
if toolKey != "" {
var existingToolID string
if keyErr := tx.QueryRowContext(ctx, `SELECT id FROM capset_methods WHERE mcp_tool_key = ?`, toolKey).Scan(&existingToolID); keyErr == nil {
return ErrMCPToolNameConflict
}
}
Comment thread
monkeyscan[bot] marked this conversation as resolved.
return err
}
return tx.Commit()
}

func (s *Store) GetCapsetMethod(ctx context.Context, capsetInstanceID, methodFullName string) (domain.CapsetMethod, error) {
Expand Down Expand Up @@ -1257,6 +1310,7 @@ CREATE TABLE IF NOT EXISTS capset_methods (
method_full_name TEXT NOT NULL,
rest_alias TEXT NOT NULL DEFAULT '',
mcp_tool_name TEXT NOT NULL DEFAULT '',
mcp_tool_key TEXT NOT NULL DEFAULT '',
enabled INTEGER NOT NULL DEFAULT 1,
created_at TEXT NOT NULL,
updated_at TEXT NOT NULL,
Expand Down
12 changes: 6 additions & 6 deletions internal/store/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,14 +935,14 @@ func TestFindToolAmbiguousAndStreamingCustomNames(t *testing.T) {
if err := s.AddCapsetInstance(ctx, domain.CapsetInstance{ID: "dev:echo-copy", CapsetID: "dev", ServiceID: "echo", InstanceID: "echo-copy", IncludeAllMethods: false, Enabled: true}); err != nil {
t.Fatal(err)
}
if err := s.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-copy", MethodFullName: "echo.v1.EchoService/Echo", MCPToolName: "echo_tool", Enabled: true}); err != nil {
t.Fatal(err)
if err := s.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-copy", MethodFullName: "echo.v1.EchoService/Echo", MCPToolName: "echo_tool", Enabled: true}); !errors.Is(err, ErrMCPToolNameConflict) {
t.Fatalf("expected duplicate MCP tool error, got %v", err)
}
if _, err := s.FindTool(ctx, "dev", "echo_tool"); err == nil || !strings.Contains(err.Error(), "ambiguous MCP tool name") {
t.Fatalf("expected ambiguous MCP tool error, got %v", err)
if _, err := s.FindTool(ctx, "dev", "echo_tool"); err != nil {
t.Fatalf("FindTool existing unique name error = %v", err)
}
if exists, err := s.MCPToolNameExists(ctx, "dev", "echo_tool"); err == nil || !strings.Contains(err.Error(), "ambiguous MCP tool name") || exists {
t.Fatalf("MCPToolNameExists ambiguous exists=%v err=%v", exists, err)
if exists, err := s.MCPToolNameExists(ctx, "dev", "echo_tool"); err != nil || !exists {
t.Fatalf("MCPToolNameExists exists=%v err=%v", exists, err)
}

if err := s.AddCapsetMethod(ctx, domain.CapsetMethod{CapsetInstanceID: "dev:echo-copy", MethodFullName: "echo.v1.EchoService/ServerStream", MCPToolName: "stream_custom", Enabled: true}); err != nil {
Expand Down
Loading