diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index 197d49c24a..4452818f61 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -40,6 +40,10 @@ Do NOT invoke for: Writing new code (golang-code-writer), docs-only changes (doc - [ ] Follows conventions in `.claude/rules/testing.md` - [ ] Both success and failure paths tested +### vMCP Code (for `pkg/vmcp/` and `cmd/vmcp/`) + +When reviewing changes that touch vMCP code, also run the `/vmcp-review` skill to check for vMCP-specific anti-patterns in addition to the standard review checklist above. + ### Backwards Compatibility - [ ] Changes won't break existing users - [ ] API/CLI changes maintain compatibility or include deprecation warnings diff --git a/.claude/rules/vmcp-anti-patterns.md b/.claude/rules/vmcp-anti-patterns.md new file mode 100644 index 0000000000..1281929c7a --- /dev/null +++ b/.claude/rules/vmcp-anti-patterns.md @@ -0,0 +1,65 @@ +--- +paths: + - "pkg/vmcp/**/*.go" + - "cmd/vmcp/**/*.go" +--- + +# vMCP Anti-Pattern Rule + +When reviewing or writing code in `pkg/vmcp/` or `cmd/vmcp/`, check changes against these anti-patterns. Flag any code that introduces or expands them. + +## 1. Context Variable Coupling + +Using `context.WithValue`/`ctx.Value` to pass domain data between middleware or from middleware to handlers. Creates invisible producer-consumer dependencies, ordering fragility, and silent degradation when values are missing. + +**Detect**: `context.WithValue` in middleware setting domain data; `ctx.Value(someKey)` reads in handlers/routers/business logic; functions whose behavior depends on specific context values. + +**Instead**: Push data onto `MultiSession` (handlers already have access); pass domain data as explicit function parameters; reserve context for trace IDs, cancellation, and deadlines only. + +## 2. Repeated Request Body Read/Restore + +Multiple middleware calling `io.ReadAll(r.Body)` then restoring with `io.NopCloser(bytes.NewReader(...))`. Fragile implicit contract — if any middleware forgets to restore, downstream handlers silently get an empty body. + +**Detect**: `io.ReadAll(r.Body)` followed by `r.Body = io.NopCloser(bytes.NewReader(...))` in middleware; multiple middleware in the same chain parsing JSON from the request body. + +**Instead**: Parse body once early in the pipeline; extend `ParsedMCPRequest` so all downstream consumers use the parsed representation; cache raw bytes alongside parsed form if needed for audit. + +## 3. God Object: Server Struct + +A single struct owning too many concerns (10+ fields spanning domains). Causes cognitive overload, makes subsystems untestable in isolation, and amplifies change risk. + +**Detect**: Structs with 10+ fields spanning different domains; constructors >50 lines or with `nolint:gocyclo`; files >500 lines handling multiple unrelated concerns; multiple mutex fields protecting different state subsets. + +**Instead**: Extract each concern into a self-contained module with its own `New()`/`Start()`/`Stop()`. Server struct should be a thin orchestrator composing pre-built subsystems. + +## 4. Middleware Overuse + +Business logic in HTTP middleware when behavior is specific to certain request types or belongs on a domain object. Adds cognitive load (10+ layer chains), wastes work on irrelevant requests, and creates invisible mutations. + +**Detect**: Middleware that checks request method/type and returns early for most cases; middleware whose sole purpose is context stuffing (see #1); middleware that wraps `ResponseWriter` or reads request body (see #2). + +**Instead**: Reserve middleware for truly cross-cutting concerns (recovery, telemetry, auth). Push behavior onto domain objects — e.g., annotation lookup as a method on `MultiSession` instead of middleware. + +## 5. SDK Coupling Leaking Through Abstractions + +SDK-specific patterns (e.g., mcp-go's two-phase session creation) escaping the adapter boundary and shaping internal architecture. + +**Detect**: Code outside `adapter/` referencing SDK-specific concepts (hooks, placeholders, two-phase creation); session management with "re-check"/"double-check" patterns from SDK lifecycle race windows. + +**Instead**: Keep the adapter layer thin and isolated. Internal session management should present a clean `CreateSession() -> (Session, error)` API. The two-phase dance should be invisible to callers. + +## 6. Configuration Object Passed Everywhere + +Threading a large `Config` struct (13+ fields) through constructors when each consumer only needs a small subset. Obscures dependencies, invites nil pointer panics, and bloats test setup. + +**Detect**: Constructors accepting `*config.Config` but only accessing a few fields; nil checks on config sub-fields in business logic; test setup building large config structs with mostly zero/nil fields. + +**Instead**: Each subsystem accepts only the config it needs via small, focused config types. Decompose the top-level config at the composition root before passing to constructors. + +## 7. Mutable Shared State Through Context + +Storing a mutable struct in context and having multiple middleware modify it in place. Violates the immutability convention, creates hidden mutation coupling, and risks data races in concurrent scenarios. + +**Detect**: Middleware mutating fields on structs retrieved from context; structs stored in context with exported mutable fields; multiple middleware reading and writing the same context value. + +**Instead**: Treat context values as immutable; create new values with `context.WithValue` if downstream needs to add info. Better yet, pass data explicitly (see #1). diff --git a/.claude/skills/vmcp-review/SKILL.md b/.claude/skills/vmcp-review/SKILL.md new file mode 100644 index 0000000000..a18969b589 --- /dev/null +++ b/.claude/skills/vmcp-review/SKILL.md @@ -0,0 +1,60 @@ +--- +name: vmcp-review +description: Reviews vMCP code changes for known anti-patterns that make the codebase harder to understand or more brittle. Use when reviewing PRs, planning features, or refactoring vMCP code. +--- + +# vMCP Code Review + +## Purpose + +Review code in `pkg/vmcp/` and `cmd/vmcp/` for known anti-patterns that increase cognitive load, create brittle dependencies, or undermine testability. This skill is used both for reviewing proposed changes and for auditing existing code. + +## Instructions + +### 1. Determine Scope + +Identify the files to review: + +- If reviewing a PR or diff, examine only the changed files under `pkg/vmcp/` and `cmd/vmcp/` +- If auditing a package, examine all `.go` files in the target package +- Skip files outside the vMCP codebase — this skill is vMCP-specific + +### 2. Anti-Pattern Detection + +For each file under review, check against the anti-patterns defined in `.claude/rules/vmcp-anti-patterns.md` (which is auto-loaded when vMCP files are read). Not every anti-pattern applies to every file — use judgment about which checks are relevant based on what the code does. + +For each finding, classify severity: + +- **Must fix**: The anti-pattern is being introduced or significantly expanded by this change +- **Should fix**: The anti-pattern exists in touched code and the change is a good opportunity to address it +- **Note**: The anti-pattern exists in nearby code but is not directly related to this change — flag for awareness only + +### 3. Present Findings + +Structure your report as: + +```markdown +## vMCP Review: [scope description] + +### Must Fix +- **[Anti-pattern name]** in `path/to/file.go:line`: [What's wrong and what to do instead] + +### Should Fix +- **[Anti-pattern name]** in `path/to/file.go:line`: [What's wrong and what to do instead] + +### Notes +- **[Anti-pattern name]** in `path/to/file.go:line`: [Brief description, for awareness] + +### Clean +No issues found for: [list anti-patterns that were checked and passed] +``` + +If no issues are found, say so explicitly — a clean review is valuable signal. + +## What This Skill Does NOT Cover + +- General Go style issues (use `golangci-lint` for that) +- Security vulnerabilities (use the security-advisor agent) +- Test quality (use the unit-test-writer agent) +- Non-vMCP code (use the general code-reviewer agent) +- Performance issues (unless they stem from an anti-pattern like repeated body parsing)