fix(mcp): resolve 17 code review findings + php route('login') fix
Some checks failed
CI / test (push) Failing after 2s

Go (17 findings from Codex sweep):
- CRITICAL: bare type assertion panic in tools_metrics.go
- CRITICAL: webviewInstance data race — added webviewMu mutex
- CRITICAL: swallowed Glob error in agentic/ingest.go
- HIGH: 9 nil-pointer dereferences on resp.StatusCode across agentic/
- HIGH: swallowed os.Open/os.Create errors in dispatch, queue, resume
- HIGH: hardcoded ~/Code/host-uk/core paths replaced with s.workspaceRoot()
- HIGH: wsServer/wsAddr data race — added wsMu to Service struct
- MEDIUM: ChannelSend stdout guard for non-stdio transports
- MEDIUM: readPlan discarded underlying error
- MEDIUM: TCP session IDs now unique per connection (RemoteAddr)
- LOW: SPDX typo in brain/provider.go

PHP (route('login') fix):
- Replace route('login') with url('/login') in 4 MCP files
- route('login') fails on mcp.* subdomains where no login route exists
- url('/login') resolves correctly on any domain

Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
Snider 2026-03-21 23:13:03 +00:00
parent 8e84a06b82
commit 907d62a545
21 changed files with 133 additions and 38 deletions

View file

@ -174,7 +174,13 @@ func (s *PrepSubsystem) dispatch(ctx context.Context, req *mcp.CallToolRequest,
// - Stdin from /dev/null
// - TERM=dumb prevents terminal control sequences
// - NO_COLOR=1 disables colour output
devNull, _ := os.Open(os.DevNull)
devNull, err := os.Open(os.DevNull)
if err != nil {
outFile.Close()
return nil, DispatchOutput{}, coreerr.E("dispatch", "failed to open /dev/null", err)
}
defer devNull.Close()
cmd := exec.Command(command, args...)
cmd.Dir = srcDir
cmd.Stdin = devNull

View file

@ -196,10 +196,13 @@ func (s *PrepSubsystem) resolveLabelIDs(ctx context.Context, org, repo string, n
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return nil
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return nil
}
var existing []struct {
ID int64 `json:"id"`
@ -252,10 +255,13 @@ func (s *PrepSubsystem) createLabel(ctx context.Context, org, repo, name string)
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 201 {
if err != nil {
return 0
}
defer resp.Body.Close()
if resp.StatusCode != 201 {
return 0
}
var result struct {
ID int64 `json:"id"`

View file

@ -23,7 +23,10 @@ func (s *PrepSubsystem) ingestFindings(wsDir string) {
}
// Read the log file
logFiles, _ := filepath.Glob(filepath.Join(wsDir, "agent-*.log"))
logFiles, err := filepath.Glob(filepath.Join(wsDir, "agent-*.log"))
if err != nil {
return
}
if len(logFiles) == 0 {
return
}

View file

@ -7,7 +7,6 @@ import (
"crypto/rand"
"encoding/hex"
"encoding/json"
"os"
"path/filepath"
"strings"
"time"
@ -313,8 +312,7 @@ func (s *PrepSubsystem) planList(_ context.Context, _ *mcp.CallToolRequest, inpu
// --- Helpers ---
func (s *PrepSubsystem) plansDir() string {
home, _ := os.UserHomeDir()
return filepath.Join(home, "Code", "host-uk", "core", ".core", "plans")
return filepath.Join(s.codePath, "host-uk", "core", ".core", "plans")
}
func planPath(dir, id string) string {
@ -354,7 +352,7 @@ func generatePlanID(title string) string {
func readPlan(dir, id string) (*Plan, error) {
data, err := coreio.Local.Read(planPath(dir, id))
if err != nil {
return nil, coreerr.E("readPlan", "plan not found: "+id, nil)
return nil, coreerr.E("readPlan", "plan not found: "+id, err)
}
var plan Plan

View file

@ -8,7 +8,6 @@ import (
"encoding/json"
"fmt"
"net/http"
"os"
"os/exec"
"path/filepath"
"strings"
@ -55,8 +54,7 @@ func (s *PrepSubsystem) createPR(ctx context.Context, _ *mcp.CallToolRequest, in
return nil, CreatePROutput{}, coreerr.E("createPR", "no Forge token configured", nil)
}
home, _ := os.UserHomeDir()
wsDir := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace", input.Workspace)
wsDir := filepath.Join(s.workspaceRoot(), input.Workspace)
srcDir := filepath.Join(wsDir, "src")
if _, err := coreio.Local.List(srcDir); err != nil {
@ -310,10 +308,13 @@ func (s *PrepSubsystem) listRepoPRs(ctx context.Context, org, repo, state string
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return nil, coreerr.E("listRepoPRs", "failed to list PRs for "+repo, err)
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return nil, coreerr.E("listRepoPRs", fmt.Sprintf("HTTP %d for "+repo, resp.StatusCode), nil)
}
var prs []struct {
Number int `json:"number"`

View file

@ -96,6 +96,11 @@ func (s *PrepSubsystem) RegisterTools(server *mcp.Server) {
// Shutdown implements mcp.SubsystemWithShutdown.
func (s *PrepSubsystem) Shutdown(_ context.Context) error { return nil }
// workspaceRoot returns the base directory for agent workspaces.
func (s *PrepSubsystem) workspaceRoot() string {
return filepath.Join(s.codePath, "host-uk", "core", ".core", "workspace")
}
// --- Input/Output types ---
// PrepInput is the input for agentic_prep_workspace.
@ -134,8 +139,7 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques
}
// Workspace root: .core/workspace/{repo}-{timestamp}/
home, _ := os.UserHomeDir()
wsRoot := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace")
wsRoot := s.workspaceRoot()
wsName := fmt.Sprintf("%s-%d", input.Repo, time.Now().Unix())
wsDir := filepath.Join(wsRoot, wsName)
@ -395,10 +399,13 @@ func (s *PrepSubsystem) pullWiki(ctx context.Context, org, repo, wsDir string) i
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return 0
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return 0
}
var pages []struct {
Title string `json:"title"`
@ -418,7 +425,11 @@ func (s *PrepSubsystem) pullWiki(ctx context.Context, org, repo, wsDir string) i
pageReq.Header.Set("Authorization", "token "+s.forgeToken)
pageResp, err := s.client.Do(pageReq)
if err != nil || pageResp.StatusCode != 200 {
if err != nil {
continue
}
if pageResp.StatusCode != 200 {
pageResp.Body.Close()
continue
}
@ -480,10 +491,13 @@ func (s *PrepSubsystem) generateContext(ctx context.Context, repo, wsDir string)
req.Header.Set("Authorization", "Bearer "+s.brainKey)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return 0
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return 0
}
respData, _ := io.ReadAll(resp.Body)
var result struct {
@ -573,10 +587,13 @@ func (s *PrepSubsystem) generateTodo(ctx context.Context, org, repo string, issu
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return
}
var issueData struct {
Title string `json:"title"`

View file

@ -105,8 +105,7 @@ func (s *PrepSubsystem) delayForAgent(agent string) time.Duration {
// countRunningByAgent counts running workspaces for a specific agent type.
func (s *PrepSubsystem) countRunningByAgent(agent string) int {
home, _ := os.UserHomeDir()
wsRoot := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace")
wsRoot := s.workspaceRoot()
entries, err := coreio.Local.List(wsRoot)
if err != nil {
@ -164,8 +163,7 @@ func (s *PrepSubsystem) canDispatch() bool {
// drainQueue finds the oldest queued workspace and spawns it if a slot is available.
// Applies rate-based delay between spawns.
func (s *PrepSubsystem) drainQueue() {
home, _ := os.UserHomeDir()
wsRoot := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace")
wsRoot := s.workspaceRoot()
entries, err := coreio.Local.List(wsRoot)
if err != nil {
@ -212,7 +210,12 @@ func (s *PrepSubsystem) drainQueue() {
continue
}
devNull, _ := os.Open(os.DevNull)
devNull, err := os.Open(os.DevNull)
if err != nil {
outFile.Close()
continue
}
cmd := exec.Command(command, args...)
cmd.Dir = srcDir
cmd.Stdin = devNull
@ -223,8 +226,10 @@ func (s *PrepSubsystem) drainQueue() {
if err := cmd.Start(); err != nil {
outFile.Close()
devNull.Close()
continue
}
devNull.Close()
st.Status = "running"
st.PID = cmd.Process.Pid

View file

@ -45,8 +45,7 @@ func (s *PrepSubsystem) resume(ctx context.Context, _ *mcp.CallToolRequest, inpu
return nil, ResumeOutput{}, coreerr.E("resume", "workspace is required", nil)
}
home, _ := os.UserHomeDir()
wsDir := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace", input.Workspace)
wsDir := filepath.Join(s.workspaceRoot(), input.Workspace)
srcDir := filepath.Join(wsDir, "src")
// Verify workspace exists
@ -103,8 +102,17 @@ func (s *PrepSubsystem) resume(ctx context.Context, _ *mcp.CallToolRequest, inpu
return nil, ResumeOutput{}, err
}
devNull, _ := os.Open(os.DevNull)
outFile, _ := os.Create(outputFile)
devNull, err := os.Open(os.DevNull)
if err != nil {
return nil, ResumeOutput{}, coreerr.E("resume", "failed to open /dev/null", err)
}
defer devNull.Close()
outFile, err := os.Create(outputFile)
if err != nil {
return nil, ResumeOutput{}, coreerr.E("resume", "failed to create log file", err)
}
cmd := exec.Command(command, args...)
cmd.Dir = srcDir
cmd.Stdin = devNull

View file

@ -105,10 +105,13 @@ func (s *PrepSubsystem) listOrgRepos(ctx context.Context, org string) ([]string,
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return nil, coreerr.E("listOrgRepos", "failed to list repos", err)
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return nil, coreerr.E("listOrgRepos", fmt.Sprintf("HTTP %d listing repos", resp.StatusCode), nil)
}
var repos []struct {
Name string `json:"name"`
@ -129,10 +132,13 @@ func (s *PrepSubsystem) listRepoIssues(ctx context.Context, org, repo, label str
req.Header.Set("Authorization", "token "+s.forgeToken)
resp, err := s.client.Do(req)
if err != nil || resp.StatusCode != 200 {
if err != nil {
return nil, coreerr.E("listRepoIssues", "failed to list issues for "+repo, err)
}
defer resp.Body.Close()
if resp.StatusCode != 200 {
return nil, coreerr.E("listRepoIssues", fmt.Sprintf("HTTP %d for "+repo, resp.StatusCode), nil)
}
var issues []struct {
Number int `json:"number"`

View file

@ -95,8 +95,7 @@ func (s *PrepSubsystem) registerStatusTool(server *mcp.Server) {
}
func (s *PrepSubsystem) status(ctx context.Context, _ *mcp.CallToolRequest, input StatusInput) (*mcp.CallToolResult, StatusOutput, error) {
home, _ := os.UserHomeDir()
wsRoot := filepath.Join(home, "Code", "host-uk", "core", ".core", "workspace")
wsRoot := s.workspaceRoot()
entries, err := coreio.Local.List(wsRoot)
if err != nil {

View file

@ -1,4 +1,4 @@
// SPDX-Licence-Identifier: EUPL-1.2
// SPDX-License-Identifier: EUPL-1.2
package brain

View file

@ -12,6 +12,7 @@ import (
"path/filepath"
"slices"
"strings"
"sync"
"forge.lthn.ai/core/go-io"
"forge.lthn.ai/core/go-log"
@ -35,6 +36,8 @@ type Service struct {
wsHub *ws.Hub // WebSocket hub for real-time streaming (optional)
wsServer *http.Server // WebSocket HTTP server (optional)
wsAddr string // WebSocket server address
wsMu sync.Mutex // Protects wsServer and wsAddr
stdioMode bool // True when running via stdio transport
tools []ToolRecord // Parallel tool registry for REST bridge
}
@ -661,6 +664,7 @@ func (s *Service) Run(ctx context.Context) error {
if addr := os.Getenv("MCP_ADDR"); addr != "" {
return s.ServeTCP(ctx, addr)
}
s.stdioMode = true
return s.server.Run(ctx, &mcp.StdioTransport{})
}

View file

@ -82,6 +82,10 @@ func (s *Service) ChannelSend(ctx context.Context, channel string, data any) {
// Write directly to stdout (stdio transport) with newline delimiter.
// The official SDK doesn't expose a way to send custom notification methods,
// so we write the JSON-RPC notification directly to the transport.
// Only write when running in stdio mode — HTTP/TCP transports don't use stdout.
if !s.stdioMode {
return
}
stdoutMu.Lock()
os.Stdout.Write(append(msg, '\n'))
stdoutMu.Unlock()

View file

@ -149,8 +149,9 @@ func (s *Service) metricsQuery(ctx context.Context, req *mcp.CallToolRequest, in
summary := ai.Summary(events)
// Build output
total, _ := summary["total"].(int)
output := MetricsQueryOutput{
Total: summary["total"].(int),
Total: total,
ByType: convertMetricCounts(summary["by_type"]),
ByRepo: convertMetricCounts(summary["by_repo"]),
ByAgent: convertMetricCounts(summary["by_agent"]),

View file

@ -4,6 +4,7 @@ import (
"context"
"encoding/base64"
"fmt"
"sync"
"time"
"forge.lthn.ai/core/go-log"
@ -11,6 +12,9 @@ import (
"github.com/modelcontextprotocol/go-sdk/mcp"
)
// webviewMu protects webviewInstance from concurrent access.
var webviewMu sync.Mutex
// webviewInstance holds the current webview connection.
// This is managed by the MCP service.
var webviewInstance *webview.Webview
@ -250,6 +254,9 @@ func (s *Service) registerWebviewTools(server *mcp.Server) {
// webviewConnect handles the webview_connect tool call.
func (s *Service) webviewConnect(ctx context.Context, req *mcp.CallToolRequest, input WebviewConnectInput) (*mcp.CallToolResult, WebviewConnectOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Security("MCP tool execution", "tool", "webview_connect", "debug_url", input.DebugURL, "user", log.Username())
if input.DebugURL == "" {
@ -288,6 +295,9 @@ func (s *Service) webviewConnect(ctx context.Context, req *mcp.CallToolRequest,
// webviewDisconnect handles the webview_disconnect tool call.
func (s *Service) webviewDisconnect(ctx context.Context, req *mcp.CallToolRequest, input WebviewDisconnectInput) (*mcp.CallToolResult, WebviewDisconnectOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_disconnect", "user", log.Username())
if webviewInstance == nil {
@ -312,6 +322,9 @@ func (s *Service) webviewDisconnect(ctx context.Context, req *mcp.CallToolReques
// webviewNavigate handles the webview_navigate tool call.
func (s *Service) webviewNavigate(ctx context.Context, req *mcp.CallToolRequest, input WebviewNavigateInput) (*mcp.CallToolResult, WebviewNavigateOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_navigate", "url", input.URL, "user", log.Username())
if webviewInstance == nil {
@ -335,6 +348,9 @@ func (s *Service) webviewNavigate(ctx context.Context, req *mcp.CallToolRequest,
// webviewClick handles the webview_click tool call.
func (s *Service) webviewClick(ctx context.Context, req *mcp.CallToolRequest, input WebviewClickInput) (*mcp.CallToolResult, WebviewClickOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_click", "selector", input.Selector, "user", log.Username())
if webviewInstance == nil {
@ -355,6 +371,9 @@ func (s *Service) webviewClick(ctx context.Context, req *mcp.CallToolRequest, in
// webviewType handles the webview_type tool call.
func (s *Service) webviewType(ctx context.Context, req *mcp.CallToolRequest, input WebviewTypeInput) (*mcp.CallToolResult, WebviewTypeOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_type", "selector", input.Selector, "user", log.Username())
if webviewInstance == nil {
@ -375,6 +394,9 @@ func (s *Service) webviewType(ctx context.Context, req *mcp.CallToolRequest, inp
// webviewQuery handles the webview_query tool call.
func (s *Service) webviewQuery(ctx context.Context, req *mcp.CallToolRequest, input WebviewQueryInput) (*mcp.CallToolResult, WebviewQueryOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_query", "selector", input.Selector, "all", input.All, "user", log.Username())
if webviewInstance == nil {
@ -433,6 +455,9 @@ func (s *Service) webviewQuery(ctx context.Context, req *mcp.CallToolRequest, in
// webviewConsole handles the webview_console tool call.
func (s *Service) webviewConsole(ctx context.Context, req *mcp.CallToolRequest, input WebviewConsoleInput) (*mcp.CallToolResult, WebviewConsoleOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_console", "clear", input.Clear, "user", log.Username())
if webviewInstance == nil {
@ -465,6 +490,9 @@ func (s *Service) webviewConsole(ctx context.Context, req *mcp.CallToolRequest,
// webviewEval handles the webview_eval tool call.
func (s *Service) webviewEval(ctx context.Context, req *mcp.CallToolRequest, input WebviewEvalInput) (*mcp.CallToolResult, WebviewEvalOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Security("MCP tool execution", "tool", "webview_eval", "user", log.Username())
if webviewInstance == nil {
@ -492,6 +520,9 @@ func (s *Service) webviewEval(ctx context.Context, req *mcp.CallToolRequest, inp
// webviewScreenshot handles the webview_screenshot tool call.
func (s *Service) webviewScreenshot(ctx context.Context, req *mcp.CallToolRequest, input WebviewScreenshotInput) (*mcp.CallToolResult, WebviewScreenshotOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_screenshot", "format", input.Format, "user", log.Username())
if webviewInstance == nil {
@ -518,6 +549,9 @@ func (s *Service) webviewScreenshot(ctx context.Context, req *mcp.CallToolReques
// webviewWait handles the webview_wait tool call.
func (s *Service) webviewWait(ctx context.Context, req *mcp.CallToolRequest, input WebviewWaitInput) (*mcp.CallToolResult, WebviewWaitOutput, error) {
webviewMu.Lock()
defer webviewMu.Unlock()
s.logger.Info("MCP tool execution", "tool", "webview_wait", "selector", input.Selector, "timeout", input.Timeout, "user", log.Username())
if webviewInstance == nil {

View file

@ -69,6 +69,9 @@ func (s *Service) wsStart(ctx context.Context, req *mcp.CallToolRequest, input W
s.logger.Security("MCP tool execution", "tool", "ws_start", "addr", addr, "user", log.Username())
s.wsMu.Lock()
defer s.wsMu.Unlock()
// Check if server is already running
if s.wsServer != nil {
return nil, WSStartOutput{

View file

@ -178,5 +178,5 @@ func (c *connConnection) Close() error {
}
func (c *connConnection) SessionID() string {
return "tcp-session" // Unique ID might be better, but optional
return "tcp-" + c.conn.RemoteAddr().String()
}

View file

@ -50,7 +50,7 @@
{{ $workspace->name }}
</span>
@else
<a href="{{ route('login') }}" class="text-sm text-cyan-600 hover:text-cyan-700 dark:text-cyan-400 dark:hover:text-cyan-300">
<a href="{{ url('/login') }}" class="text-sm text-cyan-600 hover:text-cyan-700 dark:text-cyan-400 dark:hover:text-cyan-300">
Sign in
</a>
@endif

View file

@ -113,6 +113,6 @@ class McpAuthenticate
], 401);
}
return redirect()->guest(route('login'));
return redirect()->guest(url('/login'));
}
}

View file

@ -85,7 +85,7 @@
@elseif(!$isAuthenticated && !$apiKey)
<div class="p-3 bg-amber-50 dark:bg-amber-900/20 border border-amber-200 dark:border-amber-800 rounded-lg">
<p class="text-sm text-amber-700 dark:text-amber-300">
<a href="{{ route('login') }}" class="underline hover:no-underline">{{ __('mcp::mcp.playground.auth.sign_in_prompt') }}</a>
<a href="{{ url('/login') }}" class="underline hover:no-underline">{{ __('mcp::mcp.playground.auth.sign_in_prompt') }}</a>
{{ __('mcp::mcp.playground.auth.sign_in_description') }}
</p>
</div>

View file

@ -85,7 +85,7 @@
@elseif(!$isAuthenticated && !$apiKey)
<div class="p-3 bg-amber-50 dark:bg-amber-900/20 border border-amber-200 dark:border-amber-800 rounded-lg">
<p class="text-sm text-amber-700 dark:text-amber-300">
<a href="{{ route('login') }}" class="underline hover:no-underline">Sign in</a>
<a href="{{ url('/login') }}" class="underline hover:no-underline">Sign in</a>
to create API keys, or paste an existing key above.
</p>
</div>