Skip to content

Commit 7a4748c

Browse files
authored
fix: enable tool list change notifications for MCP clients (#332)
Set ToolCapabilities.ListChanged to true so connected clients receive notifications/tools/list_changed when the server's tool list changes. Without this, clients that reconnected after a server upgrade kept their stale cached tool list and never saw newly registered tools like trino_export. Also adds debug logging to wireTrinoExport for diagnosing registration issues, and integration tests verifying trino_export registers correctly through the full initPortal path (single-connection, multi-connection, explicitly disabled, no S3, no trino).
1 parent a70a3f3 commit 7a4748c

2 files changed

Lines changed: 242 additions & 1 deletion

File tree

pkg/platform/export_adapters_test.go

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@ import (
55
"fmt"
66
"testing"
77

8+
"github.com/DATA-DOG/go-sqlmock"
9+
"github.com/modelcontextprotocol/go-sdk/mcp"
810
"github.com/stretchr/testify/assert"
911
"github.com/stretchr/testify/require"
1012

1113
"github.com/txn2/mcp-data-platform/pkg/portal"
14+
"github.com/txn2/mcp-data-platform/pkg/registry"
1215
trinokit "github.com/txn2/mcp-data-platform/pkg/toolkits/trino"
1316
)
1417

@@ -220,6 +223,241 @@ func TestParseExportConfigDefaults(t *testing.T) {
220223
assert.Equal(t, 0, cfg.MaxRows)
221224
}
222225

226+
func TestWireTrinoExport_ToolAppearsInToolList(t *testing.T) {
227+
// This is the integration test that proves trino_export actually registers
228+
// when portal + trino are both configured.
229+
230+
// Create a real trino toolkit
231+
tk, err := trinokit.New("test", trinokit.Config{
232+
Host: "localhost",
233+
User: "test",
234+
})
235+
require.NoError(t, err)
236+
defer tk.Close() //nolint:errcheck // test cleanup
237+
238+
// Verify trino_export is NOT in the tool list before wiring
239+
assert.NotContains(t, tk.Tools(), "trino_export")
240+
241+
// Create a platform with portal stores and wire export
242+
p := &Platform{
243+
config: &Config{
244+
Portal: PortalConfig{
245+
S3Bucket: "test-bucket",
246+
S3Prefix: "exports",
247+
},
248+
},
249+
portalAssetStore: portal.NewNoopAssetStore(),
250+
portalVersionStore: portal.NewNoopVersionStore(),
251+
portalShareStore: portal.NewNoopShareStore(),
252+
portalS3Client: &noopS3Client{},
253+
toolkitRegistry: newTestRegistry(tk),
254+
}
255+
256+
p.wireTrinoExport()
257+
258+
// Verify trino_export IS in the tool list after wiring
259+
assert.Contains(t, tk.Tools(), "trino_export")
260+
261+
// Verify it registers on an MCP server
262+
server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.1"}, nil)
263+
tk.RegisterTools(server)
264+
}
265+
266+
// noopS3Client implements portal.S3Client for testing.
267+
type noopS3Client struct{}
268+
269+
func (*noopS3Client) PutObject(_ context.Context, _, _ string, _ []byte, _ string) error {
270+
return nil
271+
}
272+
273+
func (*noopS3Client) GetObject(_ context.Context, _, _ string) (data []byte, contentType string, err error) { //nolint:gocritic // named for clarity
274+
return nil, "", nil
275+
}
276+
func (*noopS3Client) DeleteObject(_ context.Context, _, _ string) error { return nil }
277+
func (*noopS3Client) Close() error { return nil }
278+
279+
// newTestRegistry creates a registry with a single toolkit.
280+
func newTestRegistry(tk *trinokit.Toolkit) *registry.Registry {
281+
r := registry.NewRegistry()
282+
_ = r.Register(tk) //nolint:errcheck // test setup
283+
return r
284+
}
285+
286+
func TestWireTrinoExport_WithMultiConnectionToolkit(t *testing.T) {
287+
// Mirror the real deployment: multi-connection trino created via NewMulti
288+
multiTk, err := trinokit.NewMulti(trinokit.MultiConfig{
289+
DefaultConnection: "acme",
290+
Instances: map[string]trinokit.Config{
291+
"acme": {Host: "localhost", User: "test", Port: 8080},
292+
},
293+
})
294+
require.NoError(t, err)
295+
defer multiTk.Close() //nolint:errcheck // test cleanup
296+
297+
assert.NotContains(t, multiTk.Tools(), "trino_export")
298+
299+
p := &Platform{
300+
config: &Config{
301+
Portal: PortalConfig{
302+
S3Bucket: "portal-assets",
303+
S3Prefix: "artifacts",
304+
},
305+
},
306+
portalAssetStore: portal.NewNoopAssetStore(),
307+
portalVersionStore: portal.NewNoopVersionStore(),
308+
portalShareStore: portal.NewNoopShareStore(),
309+
portalS3Client: &noopS3Client{},
310+
toolkitRegistry: newTestRegistry(multiTk),
311+
}
312+
313+
p.wireTrinoExport()
314+
315+
assert.Contains(t, multiTk.Tools(), "trino_export",
316+
"trino_export must appear in tool list when portal+trino are both configured")
317+
}
318+
319+
func TestWireTrinoExport_SkipsWhenExplicitlyDisabled(t *testing.T) {
320+
tk, err := trinokit.New("test", trinokit.Config{Host: "localhost", User: "test"})
321+
require.NoError(t, err)
322+
defer tk.Close() //nolint:errcheck // test cleanup
323+
324+
disabled := false
325+
p := &Platform{
326+
config: &Config{
327+
Portal: PortalConfig{
328+
Export: PortalExportConfig{Enabled: &disabled},
329+
S3Bucket: "b",
330+
},
331+
},
332+
portalAssetStore: portal.NewNoopAssetStore(),
333+
portalVersionStore: portal.NewNoopVersionStore(),
334+
portalShareStore: portal.NewNoopShareStore(),
335+
portalS3Client: &noopS3Client{},
336+
toolkitRegistry: newTestRegistry(tk),
337+
}
338+
339+
p.wireTrinoExport()
340+
assert.NotContains(t, tk.Tools(), "trino_export")
341+
}
342+
343+
func TestWireTrinoExport_SkipsWhenNoPortalS3(t *testing.T) {
344+
tk, err := trinokit.New("test", trinokit.Config{Host: "localhost", User: "test"})
345+
require.NoError(t, err)
346+
defer tk.Close() //nolint:errcheck // test cleanup
347+
348+
p := &Platform{
349+
config: &Config{},
350+
portalAssetStore: portal.NewNoopAssetStore(),
351+
// portalS3Client is nil — no S3 configured
352+
toolkitRegistry: newTestRegistry(tk),
353+
}
354+
355+
p.wireTrinoExport()
356+
357+
// trino_export should NOT appear because S3 is missing
358+
assert.NotContains(t, tk.Tools(), "trino_export")
359+
}
360+
361+
func TestWireTrinoExport_SkipsWhenNoTrino(_ *testing.T) {
362+
p := &Platform{
363+
config: &Config{Portal: PortalConfig{S3Bucket: "b"}},
364+
portalAssetStore: portal.NewNoopAssetStore(),
365+
portalVersionStore: portal.NewNoopVersionStore(),
366+
portalShareStore: portal.NewNoopShareStore(),
367+
portalS3Client: &noopS3Client{},
368+
toolkitRegistry: registry.NewRegistry(), // empty — no trino
369+
}
370+
371+
p.wireTrinoExport()
372+
// No panic, no error — just silently skips
373+
}
374+
375+
func TestTrinoExportRegistersViaFullPlatformInit(t *testing.T) {
376+
// This test mirrors the real deployment: toolkit registry has trino,
377+
// portal has S3 + database. Exercises initPortal → wireTrinoExport
378+
// with a real toolkit registry (same path as the production code).
379+
db, _, err := sqlmock.New()
380+
require.NoError(t, err)
381+
defer db.Close() //nolint:errcheck // test cleanup
382+
383+
// Build the toolkit registry with trino + s3 (same as initRegistries)
384+
reg := registry.NewRegistry()
385+
registry.RegisterBuiltinFactories(reg)
386+
loader := registry.NewLoader(reg)
387+
err = loader.LoadFromMap(map[string]any{
388+
"trino": map[string]any{
389+
"enabled": true,
390+
"instances": map[string]any{
391+
"test": map[string]any{
392+
"host": "localhost",
393+
"user": "test",
394+
"port": 8080,
395+
},
396+
},
397+
"default": "test",
398+
},
399+
"s3": map[string]any{
400+
"enabled": true,
401+
"instances": map[string]any{
402+
"test": map[string]any{
403+
"endpoint": "http://localhost:9000",
404+
"region": "us-east-1",
405+
"access_key": "test",
406+
"secret_key": "test",
407+
},
408+
},
409+
},
410+
})
411+
require.NoError(t, err)
412+
413+
// Verify trino loaded
414+
trinoToolkits := reg.GetByKind("trino")
415+
require.NotEmpty(t, trinoToolkits, "trino toolkit should be in registry")
416+
417+
// Build platform with db + registry + portal config (same state as after initRegistries + initDatabase)
418+
p := &Platform{
419+
config: &Config{
420+
Toolkits: map[string]any{
421+
"s3": map[string]any{
422+
"enabled": true,
423+
"instances": map[string]any{
424+
"test": map[string]any{
425+
"endpoint": "http://localhost:9000",
426+
"region": "us-east-1",
427+
"access_key_id": "test",
428+
"secret_access_key": "test",
429+
},
430+
},
431+
},
432+
},
433+
Portal: PortalConfig{
434+
S3Connection: "test",
435+
S3Bucket: "portal-assets",
436+
S3Prefix: "artifacts",
437+
},
438+
},
439+
db: db,
440+
toolkitRegistry: reg,
441+
}
442+
443+
// Run initPortal — this creates stores and calls wireTrinoExport
444+
err = p.initPortal()
445+
require.NoError(t, err)
446+
447+
// Check trino_export appeared
448+
var exportTools []string
449+
for _, tk := range reg.GetByKind("trino") {
450+
exportTools = append(exportTools, tk.Tools()...)
451+
}
452+
453+
assert.Contains(t, exportTools, "trino_export",
454+
"trino_export must appear after initPortal when db + trino + s3 are configured. Tools found: %v", exportTools)
455+
456+
// Also verify it registers on the MCP server (simulating Start → RegisterAllTools)
457+
server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.1"}, nil)
458+
reg.RegisterAllTools(server)
459+
}
460+
223461
func TestConvertProvenanceCalls(t *testing.T) {
224462
calls := convertProvenanceCalls([]trinokit.ExportProvenanceCall{
225463
{ToolName: "trino_query", Timestamp: "2026-01-01T00:00:00Z", Parameters: map[string]any{"sql": "SELECT 1"}},

pkg/platform/platform.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1114,14 +1114,17 @@ func (p *Platform) createPortalS3Client() (portal.S3Client, error) {
11141114
// wireTrinoExport injects portal dependencies into Trino toolkits for trino_export.
11151115
func (p *Platform) wireTrinoExport() {
11161116
if isExplicitlyDisabled(p.config.Portal.Export.Enabled) {
1117+
slog.Debug("trino_export: disabled by config")
11171118
return
11181119
}
11191120
if p.portalS3Client == nil || p.portalAssetStore == nil {
1121+
slog.Debug("trino_export: portal S3 or asset store not configured, skipping")
11201122
return
11211123
}
11221124

11231125
trinoToolkits := p.toolkitRegistry.GetByKind("trino")
11241126
if len(trinoToolkits) == 0 {
1127+
slog.Debug("trino_export: no trino toolkits registered, skipping")
11251128
return
11261129
}
11271130

@@ -1951,7 +1954,7 @@ func convertIconDefs(defs map[string]IconDef) map[string]middleware.IconConfig {
19511954
func (p *Platform) buildServerCapabilities() *mcp.ServerCapabilities {
19521955
caps := &mcp.ServerCapabilities{
19531956
// Tools are always available — every platform deployment has at least platform_info.
1954-
Tools: &mcp.ToolCapabilities{},
1957+
Tools: &mcp.ToolCapabilities{ListChanged: true},
19551958
// Logging is always available for client logging support.
19561959
Logging: &mcp.LoggingCapabilities{},
19571960
}

0 commit comments

Comments
 (0)