core/mcp
8
0
Fork 0
Code Issues 4 Pull requests 2 Projects Releases Packages Wiki Activity Actions

Fix Codex review findings #12

Merged
Snider merged 2 commits from agent/fix-the-following-codex-review-findings into dev 2026-03-24 11:37:44 +00:00
Conversation 0 Commits 2 Files changed 11 +212 -33
11 changed files with 212 additions and 33 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
EXCEPTIONS.md Normal file
View file

@ -0,0 +1,17 @@
# Exceptions
Items from the Codex review that cannot be fixed, with reasons.
## 6. Compile-time interface assertions in subsystem packages
**Files:** `brain/brain.go`, `brain/direct.go`, `agentic/prep.go`, `ide/ide.go`
**Finding:** Add `var _ Subsystem = (*T)(nil)` compile-time assertions.
**Reason:** The `Subsystem` interface is defined in the parent `mcp` package. Subsystem packages (`brain`, `agentic`, `ide`) cannot import `mcp` because `mcp` already imports them via `Options.Subsystems` — this would create a circular import. The interface conformance is enforced at runtime when `RegisterTools` is called during `mcp.New()`.
## 7. Compile-time Notifier assertion on Service
**Finding:** Add `var _ Notifier = (*Service)(nil)`.
**Resolution:** Fixed — assertion added to `pkg/mcp/subsystem.go` (where the `Notifier` interface is defined). The TODO originally claimed this was already done in commit `907d62a`, but it was not present in the codebase.

12
cmd/brain-seed/main.go
View file

@ -19,7 +19,7 @@ import (
"encoding/json"
"flag"
"fmt"
"io"
goio "io"
"net/http"
"os"
"path/filepath"
@ -130,7 +130,7 @@ func main() {
filename := strings.TrimSuffix(filepath.Base(f), ".md")
if len(sections) == 0 {
fmt.Printf(" skip %s/%s (no sections)\n", project, filename)
coreerr.Warn("brain-seed: skip file (no sections)", "project", project, "file", filename)
skipped++
continue
}
@ -157,7 +157,7 @@ func main() {
}
if err := callBrainRemember(content, memType, tags, project, confidence); err != nil {
fmt.Printf(" FAIL %s/%s :: %s — %v\n", project, filename, sec.heading, err)
coreerr.Error("brain-seed: import failed", "project", project, "file", filename, "heading", sec.heading, "err", err)
errors++
continue
}
@ -197,7 +197,7 @@ func main() {
}
if err := callBrainRemember(content, "plan", tags, project, 0.6); err != nil {
fmt.Printf(" FAIL %s :: %s / %s — %v\n", project, filename, sec.heading, err)
coreerr.Error("brain-seed: plan import failed", "project", project, "file", filename, "heading", sec.heading, "err", err)
errors++
continue
}
@ -237,7 +237,7 @@ func main() {
}
if err := callBrainRemember(content, "convention", tags, project, 0.9); err != nil {
fmt.Printf(" FAIL %s :: CLAUDE.md / %s — %v\n", project, sec.heading, err)
coreerr.Error("brain-seed: claude-md import failed", "project", project, "heading", sec.heading, "err", err)
errors++
continue
}
@ -291,7 +291,7 @@ func callBrainRemember(content, memType string, tags []string, project string, c
}
defer resp.Body.Close()
respBody, _ := io.ReadAll(resp.Body)
respBody, _ := goio.ReadAll(resp.Body)
if resp.StatusCode != 200 {
return coreerr.E("callBrainRemember", "HTTP "+string(respBody), nil)

4
pkg/mcp/agentic/prep.go
View file

@ -9,7 +9,7 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"io"
goio "io"
"net/http"
"os"
"os/exec"
@ -561,7 +561,7 @@ func (s *PrepSubsystem) generateContext(ctx context.Context, repo, wsDir string)
return 0
}
respData, _ := io.ReadAll(resp.Body)
respData, _ := goio.ReadAll(resp.Body)
var result struct {
Memories []map[string]any `json:"memories"`
}

6
pkg/mcp/brain/direct.go
View file

@ -7,7 +7,7 @@ import (
"context"
"encoding/json"
"fmt"
"io"
goio "io"
"net/http"
"os"
"strings"
@ -93,7 +93,7 @@ func (s *DirectSubsystem) apiCall(ctx context.Context, method, path string, body
return nil, coreerr.E("brain.apiCall", "no API key (set CORE_BRAIN_KEY or create ~/.claude/brain.key)", nil)
}
var reqBody io.Reader
var reqBody goio.Reader
if body != nil {
data, err := json.Marshal(body)
if err != nil {
@ -116,7 +116,7 @@ func (s *DirectSubsystem) apiCall(ctx context.Context, method, path string, body
}
defer resp.Body.Close()
respData, err := io.ReadAll(resp.Body)
respData, err := goio.ReadAll(resp.Body)
if err != nil {
return nil, coreerr.E("brain.apiCall", "read response", err)
}

4
pkg/mcp/bridge.go
View file

@ -5,7 +5,7 @@ package mcp
import (
"encoding/json"
"errors"
"io"
goio "io"
"net/http"
"github.com/gin-gonic/gin"
@ -41,7 +41,7 @@ func BridgeToAPI(svc *Service, bridge *api.ToolBridge) {
var body []byte
if c.Request.Body != nil {
var err error
body, err = io.ReadAll(io.LimitReader(c.Request.Body, maxBodySize))
body, err = goio.ReadAll(goio.LimitReader(c.Request.Body, maxBodySize))
if err != nil {
c.JSON(http.StatusBadRequest, api.Fail("invalid_request", "Failed to read request body"))
return

3
pkg/mcp/subsystem.go
View file

@ -38,6 +38,9 @@ type Notifier interface {
ChannelSend(ctx context.Context, channel string, data any)
}
// Compile-time assertion: *Service implements Notifier.
var _ Notifier = (*Service)(nil)
// SubsystemWithNotifier extends Subsystem for those that emit channel events.
// SetNotifier is called after New() before any tool calls.
//

2
pkg/mcp/tools_process_ci_test.go
View file

@ -1,3 +1,5 @@
//go:build ci
package mcp
import (

13
pkg/mcp/tools_process_test.go
View file

@ -288,3 +288,16 @@ func TestWithProcessService_Good(t *testing.T) {
t.Error("Expected processService to be nil when passed nil")
}
}
// TestRegisterProcessTools_Bad_NilService verifies that tools are not registered when process service is nil.
func TestRegisterProcessTools_Bad_NilService(t *testing.T) {
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
registered := s.registerProcessTools(s.server)
if registered {
t.Error("Expected registerProcessTools to return false when processService is nil")
}
}

148
pkg/mcp/tools_webview_test.go
View file

@ -450,3 +450,151 @@ func TestWebviewWaitOutput_Good(t *testing.T) {
t.Error("Expected message to be set")
}
}
// --- Handler tests beyond nil-guard ---
// setStubWebview injects a zero-value Webview stub so handler validation
// logic beyond the nil-guard can be exercised without a running Chrome.
// The previous value is restored via t.Cleanup.
func setStubWebview(t *testing.T) {
t.Helper()
webviewMu.Lock()
old := webviewInstance
webviewInstance = &webview.Webview{}
webviewMu.Unlock()
t.Cleanup(func() {
webviewMu.Lock()
webviewInstance = old
webviewMu.Unlock()
})
}
// TestWebviewDisconnect_Good_NoConnection verifies disconnect succeeds when not connected.
func TestWebviewDisconnect_Good_NoConnection(t *testing.T) {
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, out, err := s.webviewDisconnect(ctx, nil, WebviewDisconnectInput{})
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if !out.Success {
t.Error("Expected success to be true")
}
if out.Message != "No active connection" {
t.Errorf("Expected message 'No active connection', got %q", out.Message)
}
}
// TestWebviewConnect_Bad_EmptyURL verifies connect rejects an empty debug URL.
func TestWebviewConnect_Bad_EmptyURL(t *testing.T) {
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewConnect(ctx, nil, WebviewConnectInput{DebugURL: ""})
if err == nil {
t.Error("Expected error for empty debug URL, got nil")
}
}
// TestWebviewNavigate_Bad_EmptyURL verifies navigate rejects an empty URL.
func TestWebviewNavigate_Bad_EmptyURL(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewNavigate(ctx, nil, WebviewNavigateInput{URL: ""})
if err == nil {
t.Error("Expected error for empty URL, got nil")
}
}
// TestWebviewClick_Bad_EmptySelector verifies click rejects an empty selector.
func TestWebviewClick_Bad_EmptySelector(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewClick(ctx, nil, WebviewClickInput{Selector: ""})
if err == nil {
t.Error("Expected error for empty selector, got nil")
}
}
// TestWebviewType_Bad_EmptySelector verifies type rejects an empty selector.
func TestWebviewType_Bad_EmptySelector(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewType(ctx, nil, WebviewTypeInput{Selector: "", Text: "test"})
if err == nil {
t.Error("Expected error for empty selector, got nil")
}
}
// TestWebviewQuery_Bad_EmptySelector verifies query rejects an empty selector.
func TestWebviewQuery_Bad_EmptySelector(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewQuery(ctx, nil, WebviewQueryInput{Selector: ""})
if err == nil {
t.Error("Expected error for empty selector, got nil")
}
}
// TestWebviewEval_Bad_EmptyScript verifies eval rejects an empty script.
func TestWebviewEval_Bad_EmptyScript(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewEval(ctx, nil, WebviewEvalInput{Script: ""})
if err == nil {
t.Error("Expected error for empty script, got nil")
}
}
// TestWebviewWait_Bad_EmptySelector verifies wait rejects an empty selector.
func TestWebviewWait_Bad_EmptySelector(t *testing.T) {
setStubWebview(t)
s, err := New(Options{})
if err != nil {
t.Fatalf("Failed to create service: %v", err)
}
ctx := t.Context()
_, _, err = s.webviewWait(ctx, nil, WebviewWaitInput{Selector: ""})
if err == nil {
t.Error("Expected error for empty selector, got nil")
}
}

28
pkg/mcp/transport_e2e_test.go
View file

@ -2,6 +2,7 @@ package mcp
import (
"bufio"
"context"
"encoding/json"
"fmt"
"net"
@ -10,8 +11,6 @@ import (
"strings"
"testing"
"time"
"context"
)
// jsonRPCRequest builds a raw JSON-RPC 2.0 request string with newline delimiter.
@ -148,20 +147,20 @@ func TestTCPTransport_E2E_FullRoundTrip(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Step 1: Send initialize request
// Step 1: Send initialise request
initReq := jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
"clientInfo": map[string]any{"name": "TestClient", "version": "1.0.0"},
})
if _, err := conn.Write([]byte(initReq)); err != nil {
t.Fatalf("Failed to send initialize: %v", err)
t.Fatalf("Failed to send initialise: %v", err)
}
// Read initialize response
// Read initialise response
initResp := readJSONRPCResponse(t, scanner, conn)
if initResp["error"] != nil {
t.Fatalf("Initialize returned error: %v", initResp["error"])
t.Fatalf("Initialise returned error: %v", initResp["error"])
}
result, ok := initResp["result"].(map[string]any)
if !ok {
@ -291,7 +290,7 @@ func TestTCPTransport_E2E_FileWrite(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Initialize handshake
// Initialise handshake
conn.Write([]byte(jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
@ -379,7 +378,7 @@ func TestUnixTransport_E2E_FullRoundTrip(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Step 1: Initialize
// Step 1: Initialise
conn.Write([]byte(jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
@ -387,10 +386,10 @@ func TestUnixTransport_E2E_FullRoundTrip(t *testing.T) {
})))
initResp := readJSONRPCResponse(t, scanner, conn)
if initResp["error"] != nil {
t.Fatalf("Initialize returned error: %v", initResp["error"])
t.Fatalf("Initialise returned error: %v", initResp["error"])
}
// Step 2: Send initialized notification
// Step 2: Send initialised notification
conn.Write([]byte(jsonRPCNotification("notifications/initialized")))
// Step 3: tools/list
@ -488,7 +487,7 @@ func TestUnixTransport_E2E_DirList(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Initialize
// Initialise
conn.Write([]byte(jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
@ -610,7 +609,7 @@ func TestTCPTransport_E2E_ToolsDiscovery(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Initialize
// Initialise
conn.Write([]byte(jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
@ -686,7 +685,7 @@ func TestTCPTransport_E2E_ErrorHandling(t *testing.T) {
scanner := bufio.NewScanner(conn)
scanner.Buffer(make([]byte, 64*1024), 10*1024*1024)
// Initialize
// Initialise
conn.Write([]byte(jsonRPCRequest(1, "initialize", map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
@ -737,6 +736,3 @@ func TestTCPTransport_E2E_ErrorHandling(t *testing.T) {
cancel()
<-errCh
}
// Suppress "unused import" for fmt — used in helpers
var _ = fmt.Sprintf

8
pkg/mcp/transport_tcp.go
View file

@ -4,7 +4,7 @@ import (
"bufio"
"context"
"fmt"
"io"
goio "io"
"net"
"os"
"sync"
@ -23,7 +23,7 @@ var diagMu sync.Mutex
// diagWriter is the destination for warning and diagnostic messages.
// Use diagPrintf to write to it safely.
var diagWriter io.Writer = os.Stderr
var diagWriter goio.Writer = os.Stderr
// diagPrintf writes a formatted message to diagWriter under the mutex.
func diagPrintf(format string, args ...any) {
@ -34,7 +34,7 @@ func diagPrintf(format string, args ...any) {
// setDiagWriter swaps the diagnostic writer and returns the previous one.
// Used by tests to capture output without racing.
func setDiagWriter(w io.Writer) io.Writer {
func setDiagWriter(w goio.Writer) goio.Writer {
diagMu.Lock()
defer diagMu.Unlock()
old := diagWriter
@ -156,7 +156,7 @@ func (c *connConnection) Read(ctx context.Context) (jsonrpc.Message, error) {
return nil, err
}
// EOF - connection closed cleanly
return nil, io.EOF
return nil, goio.EOF
}
line := c.scanner.Bytes()
return jsonrpc.DecodeMessage(line)