Add coding conventions derived from recent PR review feedback

Captures patterns flagged in reviews across interface design,
testing, operator development, and PR hygiene — so they apply
automatically going forward rather than being caught in review.

Author: taskbot

.claude/rules/go-style.md

@@ -13,6 +13,36 @@ Applies to all Go files in the project.
 - Separate business logic from transport/protocol concerns
 - Keep packages focused on single responsibilities
 
+## Interface Design
+
+Check these whenever adding a method to an interface or defining a new type:
+
+- **Minimal surface**: Don't add interface methods that duplicate the semantics of existing ones. If an existing method already answers the question (possibly with a side effect), don't add a separate method for the same check.
+- **No no-op implementations**: If a method is only meaningful for some implementations of an interface, the interface is too broad. A no-op silently breaks callers that depend on the method working. Narrow the interface or use a separate capability interface.
+- **Avoid parallel types that drift**: Don't define a separate config/data type that mirrors an existing one. Embed or reuse the original — two parallel structs require a conversion step and will diverge over time.
+
+## Constants for Repeated String Literals
+
+When a string literal is used as a key or identifier in three or more places, extract it to a named `const`. Silent typos in repeated raw strings are harder to find than a single compile-time reference.
+
+```go
+// Good
+const metadataKeyBackendURL = "backend_url"
+
+// Avoid: "backend_url" repeated inline across 3+ call sites
+```
+
+## URL Validation
+
+Whenever code calls `url.Parse` and then uses `Scheme` or `Host`, validate the result. `url.Parse` accepts malformed or relative inputs without error — a bare hostname parses successfully with empty `Scheme` and `Host`. When an absolute URL is required:
+
+```go
+u, err := url.Parse(raw)
+if err != nil || u.Scheme == "" || u.Host == "" {
+    return fmt.Errorf("invalid absolute URL: %q", raw)
+}
+```
+
 ## SPDX License Headers
 
 All Go files require SPDX headers at the top:
@@ -54,6 +84,31 @@ if someCondition {
 - Use `fmt.Errorf` with `%w` to preserve error chains; don't wrap excessively
 - Use `recover()` sparingly — only at top-level API/CLI boundaries
 
+## In-Memory Caches Must Be Bounded
+
+Any in-memory cache must have a capacity limit. Without one, entries that are never explicitly evicted accumulate indefinitely — leaking memory, connections, or file handles. Apply this whenever you introduce a `sync.Map`, a plain `map` guarded by a mutex, or any other structure used as a cache:
+
+- Set a maximum entry count and evict on overflow (LRU, FIFO, or random).
+- Document the bound and the eviction policy in the type's doc comment.
+- If Redis or another remote store provides TTL-based expiry, the local cache still needs a size cap — remote TTLs do not bound local memory.
+
+## Document Architectural Constraints on Exported Functions
+
+When an exported function or constructor changes behavior based on injected infrastructure (storage backend, transport mode, external client), its doc comment must state what the injection does and does not solve. Callers cannot be expected to infer distributed-system constraints from the implementation.
+
+Include at minimum:
+- What the injected component enables (e.g., cross-replica metadata sharing).
+- What it does *not* solve (e.g., cross-replica message delivery, fan-out).
+- Any caller responsibility that follows (e.g., session affinity at the load balancer).
+
+## Concurrency Comments
+
+Keep comments about mutexes, locks, and concurrency accurate — they are easy to get wrong and mislead future readers:
+
+- Only say a lock "must be held" or "is already held" if you have verified it at that call site.
+- Do not claim an operation would deadlock without confirming that the lock in question would actually be re-acquired.
+- When a comment describes a concurrency invariant (e.g., "called with mu held"), add it to the function's doc comment so it travels with the signature, not inline at the call site.
+
 ## Logging
 
 - **Silent success** — no output at INFO or above for successful operations

.claude/rules/operator.md

@@ -35,6 +35,10 @@ See `cmd/thv-operator/DESIGN.md` for detailed decision guidelines.
 - Chainsaw tests require a real Kubernetes cluster
 - Status updates require a separate client patch (`r.Status().Update()`)
 
+## Status Condition Parity
+
+When adding a status condition to one CRD type, check all parallel types (e.g., `MCPServer` and `VirtualMCPServer`) for the same condition. Conditions that warn about misconfiguration or unsupported states should be consistent across types that share the same feature set — a gap means one type silently accepts invalid config that the other rejects.
+
 ## Key Operator Commands
 
 ```bash

.claude/rules/pr-creation.md

@@ -16,6 +16,10 @@ Do NOT leave optional sections empty or with only placeholder/template text. Eit
 - **Does this introduce a user-facing change?**: Describe the change from the user's perspective. Write "No" if not applicable.
 - **Special notes for reviewers**: Non-obvious design decisions, known limitations, areas wanting extra scrutiny, or planned follow-up work.
 
+## PR Scope
+
+Each PR must contain only related changes. If a bug fix, refactor, or unrelated cleanup is discovered while working on a feature, open a separate PR for it. Mixed-scope PRs are harder to review and harder to revert cleanly.
+
 ## Style guidelines
 
 - Keep the PR title under 70 characters, imperative mood, no trailing period.

.claude/rules/testing.md

@@ -32,9 +32,13 @@ Applies to test files and test directories.
 2. **Redundancy**: Avoid overlapping test cases exercising the same code path
 3. **Value**: Every test must add meaningful coverage — remove tests that don't
 4. **Consolidation**: Consolidate small test functions into a single table-driven test when they test the same function
-5. **Naming**: Use clear, descriptive test names that convey the scenario
+5. **Naming**: Test names must match what they actually assert — if the assertion changes, update the name too.
 6. **Boilerplate**: Minimize setup code; extract shared setup into helpers with `t.Helper()`
 
+## E2E Test Coverage
+
+E2E tests must verify functional behavior, not just infrastructure state. Confirming that pods are ready or that counts are correct is not sufficient — the test must also exercise the actual code path (send traffic, trigger the feature) to prove it works end-to-end.
+
 ## Test Scope
 
 Tests must only test code in the package under test. Do NOT test behavior of dependencies, external packages, or transitive functionality.
@@ -55,3 +59,29 @@ Write tests isolated from other tests that may set the same env vars. Use `t.Set
 ## Port Numbers
 
 Use random ports (e.g., `net.Listen("tcp", ":0")`) to let the OS assign a free port. Do not use hardcoded port numbers — even large ones can clash with running services.
+
+## Test Hooks in Production Structs
+
+Do not add test-only hook fields (nil-checked `func()` fields) to production structs. A field documented as "nil in production" signals the concern belongs outside the production type. Preferred alternatives:
+
+- **Interface seam**: Replace the internal component with an interface; tests inject a wrapper that adds the needed synchronization or observation.
+- **Functional constructor options**: Expose hook injection only through a constructor option so the production call site stays clean.
+- **Test at the observable boundary**: Control timing through the mock/stub's own behavior rather than hooking into production internals.
+
+## Concurrent Tests: Always Add Timeouts to Blocking Barriers
+
+Blocking operations in tests (`WaitGroup.Wait()`, channel receives, `sync.Cond.Wait()`) must have a timeout/fail-fast path. Without one, a panicking goroutine or regression in synchronization logic causes the test to hang until the global `go test` timeout.
+
+```go
+// Good: fail fast with a clear message
+done := make(chan struct{})
+go func() { wg.Wait(); close(done) }()
+select {
+case <-done:
+case <-time.After(5 * time.Second):
+    t.Fatal("timeout waiting for goroutines to synchronise")
+}
+
+// Avoid: hangs indefinitely on deadlock
+wg.Wait()
+```

.claude/rules/vmcp-anti-patterns.md

@@ -79,3 +79,11 @@ Adding caches, connection pools, or other performance optimizations without evid
 **Detect**: Caches introduced without profiling data or load estimates showing the uncached path is too slow; connection pools or object pools where the allocation cost hasn't been measured; complexity added to avoid overhead (e.g., TLS handshakes, serialization) at request rates where the overhead is negligible.
 
 **Instead**: Start with the straightforward implementation. Measure under realistic load. Add optimization only when measurements show it's needed, and document the evidence in the commit or PR description.
+
+## 10. Mutable Domain Objects with Mutex Protection
+
+Adding a mutex to a domain object (session, config, metadata record) and mutating it in place when state changes. This pattern requires callers to coordinate locking, produces stale copies on other replicas, and adds complexity every time a new mutation is needed.
+
+**Detect**: Mutex fields on domain structs; mutation methods on types that were previously read-only; in-place map or field writes guarded by an object-level lock.
+
+**Instead**: Treat domain objects as immutable snapshots. When state changes, update the storage layer and evict the local cache entry. The next read reconstructs the object from updated storage — no mutation, no lock, and cross-replica consistency is automatic because all replicas read from the same source of truth.