Skip to content

Commit 65b9399

Browse files
authored
Eliminate duplicate sys tool metadata in registerSysTools() (#1349)
The `registerSysTools()` function in `internal/server/unified.go` was duplicating tool metadata (Name, Description, InputSchema) for each sys tool—once when storing in the internal `ToolInfo` map and again when registering with the SDK. ## Changes - **Added `registerSysTool()` helper function** that encapsulates both internal storage and SDK registration, accepting metadata parameters once - **Refactored `sys___init` and `sys___list_servers`** to use the helper, eliminating ~22 lines of duplicated metadata definitions ## Example Before: ```go // Store tool info us.tools["sys___init"] = &ToolInfo{ Name: "sys___init", Description: "Initialize the MCPG system...", InputSchema: map[string]interface{}{...}, // ... } // Register with SDK (duplicate metadata) sdk.AddTool(us.server, &sdk.Tool{ Name: "sys___init", Description: "Initialize the MCPG system...", InputSchema: map[string]interface{}{...}, }, handler) ``` After: ```go us.registerSysTool( "sys___init", "Initialize the MCPG system...", map[string]interface{}{...}, handler, ) ``` This establishes a single source of truth for sys tool metadata and reduces the risk of schema drift between internal storage and SDK registration. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1598906572/b275/launcher.test /tmp/go-build1598906572/b275/launcher.test -test.testlogfile=/tmp/go-build1598906572/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go fmtsort/sort.go 64/pkg/tool/linu-o --output-dir /tmp/playwright--unsafeptr=false --allowed-origin-unreachable=false 64/pkg/tool/linu/tmp/go-build1598906572/b064/vet.cfg` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1598906572/b260/config.test /tmp/go-build1598906572/b260/config.test -test.testlogfile=/tmp/go-build1598906572/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go base64/base64.go--64 64/pkg/tool/linu-o` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1598906572/b275/launcher.test /tmp/go-build1598906572/b275/launcher.test -test.testlogfile=/tmp/go-build1598906572/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go fmtsort/sort.go 64/pkg/tool/linu-o --output-dir /tmp/playwright--unsafeptr=false --allowed-origin-unreachable=false 64/pkg/tool/linu/tmp/go-build1598906572/b064/vet.cfg` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1598906572/b275/launcher.test /tmp/go-build1598906572/b275/launcher.test -test.testlogfile=/tmp/go-build1598906572/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go fmtsort/sort.go 64/pkg/tool/linu-o --output-dir /tmp/playwright--unsafeptr=false --allowed-origin-unreachable=false 64/pkg/tool/linu/tmp/go-build1598906572/b064/vet.cfg` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1598906572/b284/mcp.test /tmp/go-build1598906572/b284/mcp.test -test.testlogfile=/tmp/go-build1598906572/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go1.25.6 -c=4 -nolocalimports -importcfg /tmp/go-build1598906572/b191/importcfg -pack /opt/hostedtoolcache/go/1.25.6/x64/src/net/http/internal/httpcommon/httpcommon.go` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[duplicate-code] Duplicate Code Analysis Report</issue_title> > <issue_description>*Analysis of commit be3b856* > > ## Summary > > This analysis found **1 significant duplication pattern** in the Go source files. The duplication occurs in `internal/server/unified.go` where sys tool metadata (Name, Description, InputSchema) is defined twice per tool — once for internal `ToolInfo` storage and again for the SDK `sdk.Tool` registration. > > ## Detected Patterns > > 1. **Sys Tool Struct Data Duplication** - Severity: Medium - See sub-issue #1348 > > ## Overall Impact > > - **Total Duplicated Lines**: ~22 lines across 2 sys tools > - **Affected Files**: 1 (`internal/server/unified.go`) > - **Maintainability Risk**: Medium — schema/description changes to sys tools require updating two locations, risking drift > - **Refactoring Priority**: Low-Medium — localized to `registerSysTools()`, no runtime risk, but creates update burden > > ## Next Steps > > 1. Review individual pattern sub-issue for detailed analysis > 2. Implement a `registerSysTool` helper that accepts name/description/schema once and handles both internal storage and SDK registration > 3. Verify no behavioral change after refactoring with existing tests > > ## Analysis Metadata > > - **Analyzed Files**: 64 Go source files (excluding test files) > - **Detection Method**: Semantic and structural code analysis > - **Commit**: be3b856 > - **Analysis Date**: 2026-02-24</issue_description> > > ## Comments on the Issue (you are @claude[agent] in this section) > > <comments> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #1347
2 parents 3c2d85f + c92839d commit 65b9399

1 file changed

Lines changed: 37 additions & 45 deletions

File tree

internal/server/unified.go

Lines changed: 37 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,29 @@ func (us *UnifiedServer) registerToolsFromBackend(serverID string) error {
406406
return nil
407407
}
408408

409+
// registerSysTool is a helper function that registers a sys tool by storing its metadata
410+
// in the internal tools map and registering it with the SDK. This eliminates duplication
411+
// of tool metadata (Name, Description, InputSchema) that was previously defined twice.
412+
func (us *UnifiedServer) registerSysTool(name, description string, inputSchema map[string]interface{}, handler func(context.Context, *sdk.CallToolRequest, interface{}) (*sdk.CallToolResult, interface{}, error)) {
413+
// Store tool info
414+
us.toolsMu.Lock()
415+
us.tools[name] = &ToolInfo{
416+
Name: name,
417+
Description: description,
418+
InputSchema: inputSchema,
419+
BackendID: "sys",
420+
Handler: handler,
421+
}
422+
us.toolsMu.Unlock()
423+
424+
// Register with SDK
425+
sdk.AddTool(us.server, &sdk.Tool{
426+
Name: name,
427+
Description: description,
428+
InputSchema: inputSchema,
429+
}, handler)
430+
}
431+
409432
// registerSysTools registers built-in sys tools
410433
func (us *UnifiedServer) registerSysTools() error {
411434
// Create sys_init handler
@@ -464,12 +487,11 @@ func (us *UnifiedServer) registerSysTools() error {
464487
return nil, result, nil
465488
}
466489

467-
// Store sys_init tool info
468-
us.toolsMu.Lock()
469-
us.tools["sys___init"] = &ToolInfo{
470-
Name: "sys___init",
471-
Description: "Initialize the MCPG system and get available MCP servers",
472-
InputSchema: map[string]interface{}{
490+
// Register sys_init tool using helper
491+
us.registerSysTool(
492+
"sys___init",
493+
"Initialize the MCPG system and get available MCP servers",
494+
map[string]interface{}{
473495
"type": "object",
474496
"properties": map[string]interface{}{
475497
"token": map[string]interface{}{
@@ -478,25 +500,8 @@ func (us *UnifiedServer) registerSysTools() error {
478500
},
479501
},
480502
},
481-
BackendID: "sys",
482-
Handler: sysInitHandler,
483-
}
484-
us.toolsMu.Unlock()
485-
486-
// Register with SDK
487-
sdk.AddTool(us.server, &sdk.Tool{
488-
Name: "sys___init",
489-
Description: "Initialize the MCPG system and get available MCP servers",
490-
InputSchema: map[string]interface{}{
491-
"type": "object",
492-
"properties": map[string]interface{}{
493-
"token": map[string]interface{}{
494-
"type": "string",
495-
"description": "Authentication token for session initialization (can be empty for first call)",
496-
},
497-
},
498-
},
499-
}, sysInitHandler)
503+
sysInitHandler,
504+
)
500505

501506
// Create sys_list_servers handler
502507
sysListHandler := func(ctx context.Context, req *sdk.CallToolRequest, args interface{}) (*sdk.CallToolResult, interface{}, error) {
@@ -524,29 +529,16 @@ func (us *UnifiedServer) registerSysTools() error {
524529
return nil, result, nil
525530
}
526531

527-
// Store sys_list_servers tool info
528-
us.toolsMu.Lock()
529-
us.tools["sys___list_servers"] = &ToolInfo{
530-
Name: "sys___list_servers",
531-
Description: "List all configured MCP backend servers",
532-
InputSchema: map[string]interface{}{
533-
"type": "object",
534-
"properties": map[string]interface{}{},
535-
},
536-
BackendID: "sys",
537-
Handler: sysListHandler,
538-
}
539-
us.toolsMu.Unlock()
540-
541-
// Register with SDK
542-
sdk.AddTool(us.server, &sdk.Tool{
543-
Name: "sys___list_servers",
544-
Description: "List all configured MCP backend servers",
545-
InputSchema: map[string]interface{}{
532+
// Register sys_list_servers tool using helper
533+
us.registerSysTool(
534+
"sys___list_servers",
535+
"List all configured MCP backend servers",
536+
map[string]interface{}{
546537
"type": "object",
547538
"properties": map[string]interface{}{},
548539
},
549-
}, sysListHandler)
540+
sysListHandler,
541+
)
550542

551543
log.Println("Registered 2 sys tools")
552544
return nil

0 commit comments

Comments
 (0)