Skip to content

Commit fc94510

Browse files
Dumbrisclaude
andauthored
feat: refactor REST endpoints to use management service layer (smart-mcp-proxy#153)
* feat: refactor REST endpoints to use management service layer This refactoring addresses spec 004 compliance by ensuring REST endpoints delegate to the management service instead of bypassing it. This eliminates architectural violations identified in PR smart-mcp-proxy#152 review. Changes: - Extended ManagementService interface with GetServerTools() and TriggerOAuthLogin() - Refactored /api/v1/servers/{id}/tools to use management service - Refactored /api/v1/servers/{id}/login to use management service - Added RuntimeOperations interface methods to Runtime type - Enhanced test mocks to support new methods - Added 7 unit tests with 82.1% coverage (exceeds 80% target) - Updated CLAUDE.md with new tool operation methods Architecture improvements: - Configuration gates enforced centrally (disable_management, read_only) - Proper layering maintained: REST → Management → Runtime → StateView - Lock-free reads via StateView cache (<10ms for GetServerTools) - Event emissions via existing event bus (servers.changed on OAuth completion) Testing: - All 45 tasks completed (T001-T045) - 25/25 E2E API tests passing - Zero breaking changes to REST API contracts - CLI commands work correctly via socket/REST delegation - Linter: 0 issues Spec: 005-rest-management-integration Closes: architectural violation from PR smart-mcp-proxy#152 review 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: make TestDetectSocketPath_Auth platform-aware for Windows named pipes Windows named pipes use global namespace (\\.\pipe\mcpproxy-<username>-<hash>) instead of file paths within the data directory. Updated test to check for named pipe prefix on Windows and data directory containment on Unix. Fixes smart-mcp-proxy#153 PR build test failure on Windows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: make TestDetectSocketPath_Tools platform-aware for Windows Apply the same Windows named pipe fix to tools_cmd_test.go that was applied to auth_cmd_test.go. Windows named pipes use global namespace and don't contain the data directory path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b5bba51 commit fc94510

16 files changed

Lines changed: 2705 additions & 6 deletions

File tree

CLAUDE.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,11 +357,12 @@ The management service (`internal/management/`) provides a centralized business
357357

358358
**Key Components**:
359359

360-
- **Service Interface** (`service.go:16-64`): Defines all management operations
360+
- **Service Interface** (`service.go:16-102`): Defines all management operations
361361
- Single-server: `RestartServer()`, `EnableServer()`, `DisableServer()`
362362
- Bulk operations: `RestartAll()`, `EnableAll()`, `DisableAll()`
363363
- Diagnostics: `GetServerHealth()`, `RunDiagnostics()`
364364
- Server CRUD: `AddServer()`, `RemoveServer()`, `QuarantineServer()`
365+
- Tool operations: `GetServerTools()`, `TriggerOAuthLogin()` (added in spec 005)
365366

366367
- **Configuration Gates**: All operations respect centralized configuration guards
367368
- `disable_management`: Blocks all write operations when true
@@ -831,6 +832,7 @@ When making changes to this codebase, ensure you understand the modular architec
831832
- BBolt (existing `server_{serverID}_tool_calls` buckets, new `sessions` bucket) (003-tool-annotations-webui)
832833
- Go 1.24.0 (004-management-health-refactor)
833834
- BBolt embedded database (`~/.mcpproxy/config.db`) for server configurations, quarantine status, and tool statistics (004-management-health-refactor)
835+
- BBolt embedded database (`~/.mcpproxy/config.db`) - used by existing runtime, no changes required (005-rest-management-integration)
834836

835837
## Recent Changes
836838
- 003-tool-annotations-webui: Added Go 1.21+, TypeScript/Vue 3

cmd/mcpproxy/auth_cmd_test.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package main
22

33
import (
4+
"runtime"
5+
"strings"
46
"testing"
57

68
"mcpproxy-go/internal/socket"
@@ -32,5 +34,16 @@ func TestDetectSocketPath_Auth(t *testing.T) {
3234
socketPath := socket.DetectSocketPath(tmpDir)
3335

3436
assert.NotEmpty(t, socketPath, "DetectSocketPath should return non-empty path")
35-
assert.Contains(t, socketPath, tmpDir, "Socket path should be within data directory")
37+
38+
// Platform-specific assertions
39+
if runtime.GOOS == "windows" {
40+
// Windows: Named pipes use global namespace with hash
41+
assert.True(t, strings.HasPrefix(socketPath, "npipe:////./pipe/mcpproxy-"),
42+
"Windows socket should be a named pipe")
43+
} else {
44+
// Unix: Socket file is within data directory
45+
assert.Contains(t, socketPath, tmpDir, "Socket path should be within data directory")
46+
assert.True(t, strings.HasPrefix(socketPath, "unix://"),
47+
"Unix socket should have unix:// prefix")
48+
}
3649
}

cmd/mcpproxy/tools_cmd_test.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package main
22

33
import (
4+
"runtime"
5+
"strings"
46
"testing"
57

68
"mcpproxy-go/internal/socket"
@@ -24,5 +26,16 @@ func TestDetectSocketPath_Tools(t *testing.T) {
2426
socketPath := socket.DetectSocketPath(tmpDir)
2527

2628
assert.NotEmpty(t, socketPath, "DetectSocketPath should return non-empty path")
27-
assert.Contains(t, socketPath, tmpDir, "Socket path should be within data directory")
29+
30+
// Platform-specific assertions
31+
if runtime.GOOS == "windows" {
32+
// Windows: Named pipes use global namespace with hash
33+
assert.True(t, strings.HasPrefix(socketPath, "npipe:////./pipe/mcpproxy-"),
34+
"Windows socket should be a named pipe")
35+
} else {
36+
// Unix: Socket file is within data directory
37+
assert.Contains(t, socketPath, tmpDir, "Socket path should be within data directory")
38+
assert.True(t, strings.HasPrefix(socketPath, "unix://"),
39+
"Unix socket should have unix:// prefix")
40+
}
2841
}

internal/httpapi/contracts_test.go

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,61 @@ import (
2424
// MockServerController implements ServerController for testing
2525
type MockServerController struct{}
2626

27+
// mockManagementService provides a test implementation of management service methods
28+
type mockManagementService struct{}
29+
30+
func (m *mockManagementService) ListServers(ctx context.Context) ([]*contracts.Server, *contracts.ServerStats, error) {
31+
return []*contracts.Server{
32+
{
33+
ID: "test-server",
34+
Name: "test-server",
35+
Protocol: "stdio",
36+
Command: "echo",
37+
Args: []string{"hello"},
38+
Enabled: true,
39+
Quarantined: false,
40+
Connected: true,
41+
Status: "Ready",
42+
ToolCount: 5,
43+
ReconnectCount: 0,
44+
Authenticated: false,
45+
},
46+
}, &contracts.ServerStats{
47+
TotalServers: 1,
48+
ConnectedServers: 1,
49+
QuarantinedServers: 0,
50+
TotalTools: 5,
51+
}, nil
52+
}
53+
54+
func (m *mockManagementService) EnableServer(ctx context.Context, name string, enabled bool) error {
55+
return nil
56+
}
57+
58+
func (m *mockManagementService) RestartServer(ctx context.Context, name string) error {
59+
return nil
60+
}
61+
62+
func (m *mockManagementService) GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error) {
63+
return []map[string]interface{}{
64+
{
65+
"name": "echo_tool",
66+
"server_name": name,
67+
"description": "A simple echo tool for testing",
68+
"usage": 10,
69+
},
70+
}, nil
71+
}
72+
73+
func (m *mockManagementService) TriggerOAuthLogin(ctx context.Context, name string) error {
74+
return nil
75+
}
76+
2777
func (m *MockServerController) IsRunning() bool { return true }
2878
func (m *MockServerController) GetListenAddress() string { return ":8080" }
29-
func (m *MockServerController) GetManagementService() interface{} { return nil }
79+
func (m *MockServerController) GetManagementService() interface{} {
80+
return &mockManagementService{}
81+
}
3082
func (m *MockServerController) GetUpstreamStats() map[string]interface{} {
3183
return map[string]interface{}{
3284
"servers": map[string]interface{}{

internal/httpapi/server.go

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,8 +1054,28 @@ func (s *Server) handleServerLogin(w http.ResponseWriter, r *http.Request) {
10541054
return
10551055
}
10561056

1057-
if err := s.controller.TriggerOAuthLogin(serverID); err != nil {
1057+
// NEW: Call management service instead of controller (T017)
1058+
mgmtSvc, ok := s.controller.GetManagementService().(interface {
1059+
TriggerOAuthLogin(ctx context.Context, name string) error
1060+
})
1061+
if !ok {
1062+
s.logger.Error("Management service not available or missing TriggerOAuthLogin method")
1063+
s.writeError(w, http.StatusInternalServerError, "Management service not available")
1064+
return
1065+
}
1066+
1067+
if err := mgmtSvc.TriggerOAuthLogin(r.Context(), serverID); err != nil {
10581068
s.logger.Error("Failed to trigger OAuth login", "server", serverID, "error", err)
1069+
1070+
// Map errors to HTTP status codes (T019)
1071+
if strings.Contains(err.Error(), "management disabled") || strings.Contains(err.Error(), "read-only") {
1072+
s.writeError(w, http.StatusForbidden, err.Error())
1073+
return
1074+
}
1075+
if strings.Contains(err.Error(), "not found") {
1076+
s.writeError(w, http.StatusNotFound, fmt.Sprintf("Server not found: %s", serverID))
1077+
return
1078+
}
10591079
s.writeError(w, http.StatusInternalServerError, fmt.Sprintf("Failed to trigger login: %v", err))
10601080
return
10611081
}
@@ -1159,9 +1179,25 @@ func (s *Server) handleGetServerTools(w http.ResponseWriter, r *http.Request) {
11591179
return
11601180
}
11611181

1162-
tools, err := s.controller.GetServerTools(serverID)
1182+
// NEW: Call management service instead of controller (T016)
1183+
mgmtSvc, ok := s.controller.GetManagementService().(interface {
1184+
GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error)
1185+
})
1186+
if !ok {
1187+
s.logger.Error("Management service not available or missing GetServerTools method")
1188+
s.writeError(w, http.StatusInternalServerError, "Management service not available")
1189+
return
1190+
}
1191+
1192+
tools, err := mgmtSvc.GetServerTools(r.Context(), serverID)
11631193
if err != nil {
11641194
s.logger.Error("Failed to get server tools", "server", serverID, "error", err)
1195+
1196+
// Map errors to HTTP status codes (T018)
1197+
if strings.Contains(err.Error(), "not found") {
1198+
s.writeError(w, http.StatusNotFound, fmt.Sprintf("Server not found: %s", serverID))
1199+
return
1200+
}
11651201
s.writeError(w, http.StatusInternalServerError, fmt.Sprintf("Failed to get tools: %v", err))
11661202
return
11671203
}

internal/management/service.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,24 @@ type Service interface {
8181
// AuthStatus returns detailed OAuth authentication status for a specific server.
8282
// Returns nil if server doesn't use OAuth or doesn't exist.
8383
AuthStatus(ctx context.Context, name string) (*contracts.AuthStatus, error)
84+
85+
// Server Tool Operations
86+
87+
// GetServerTools retrieves all available tools for a specific upstream MCP server.
88+
// Delegates to runtime's GetServerTools() which reads from StateView cache.
89+
// This is a read-only operation that completes in <10ms (in-memory cache read).
90+
// Returns empty array if server has no tools.
91+
// Returns error if server name is empty, server not found, or server not connected.
92+
GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error)
93+
94+
// TriggerOAuthLogin initiates an OAuth 2.x authentication flow for a specific server.
95+
// Delegates to upstream manager's StartManualOAuth() which launches browser-based flow.
96+
// This operation respects disable_management and read_only configuration gates.
97+
// Emits "servers.changed" event on successful OAuth completion.
98+
// Method returns immediately after starting OAuth flow (actual completion is asynchronous).
99+
// Returns error if server name is empty, server not found, config gates block operation,
100+
// or server doesn't support OAuth.
101+
TriggerOAuthLogin(ctx context.Context, name string) error
84102
}
85103

86104
// EventEmitter defines the interface for emitting runtime events.
@@ -96,6 +114,8 @@ type RuntimeOperations interface {
96114
RestartServer(serverName string) error
97115
GetAllServers() ([]map[string]interface{}, error)
98116
BulkEnableServers(serverNames []string, enabled bool) (map[string]error, error)
117+
GetServerTools(serverName string) ([]map[string]interface{}, error)
118+
TriggerOAuthLogin(serverName string) error
99119
}
100120

101121
// service implements the Service interface with dependency injection.
@@ -569,6 +589,47 @@ func (s *service) DisableAll(ctx context.Context) (*BulkOperationResult, error)
569589

570590
// Doctor is now implemented in diagnostics.go (T040-T044)
571591

592+
// GetServerTools retrieves all tools for a specific upstream server (T013).
593+
// This method delegates to runtime's GetServerTools() which reads from StateView cache.
594+
func (s *service) GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error) {
595+
// Validate input
596+
if name == "" {
597+
return nil, fmt.Errorf("server name required")
598+
}
599+
600+
// Delegate to runtime (existing implementation)
601+
tools, err := s.runtime.GetServerTools(name)
602+
if err != nil {
603+
return nil, fmt.Errorf("failed to get tools: %w", err)
604+
}
605+
606+
return tools, nil
607+
}
608+
609+
// TriggerOAuthLogin initiates OAuth authentication flow for a server (T014).
610+
// This method checks config gates, delegates to runtime, and emits events on completion.
611+
func (s *service) TriggerOAuthLogin(ctx context.Context, name string) error {
612+
// Validate input
613+
if name == "" {
614+
return fmt.Errorf("server name required")
615+
}
616+
617+
// Check configuration gates (T015)
618+
if err := s.checkWriteGates(); err != nil {
619+
return err
620+
}
621+
622+
// Delegate to runtime (existing implementation)
623+
if err := s.runtime.TriggerOAuthLogin(name); err != nil {
624+
return fmt.Errorf("failed to start OAuth: %w", err)
625+
}
626+
627+
// Event will be emitted by upstream manager on OAuth completion
628+
// (existing behavior - no changes needed)
629+
630+
return nil
631+
}
632+
572633
// AuthStatus returns detailed OAuth authentication status for a specific server.
573634
func (s *service) AuthStatus(ctx context.Context, name string) (*contracts.AuthStatus, error) {
574635
// TODO: Implement later (not in critical path)

0 commit comments

Comments
 (0)