From bf740673e063ef55c56685e101b421a5f109a21a Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Mon, 16 Mar 2026 17:44:32 -0700 Subject: [PATCH 1/6] Add vMCP anti-pattern review skill Introduces a Claude Code skill that reviews vMCP code for 9 known anti-patterns: context variable coupling, repeated body read/restore, god object, middleware overuse, string-based error classification, silent error swallowing, SDK coupling leakage, config sprawl, and mutable shared state through context. Each anti-pattern includes detection guidance and concrete alternatives. Co-Authored-By: Claude Opus 4.6 --- .../vmcp-anti-pattern-review/ANTI-PATTERNS.md | 306 ++++++++++++++++++ .../skills/vmcp-anti-pattern-review/SKILL.md | 61 ++++ 2 files changed, 367 insertions(+) create mode 100644 .claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md create mode 100644 .claude/skills/vmcp-anti-pattern-review/SKILL.md diff --git a/.claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md b/.claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md new file mode 100644 index 0000000000..28043ff38c --- /dev/null +++ b/.claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md @@ -0,0 +1,306 @@ +# vMCP Anti-Pattern Catalog + +This catalog defines anti-patterns specific to the vMCP codebase. Each entry explains what the pattern is, why it's destructive, how to detect it, and what authors should do instead. + +--- + +## 1. Context Variable Coupling + +### Definition + +Using `context.WithValue` / `context.Value` to pass domain data between middleware layers or from middleware to handlers, creating invisible producer-consumer dependencies. + +### Why It's Destructive + +- **Invisible dependencies**: You cannot see from a function signature that it requires specific context values to be present. The compiler can't help you. +- **Ordering fragility**: Producers must run before consumers, but nothing enforces this. Reordering middleware silently breaks the system. +- **Silent degradation**: Consumers typically check `ok` and silently degrade (return nil, skip logic) when the value is missing, rather than failing loudly. +- **Testing burden**: Every test must manually construct context with the right values, and forgetting one produces confusing test failures. + +### How to Detect + +Look for: +- `context.WithValue` calls in middleware that set domain data (not request-scoped infrastructure like trace IDs) +- `ctx.Value(someKey)` reads in handlers, routers, or business logic +- Functions that accept `context.Context` but whose behavior depends on specific values being present in that context +- Multiple packages importing a shared context key package + +### Known Instances + +- `DiscoveredCapabilities`: Set in `pkg/vmcp/discovery/middleware.go`, read in 5 locations across `router/`, `server/`, `composer/` +- `HealthCheckMarker`: Set in `pkg/vmcp/health/`, read in `pkg/vmcp/auth/strategies/` to skip authentication — a behavioral side-channel through context +- `BackendInfo`: Set by audit middleware, then **mutated in place** by `server/backend_enrichment.go` — context values should be immutable + +### What to Do Instead + +- **Push data onto the `MultiSession` object.** The session is a well-typed, 1:1-with-request domain object that is decoupled from middleware and protocol concerns. Instead of stuffing discovered capabilities into context for 5 downstream readers, make them a field or method on `MultiSession`. Handlers already have access to the session — let them ask it for what they need. Data that is immutable and known early (e.g., discovered capabilities for the session) can be provided at construction time. Data that varies per-request can be set on the session at request time. +- **Pass domain data as explicit function/method parameters** when the session isn't the right home. If a handler needs something, accept it as a parameter. +- **For the health check marker specifically**: Pass a flag or option to the auth strategy rather than smuggling it through context. Authentication behavior should not depend on invisible context state. +- **Context is appropriate for**: trace IDs, cancellation, deadlines, and request-scoped infrastructure that is truly cross-cutting. It is not appropriate for domain data that specific code paths depend on. + +--- + +## 2. Repeated Request Body Read/Restore + +### Definition + +Multiple middleware layers independently call `io.ReadAll(r.Body)`, parse the JSON, then restore the body with `io.NopCloser(bytes.NewReader(...))` for the next layer in the chain. + +### Why It's Destructive + +- **Fragile implicit contract**: If any middleware forgets to restore the body (or restores it incorrectly), all downstream handlers get an empty body with no error signal. +- **Redundant work**: The same JSON is parsed independently 3+ times per request (parser middleware, audit middleware, backend enrichment middleware). +- **Error-prone**: Each read/restore is a place where bugs can hide — partial reads, buffer misuse, or failure to restore on error paths. +- **Invisible coupling**: Adding a new middleware that reads the body requires understanding that this pattern exists and that restore is mandatory. + +### How to Detect + +Look for: +- `io.ReadAll(r.Body)` followed by `r.Body = io.NopCloser(bytes.NewReader(...))` in middleware +- Multiple middleware in the same chain that each parse JSON from the request body +- Comments like "restore body for next handler" + +### Known Instances + +- `pkg/mcp/parser.go:85` — reads body, parses JSON-RPC, restores +- `pkg/audit/auditor.go:182` — reads body for logging, restores +- `pkg/vmcp/server/backend_enrichment.go:26` — reads body to look up backend name, restores +- `pkg/mcp/tool_filter.go:229` — reads body, filters tools, restores (sometimes with modified content) + +### What to Do Instead + +- **Parse the request body once**, early in the pipeline. The `ParsedMCPRequest` in context already partially does this — extend it so that all downstream consumers use the parsed representation instead of re-reading raw bytes. +- **If raw bytes are needed** (e.g., for audit logging), cache them alongside the parsed form in the same early-parse step. +- **New middleware must never re-read the body.** If a middleware needs request data, it should consume the already-parsed representation. + +--- + +## 3. God Object: Server Struct + +### Definition + +A single struct that owns and orchestrates too many distinct concerns, becoming the central point through which all logic flows. In vMCP, `Server` in `pkg/vmcp/server/server.go` manages HTTP lifecycle, session management, health monitoring, status reporting, capability aggregation, composite tools, MCP SDK integration, telemetry, audit, auth/authz, optimization, and graceful shutdown. + +### Why It's Destructive + +- **Cognitive overload**: 1,265 lines, 21 struct fields, multiple mutexes. Understanding any one concern requires reading the entire file. +- **Untestable in isolation**: You cannot instantiate the health subsystem without wiring up telemetry, sessions, discovery, and the MCP SDK. +- **Initialization complexity**: The 183-line `New()` constructor has a `nolint:gocyclo` suppression. `Start()` has implicit ordering dependencies between subsystem initialization steps. +- **Change amplification**: Modifying one concern (e.g., health monitoring) risks breaking unrelated concerns because they share the same struct and initialization path. + +### How to Detect + +Look for: +- Structs with 10+ fields spanning different domains +- Constructors longer than 50 lines or with `nolint:gocyclo` +- Files longer than 500 lines that handle multiple unrelated concerns +- Multiple `sync.Mutex` / `sync.RWMutex` fields protecting different subsets of state +- A `Start()` or `Run()` method that initializes many unrelated subsystems in sequence + +### Known Instances + +- `pkg/vmcp/server/server.go` — the primary instance. 14+ concerns, 21 fields, 183-line constructor, 137-line `Start()` method. + +### What to Do Instead + +- **Extract each concern into a self-contained module** that owns its own construction, initialization, and lifecycle. For example: `HealthSubsystem`, `SessionSubsystem`, `AuditSubsystem`, each with their own `New()` and `Start()`/`Stop()`. +- **Construct comprehensive units outside of server.go.** Each module should hide its internal complexity. Server.go should compose pre-built units, not orchestrate step-by-step construction of everything. +- **The Server struct should be a thin orchestrator**: receive pre-built subsystems, wire them together, and manage lifecycle. It should not know the internal details of how any subsystem works. +- **Use a builder or staged initialization pattern** where each stage's inputs and outputs are typed, making ordering constraints explicit and compiler-enforced. + +--- + +## 4. Middleware Overuse for Non-Cross-Cutting Concerns + +### Definition + +Implementing business logic as HTTP middleware when the behavior is specific to certain request types, could be a direct method call, or belongs on a domain object. + +### Why It's Destructive + +- **Cognitive load**: A 10+ layer middleware chain means understanding request processing requires mentally tracing through all layers in order, including the wrapping-order vs. execution-order inversion. +- **Wasted work**: Method-specific middleware (e.g., annotation enrichment only applies to `tools/call`) runs on every request and bails early for irrelevant methods. +- **Invisible mutations**: Middleware that modify the request (body reads, context injection, response wrapping) create side effects that downstream code silently depends on. +- **Wrong home for the logic**: When middleware enriches or transforms data, it's often doing work that belongs on the domain objects themselves. + +### How to Detect + +Look for: +- Middleware that checks the request method/type and returns early for most cases +- Middleware whose sole purpose is to look something up and stuff it into context (see anti-pattern #1) +- Middleware that wraps `ResponseWriter` to intercept and transform responses +- Middleware that reads the request body (see anti-pattern #2) + +### Known Instances + +- `AnnotationEnrichmentMiddleware` (`server/annotation_enrichment.go`) — only processes `tools/call` but runs on every request +- `backendEnrichmentMiddleware` (`server/backend_enrichment.go`) — re-parses the request body just to set a field for audit logging +- Authorization middleware response filtering (`pkg/authz/middleware.go`) — buffers entire responses to filter list operations at the HTTP layer + +### What to Do Instead + +- **Reserve middleware for truly cross-cutting concerns**: recovery, telemetry, authentication — things that apply uniformly to all requests. +- **Push behavior onto domain objects.** For example, instead of an annotation enrichment middleware that runs on every request, make annotation lookup a method on `MultiSession` or the capabilities object. The handler that processes `tools/call` can call it directly. +- **For response transformation** (e.g., authz filtering list results): do it at the data layer before the response is serialized, rather than intercepting and re-parsing HTTP responses. +- **For audit enrichment**: make it a step in the audit logging call, not a separate middleware that mutates shared context state. + +--- + +## 5. String-Based Error Classification + +### Definition + +Categorizing errors by pattern-matching against their `.Error()` string representation (e.g., `strings.Contains(errLower, "401 unauthorized")`) instead of using typed errors or sentinel values. + +### Why It's Destructive + +- **Fragile**: Error message text is not a stable API. Any upstream library or refactor that changes wording breaks the classification silently. +- **False positives**: Substring matching can match unrelated errors that happen to contain the same words. +- **Undermines proper error types**: The codebase already defines sentinel errors, but the string-matching fallbacks invite bypassing them. + +### How to Detect + +Look for: +- `strings.Contains(err.Error(), ...)` or `strings.Contains(strings.ToLower(err.Error()), ...)` +- Functions named `Is*Error` that don't use `errors.Is()` or `errors.As()` +- Comments like "fallback mechanism" or "pattern matching" near error classification code + +### Known Instances + +- `pkg/vmcp/errors.go:71-178` — `IsAuthenticationError()`, `IsAuthorizationError()`, `IsNetworkError()`, `IsTimeoutError()` all use string matching + +### What to Do Instead + +- **Use `errors.Is()` and `errors.As()` exclusively** for error classification. +- **Wrap errors at the boundary** where they enter the codebase from external libraries: `fmt.Errorf("backend auth failed: %w", ErrAuthentication)`. +- **Remove string-matching classification functions.** If they exist as a "fallback," the real fix is ensuring all error producers wrap with the correct sentinel. + +--- + +## 6. Silent Error Swallowing + +### Definition + +Logging an error but not returning it to the caller, allowing the operation to proceed as if it succeeded. + +### Why It's Destructive + +- **Invisible failures**: The caller cannot distinguish success from failure, so it continues operating on invalid state. +- **Debugging requires log inspection**: Instead of errors propagating to where they can be handled, they're buried in log output that may not be monitored. +- **Cascading failures**: A silently swallowed error early in a flow causes confusing failures later, far from the root cause. + +### How to Detect + +Look for: +- `slog.Error(...)` or `slog.Warn(...)` followed by `return ""`, `return nil`, or `continue` instead of `return err` +- Functions that return zero values on error paths without returning the error +- Functions whose return type doesn't include `error` but that can fail internally +- Comments like "astronomically unlikely" or "best effort" near error handling + +### Known Instances + +- `pkg/vmcp/server/sessionmanager/session_manager.go:99-106` — `Generate()` returns `""` on storage failure instead of an error +- `pkg/vmcp/composer/workflow_engine.go:151` — state persistence failure downgraded to a warning + +### What to Do Instead + +- **Return errors to callers.** If the function signature can't accommodate an error (e.g., it satisfies an external interface), either change the interface, use a wrapper, or use a different pattern. +- **If an error truly can be ignored**, document the specific invariant that makes it safe with a comment explaining *why*, not just that it's being ignored. +- **Never assume a failure is "astronomically unlikely."** If it can happen, handle it. + +--- + +## 7. SDK Coupling Leaking Through Abstractions + +### Definition + +Patterns forced by an external SDK (e.g., mark3labs/mcp-go) that leak beyond the adapter boundary and shape internal architecture. + +### Why It's Destructive + +- **Viral complexity**: The two-phase session creation pattern (Generate placeholder, then CreateSession replaces it) exists because of the SDK's hook-based lifecycle. If this pattern leaks into session management, health checking, or routing, switching SDKs becomes prohibitively expensive. +- **Race windows**: The two-phase pattern creates a window between placeholder creation and session finalization where the session can be terminated, requiring fragile double-check logic. +- **Tight coupling**: Internal code that knows about placeholders, hooks, or SDK-specific lifecycle stages is coupled to the SDK's design decisions. + +### How to Detect + +Look for: +- Code outside `adapter/` or integration layers that references SDK-specific concepts (hooks, placeholders, two-phase creation) +- Session management code with "re-check" or "double-check" patterns that exist because of race windows introduced by the SDK's lifecycle +- Comments referencing specific SDK behavior or limitations + +### Known Instances + +- `pkg/vmcp/server/sessionmanager/session_manager.go` — the two-phase `Generate()` / `CreateSession()` pattern with double-check logic (lines 144-167) + +### What to Do Instead + +- **Accept the two-phase pattern as a necessary evil** at the SDK boundary, but keep it contained. +- **The adapter layer should be thin and isolated.** Internal session management should present a clean `CreateSession() -> (Session, error)` API. The two-phase dance should be invisible to callers. +- **Avoid adding new code that depends on SDK-specific lifecycle stages.** If you find yourself writing code that needs to know about placeholders or hooks, you're letting SDK coupling leak. +- **When evaluating SDK alternatives**, the adapter boundary should be the only code that changes. + +--- + +## 8. Configuration Object Passed Everywhere + +### Definition + +A single large `Config` struct (13+ fields, each pointing to nested sub-configs) that gets threaded through constructors and methods across the codebase, even though each consumer only needs a small subset. + +### Why It's Destructive + +- **Unclear dependencies**: When a constructor accepts `*Config`, you can't tell which fields it actually uses without reading the implementation. +- **Nil pointer risk**: Every field is a pointer to a nested struct. Accessing `cfg.IncomingAuth.OIDC.ClientSecretEnv` requires nil checks at each level, and missing checks cause panics. +- **Change amplification**: Adding a new config field affects the entire config surface area, even though only one consumer needs it. +- **Testing burden**: Tests must construct the full config struct even though the code under test only reads 2-3 fields. + +### How to Detect + +Look for: +- Constructors or functions that accept `*config.Config` or `*server.Config` but only access a few fields +- Nil checks on config sub-fields scattered through business logic +- Test setup code that builds large config structs with mostly zero/nil fields +- Config structs with 10+ fields that span different domains + +### Known Instances + +- `pkg/vmcp/config/config.go:89-159` — 13 fields including auth, aggregation, telemetry, audit, optimizer +- `pkg/vmcp/server/server.go:83-165` — `server.Config` wraps the vmcp config and adds more fields + +### What to Do Instead + +- **Each subsystem should accept only the config it needs.** Define small config types close to their consumer: `NewHealthMonitor(cfg health.MonitorConfig)` not `NewHealthMonitor(cfg *vmcp.Config)`. +- **The top-level config is for parsing/loading only.** Decompose it into per-subsystem configs before passing to constructors. +- **Config decomposition should happen at the composition root** (currently server.go, ideally a dedicated wiring function). Each subsystem receives exactly what it needs and nothing more. + +--- + +## 9. Mutable Shared State Through Context + +### Definition + +Storing a mutable struct in context and having multiple middleware modify it in place, rather than treating context values as immutable. + +### Why It's Destructive + +- **Violates context contract**: Context values are conventionally immutable. Code that reads a context value doesn't expect it to change after retrieval. +- **Hidden mutation**: One middleware mutates state that another middleware set, with no signal in the code that this coupling exists. +- **Concurrency risk**: If the same context is shared across goroutines (e.g., in streaming/SSE scenarios), in-place mutation of context values is a data race. + +### How to Detect + +Look for: +- Middleware that calls `someStruct.Field = value` on a struct retrieved from context (not replaced with `context.WithValue`) +- Structs stored in context with exported mutable fields +- Multiple middleware that both read and write the same context value + +### Known Instances + +- `pkg/vmcp/server/backend_enrichment.go:54` — retrieves `BackendInfo` from context (set by audit middleware) and mutates its `BackendName` field in place + +### What to Do Instead + +- **Treat context values as immutable.** If downstream code needs to add information, create a new value and set it with `context.WithValue`. +- **Better yet, don't use context for this at all** (see anti-pattern #1). Pass the data explicitly so that mutation points are visible in the code. +- **If shared state is truly needed**, use a dedicated request-scoped struct with clear ownership semantics and document which code is allowed to write to which fields. diff --git a/.claude/skills/vmcp-anti-pattern-review/SKILL.md b/.claude/skills/vmcp-anti-pattern-review/SKILL.md new file mode 100644 index 0000000000..d0a9a56097 --- /dev/null +++ b/.claude/skills/vmcp-anti-pattern-review/SKILL.md @@ -0,0 +1,61 @@ +--- +name: vmcp-anti-pattern-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 Anti-Pattern 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 +- If auditing a package, examine all `.go` files in the target package +- Focus on `pkg/vmcp/` and `cmd/vmcp/` — these anti-patterns are specific to the vMCP codebase + +### 2. Check Each Anti-Pattern + +For each file under review, check against the anti-pattern catalog in [ANTI-PATTERNS.md](ANTI-PATTERNS.md). Not every anti-pattern applies to every file — use judgment about which checks are relevant based on what the code does. + +### 3. Classify Findings + +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 + +### 4. Present Findings + +Structure your report as: + +```markdown +## Anti-Pattern 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) +- Performance issues (unless they stem from an anti-pattern like repeated body parsing) From b2800c12463fb3584e82dafa2ed18c43fbf7152e Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Tue, 17 Mar 2026 12:03:43 -0700 Subject: [PATCH 2/6] Consolidate vMCP anti-pattern review into shared skill and rule Move anti-patterns catalog to docs/vmcp-anti-patterns.md as a single source of truth. Create a glob-scoped rule for automatic loading when editing vMCP code, rename the skill to vmcp-review, and integrate it into the code-reviewer agent as an additional check for vMCP changes. Co-Authored-By: Claude Opus 4.6 --- .claude/agents/code-reviewer.md | 18 ++++++++++++++ .claude/rules/vmcp-anti-patterns.md | 24 +++++++++++++++++++ .../SKILL.md | 19 +++++++-------- .../vmcp-anti-patterns.md | 0 4 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 .claude/rules/vmcp-anti-patterns.md rename .claude/skills/{vmcp-anti-pattern-review => vmcp-review}/SKILL.md (78%) rename .claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md => docs/vmcp-anti-patterns.md (100%) diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index 197d49c24a..1aca1a1a26 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -40,6 +40,24 @@ 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 + +### Documentation +- [ ] Public APIs have godoc comments +- [ ] Complex logic has explanatory comments +- [ ] README/docs updated if behavior changes +- [ ] Commit messages follow CONTRIBUTING.md guidelines + +### 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. + +### Performance +- [ ] No unnecessary allocations in hot paths +- [ ] Appropriate use of buffers and pooling +- [ ] Database queries optimized +- [ ] Container operations batched where possible + +>>>>>>> 05f039e8 (Consolidate vMCP anti-pattern review into shared skill and rule) ### 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..2644b37ec5 --- /dev/null +++ b/.claude/rules/vmcp-anti-patterns.md @@ -0,0 +1,24 @@ +--- +description: Checks vMCP code changes against known anti-patterns +globs: + - "pkg/vmcp/**/*.go" + - "cmd/vmcp/**/*.go" +--- + +# vMCP Anti-Pattern Rule + +When reviewing or writing code in `pkg/vmcp/` or `cmd/vmcp/`, check changes against the anti-pattern catalog in `docs/vmcp-anti-patterns.md`. + +Flag any code that introduces or expands these patterns: + +1. **Context Variable Coupling** — using `context.WithValue`/`ctx.Value` for domain data +2. **Repeated Request Body Read/Restore** — multiple middleware calling `io.ReadAll(r.Body)` then restoring +3. **God Object: Server Struct** — single struct owning too many concerns (10+ fields spanning domains) +4. **Middleware Overuse** — business logic in middleware instead of domain objects or direct calls +5. **String-Based Error Classification** — `strings.Contains(err.Error(), ...)` instead of `errors.Is`/`errors.As` +6. **Silent Error Swallowing** — logging errors but not returning them to callers +7. **SDK Coupling Leaking Through Abstractions** — SDK-specific patterns escaping adapter boundaries +8. **Configuration Object Passed Everywhere** — threading a large `Config` struct through code that only needs a subset +9. **Mutable Shared State Through Context** — modifying structs retrieved from context in place + +Refer to `docs/vmcp-anti-patterns.md` for full details, detection guidance, and recommended alternatives for each pattern. diff --git a/.claude/skills/vmcp-anti-pattern-review/SKILL.md b/.claude/skills/vmcp-review/SKILL.md similarity index 78% rename from .claude/skills/vmcp-anti-pattern-review/SKILL.md rename to .claude/skills/vmcp-review/SKILL.md index d0a9a56097..17deeafef3 100644 --- a/.claude/skills/vmcp-anti-pattern-review/SKILL.md +++ b/.claude/skills/vmcp-review/SKILL.md @@ -1,9 +1,9 @@ --- -name: vmcp-anti-pattern-review +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 Anti-Pattern Review +# vMCP Code Review ## Purpose @@ -15,15 +15,13 @@ Review code in `pkg/vmcp/` and `cmd/vmcp/` for known anti-patterns that increase Identify the files to review: -- If reviewing a PR or diff, examine only the changed files +- 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 -- Focus on `pkg/vmcp/` and `cmd/vmcp/` — these anti-patterns are specific to the vMCP codebase +- Skip files outside the vMCP codebase — this skill is vMCP-specific -### 2. Check Each Anti-Pattern +### 2. Anti-Pattern Detection -For each file under review, check against the anti-pattern catalog in [ANTI-PATTERNS.md](ANTI-PATTERNS.md). Not every anti-pattern applies to every file — use judgment about which checks are relevant based on what the code does. - -### 3. Classify Findings +For each file under review, check against the anti-pattern catalog in `docs/vmcp-anti-patterns.md`. 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: @@ -31,12 +29,12 @@ For each finding, classify severity: - **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 -### 4. Present Findings +### 3. Present Findings Structure your report as: ```markdown -## Anti-Pattern Review: [scope description] +## vMCP Review: [scope description] ### Must Fix - **[Anti-pattern name]** in `path/to/file.go:line`: [What's wrong and what to do instead] @@ -58,4 +56,5 @@ If no issues are found, say so explicitly — a clean review is valuable signal. - 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) diff --git a/.claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md b/docs/vmcp-anti-patterns.md similarity index 100% rename from .claude/skills/vmcp-anti-pattern-review/ANTI-PATTERNS.md rename to docs/vmcp-anti-patterns.md From 96cbb59b4d419de5242f10c0f21da9e571e95419 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Tue, 17 Mar 2026 12:49:58 -0700 Subject: [PATCH 3/6] remove merge conflict marker --- .claude/agents/code-reviewer.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index 1aca1a1a26..6ab230abf9 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -57,7 +57,6 @@ When reviewing changes that touch vMCP code, also run the `/vmcp-review` skill t - [ ] Database queries optimized - [ ] Container operations batched where possible ->>>>>>> 05f039e8 (Consolidate vMCP anti-pattern review into shared skill and rule) ### Backwards Compatibility - [ ] Changes won't break existing users - [ ] API/CLI changes maintain compatibility or include deprecation warnings From 91fbda1b0a4502fce7b6b62731e78714aa720463 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Wed, 18 Mar 2026 07:59:41 -0700 Subject: [PATCH 4/6] Address review feedback on vMCP anti-pattern rule and skill - Fix frontmatter: use `paths` instead of `globs`, remove unsupported `description` - Inline anti-pattern detection guidance directly in the rule instead of referencing docs/vmcp-anti-patterns.md, so the agent doesn't need an extra file read - Remove items 5 (String-Based Error Classification) and 6 (Silent Error Swallowing) since they're already covered by go-style.md - Update skill to reference the rule file instead of docs/ - Renumber remaining anti-patterns (now 1-7 instead of 1-9) Co-Authored-By: Claude Opus 4.6 --- .claude/rules/vmcp-anti-patterns.md | 69 +++++++++++++++++++++++------ .claude/skills/vmcp-review/SKILL.md | 2 +- 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/.claude/rules/vmcp-anti-patterns.md b/.claude/rules/vmcp-anti-patterns.md index 2644b37ec5..1281929c7a 100644 --- a/.claude/rules/vmcp-anti-patterns.md +++ b/.claude/rules/vmcp-anti-patterns.md @@ -1,24 +1,65 @@ --- -description: Checks vMCP code changes against known anti-patterns -globs: +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 the anti-pattern catalog in `docs/vmcp-anti-patterns.md`. +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. -Flag any code that introduces or expands these patterns: +## 1. Context Variable Coupling -1. **Context Variable Coupling** — using `context.WithValue`/`ctx.Value` for domain data -2. **Repeated Request Body Read/Restore** — multiple middleware calling `io.ReadAll(r.Body)` then restoring -3. **God Object: Server Struct** — single struct owning too many concerns (10+ fields spanning domains) -4. **Middleware Overuse** — business logic in middleware instead of domain objects or direct calls -5. **String-Based Error Classification** — `strings.Contains(err.Error(), ...)` instead of `errors.Is`/`errors.As` -6. **Silent Error Swallowing** — logging errors but not returning them to callers -7. **SDK Coupling Leaking Through Abstractions** — SDK-specific patterns escaping adapter boundaries -8. **Configuration Object Passed Everywhere** — threading a large `Config` struct through code that only needs a subset -9. **Mutable Shared State Through Context** — modifying structs retrieved from context in place +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. -Refer to `docs/vmcp-anti-patterns.md` for full details, detection guidance, and recommended alternatives for each pattern. +**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 index 17deeafef3..a18969b589 100644 --- a/.claude/skills/vmcp-review/SKILL.md +++ b/.claude/skills/vmcp-review/SKILL.md @@ -21,7 +21,7 @@ Identify the files to review: ### 2. Anti-Pattern Detection -For each file under review, check against the anti-pattern catalog in `docs/vmcp-anti-patterns.md`. Not every anti-pattern applies to every file — use judgment about which checks are relevant based on what the code does. +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: From 3bd889b9a6fede5abe0494d9e2ebe6a486227644 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Wed, 18 Mar 2026 08:01:36 -0700 Subject: [PATCH 5/6] Remove docs/vmcp-anti-patterns.md The anti-pattern catalog is now inlined directly in the rule file (.claude/rules/vmcp-anti-patterns.md), so this file is no longer referenced by any agent or skill. Co-Authored-By: Claude Opus 4.6 --- docs/vmcp-anti-patterns.md | 306 ------------------------------------- 1 file changed, 306 deletions(-) delete mode 100644 docs/vmcp-anti-patterns.md diff --git a/docs/vmcp-anti-patterns.md b/docs/vmcp-anti-patterns.md deleted file mode 100644 index 28043ff38c..0000000000 --- a/docs/vmcp-anti-patterns.md +++ /dev/null @@ -1,306 +0,0 @@ -# vMCP Anti-Pattern Catalog - -This catalog defines anti-patterns specific to the vMCP codebase. Each entry explains what the pattern is, why it's destructive, how to detect it, and what authors should do instead. - ---- - -## 1. Context Variable Coupling - -### Definition - -Using `context.WithValue` / `context.Value` to pass domain data between middleware layers or from middleware to handlers, creating invisible producer-consumer dependencies. - -### Why It's Destructive - -- **Invisible dependencies**: You cannot see from a function signature that it requires specific context values to be present. The compiler can't help you. -- **Ordering fragility**: Producers must run before consumers, but nothing enforces this. Reordering middleware silently breaks the system. -- **Silent degradation**: Consumers typically check `ok` and silently degrade (return nil, skip logic) when the value is missing, rather than failing loudly. -- **Testing burden**: Every test must manually construct context with the right values, and forgetting one produces confusing test failures. - -### How to Detect - -Look for: -- `context.WithValue` calls in middleware that set domain data (not request-scoped infrastructure like trace IDs) -- `ctx.Value(someKey)` reads in handlers, routers, or business logic -- Functions that accept `context.Context` but whose behavior depends on specific values being present in that context -- Multiple packages importing a shared context key package - -### Known Instances - -- `DiscoveredCapabilities`: Set in `pkg/vmcp/discovery/middleware.go`, read in 5 locations across `router/`, `server/`, `composer/` -- `HealthCheckMarker`: Set in `pkg/vmcp/health/`, read in `pkg/vmcp/auth/strategies/` to skip authentication — a behavioral side-channel through context -- `BackendInfo`: Set by audit middleware, then **mutated in place** by `server/backend_enrichment.go` — context values should be immutable - -### What to Do Instead - -- **Push data onto the `MultiSession` object.** The session is a well-typed, 1:1-with-request domain object that is decoupled from middleware and protocol concerns. Instead of stuffing discovered capabilities into context for 5 downstream readers, make them a field or method on `MultiSession`. Handlers already have access to the session — let them ask it for what they need. Data that is immutable and known early (e.g., discovered capabilities for the session) can be provided at construction time. Data that varies per-request can be set on the session at request time. -- **Pass domain data as explicit function/method parameters** when the session isn't the right home. If a handler needs something, accept it as a parameter. -- **For the health check marker specifically**: Pass a flag or option to the auth strategy rather than smuggling it through context. Authentication behavior should not depend on invisible context state. -- **Context is appropriate for**: trace IDs, cancellation, deadlines, and request-scoped infrastructure that is truly cross-cutting. It is not appropriate for domain data that specific code paths depend on. - ---- - -## 2. Repeated Request Body Read/Restore - -### Definition - -Multiple middleware layers independently call `io.ReadAll(r.Body)`, parse the JSON, then restore the body with `io.NopCloser(bytes.NewReader(...))` for the next layer in the chain. - -### Why It's Destructive - -- **Fragile implicit contract**: If any middleware forgets to restore the body (or restores it incorrectly), all downstream handlers get an empty body with no error signal. -- **Redundant work**: The same JSON is parsed independently 3+ times per request (parser middleware, audit middleware, backend enrichment middleware). -- **Error-prone**: Each read/restore is a place where bugs can hide — partial reads, buffer misuse, or failure to restore on error paths. -- **Invisible coupling**: Adding a new middleware that reads the body requires understanding that this pattern exists and that restore is mandatory. - -### How to Detect - -Look for: -- `io.ReadAll(r.Body)` followed by `r.Body = io.NopCloser(bytes.NewReader(...))` in middleware -- Multiple middleware in the same chain that each parse JSON from the request body -- Comments like "restore body for next handler" - -### Known Instances - -- `pkg/mcp/parser.go:85` — reads body, parses JSON-RPC, restores -- `pkg/audit/auditor.go:182` — reads body for logging, restores -- `pkg/vmcp/server/backend_enrichment.go:26` — reads body to look up backend name, restores -- `pkg/mcp/tool_filter.go:229` — reads body, filters tools, restores (sometimes with modified content) - -### What to Do Instead - -- **Parse the request body once**, early in the pipeline. The `ParsedMCPRequest` in context already partially does this — extend it so that all downstream consumers use the parsed representation instead of re-reading raw bytes. -- **If raw bytes are needed** (e.g., for audit logging), cache them alongside the parsed form in the same early-parse step. -- **New middleware must never re-read the body.** If a middleware needs request data, it should consume the already-parsed representation. - ---- - -## 3. God Object: Server Struct - -### Definition - -A single struct that owns and orchestrates too many distinct concerns, becoming the central point through which all logic flows. In vMCP, `Server` in `pkg/vmcp/server/server.go` manages HTTP lifecycle, session management, health monitoring, status reporting, capability aggregation, composite tools, MCP SDK integration, telemetry, audit, auth/authz, optimization, and graceful shutdown. - -### Why It's Destructive - -- **Cognitive overload**: 1,265 lines, 21 struct fields, multiple mutexes. Understanding any one concern requires reading the entire file. -- **Untestable in isolation**: You cannot instantiate the health subsystem without wiring up telemetry, sessions, discovery, and the MCP SDK. -- **Initialization complexity**: The 183-line `New()` constructor has a `nolint:gocyclo` suppression. `Start()` has implicit ordering dependencies between subsystem initialization steps. -- **Change amplification**: Modifying one concern (e.g., health monitoring) risks breaking unrelated concerns because they share the same struct and initialization path. - -### How to Detect - -Look for: -- Structs with 10+ fields spanning different domains -- Constructors longer than 50 lines or with `nolint:gocyclo` -- Files longer than 500 lines that handle multiple unrelated concerns -- Multiple `sync.Mutex` / `sync.RWMutex` fields protecting different subsets of state -- A `Start()` or `Run()` method that initializes many unrelated subsystems in sequence - -### Known Instances - -- `pkg/vmcp/server/server.go` — the primary instance. 14+ concerns, 21 fields, 183-line constructor, 137-line `Start()` method. - -### What to Do Instead - -- **Extract each concern into a self-contained module** that owns its own construction, initialization, and lifecycle. For example: `HealthSubsystem`, `SessionSubsystem`, `AuditSubsystem`, each with their own `New()` and `Start()`/`Stop()`. -- **Construct comprehensive units outside of server.go.** Each module should hide its internal complexity. Server.go should compose pre-built units, not orchestrate step-by-step construction of everything. -- **The Server struct should be a thin orchestrator**: receive pre-built subsystems, wire them together, and manage lifecycle. It should not know the internal details of how any subsystem works. -- **Use a builder or staged initialization pattern** where each stage's inputs and outputs are typed, making ordering constraints explicit and compiler-enforced. - ---- - -## 4. Middleware Overuse for Non-Cross-Cutting Concerns - -### Definition - -Implementing business logic as HTTP middleware when the behavior is specific to certain request types, could be a direct method call, or belongs on a domain object. - -### Why It's Destructive - -- **Cognitive load**: A 10+ layer middleware chain means understanding request processing requires mentally tracing through all layers in order, including the wrapping-order vs. execution-order inversion. -- **Wasted work**: Method-specific middleware (e.g., annotation enrichment only applies to `tools/call`) runs on every request and bails early for irrelevant methods. -- **Invisible mutations**: Middleware that modify the request (body reads, context injection, response wrapping) create side effects that downstream code silently depends on. -- **Wrong home for the logic**: When middleware enriches or transforms data, it's often doing work that belongs on the domain objects themselves. - -### How to Detect - -Look for: -- Middleware that checks the request method/type and returns early for most cases -- Middleware whose sole purpose is to look something up and stuff it into context (see anti-pattern #1) -- Middleware that wraps `ResponseWriter` to intercept and transform responses -- Middleware that reads the request body (see anti-pattern #2) - -### Known Instances - -- `AnnotationEnrichmentMiddleware` (`server/annotation_enrichment.go`) — only processes `tools/call` but runs on every request -- `backendEnrichmentMiddleware` (`server/backend_enrichment.go`) — re-parses the request body just to set a field for audit logging -- Authorization middleware response filtering (`pkg/authz/middleware.go`) — buffers entire responses to filter list operations at the HTTP layer - -### What to Do Instead - -- **Reserve middleware for truly cross-cutting concerns**: recovery, telemetry, authentication — things that apply uniformly to all requests. -- **Push behavior onto domain objects.** For example, instead of an annotation enrichment middleware that runs on every request, make annotation lookup a method on `MultiSession` or the capabilities object. The handler that processes `tools/call` can call it directly. -- **For response transformation** (e.g., authz filtering list results): do it at the data layer before the response is serialized, rather than intercepting and re-parsing HTTP responses. -- **For audit enrichment**: make it a step in the audit logging call, not a separate middleware that mutates shared context state. - ---- - -## 5. String-Based Error Classification - -### Definition - -Categorizing errors by pattern-matching against their `.Error()` string representation (e.g., `strings.Contains(errLower, "401 unauthorized")`) instead of using typed errors or sentinel values. - -### Why It's Destructive - -- **Fragile**: Error message text is not a stable API. Any upstream library or refactor that changes wording breaks the classification silently. -- **False positives**: Substring matching can match unrelated errors that happen to contain the same words. -- **Undermines proper error types**: The codebase already defines sentinel errors, but the string-matching fallbacks invite bypassing them. - -### How to Detect - -Look for: -- `strings.Contains(err.Error(), ...)` or `strings.Contains(strings.ToLower(err.Error()), ...)` -- Functions named `Is*Error` that don't use `errors.Is()` or `errors.As()` -- Comments like "fallback mechanism" or "pattern matching" near error classification code - -### Known Instances - -- `pkg/vmcp/errors.go:71-178` — `IsAuthenticationError()`, `IsAuthorizationError()`, `IsNetworkError()`, `IsTimeoutError()` all use string matching - -### What to Do Instead - -- **Use `errors.Is()` and `errors.As()` exclusively** for error classification. -- **Wrap errors at the boundary** where they enter the codebase from external libraries: `fmt.Errorf("backend auth failed: %w", ErrAuthentication)`. -- **Remove string-matching classification functions.** If they exist as a "fallback," the real fix is ensuring all error producers wrap with the correct sentinel. - ---- - -## 6. Silent Error Swallowing - -### Definition - -Logging an error but not returning it to the caller, allowing the operation to proceed as if it succeeded. - -### Why It's Destructive - -- **Invisible failures**: The caller cannot distinguish success from failure, so it continues operating on invalid state. -- **Debugging requires log inspection**: Instead of errors propagating to where they can be handled, they're buried in log output that may not be monitored. -- **Cascading failures**: A silently swallowed error early in a flow causes confusing failures later, far from the root cause. - -### How to Detect - -Look for: -- `slog.Error(...)` or `slog.Warn(...)` followed by `return ""`, `return nil`, or `continue` instead of `return err` -- Functions that return zero values on error paths without returning the error -- Functions whose return type doesn't include `error` but that can fail internally -- Comments like "astronomically unlikely" or "best effort" near error handling - -### Known Instances - -- `pkg/vmcp/server/sessionmanager/session_manager.go:99-106` — `Generate()` returns `""` on storage failure instead of an error -- `pkg/vmcp/composer/workflow_engine.go:151` — state persistence failure downgraded to a warning - -### What to Do Instead - -- **Return errors to callers.** If the function signature can't accommodate an error (e.g., it satisfies an external interface), either change the interface, use a wrapper, or use a different pattern. -- **If an error truly can be ignored**, document the specific invariant that makes it safe with a comment explaining *why*, not just that it's being ignored. -- **Never assume a failure is "astronomically unlikely."** If it can happen, handle it. - ---- - -## 7. SDK Coupling Leaking Through Abstractions - -### Definition - -Patterns forced by an external SDK (e.g., mark3labs/mcp-go) that leak beyond the adapter boundary and shape internal architecture. - -### Why It's Destructive - -- **Viral complexity**: The two-phase session creation pattern (Generate placeholder, then CreateSession replaces it) exists because of the SDK's hook-based lifecycle. If this pattern leaks into session management, health checking, or routing, switching SDKs becomes prohibitively expensive. -- **Race windows**: The two-phase pattern creates a window between placeholder creation and session finalization where the session can be terminated, requiring fragile double-check logic. -- **Tight coupling**: Internal code that knows about placeholders, hooks, or SDK-specific lifecycle stages is coupled to the SDK's design decisions. - -### How to Detect - -Look for: -- Code outside `adapter/` or integration layers that references SDK-specific concepts (hooks, placeholders, two-phase creation) -- Session management code with "re-check" or "double-check" patterns that exist because of race windows introduced by the SDK's lifecycle -- Comments referencing specific SDK behavior or limitations - -### Known Instances - -- `pkg/vmcp/server/sessionmanager/session_manager.go` — the two-phase `Generate()` / `CreateSession()` pattern with double-check logic (lines 144-167) - -### What to Do Instead - -- **Accept the two-phase pattern as a necessary evil** at the SDK boundary, but keep it contained. -- **The adapter layer should be thin and isolated.** Internal session management should present a clean `CreateSession() -> (Session, error)` API. The two-phase dance should be invisible to callers. -- **Avoid adding new code that depends on SDK-specific lifecycle stages.** If you find yourself writing code that needs to know about placeholders or hooks, you're letting SDK coupling leak. -- **When evaluating SDK alternatives**, the adapter boundary should be the only code that changes. - ---- - -## 8. Configuration Object Passed Everywhere - -### Definition - -A single large `Config` struct (13+ fields, each pointing to nested sub-configs) that gets threaded through constructors and methods across the codebase, even though each consumer only needs a small subset. - -### Why It's Destructive - -- **Unclear dependencies**: When a constructor accepts `*Config`, you can't tell which fields it actually uses without reading the implementation. -- **Nil pointer risk**: Every field is a pointer to a nested struct. Accessing `cfg.IncomingAuth.OIDC.ClientSecretEnv` requires nil checks at each level, and missing checks cause panics. -- **Change amplification**: Adding a new config field affects the entire config surface area, even though only one consumer needs it. -- **Testing burden**: Tests must construct the full config struct even though the code under test only reads 2-3 fields. - -### How to Detect - -Look for: -- Constructors or functions that accept `*config.Config` or `*server.Config` but only access a few fields -- Nil checks on config sub-fields scattered through business logic -- Test setup code that builds large config structs with mostly zero/nil fields -- Config structs with 10+ fields that span different domains - -### Known Instances - -- `pkg/vmcp/config/config.go:89-159` — 13 fields including auth, aggregation, telemetry, audit, optimizer -- `pkg/vmcp/server/server.go:83-165` — `server.Config` wraps the vmcp config and adds more fields - -### What to Do Instead - -- **Each subsystem should accept only the config it needs.** Define small config types close to their consumer: `NewHealthMonitor(cfg health.MonitorConfig)` not `NewHealthMonitor(cfg *vmcp.Config)`. -- **The top-level config is for parsing/loading only.** Decompose it into per-subsystem configs before passing to constructors. -- **Config decomposition should happen at the composition root** (currently server.go, ideally a dedicated wiring function). Each subsystem receives exactly what it needs and nothing more. - ---- - -## 9. Mutable Shared State Through Context - -### Definition - -Storing a mutable struct in context and having multiple middleware modify it in place, rather than treating context values as immutable. - -### Why It's Destructive - -- **Violates context contract**: Context values are conventionally immutable. Code that reads a context value doesn't expect it to change after retrieval. -- **Hidden mutation**: One middleware mutates state that another middleware set, with no signal in the code that this coupling exists. -- **Concurrency risk**: If the same context is shared across goroutines (e.g., in streaming/SSE scenarios), in-place mutation of context values is a data race. - -### How to Detect - -Look for: -- Middleware that calls `someStruct.Field = value` on a struct retrieved from context (not replaced with `context.WithValue`) -- Structs stored in context with exported mutable fields -- Multiple middleware that both read and write the same context value - -### Known Instances - -- `pkg/vmcp/server/backend_enrichment.go:54` — retrieves `BackendInfo` from context (set by audit middleware) and mutates its `BackendName` field in place - -### What to Do Instead - -- **Treat context values as immutable.** If downstream code needs to add information, create a new value and set it with `context.WithValue`. -- **Better yet, don't use context for this at all** (see anti-pattern #1). Pass the data explicitly so that mutation points are visible in the code. -- **If shared state is truly needed**, use a dedicated request-scoped struct with clear ownership semantics and document which code is allowed to write to which fields. From b1a44deb1288a856b9e611c9423a515b708de26c Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Wed, 18 Mar 2026 08:03:07 -0700 Subject: [PATCH 6/6] remove added code from merge --- .claude/agents/code-reviewer.md | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index 6ab230abf9..4452818f61 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -40,23 +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 - -### Documentation -- [ ] Public APIs have godoc comments -- [ ] Complex logic has explanatory comments -- [ ] README/docs updated if behavior changes -- [ ] Commit messages follow CONTRIBUTING.md guidelines - ### 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. -### Performance -- [ ] No unnecessary allocations in hot paths -- [ ] Appropriate use of buffers and pooling -- [ ] Database queries optimized -- [ ] Container operations batched where possible - ### Backwards Compatibility - [ ] Changes won't break existing users - [ ] API/CLI changes maintain compatibility or include deprecation warnings