From 301831368920e79e27832c39f4eb6588ffdb0d91 Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 2 Feb 2026 07:04:17 +0000 Subject: [PATCH] fix: address CodeRabbit review feedback for PR #181 - internal/cmd/dev/cmd_file_sync.go: Add EnsureDir error handling before Copy - internal/cmd/docs/cmd_sync.go: Add EnsureDir error handling for parent dirs - internal/cmd/sdk/generators/go.go: Use log.E() helper instead of fmt.Errorf - pkg/io/local/client.go: Handle Windows drive-root paths in path() - pkg/log/errors.go: Avoid leading colon when Op is empty, preserve Code in Wrap - pkg/log/errors_test.go: Rename tests to follow _Good/_Bad/_Ugly suffix pattern - pkg/mcp/transport_tcp.go: Fix ctx cancellation, increase scanner buffer, use io.EOF Co-Authored-By: Claude Opus 4.5 --- internal/cmd/dev/cmd_file_sync.go | 6 +++- internal/cmd/docs/cmd_sync.go | 5 ++- internal/cmd/sdk/generators/go.go | 5 +-- pkg/io/local/client.go | 4 +++ pkg/log/errors.go | 24 ++++++++++---- pkg/log/errors_test.go | 53 ++++++++++++++++++++++++------- pkg/mcp/transport_tcp.go | 20 +++++++++--- 7 files changed, 91 insertions(+), 26 deletions(-) diff --git a/internal/cmd/dev/cmd_file_sync.go b/internal/cmd/dev/cmd_file_sync.go index 4886683a..837e1298 100644 --- a/internal/cmd/dev/cmd_file_sync.go +++ b/internal/cmd/dev/cmd_file_sync.go @@ -131,7 +131,11 @@ func runFileSync(source string) error { } } else { // Ensure dir exists - coreio.Local.EnsureDir(filepath.Dir(destPath)) + if err := coreio.Local.EnsureDir(filepath.Dir(destPath)); err != nil { + cli.Print(" %s %s: copy failed: %s\n", errorStyle.Render("x"), repoName, err) + failed++ + continue + } if err := coreio.Copy(coreio.Local, source, coreio.Local, destPath); err != nil { cli.Print(" %s %s: copy failed: %s\n", errorStyle.Render("x"), repoName, err) failed++ diff --git a/internal/cmd/docs/cmd_sync.go b/internal/cmd/docs/cmd_sync.go index a1611056..d7799ac7 100644 --- a/internal/cmd/docs/cmd_sync.go +++ b/internal/cmd/docs/cmd_sync.go @@ -140,7 +140,10 @@ func runDocsSync(registryPath string, outputDir string, dryRun bool) error { src := filepath.Join(docsDir, f) dst := filepath.Join(repoOutDir, f) // Ensure parent dir - io.Local.EnsureDir(filepath.Dir(dst)) + if err := io.Local.EnsureDir(filepath.Dir(dst)); err != nil { + cli.Print(" %s %s: %s\n", errorStyle.Render("✗"), f, err) + continue + } if err := io.Copy(io.Local, src, io.Local, dst); err != nil { cli.Print(" %s %s: %s\n", errorStyle.Render("✗"), f, err) diff --git a/internal/cmd/sdk/generators/go.go b/internal/cmd/sdk/generators/go.go index 0aff5279..b7902900 100644 --- a/internal/cmd/sdk/generators/go.go +++ b/internal/cmd/sdk/generators/go.go @@ -8,6 +8,7 @@ import ( "path/filepath" coreio "github.com/host-uk/core/pkg/io" + "github.com/host-uk/core/pkg/log" ) // GoGenerator generates Go SDKs from OpenAPI specs. @@ -37,7 +38,7 @@ func (g *GoGenerator) Install() string { // Generate creates SDK from OpenAPI spec. func (g *GoGenerator) Generate(ctx context.Context, opts Options) error { if err := coreio.Local.EnsureDir(opts.OutputDir); err != nil { - return fmt.Errorf("go.Generate: failed to create output dir: %w", err) + return log.E("go.Generate", "failed to create output dir", err) } if g.Available() { @@ -59,7 +60,7 @@ func (g *GoGenerator) generateNative(ctx context.Context, opts Options) error { cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { - return fmt.Errorf("go.generateNative: %w", err) + return log.E("go.generateNative", "oapi-codegen failed", err) } goMod := fmt.Sprintf("module %s\n\ngo 1.21\n", opts.PackageName) diff --git a/pkg/io/local/client.go b/pkg/io/local/client.go index f17a4da5..4e232fef 100644 --- a/pkg/io/local/client.go +++ b/pkg/io/local/client.go @@ -31,6 +31,10 @@ func (m *Medium) path(p string) string { } clean := strings.ReplaceAll(p, "..", ".") if filepath.IsAbs(clean) { + // Handle Windows drive root (e.g. "C:\") + if len(clean) == 3 && clean[1] == ':' && (clean[2] == '\\' || clean[2] == '/') { + return clean + } return filepath.Clean(clean) } return filepath.Join(m.root, clean) diff --git a/pkg/log/errors.go b/pkg/log/errors.go index 838436f2..087160dc 100644 --- a/pkg/log/errors.go +++ b/pkg/log/errors.go @@ -21,16 +21,20 @@ type Err struct { // Error implements the error interface. func (e *Err) Error() string { + var prefix string + if e.Op != "" { + prefix = e.Op + ": " + } if e.Err != nil { if e.Code != "" { - return fmt.Sprintf("%s: %s [%s]: %v", e.Op, e.Msg, e.Code, e.Err) + return fmt.Sprintf("%s%s [%s]: %v", prefix, e.Msg, e.Code, e.Err) } - return fmt.Sprintf("%s: %s: %v", e.Op, e.Msg, e.Err) + return fmt.Sprintf("%s%s: %v", prefix, e.Msg, e.Err) } if e.Code != "" { - return fmt.Sprintf("%s: %s [%s]", e.Op, e.Msg, e.Code) + return fmt.Sprintf("%s%s [%s]", prefix, e.Msg, e.Code) } - return fmt.Sprintf("%s: %s", e.Op, e.Msg) + return fmt.Sprintf("%s%s", prefix, e.Msg) } // Unwrap returns the underlying error for use with errors.Is and errors.As. @@ -54,13 +58,21 @@ func E(op, msg string, err error) error { } // Wrap wraps an error with operation context. -// Alias for E() for semantic clarity when wrapping existing errors. +// Preserves error Code if the wrapped error is an *Err. // // Example: // // return log.Wrap(err, "db.Query", "database query failed") func Wrap(err error, op, msg string) error { - return E(op, msg, err) + if err == nil { + return nil + } + // Preserve Code from wrapped *Err + var logErr *Err + if As(err, &logErr) && logErr.Code != "" { + return &Err{Op: op, Msg: msg, Err: err, Code: logErr.Code} + } + return &Err{Op: op, Msg: msg, Err: err} } // WrapCode wraps an error with operation context and error code. diff --git a/pkg/log/errors_test.go b/pkg/log/errors_test.go index 99640549..7be67c66 100644 --- a/pkg/log/errors_test.go +++ b/pkg/log/errors_test.go @@ -29,6 +29,20 @@ func TestErr_Error_Good(t *testing.T) { assert.Equal(t, "cache.Get: miss", err.Error()) } +func TestErr_Error_EmptyOp_Good(t *testing.T) { + // No Op - should not have leading colon + err := &Err{Msg: "just a message"} + assert.Equal(t, "just a message", err.Error()) + + // No Op with code + err = &Err{Msg: "error with code", Code: "ERR_CODE"} + assert.Equal(t, "error with code [ERR_CODE]", err.Error()) + + // No Op with underlying error + err = &Err{Msg: "wrapped", Err: errors.New("underlying")} + assert.Equal(t, "wrapped: underlying", err.Error()) +} + func TestErr_Unwrap_Good(t *testing.T) { underlying := errors.New("underlying error") err := &Err{Op: "test", Msg: "wrapped", Err: underlying} @@ -51,7 +65,7 @@ func TestE_Good(t *testing.T) { assert.Equal(t, underlying, logErr.Err) } -func TestE_Good_NilError(t *testing.T) { +func TestE_NilError_Good(t *testing.T) { // Should return nil when wrapping nil err := E("op.Name", "message", nil) assert.Nil(t, err) @@ -67,6 +81,23 @@ func TestWrap_Good(t *testing.T) { assert.True(t, errors.Is(err, underlying)) } +func TestWrap_PreservesCode_Good(t *testing.T) { + // Create an error with a code + inner := WrapCode(errors.New("base"), "VALIDATION_ERROR", "inner.Op", "validation failed") + + // Wrap it - should preserve the code + outer := Wrap(inner, "outer.Op", "outer context") + + assert.NotNil(t, outer) + assert.Equal(t, "VALIDATION_ERROR", ErrCode(outer)) + assert.Contains(t, outer.Error(), "[VALIDATION_ERROR]") +} + +func TestWrap_NilError_Good(t *testing.T) { + err := Wrap(nil, "op", "msg") + assert.Nil(t, err) +} + func TestWrapCode_Good(t *testing.T) { underlying := errors.New("validation failed") err := WrapCode(underlying, "INVALID_INPUT", "api.Validate", "bad request") @@ -79,7 +110,7 @@ func TestWrapCode_Good(t *testing.T) { assert.Contains(t, err.Error(), "[INVALID_INPUT]") } -func TestWrapCode_Good_NilError(t *testing.T) { +func TestWrapCode_NilError_Good(t *testing.T) { err := WrapCode(nil, "CODE", "op", "msg") assert.Nil(t, err) } @@ -134,7 +165,7 @@ func TestOp_Good(t *testing.T) { assert.Equal(t, "mypackage.MyFunc", Op(err)) } -func TestOp_Good_NotLogError(t *testing.T) { +func TestOp_PlainError_Good(t *testing.T) { err := errors.New("plain error") assert.Equal(t, "", Op(err)) } @@ -144,7 +175,7 @@ func TestErrCode_Good(t *testing.T) { assert.Equal(t, "ERR_CODE", ErrCode(err)) } -func TestErrCode_Good_NoCode(t *testing.T) { +func TestErrCode_NoCode_Good(t *testing.T) { err := E("op", "msg", errors.New("base")) assert.Equal(t, "", ErrCode(err)) } @@ -154,12 +185,12 @@ func TestMessage_Good(t *testing.T) { assert.Equal(t, "the message", Message(err)) } -func TestMessage_Good_PlainError(t *testing.T) { +func TestMessage_PlainError_Good(t *testing.T) { err := errors.New("plain message") assert.Equal(t, "plain message", Message(err)) } -func TestMessage_Good_Nil(t *testing.T) { +func TestMessage_Nil_Good(t *testing.T) { assert.Equal(t, "", Message(nil)) } @@ -171,12 +202,12 @@ func TestRoot_Good(t *testing.T) { assert.Equal(t, root, Root(level2)) } -func TestRoot_Good_SingleError(t *testing.T) { +func TestRoot_SingleError_Good(t *testing.T) { err := errors.New("single") assert.Equal(t, err, Root(err)) } -func TestRoot_Good_Nil(t *testing.T) { +func TestRoot_Nil_Good(t *testing.T) { assert.Nil(t, Root(nil)) } @@ -205,7 +236,7 @@ func TestLogError_Good(t *testing.T) { assert.Contains(t, output, "op=db.Connect") } -func TestLogError_Good_NilError(t *testing.T) { +func TestLogError_NilError_Good(t *testing.T) { var buf bytes.Buffer logger := New(Options{Level: LevelDebug, Output: &buf}) SetDefault(logger) @@ -233,7 +264,7 @@ func TestLogWarn_Good(t *testing.T) { assert.Contains(t, output, "falling back to db") } -func TestLogWarn_Good_NilError(t *testing.T) { +func TestLogWarn_NilError_Good(t *testing.T) { var buf bytes.Buffer logger := New(Options{Level: LevelDebug, Output: &buf}) SetDefault(logger) @@ -244,7 +275,7 @@ func TestLogWarn_Good_NilError(t *testing.T) { assert.Empty(t, buf.String()) } -func TestMust_Good_NoError(t *testing.T) { +func TestMust_NoError_Good(t *testing.T) { // Should not panic when error is nil assert.NotPanics(t, func() { Must(nil, "test", "should not panic") diff --git a/pkg/mcp/transport_tcp.go b/pkg/mcp/transport_tcp.go index f7b5f1e5..0e6e0f7e 100644 --- a/pkg/mcp/transport_tcp.go +++ b/pkg/mcp/transport_tcp.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "fmt" + "io" "net" "os" @@ -11,6 +12,9 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) +// maxMCPMessageSize is the maximum size for MCP JSON-RPC messages (10 MB). +const maxMCPMessageSize = 10 * 1024 * 1024 + // TCPTransport manages a TCP listener for MCP. type TCPTransport struct { addr string @@ -36,6 +40,12 @@ func (s *Service) ServeTCP(ctx context.Context, addr string) error { } defer t.listener.Close() + // Close listener when context is cancelled to unblock Accept + go func() { + <-ctx.Done() + t.listener.Close() + }() + if addr == "" { addr = t.listener.Addr().String() } @@ -84,9 +94,11 @@ type connTransport struct { } func (t *connTransport) Connect(ctx context.Context) (mcp.Connection, error) { + scanner := bufio.NewScanner(t.conn) + scanner.Buffer(make([]byte, 64*1024), maxMCPMessageSize) return &connConnection{ conn: t.conn, - scanner: bufio.NewScanner(t.conn), + scanner: scanner, }, nil } @@ -102,10 +114,8 @@ func (c *connConnection) Read(ctx context.Context) (jsonrpc.Message, error) { if err := c.scanner.Err(); err != nil { return nil, err } - // EOF - // Return error to signal closure, as per Scanner contract? - // SDK usually expects error on close. - return nil, fmt.Errorf("EOF") + // EOF - connection closed cleanly + return nil, io.EOF } line := c.scanner.Bytes() return jsonrpc.DecodeMessage(line)