SOLID principle violations

[Agent-Hellboy]

@coderabbitai, please find all SOLID principle violations for my junior, please explain them in detail, and find all instances across all components. You have to teach him about all those principles as well with violations in the codebase

[Agent-Hellboy]

@coderabbitai ?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's explore the repository structure to understand the codebase
echo "=== Repository structure ==="
fd -t f -e py -e js -e ts | head -30

echo -e "\n=== Main directories ==="
ls -la

echo -e "\n=== Source files ==="
fd -t f -e py . src 2>/dev/null || fd -t f -e py | grep -v __pycache__ | grep -v .pyc

Length of output: 7825

---

🏁 Script executed:

#!/bin/bash
# Let's look at the main package structure
echo "=== Package.json or setup.py ==="
cat package.json 2>/dev/null || cat setup.py 2>/dev/null || cat pyproject.toml 2>/dev/null

echo -e "\n=== README content ==="
cat README.md 2>/dev/null | head -50

Length of output: 5725

---

🏁 Script executed:

#!/bin/bash
# Let's find and read the main source files
echo "=== Main source files ==="
fd -t f -e py | grep -v __pycache__ | grep -v test | grep -v .pyc | while read file; do
  echo "--- $file ---"
  cat "$file"
  echo -e "\n"
done

Length of output: 278920

---

@Agent-Hellboy, I'll conduct a comprehensive SOLID principles analysis of your codebase. Let me examine the code structure for violations and provide detailed explanations.

---

🏁 Script executed:

#!/bin/bash
# Analyze key classes for SOLID violations
echo "=== Analyzing MCPFuzzerClient ==="
cat mcp_fuzzer/client/base.py | head -100

echo -e "\n=== Analyzing ProtocolClient ==="
cat mcp_fuzzer/client/protocol_client.py | head -80

echo -e "\n=== Analyzing ToolClient ==="
cat mcp_fuzzer/client/tool_client.py | head -80

echo -e "\n=== Analyzing CLI args ==="
cat mcp_fuzzer/cli/args.py | grep -A 10 "def get_cli_config"

echo -e "\n=== Analyzing SafetyFilter ==="
cat mcp_fuzzer/safety_system/safety.py | head -100

Length of output: 12757

---

🏁 Script executed:

#!/bin/bash
# Analyze transport and strategy patterns
echo "=== Transport Factory ==="
cat mcp_fuzzer/transport/factory.py

echo -e "\n=== Strategy Manager ==="
cat mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py | head -80

echo -e "\n=== Auth Providers ==="
cat mcp_fuzzer/auth/providers.py | head -60

Length of output: 9530

---

🏁 Script executed:

#!/bin/bash
# Count class responsibilities
echo "=== Classes with multiple responsibilities ==="
echo "MCPFuzzerClient methods:"
grep "def " mcp_fuzzer/client/base.py | wc -l

echo "ProtocolClient methods:"
grep "def " mcp_fuzzer/client/protocol_client.py | wc -l

echo "ToolClient methods:"
grep "def " mcp_fuzzer/client/tool_client.py | wc -l

echo "ProcessManager methods:"
grep "def " mcp_fuzzer/fuzz_engine/runtime/manager.py | wc -l

echo -e "\n=== CLI functions ==="
grep "^def " mcp_fuzzer/cli/args.py

Length of output: 613

---

@Agent-Hellboy Thanks for the ping — I’ve started a full SOLID review. I’ve already scanned core modules and I’m continuing with targeted reads to cite exact lines and propose fixes. If you want me to proceed now with the full written report, say “go”. If you prefer me to dig deeper in transports/reporters first, I can run a few more quick scans and include those too.

[Agent-Hellboy]

@coderabbitai, dig deeper into all components. He is a noob in software design, you have to share references for him to understand those principles, and with code example, you have to explain all violations.
Also take inspiration from #129

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's look at PR #129 for inspiration
echo "=== PR #129 Details ==="
gh pr view 129 --json title,body,comments --jq '.title, .body, .comments[].body' 2>/dev/null || echo "Unable to fetch PR details"

echo -e "\n=== Let's analyze all components systematically ==="

# 1. Client components
echo -e "\n=== CLIENT COMPONENTS ==="
echo "--- base.py (MCPFuzzerClient) ---"
cat mcp_fuzzer/client/base.py

echo -e "\n--- protocol_client.py ---"
cat mcp_fuzzer/client/protocol_client.py

echo -e "\n--- tool_client.py ---"
cat mcp_fuzzer/client/tool_client.py

echo -e "\n--- resource_client.py ---"
cat mcp_fuzzer/client/resource_client.py

echo -e "\n--- prompt_client.py ---"
cat mcp_fuzzer/client/prompt_client.py

Length of output: 112242

---

🏁 Script executed:

#!/bin/bash
# 2. Strategy components
echo "=== STRATEGY COMPONENTS ==="
echo "--- strategy_manager.py ---"
cat mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py

echo -e "\n--- tool_strategies.py ---"
cat mcp_fuzzer/strategy/tool_strategies.py

echo -e "\n--- resource_strategies.py ---"
cat mcp_fuzzer/strategy/resource_strategies.py

echo -e "\n--- prompt_strategies.py ---"
cat mcp_fuzzer/strategy/prompt_strategies.py

echo -e "\n--- protocol_strategies.py ---"
cat mcp_fuzzer/strategy/protocol_strategies.py

Length of output: 9444

---

🏁 Script executed:

#!/bin/bash
# 3. Transport components
echo "=== TRANSPORT COMPONENTS ==="
echo "--- factory.py ---"
cat mcp_fuzzer/transport/factory.py

echo -e "\n--- stdio.py ---"
cat mcp_fuzzer/transport/stdio.py

echo -e "\n--- sse.py ---"
cat mcp_fuzzer/transport/sse.py

echo -e "\n--- websocket.py ---"
cat mcp_fuzzer/transport/websocket.py

Length of output: 34126

---

🏁 Script executed:

#!/bin/bash
# 4. Auth, Safety, Runtime components
echo "=== AUTH COMPONENTS ==="
cat mcp_fuzzer/auth/providers.py

echo -e "\n=== SAFETY COMPONENTS ==="
cat mcp_fuzzer/safety_system/safety.py

echo -e "\n=== RUNTIME COMPONENTS ==="
echo "--- manager.py ---"
cat mcp_fuzzer/fuzz_engine/runtime/manager.py

echo -e "\n--- server_monitor.py ---"
cat mcp_fuzzer/fuzz_engine/runtime/server_monitor.py

Length of output: 40656

---

🏁 Script executed:

#!/bin/bash
# 5. CLI and Reporter components
echo "=== CLI COMPONENTS ==="
cat mcp_fuzzer/cli/args.py

echo -e "\n=== REPORTER COMPONENTS ==="
echo "--- base.py ---"
cat mcp_fuzzer/reporter/base.py

echo -e "\n--- json_reporter.py ---"
cat mcp_fuzzer/reporter/json_reporter.py

echo -e "\n--- html_reporter.py ---"
cat mcp_fuzzer/reporter/html_reporter.py

echo -e "\n--- terminal_reporter.py ---"
cat mcp_fuzzer/reporter/terminal_reporter.py

Length of output: 23423

---

🏁 Script executed:

#!/bin/bash
# 6. Analyze class structures and dependencies
echo "=== CLASS METHOD COUNTS ==="
for file in mcp_fuzzer/client/*.py mcp_fuzzer/strategy/*.py mcp_fuzzer/transport/*.py; do
  if [ -f "$file" ]; then
    echo "--- $file ---"
    grep -c "def " "$file" 2>/dev/null || echo "0"
  fi
done

echo -e "\n=== LOOKING FOR TIGHT COUPLING ==="
echo "Import analysis:"
rg "^from mcp_fuzzer" --type py -n | head -100

Length of output: 8811

---

🏁 Script executed:

#!/bin/bash
# 7. Check for God classes and violations
echo "=== ANALYZING GOD CLASSES ==="
echo "Lines per class:"
for file in mcp_fuzzer/client/base.py mcp_fuzzer/fuzz_engine/runtime/manager.py mcp_fuzzer/cli/args.py; do
  if [ -f "$file" ]; then
    echo "--- $file: $(wc -l < "$file") lines ---"
  fi
done

echo -e "\n=== CHECKING INTERFACE SEGREGATION ==="
echo "Abstract base classes:"
rg "class.*\(ABC\)|@abstractmethod" --type py -n -A 2

Length of output: 1739

---

@Agent-Hellboy I'll provide a comprehensive SOLID principles education with all violations found in your codebase. This will help your junior understand both the theory and practical applications.

---

🎓 SOLID Principles Deep Dive & Codebase Analysis

Table of Contents

1. What is SOLID?
2. Single Responsibility Principle (SRP)
3. Open/Closed Principle (OCP)
4. Liskov Substitution Principle (LSP)
5. Interface Segregation Principle (ISP)
6. Dependency Inversion Principle (DIP)

---

What is SOLID?

SOLID is an acronym for five design principles that make software designs more understandable, flexible, and maintainable. These principles were introduced by Robert C. Martin (Uncle Bob).

📚 Essential Reading:

• Clean Code by Robert C. Martin
• SOLID Principles Explained
• Uncle Bob's Blog

---

1. Single Responsibility Principle (SRP)

📖 Definition

"A class should have one, and only one, reason to change."

A class should do ONE thing and do it well. If you can describe what a class does using "AND", it's likely violating SRP.

📚 References

• SRP Article by Uncle Bob
• Refactoring Guru - SRP

❌ VIOLATION #1: MCPFuzzerClient (God Class)

File: mcp_fuzzer/client/base.py (199 lines, 18 methods)

Problem: This class has MULTIPLE responsibilities:

1. Tool fuzzing coordination
2. Protocol fuzzing coordination
3. Report generation
4. Safety system management
5. Authentication management
6. Transport lifecycle management
7. Cleanup operations

class MCPFuzzerClient:
    """
    Main client for fuzzing MCP servers.
    
    VIOLATION: This class does TOO MANY things!
    - Coordinates tool fuzzing
    - Coordinates protocol fuzzing
    - Manages reporting
    - Manages safety system
    - Manages authentication
    - Manages transport lifecycle
    """
    
    def __init__(
        self,
        transport,
        auth_manager: AuthManager | None = None,
        tool_timeout: float | None = None,
        reporter: FuzzerReporter | None = None,
        safety_system: SafetyProvider | None = None,
        # ... too many parameters!
    ):
        # Responsibility 1: Tool management
        self.tool_client = tool_client or ToolClient(...)
        
        # Responsibility 2: Protocol management
        self.protocol_client = protocol_client or ProtocolClient(...)
        
        # Responsibility 3: Reporting
        self._reporter = reporter or FuzzerReporter(...)
        
        # Responsibility 4: Safety
        self.safety_system = safety_system or SafetyFilter()
        
        # Responsibility 5: Auth
        self.auth_manager = auth_manager or AuthManager()

    # Responsibility 1: Tool fuzzing (7 methods)
    async def fuzz_tool(self, tool, runs=10, tool_timeout=None): ...
    async def fuzz_all_tools(self, runs_per_tool=10, tool_timeout=None): ...
    async def fuzz_tool_both_phases(self, tool, runs_per_phase=5): ...
    
    # Responsibility 2: Protocol fuzzing (2 methods)
    async def fuzz_protocol_type(self, protocol_type, runs=10): ...
    async def fuzz_all_protocol_types(self, runs_per_type=5): ...
    
    # Responsibility 3: Reporting (7 methods)
    def print_tool_summary(self, results): ...
    def print_protocol_summary(self, results): ...
    def print_safety_statistics(self): ...
    
    # Responsibility 4: Cleanup
    async def cleanup(self): ...

Why This is Bad:

• Changes to tool fuzzing logic require modifying this class
• Changes to protocol fuzzing logic require modifying this class
• Changes to reporting require modifying this class
• Changes to safety require modifying this class
• Hard to test individual responsibilities
• Hard to reuse components independently

✅ Better Approach (Inspired by PR #129):

# Separate responsibilities into focused classes

class FuzzingOrchestrator:
    """SINGLE responsibility: Coordinate fuzzing operations"""
    def __init__(self, tool_fuzzer, protocol_fuzzer):
        self.tool_fuzzer = tool_fuzzer
        self.protocol_fuzzer = protocol_fuzzer
    
    async def execute_fuzzing(self, config): ...

class ToolFuzzingCoordinator:
    """SINGLE responsibility: Tool fuzzing coordination"""
    def __init__(self, client, executor):
        self.client = client
        self.executor = executor
    
    async def fuzz_tools(self, config): ...

class ProtocolFuzzingCoordinator:
    """SINGLE responsibility: Protocol fuzzing coordination"""
    def __init__(self, client, executor):
        self.client = client
        self.executor = executor
    
    async def fuzz_protocols(self, config): ...

class ReportingCoordinator:
    """SINGLE responsibility: Report generation"""
    def __init__(self, reporter):
        self.reporter = reporter
    
    def generate_reports(self, results): ...

---

❌ VIOLATION #2: ProcessManager (God Class)

File: mcp_fuzzer/fuzz_engine/runtime/manager.py (565 lines!)

Problem: This massive class has TOO MANY responsibilities:

1. Process lifecycle management (start/stop)
2. Process monitoring
3. Watchdog integration
4. Signal handling
5. Statistics collection
6. Process registry management
7. Activity tracking

class ProcessManager:
    """
    VIOLATION: 565 lines doing everything related to processes!
    """
    
    def __init__(self, config, config_dict=None):
        # Responsibility 1: Watchdog
        self.watchdog = ProcessWatchdog(self.config)
        
        # Responsibility 2: Process registry
        self._processes: dict[int, dict[str, Any]] = {}
        
        # Responsibility 3: Locking
        self._lock = None

    # Responsibility 1: Process lifecycle (5 methods)
    async def start_process(self, config): ...
    async def stop_process(self, pid, force=False): ...
    async def stop_all_processes(self, force=False): ...
    
    # Responsibility 2: Process monitoring (3 methods)
    async def get_process_status(self, pid): ...
    async def list_processes(self): ...
    async def wait_for_process(self, pid, timeout=None): ...
    
    # Responsibility 3: Signal handling (4 methods)
    async def send_timeout_signal(self, pid, signal_type="timeout"): ...
    async def _send_term_signal(self, pid, process, name): ...
    async def _send_kill_signal(self, pid, process, name): ...
    async def _send_interrupt_signal(self, pid, process, name): ...
    
    # Responsibility 4: Statistics (2 methods)
    async def get_stats(self): ...
    async def cleanup_finished_processes(self): ...
    
    # Responsibility 5: Activity tracking (2 methods)
    async def update_activity(self, pid): ...
    def _get_activity_timestamp(self): ...

✅ Better Approach:

class ProcessLifecycleManager:
    """SINGLE responsibility: Start and stop processes"""
    async def start(self, config): ...
    async def stop(self, pid): ...

class ProcessRegistry:
    """SINGLE responsibility: Track running processes"""
    def register(self, pid, process): ...
    def unregister(self, pid): ...
    def get_process(self, pid): ...

class ProcessSignalHandler:
    """SINGLE responsibility: Send signals to processes"""
    async def send_term(self, pid): ...
    async def send_kill(self, pid): ...
    async def send_interrupt(self, pid): ...

class ProcessMonitor:
    """SINGLE responsibility: Monitor process health"""
    async def get_status(self, pid): ...
    async def get_statistics(self): ...

---

❌ VIOLATION #3: CLI Args Module (God Module)

File: mcp_fuzzer/cli/args.py (671 lines!)

Problem: This module does EVERYTHING for CLI:

1. Argument parsing
2. Argument validation
3. Logging setup
4. Configuration file loading
5. Authentication resolution
6. Config merging
7. Default value management

# TOO MANY responsibilities in one file!

def create_argument_parser(): 
    # Responsibility 1: Define ALL arguments (200+ lines)
    parser = argparse.ArgumentParser(...)
    parser.add_argument("--mode", ...)
    parser.add_argument("--protocol", ...)
    # ... 50+ more arguments!
    return parser

def parse_arguments():
    # Responsibility 2: Parse args
    ...

def validate_arguments(args):
    # Responsibility 3: Validate args
    ...

def setup_logging(args):
    # Responsibility 4: Configure logging
    ...

def _resolve_auth_manager(args):
    # Responsibility 5: Resolve authentication
    ...

def get_cli_config():
    # Responsibility 6: Load config files
    # Responsibility 7: Merge configs
    # Responsibility 8: Transfer config values
    # ... 100+ lines doing many things!
    ...

✅ Better Approach:

# cli/parser.py
class CLIArgumentParser:
    """SINGLE responsibility: Parse CLI arguments"""
    def parse(self, argv): ...

# cli/validator.py
class ArgumentValidator:
    """SINGLE responsibility: Validate arguments"""
    def validate(self, args): ...

# cli/config_loader.py
class ConfigurationLoader:
    """SINGLE responsibility: Load configuration"""
    def load_from_file(self, path): ...
    def merge_with_args(self, config, args): ...

# cli/logging_setup.py
class LoggingConfigurator:
    """SINGLE responsibility: Configure logging"""
    def setup(self, level, verbose): ...

# cli/auth_resolver.py
class AuthenticationResolver:
    """SINGLE responsibility: Resolve auth config"""
    def resolve(self, args): ...

---

2. Open/Closed Principle (OCP)

📖 Definition

"Software entities should be open for extension, but closed for modification."

You should be able to add new functionality without changing existing code.

📚 References

• OCP by Uncle Bob
• Refactoring Guru - OCP

❌ VIOLATION #1: Transport Factory (Hardcoded Logic)

File: mcp_fuzzer/transport/factory.py

Problem: Adding a new transport requires modifying the factory function with more if-else statements.

def create_transport(url_or_protocol: str, endpoint: str | None = None, **kwargs):
    """
    VIOLATION: Every new transport type requires modifying this function!
    """
    # ... parsing logic ...
    
    # Hardcoded if-else chain - CLOSED for extension!
    if scheme in ("http", "https"):
        return registry.create_transport("http", url_or_protocol, **kwargs)
    if scheme == "sse":
        # Convert sse://host/path to http://host/path
        http_url = urlunparse(...)
        return registry.create_transport("sse", http_url, **kwargs)
    if scheme == "stdio":
        # Allow stdio:cmd or stdio://cmd
        cmd = ...
        return registry.create_transport("stdio", cmd, **kwargs)
    if scheme == "streamablehttp":
        http_url = urlunparse(...)
        return registry.create_transport("streamablehttp", http_url, **kwargs)
    
    # Adding websocket? Must modify this function!
    # if scheme == "websocket":
    #     return registry.create_transport("websocket", ...)
    
    raise TransportRegistrationError(f"Unsupported URL scheme: {scheme}")

Why This is Bad:

• Every new transport requires modifying this function
• Risk of breaking existing transports when adding new ones
• Violates "closed for modification" principle

✅ Better Approach (Strategy Pattern):

# Define transport creation strategy
class TransportCreationStrategy(ABC):
    @abstractmethod
    def can_handle(self, scheme: str) -> bool: ...
    
    @abstractmethod
    def create(self, url: str, **kwargs) -> TransportProtocol: ...

# Each transport has its own strategy
class HTTPTransportStrategy(TransportCreationStrategy):
    def can_handle(self, scheme: str) -> bool:
        return scheme in ("http", "https")
    
    def create(self, url: str, **kwargs) -> TransportProtocol:
        return HTTPTransport(url, **kwargs)

class SSETransportStrategy(TransportCreationStrategy):
    def can_handle(self, scheme: str) -> bool:
        return scheme == "sse"
    
    def create(self, url: str, **kwargs) -> TransportProtocol:
        http_url = self._convert_sse_to_http(url)
        return SSETransport(http_url, **kwargs)

# Factory uses strategies - OPEN for extension!
class TransportFactory:
    def __init__(self):
        self.strategies: list[TransportCreationStrategy] = []
    
    def register_strategy(self, strategy: TransportCreationStrategy):
        """Add new transport without modifying factory!"""
        self.strategies.append(strategy)
    
    def create(self, url: str, **kwargs) -> TransportProtocol:
        parsed = urlparse(url)
        for strategy in self.strategies:
            if strategy.can_handle(parsed.scheme):
                return strategy.create(url, **kwargs)
        raise TransportRegistrationError(f"Unsupported scheme: {parsed.scheme}")

# Usage - Adding new transport is EASY!
factory = TransportFactory()
factory.register_strategy(HTTPTransportStrategy())
factory.register_strategy(SSETransportStrategy())
factory.register_strategy(StdioTransportStrategy())

# New transport? Just add strategy - NO modification needed!
factory.register_strategy(WebSocketTransportStrategy())

---

❌ VIOLATION #2: Phase Selection (Hardcoded If-Else)

File: mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py

Problem: Adding new fuzzing phases requires modifying this function.

class ToolStrategies:
    @staticmethod
    async def fuzz_tool_arguments(tool, phase="aggressive"):
        """
        VIOLATION: Adding new phases requires modifying this method!
        """
        if phase == ToolStrategies.REALISTIC_PHASE:
            return await fuzz_tool_arguments_realistic(tool)
        else:  # aggressive
            return fuzz_tool_arguments_aggressive(tool)
        
        # Want to add "chaos" phase? Must modify this code!
        # elif phase == "chaos":
        #     return fuzz_tool_arguments_chaos(tool)

✅ Better Approach (Strategy Pattern):

class FuzzingPhaseStrategy(ABC):
    @abstractmethod
    async def fuzz_arguments(self, tool): ...

class RealisticPhaseStrategy(FuzzingPhaseStrategy):
    async def fuzz_arguments(self, tool):
        return await fuzz_tool_arguments_realistic(tool)

class AggressivePhaseStrategy(FuzzingPhaseStrategy):
    async def fuzz_arguments(self, tool):
        return fuzz_tool_arguments_aggressive(tool)

class ChaosPhaseStrategy(FuzzingPhaseStrategy):
    """NEW phase - no modification of existing code!"""
    async def fuzz_arguments(self, tool):
        return fuzz_tool_arguments_chaos(tool)

class ToolStrategies:
    def __init__(self):
        self.strategies = {
            "realistic": RealisticPhaseStrategy(),
            "aggressive": AggressivePhaseStrategy(),
            # Add new phases without modifying class!
            "chaos": ChaosPhaseStrategy(),
        }
    
    async def fuzz_tool_arguments(self, tool, phase="aggressive"):
        strategy = self.strategies.get(phase)
        if not strategy:
            raise ValueError(f"Unknown phase: {phase}")
        return await strategy.fuzz_arguments(tool)

---

3. Liskov Substitution Principle (LSP)

📖 Definition

"Objects of a superclass should be replaceable with objects of a subclass without breaking the application."

Subclasses must be substitutable for their base classes without altering the correctness of the program.

📚 References

• LSP by Uncle Bob
• Liskov Substitution Principle Explained

❌ VIOLATION #1: Transport Implementations

File: mcp_fuzzer/transport/base.py and implementations

Problem: Different transports have incompatible behaviors that violate substitutability.

class TransportProtocol(ABC):
    """Base protocol that all transports should follow"""
    @abstractmethod
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        pass

# Implementation 1: HTTPTransport - works as expected
class HTTPTransport(TransportProtocol):
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        # Returns dict response
        response = await self.client.post(...)
        return response.json()

# Implementation 2: SSETransport - VIOLATES LSP!
class SSETransport(TransportProtocol):
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        """
        VIOLATION: This raises NotImplementedError!
        Breaks substitutability - can't use where HTTPTransport is expected!
        """
        raise NotImplementedError("SSETransport does not support send_request")

# Implementation 3: StdioTransport - Different behavior!
class StdioTransport(TransportProtocol):
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        """
        VIOLATION: Uses module-level sys.stdin/stdout in _send_request
        Not truly async - different behavior than HTTPTransport!
        """
        # This compatibility method uses synchronous I/O!
        message = {**payload, "id": self.request_id, "jsonrpc": "2.0"}
        sys.stdout.write(json.dumps(message))  # Synchronous!
        line = sys.stdin.readline()  # Blocking!

Why This is Bad:

• Cannot substitute SSETransport where HTTPTransport is expected
• Code that works with HTTPTransport may fail with StdioTransport
• Client code needs to know which transport type is being used

Example of Breakage:

async def fuzz_with_transport(transport: TransportProtocol):
    """This function expects ANY transport to work"""
    # Works with HTTPTransport
    result = await transport.send_request("tools/list", {})
    
    # FAILS with SSETransport - raises NotImplementedError!
    # BLOCKS with StdioTransport - synchronous I/O!

# This violates LSP - substitution causes failure
http_transport = HTTPTransport("http://localhost")
await fuzz_with_transport(http_transport)  # ✅ Works

sse_transport = SSETransport("http://localhost/sse")
await fuzz_with_transport(sse_transport)  # ❌ Crashes!

✅ Better Approach:

# Option 1: Split interfaces (see ISP below)
class StreamingTransport(ABC):
    """For transports that only support streaming"""
    @abstractmethod
    async def stream_request(self, method: str, params: dict | None = None):
        pass

class RequestResponseTransport(ABC):
    """For transports that support request/response"""
    @abstractmethod
    async def send_request(self, method: str, params: dict | None = None) -> dict:
        pass

# SSE only implements streaming
class SSETransport(StreamingTransport):
    async def stream_request(self, method: str, params: dict | None = None):
        # Implementation that makes sense for SSE
        ...

# HTTP implements both
class HTTPTransport(RequestResponseTransport, StreamingTransport):
    async def send_request(self, method: str, params: dict | None = None) -> dict:
        ...
    
    async def stream_request(self, method: str, params: dict | None = None):
        ...

# Option 2: Make all transports truly async
class StdioTransport(RequestResponseTransport):
    async def send_request(self, method: str, params: dict | None = None) -> dict:
        """Now truly async - substitutable!"""
        if not self._initialized:
            await self._ensure_connection()
        
        # Use asyncio subprocess for true async I/O
        message_str = json.dumps(message) + "\n"
        self.stdin.write(message_str.encode())
        await self.stdin.drain()  # Async!
        
        line = await self.stdout.readline()  # Async!
        return json.loads(line.decode())

---

4. Interface Segregation Principle (ISP)

📖 Definition

"Clients should not be forced to depend on interfaces they do not use."

Many small, specific interfaces are better than one large, general-purpose interface.

📚 References

• ISP by Uncle Bob
• Interface Segregation Principle

❌ VIOLATION #1: TransportProtocol (Fat Interface)

File: mcp_fuzzer/transport/base.py

Problem: All transports must implement ALL methods, even if they don't need them.

class TransportProtocol(ABC):
    """
    VIOLATION: Fat interface - forces all implementations to have ALL methods!
    """
    @abstractmethod
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        """Required for ALL transports"""
        pass

    @abstractmethod
    async def send_raw(self, payload: dict) -> Any:
        """Required for ALL transports"""
        pass

    @abstractmethod
    async def send_notification(self, method: str, params: dict | None = None) -> None:
        """Required for ALL transports"""
        pass

    async def call_tool(self, tool_name: str, arguments: dict) -> dict:
        """Required for ALL transports"""
        pass

    async def get_tools(self) -> list:
        """Required for ALL transports"""
        pass
    
    # ... more methods that not all transports need!
    
    @abstractmethod
    async def _stream_request(self, payload: dict):
        """
        PROBLEM: Not all transports support streaming!
        But they're forced to implement this method!
        """
        pass

# Result: SSETransport forced to implement send_request even though it doesn't support it
class SSETransport(TransportProtocol):
    async def send_request(self, method: str, params: dict | None = None) -> Any:
        # Forced to implement - but can't support it!
        raise NotImplementedError("SSETransport does not support send_request")

Why This is Bad:

• Forces implementations to provide methods they don't need
• Leads to dummy implementations or NotImplementedError
• Violates LSP (substitutability)
• Makes testing harder

✅ Better Approach (Segregated Interfaces):

# Split into smaller, focused interfaces

class BasicTransport(ABC):
    """Minimal interface - just sending/receiving"""
    @abstractmethod
    async def send(self, payload: dict) -> dict:
        pass

class ToolTransport(ABC):
    """Interface for tool operations"""
    @abstractmethod
    async def call_tool(self, tool_name: str, arguments: dict) -> dict:
        pass
    
    @abstractmethod
    async def get_tools(self) -> list:
        pass

class StreamingTransport(ABC):
    """Interface for streaming operations"""
    @abstractmethod
    async def stream(self, payload: dict):
        pass

class NotificationTransport(ABC):
    """Interface for fire-and-forget notifications"""
    @abstractmethod
    async def send_notification(self, method: str, params: dict | None = None):
        pass

# Now implementations only implement what they need!

class HTTPTransport(BasicTransport, ToolTransport, NotificationTransport):
    """HTTP supports everything except streaming"""
    async def send(self, payload: dict) -> dict: ...
    async def call_tool(self, tool_name: str, arguments: dict) -> dict: ...
    async def get_tools(self) -> list: ...
    async def send_notification(self, method: str, params: dict | None = None): ...

class SSETransport(StreamingTransport):
    """SSE only supports streaming - clean!"""
    async def stream(self, payload: dict): ...
    # No need to implement send_request!

class StdioTransport(BasicTransport, ToolTransport):
    """Stdio supports basic operations and tools"""
    async def send(self, payload: dict) -> dict: ...
    async def call_tool(self, tool_name: str, arguments: dict) -> dict: ...
    async def get_tools(self) -> list: ...
    # No streaming interface - clean!

# Usage - type hints make capabilities clear
def use_basic_transport(transport: BasicTransport):
    """Only needs basic send/receive"""
    await transport.send({"method": "ping"})

def use_streaming_transport(transport: StreamingTransport):
    """Only needs streaming"""
    async for chunk in transport.stream({"method": "stream"}):
        process(chunk)

---

❌ VIOLATION #2: SafetyProvider (Too Many Methods)

File: mcp_fuzzer/safety_system/safety.py

Problem: Not all safety providers need all these methods.

@runtime_checkable
class SafetyProvider(Protocol):
    """
    VIOLATION: Fat interface forcing all providers to implement everything!
    """
    def set_fs_root(self, root: str | Path) -> None: ...
    
    def sanitize_tool_arguments(self, tool_name: str, arguments: dict) -> dict: ...
    
    def should_skip_tool_call(self, tool_name: str, arguments: dict) -> bool: ...
    
    def create_safe_mock_response(self, tool_name: str) -> dict: ...
    
    def log_blocked_operation(self, tool_name: str, arguments: dict, reason: str) -> None: ...
    
    # What if I want a simple safety provider that only sanitizes?
    # Still forced to implement ALL of these!

✅ Better Approach:

class ArgumentSanitizer(Protocol):
    """Focused on sanitizing arguments"""
    def sanitize_tool_arguments(self, tool_name: str, arguments: dict) -> dict: ...

class CallFilter(Protocol):
    """Focused on filtering calls"""
    def should_skip_tool_call(self, tool_name: str, arguments: dict) -> bool: ...

class SafetyLogger(Protocol):
    """Focused on logging"""
    def log_blocked_operation(self, tool_name: str, arguments: dict, reason: str) -> None: ...

class FilesystemSafety(Protocol):
    """Focused on filesystem operations"""
    def set_fs_root(self, root: str | Path) -> None: ...

# Now implementations pick what they need!
class SimpleSanitizer:
    """Just sanitizes - doesn't need other methods!"""
    def sanitize_tool_arguments(self, tool_name: str, arguments: dict) -> dict:
        return self._remove_dangerous_values(arguments)

class FullSafetyFilter(ArgumentSanitizer, CallFilter, SafetyLogger, FilesystemSafety):
    """Full implementation for complete safety"""
    def sanitize_tool_arguments(self, tool_name: str, arguments: dict) -> dict: ...
    def should_skip_tool_call(self, tool_name: str, arguments: dict) -> bool: ...
    def log_blocked_operation(self, tool_name: str, arguments: dict, reason: str) -> None: ...
    def set_fs_root(self, root: str | Path) -> None: ...

---

5. Dependency Inversion Principle (DIP)

📖 Definition

"High-level modules should not depend on low-level modules. Both should depend on abstractions."

Depend on abstractions (interfaces), not concrete implementations.

📚 References

• DIP by Uncle Bob
• Dependency Inversion Principle

❌ VIOLATION #1: Direct Instantiation Everywhere

File: mcp_fuzzer/client/base.py

Problem: Client directly instantiates concrete classes instead of depending on abstractions.

class MCPFuzzerClient:
    def __init__(self, transport, auth_manager=None, ...):
        """
        VIOLATION: Direct instantiation of concrete classes!
        High-level client depends on low-level implementations!
        """
        # Concrete dependency #1
        self.auth_manager = auth_manager or AuthManager()  # Concrete class!
        
        # Concrete dependency #2
        self._reporter = reporter or FuzzerReporter(safety_system=safety_system)  # Concrete!
        
        # Concrete dependency #3
        if not safety_enabled:
            self.safety_system = None
        else:
            self.safety_system = safety_system or SafetyFilter()  # Concrete!
        
        # Concrete dependency #4
        self.tool_client = tool_client or ToolClient(  # Concrete!
            transport=transport,
            auth_manager=self.auth_manager,  # Concrete!
            safety_system=self.safety_system,  # Concrete!
        )
        
        # Concrete dependency #5
        self.protocol_client = protocol_client or ProtocolClient(  # Concrete!
            transport=transport,
            safety_system=self.safety_system,  # Concrete!
        )

Why This is Bad:

• Cannot easily swap implementations for testing
• Tightly coupled to specific implementations
• Hard to mock dependencies
• Cannot configure behavior without changing code

✅ Better Approach (Dependency Injection):

# Define abstractions (interfaces)
class AuthenticationProvider(ABC):
    @abstractmethod
    def get_auth_for_tool(self, tool_name: str) -> dict: ...

class ReportingService(ABC):
    @abstractmethod
    def report_tool_result(self, result): ...

class SafetyService(ABC):
    @abstractmethod
    def sanitize_arguments(self, tool_name: str, args: dict) -> dict: ...

class ToolFuzzingService(ABC):
    @abstractmethod
    async def fuzz_tool(self, tool, runs: int): ...

class ProtocolFuzzingService(ABC):
    @abstractmethod
    async def fuzz_protocol(self, protocol_type, runs: int): ...

# High-level client depends on abstractions
class MCPFuzzerClient:
    def __init__(
        self,
        transport: TransportProtocol,  # Abstract!
        auth_provider: AuthenticationProvider,  # Abstract!
        reporter: ReportingService,  # Abstract!
        safety_service: SafetyService,  # Abstract!
        tool_fuzzer: ToolFuzzingService,  # Abstract!
        protocol_fuzzer: ProtocolFuzzingService,  # Abstract!
    ):
        """
        GOOD: Depends on abstractions, not concrete classes!
        Dependencies injected from outside!
        """
        self.transport = transport
        self.auth_provider = auth_provider
        self.reporter = reporter
        self.safety_service = safety_service
        self.tool_fuzzer = tool_fuzzer
        self.protocol_fuzzer = protocol_fuzzer
    
    async def fuzz_tool(self, tool, runs=10):
        # Use abstraction - don't care about implementation!
        return await self.tool_fuzzer.fuzz_tool(tool, runs)

# Factory or builder creates concrete implementations
class ClientBuilder:
    """Handles concrete instantiation - not the client!"""
    def build(self, config) -> MCPFuzzerClient:
        # Create concrete implementations based on config
        transport = self._create_transport(config)
        auth_provider = self._create_auth_provider(config)
        reporter = self._create_reporter(config)
        safety_service = self._create_safety_service(config)
        tool_fuzzer = self._create_tool_fuzzer(config)
        protocol_fuzzer = self._create_protocol_fuzzer(config)
        
        # Inject all dependencies
        return MCPFuzzerClient(
            transport=transport,
            auth_provider=auth_provider,
            reporter=reporter,
            safety_service=safety_service,
            tool_fuzzer=tool_fuzzer,
            protocol_fuzzer=protocol_fuzzer,
        )

# Now testing is easy!
def test_fuzzer():
    # Mock all dependencies
    mock_transport = MockTransport()
    mock_auth = MockAuthProvider()
    mock_reporter = MockReporter()
    mock_safety = MockSafetyService()
    mock_tool_fuzzer = MockToolFuzzer()
    mock_protocol_fuzzer = MockProtocolFuzzer()
    
    # Inject mocks
    client = MCPFuzzerClient(
        transport=mock_transport,
        auth_provider=mock_auth,
        reporter=mock_reporter,
        safety_service=mock_safety,
        tool_fuzzer=mock_tool_fuzzer,
        protocol_fuzzer=mock_protocol_fuzzer,
    )
    
    # Easy to test!
    await client.fuzz_tool(test_tool)

---

❌ VIOLATION #2: ToolClient Directly Creates Dependencies

File: mcp_fuzzer/client/tool_client.py

Problem: Creates concrete dependencies instead of receiving abstractions.

class ToolClient:
    def __init__(
        self,
        transport,
        auth_manager=None,
        safety_system=None,
        max_concurrency=5,
    ):
        """
        VIOLATION: Creates concrete dependencies!
        """
        self.transport = transport
        
        # Concrete instantiation #1
        self.auth_manager = auth_manager or AuthManager()  # Concrete!
        
        # Concrete instantiation #2
        if not enable_safety:
            self.safety_system = None
        else:
            self.safety_system = safety_system or SafetyFilter()  # Concrete!
        
        # Concrete instantiation #3
        self.tool_fuzzer = ToolFuzzer(  # Concrete!
            max_concurrency=max_concurrency,
            safety_system=self.safety_system,
        )

✅ Better Approach:

class ToolClient:
    def __init__(
        self,
        transport: TransportProtocol,  # Abstract!
        auth_provider: AuthenticationProvider,  # Abstract!
        safety_service: SafetyService | None,  # Abstract!
        fuzzer: ToolFuzzingService,  # Abstract!
    ):
        """
        GOOD: All dependencies injected as abstractions!
        No concrete instantiation!
        """
        self.transport = transport
        self.auth_provider = auth_provider
        self.safety_service = safety_service
        self.fuzzer = fuzzer
    
    async def fuzz_tool(self, tool, runs=10):
        # Use injected dependencies
        auth = self.auth_provider.get_auth_for_tool(tool.name)
        if self.safety_service:
            args = self.safety_service.sanitize_arguments(tool.name, args)
        return await self.fuzzer.fuzz_tool(tool, runs)

---

❌ VIOLATION #3: CLI Creates Concrete Transport

File: mcp_fuzzer/cli/args.py

Problem: CLI code directly creates concrete transport instances.

def build_unified_client_args(args):
    """
    VIOLATION: Function creates concrete transport!
    """
    # Direct creation of concrete class
    from ..transport import create_transport  # Concrete function!
    from ..auth import AuthManager  # Concrete class!
    
    # Creates concrete transport directly
    transport = create_transport(args.protocol, args.endpoint)  # Concrete!
    
    # Creates concrete auth manager directly
    auth_manager = AuthManager()  # Concrete!
    
    # Everything is concrete!
    return {
        "transport": transport,  # Concrete!
        "auth_manager": auth_manager,  # Concrete!
    }

✅ Better Approach (Inspired by PR #129):

# Define factories as abstractions
class TransportFactory(ABC):
    @abstractmethod
    def create(self, protocol: str, endpoint: str) -> TransportProtocol: ...

class AuthFactory(ABC):
    @abstractmethod
    def create(self, config: dict) -> AuthenticationProvider: ...

# Dependency container holds abstractions
@dataclass
class Dependencies:
    transport_factory: TransportFactory
    auth_factory: AuthFactory
    safety_factory: SafetyFactory
    # All abstractions!

# CLI uses abstractions
def build_client(args: Namespace, deps: Dependencies) -> MCPFuzzerClient:
    """
    GOOD: Depends on abstract factories!
    """
    # Use abstract factories
    transport = deps.transport_factory.create(args.protocol, args.endpoint)
    auth_provider = deps.auth_factory.create(args.auth_config)
    safety_service = deps.safety_factory.create(args.safety_config)
    
    # Build with abstractions
    return ClientBuilder(
        transport=transport,
        auth_provider=auth_provider,
        safety_service=safety_service,
    ).build()

# Concrete implementations provided at app entry point
def main():
    # Wire up concrete implementations at root
    deps = Dependencies(
        transport_factory=DefaultTransportFactory(),
        auth_factory=DefaultAuthFactory(),
        safety_factory=DefaultSafetyFactory(),
    )
    
    args = parse_args()
    client = build_client(args, deps)
    # ...

---

Summary & Action Items

🎯 Key Takeaways for Your Junior

1. SRP: If a class does multiple things, split it into focused classes
2. OCP: Use strategies/plugins instead of if-else chains for new features
3. LSP: Subclasses must be truly substitutable for their base class
4. ISP: Many small interfaces > one large interface
5. DIP: Depend on abstractions, inject dependencies

📋 Recommended Refactoring Priority

1. High Priority - SRP Violations:

   • Split MCPFuzzerClient into orchestrator + coordinators
   • Split ProcessManager into lifecycle + registry + monitor
   • Split CLI args.py into parser + validator + config loader

2. Medium Priority - DIP Violations:

   • Add dependency injection to MCPFuzzerClient
   • Create abstract factories for transports/auth/safety
   • Use builder pattern for complex object creation

3. Lower Priority - OCP/ISP Violations:

   • Refactor transport factory to use strategy pattern
   • Split TransportProtocol into smaller interfaces
   • Add plugin system for new fuzzing phases

📚 Next Steps for Learning

1. Read "Clean Code" by Robert C. Martin (Chapters 10-12)
2. Study PR extensive design changes #129 to see dependency injection in action
3. Practice by refactoring one violation at a time
4. Write tests first to ensure behavior doesn't change

Would you like me to:

1. Create detailed refactoring plans for specific violations?
2. Generate example code for any of the suggested improvements?
3. Explain any principle in more depth with additional examples?

---

🧠 Learnings used

Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.

[Agent-Hellboy]

@coderabbitai find code smells and design smells other than SOLID, and explain them with references

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

---

---

---

---

---

---

🏁 Script executed:

#!/bin/bash
# Analyze code smells systematically

echo "=== 1. LONG METHODS (methods with > 50 lines) ==="
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    # Find method definitions and count lines until next method or class
    awk '/^[[:space:]]*def / {
      if (method_name != "") {
        lines = NR - start_line
        if (lines > 50) {
          print file ":" start_line ":" method_name " (" lines " lines)"
        }
      }
      method_name = $0
      start_line = NR
    }
    /^class / {
      if (method_name != "") {
        lines = NR - start_line
        if (lines > 50) {
          print file ":" start_line ":" method_name " (" lines " lines)"
        }
      }
      method_name = ""
    }
    END {
      if (method_name != "") {
        lines = NR - start_line
        if (lines > 50) {
          print file ":" start_line ":" method_name " (" lines " lines)"
        }
      }
    }' file="$file" "$file"
  fi
done | head -20

echo -e "\n=== 2. LARGE CLASSES (classes with > 200 lines) ==="
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    wc -l "$file" 2>/dev/null | awk '$1 > 200 {print}'
  fi
done | sort -rn | head -15

echo -e "\n=== 3. MAGIC NUMBERS (hardcoded numbers in code) ==="
rg -n '\b(10|20|30|50|100|200|500|1000)\b' --type py -g '!test*' -g '!*_test.py' | head -30

echo -e "\n=== 4. MAGIC STRINGS (hardcoded strings) ==="
rg -n '"(http://|https://|localhost|127\.0\.0\.1)"' --type py | head -20

echo -e "\n=== 5. LONG PARAMETER LISTS (functions with > 5 params) ==="
rg -n 'def \w+\([^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*' --type py | head -30

Length of output: 6974

---

🏁 Script executed:

#!/bin/bash
# More code smell analysis

echo "=== 6. DUPLICATE CODE PATTERNS ==="
echo "--- Repeated exception handling ---"
rg -n 'try:' --type py -A 3 | grep -A 3 'except.*:' | head -40

echo -e "\n=== 7. NESTED CONDITIONALS (deeply nested if statements) ==="
rg -n '^\s{16,}if ' --type py | head -30

echo -e "\n=== 8. GOD OBJECTS (classes importing many modules) ==="
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    count=$(grep -c '^import\|^from.*import' "$file" 2>/dev/null || echo 0)
    if [ "$count" -gt 15 ]; then
      echo "$file: $count imports"
    fi
  fi
done | sort -t: -k2 -rn | head -15

echo -e "\n=== 9. PRIMITIVE OBSESSION (using primitives instead of objects) ==="
rg -n 'def.*\(.*: str.*,.*: str.*,.*: str' --type py | head -20

echo -e "\n=== 10. FEATURE ENVY (methods using other class's data heavily) ==="
rg -n 'self\.\w+\.\w+\.\w+\.\w+' --type py | head -30

Length of output: 8008

---

🏁 Script executed:

#!/bin/bash
# Even more smell analysis

echo "=== 11. DATA CLUMPS (same group of variables used together) ==="
echo "--- Timeout parameters appearing together ---"
rg -n 'timeout.*tool_timeout' --type py
rg -n 'timeout.*response_timeout' --type py

echo -e "\n=== 12. LAZY CLASS (classes doing too little) ==="
echo "Looking for very small classes..."
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    # Find classes with very few methods
    awk '/^class / {
      classname = $2
      methods = 0
      in_class = 1
    }
    in_class && /^[[:space:]]+def / {
      methods++
    }
    in_class && /^class / && NR > 1 {
      if (methods > 0 && methods < 3) {
        print FILENAME ":" classname " (" methods " methods)"
      }
      classname = $2
      methods = 0
    }
    END {
      if (in_class && methods > 0 && methods < 3) {
        print FILENAME ":" classname " (" methods " methods)"
      }
    }' "$file"
  fi
done | head -20

echo -e "\n=== 13. DEAD CODE / UNUSED IMPORTS ==="
echo "--- Unused imports (simple check) ---"
rg -n '^import \w+$' --type py | head -20

echo -e "\n=== 14. INAPPROPRIATE INTIMACY (classes accessing private members of others) ==="
rg -n '\._[a-z]\w+\._[a-z]' --type py | head -20

echo -e "\n=== 15. COMMENTS AS DEODORANT (excessive comments hiding bad code) ==="
echo "Files with high comment ratio:"
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    total=$(wc -l < "$file")
    comments=$(grep -c '^\s*#' "$file" 2>/dev/null || echo 0)
    if [ "$total" -gt 50 ] && [ "$comments" -gt 20 ]; then
      ratio=$((comments * 100 / total))
      if [ "$ratio" -gt 15 ]; then
        echo "$file: $comments comments / $total lines ($ratio%)"
      fi
    fi
  fi
done | head -15

Length of output: 4619

---

🏁 Script executed:

#!/bin/bash
# Analyze specific problematic patterns

echo "=== 16. MIDDLE MAN (classes that just delegate) ==="
echo "Looking for classes with lots of delegation..."
rg -n 'return.*self\.\w+\.\w+\(' --type py -A 1 -B 1 | head -40

echo -e "\n=== 17. MESSAGE CHAINS (Law of Demeter violations) ==="
rg -n '\w+\.\w+\.\w+\.\w+\(' --type py | head -30

echo -e "\n=== 18. SHOTGUN SURGERY indicators (single concept scattered) ==="
echo "--- Multiple files handling safety ---"
rg -l 'safety' --type py | wc -l
rg -l 'SafetyFilter|SafetyProvider' --type py

echo -e "\n--- Multiple files handling auth ---"
rg -l 'auth' --type py | wc -l
rg -l 'AuthManager|AuthProvider' --type py

echo -e "\n=== 19. DIVERGENT CHANGE indicators (class changes for many reasons) ==="
echo "Files with high method count (likely doing many things):"
for file in $(fd -e py -t f); do
  if [ -f "$file" ]; then
    methods=$(grep -c '^[[:space:]]*def ' "$file" 2>/dev/null || echo 0)
    if [ "$methods" -gt 15 ]; then
      echo "$file: $methods methods"
    fi
  fi
done | sort -t: -k2 -rn | head -10

echo -e "\n=== 20. SWITCH STATEMENTS (type checking) ==="
rg -n 'if.*isinstance\(|if.*type\(' --type py | head -30

Length of output: 12659

---

🏁 Script executed:

#!/bin/bash
# Final patterns and metrics

echo "=== 21. TEMPORARY FIELD (fields only used sometimes) ==="
echo "Looking for optional/None fields..."
rg -n 'self\.\w+\s*=\s*None' --type py | head -25

echo -e "\n=== 22. REFUSED BEQUEST (subclass doesn't use parent's methods) ==="
echo "Looking for NotImplementedError..."
rg -n 'raise NotImplementedError' --type py -B 3

echo -e "\n=== 23. PARALLEL INHERITANCE HIERARCHIES ==="
echo "Looking for parallel class structures..."
rg -l 'class.*Client' --type py
rg -l 'class.*Strategy' --type py

echo -e "\n=== 24. INCOMPLETE LIBRARY CLASS (missing utility methods) ==="
echo "Looking for monkey patching or extensions..."
rg -n 'setattr|__setattr__|__getattr__' --type py | head -20

echo -e "\n=== 25. CALLBACK HELL (nested async callbacks) ==="
echo "Looking for nested async patterns..."
rg -n 'async.*async.*async' --type py | head -20
rg -n '^\s{12,}await.*await' --type py | head -20

Length of output: 3546

---

🏁 Script executed:

#!/bin/bash
# Get specific examples for detailed analysis

echo "=== GETTING SPECIFIC EXAMPLES ==="

echo -e "\n--- Example: Long method in cli/args.py ---"
sed -n '400,500p' mcp_fuzzer/cli/args.py

echo -e "\n--- Example: Long parameter list ---"
sed -n '1,100p' mcp_fuzzer/client/base.py | grep -A 15 'def __init__'

echo -e "\n--- Example: Duplicate exception handling ---"
rg -n 'try:' --type py -A 5 mcp_fuzzer/client/tool_client.py | head -40

echo -e "\n--- Example: Magic numbers ---"
rg -n '\b(10|100|500)\b' --type py mcp_fuzzer/fuzz_engine/ | head -20

echo -e "\n--- Example: Data clumps ---"
rg -n 'timeout' --type py mcp_fuzzer/client/base.py

Length of output: 8832

---

@Agent-Hellboy I'll now provide a comprehensive code smells and design smells analysis with detailed explanations and references for your junior developer.

---

🔍 Code Smells & Design Smells Deep Dive

Table of Contents

1. What are Code Smells?
2. Method-Level Smells
3. Class-Level Smells
4. Data-Level Smells
5. Structural Smells

---

What are Code Smells?

Code smells are indicators of potential problems in your code. They're not bugs, but rather weaknesses in design that may slow down development or increase the risk of bugs in the future. The term was popularized by Martin Fowler in his book "Refactoring."

📚 Essential References

• Refactoring by Martin Fowler - The definitive guide
• Refactoring Guru - Code Smells - Interactive catalog
• Clean Code by Robert C. Martin
• Working Effectively with Legacy Code by Michael Feathers

---

Method-Level Smells

1. 🐘 Long Method

📖 Definition

Methods that are too long and try to do too many things. Generally, methods longer than 20-30 lines should be scrutinized.

📚 References

• Refactoring Guru - Long Method
• Martin Fowler: "The longer a method is, the more difficult it is to understand."

❌ VIOLATION: create_argument_parser() - 332 Lines!

File: mcp_fuzzer/cli/args.py:12-344

Problem: This method is massive and does ONE thing in way too many lines.

def create_argument_parser() -> argparse.ArgumentParser:
    """
    VIOLATION: 332 LINES of argument definitions!
    This is a TEXTBOOK example of Long Method smell.
    """
    parser = argparse.ArgumentParser(...)
    
    # Lines 12-50: Basic arguments
    parser.add_argument("protocol", ...)
    parser.add_argument("endpoint", ...)
    parser.add_argument("--mode", ...)
    
    # Lines 51-100: Tool arguments
    parser.add_argument("--tool", ...)
    parser.add_argument("--tool-timeout", ...)
    
    # Lines 101-150: Protocol arguments
    parser.add_argument("--protocol-type", ...)
    
    # Lines 151-200: Safety arguments
    parser.add_argument("--no-safety", ...)
    parser.add_argument("--fs-root", ...)
    
    # Lines 201-250: Reporter arguments
    parser.add_argument("--export-json", ...)
    parser.add_argument("--export-csv", ...)
    
    # Lines 251-300: Auth arguments
    parser.add_argument("--auth-config", ...)
    
    # Lines 301-332: Config arguments
    parser.add_argument("--config", ...)
    
    return parser  # Finally!

Why This is Bad:

• Impossible to understand at a glance
• Hard to test individual argument groups
• Difficult to maintain - changes anywhere require understanding everything
• Violates Single Level of Abstraction Principle

✅ Better Approach:

class ArgumentParserBuilder:
    """Build argument parser step by step"""
    
    def __init__(self):
        self.parser = argparse.ArgumentParser(...)
    
    def add_basic_arguments(self) -> "ArgumentParserBuilder":
        """Add protocol, endpoint, mode (10-15 lines)"""
        self.parser.add_argument("protocol", ...)
        self.parser.add_argument("endpoint", ...)
        self.parser.add_argument("--mode", ...)
        return self
    
    def add_tool_arguments(self) -> "ArgumentParserBuilder":
        """Add tool-related arguments (10-15 lines)"""
        self.parser.add_argument("--tool", ...)
        self.parser.add_argument("--tool-timeout", ...)
        return self
    
    def add_protocol_arguments(self) -> "ArgumentParserBuilder":
        """Add protocol-related arguments (10-15 lines)"""
        self.parser.add_argument("--protocol-type", ...)
        return self
    
    def add_safety_arguments(self) -> "ArgumentParserBuilder":
        """Add safety-related arguments (10-15 lines)"""
        self.parser.add_argument("--no-safety", ...)
        self.parser.add_argument("--fs-root", ...)
        return self
    
    def add_reporter_arguments(self) -> "ArgumentParserBuilder":
        """Add export/reporting arguments (10-15 lines)"""
        self.parser.add_argument("--export-json", ...)
        self.parser.add_argument("--export-csv", ...)
        return self
    
    def add_auth_arguments(self) -> "ArgumentParserBuilder":
        """Add authentication arguments (10-15 lines)"""
        self.parser.add_argument("--auth-config", ...)
        return self
    
    def build(self) -> argparse.ArgumentParser:
        """Return the configured parser"""
        return self.parser

# Usage - clean and testable!
def create_argument_parser() -> argparse.ArgumentParser:
    """Now just 5 lines!"""
    return (ArgumentParserBuilder()
        .add_basic_arguments()
        .add_tool_arguments()
        .add_protocol_arguments()
        .add_safety_arguments()
        .add_reporter_arguments()
        .add_auth_arguments()
        .build())

---

❌ VIOLATION: get_cli_config() - 210 Lines!

File: mcp_fuzzer/cli/args.py:428-638

Problem: This method does EVERYTHING related to configuration.

def get_cli_config() -> dict[str, Any]:
    """
    VIOLATION: 210 lines doing:
    1. Getting module references (lines 428-465)
    2. Parsing arguments (lines 466-475)
    3. Debug logging (lines 477-484)
    4. Loading config files (lines 486-502)
    5. Transferring 50+ config values (lines 504-620)
    6. Validating arguments (lines 622-638)
    """
    # 40 lines of module resolution
    cli_module = sys.modules.get("mcp_fuzzer.cli")
    _parse = getattr(cli_module, "parse_arguments", parse_arguments) if cli_module else parse_arguments
    _validate = getattr(cli_module, "validate_arguments", validate_arguments) if cli_module else validate_arguments
    # ... 37 more lines of this!
    
    # Parse args
    args = _parse()
    
    # Load config
    if args.config:
        try:
            config_data = load_config_file(args.config)
            config.update(config_data)
        except Exception as e:
            raise ConfigFileError(...)
    
    # 120 lines of config key transfers!
    _config_keys_to_transfer = [
        ("endpoint", "endpoint"),
        ("protocol", "protocol"),
        # ... 50+ more tuples!
    ]
    
    for args_key, config_key in _config_keys_to_transfer:
        # ... complex logic ...
        pass
    
    # Validation
    _validate(args)
    
    # More processing...
    return {"args": args, "config": config}

✅ Better Approach:

class ConfigurationLoader:
    """SINGLE responsibility: Load configuration"""
    def load_from_file(self, path: str) -> dict: ...
    def load_from_defaults(self) -> dict: ...

class ConfigurationMerger:
    """SINGLE responsibility: Merge configs"""
    def merge(self, cli_args: dict, file_config: dict) -> dict: ...

class ConfigurationValidator:
    """SINGLE responsibility: Validate config"""
    def validate(self, config: dict) -> None: ...

# Orchestrate the steps
def get_cli_config() -> dict[str, Any]:
    """Now clean and focused - 15 lines"""
    # Parse CLI
    args = ArgumentParser().parse()
    
    # Load file config
    loader = ConfigurationLoader()
    file_config = loader.load_from_file(args.config) if args.config else {}
    
    # Merge
    merger = ConfigurationMerger()
    config = merger.merge(vars(args), file_config)
    
    # Validate
    validator = ConfigurationValidator()
    validator.validate(config)
    
    return config

---

2. 📋 Long Parameter List

📖 Definition

Methods with more than 3-4 parameters are hard to understand and use. They often indicate the method is trying to do too much.

📚 References

• Refactoring Guru - Long Parameter List
• Clean Code: "Functions should have a small number of arguments"

❌ VIOLATION: MCPFuzzerClient.init() - 9 Parameters!

File: mcp_fuzzer/client/base.py:25-53

class MCPFuzzerClient:
    def __init__(
        self,
        transport,                      # 1
        auth_manager: AuthManager | None = None,  # 2
        tool_timeout: float | None = None,  # 3
        reporter: FuzzerReporter | None = None,  # 4
        safety_system: SafetyProvider | None = None,  # 5
        safety_enabled: bool = True,    # 6
        max_concurrency: int = 5,       # 7
        tool_client: ToolClient | None = None,  # 8
        protocol_client: ProtocolClient | None = None,  # 9
    ):
        """
        VIOLATION: 9 parameters - too many to remember!
        Hard to use, hard to test, hard to maintain.
        """
        pass

Why This is Bad:

• Hard to remember parameter order
• Difficult to test - need to provide 9 values
• Adding new parameter requires changing all callers
• Indicates class has too many responsibilities

✅ Better Approach (Parameter Object Pattern):

@dataclass
class FuzzerConfig:
    """Parameter Object - groups related parameters"""
    transport: TransportProtocol
    tool_timeout: float | None = None
    safety_enabled: bool = True
    max_concurrency: int = 5

@dataclass
class FuzzerDependencies:
    """Dependency Object - groups injectable dependencies"""
    auth_manager: AuthenticationProvider | None = None
    reporter: ReportingService | None = None
    safety_system: SafetyService | None = None
    tool_client: ToolFuzzingService | None = None
    protocol_client: ProtocolFuzzingService | None = None

class MCPFuzzerClient:
    def __init__(
        self,
        config: FuzzerConfig,           # 1 - All configuration
        dependencies: FuzzerDependencies,  # 2 - All dependencies
    ):
        """
        GOOD: Only 2 parameters!
        Clear separation of concerns.
        Easy to extend without breaking existing code.
        """
        self.config = config
        self.deps = dependencies

# Usage is much cleaner!
config = FuzzerConfig(
    transport=my_transport,
    tool_timeout=30.0,
    safety_enabled=True,
)

deps = FuzzerDependencies(
    auth_manager=my_auth,
    reporter=my_reporter,
)

client = MCPFuzzerClient(config, deps)

---

3. 🔁 Duplicate Code

📖 Definition

The same code structure appears in more than one place. Violates DRY (Don't Repeat Yourself) principle.

📚 References

• Refactoring Guru - Duplicate Code
• The Pragmatic Programmer: "DRY - Don't Repeat Yourself"

❌ VIOLATION: Repeated Exception Handling

Found in multiple files:

# Pattern repeated in mcp_fuzzer/client/tool_client.py
try:
    tools = await self.transport.get_tools()
    if not tools:
        self._logger.warning("Server returned an empty list of tools.")
        return []
    self._logger.info(f"Found {len(tools)} tools to fuzz")
except Exception as e:
    self._logger.error(f"Failed to get tools: {e}")
    raise

# Same pattern in mcp_fuzzer/client/protocol_client.py
try:
    result = await self.transport.send_request(method, params)
    if not result:
        self._logger.warning("Server returned empty result")
        return {}
    self._logger.info(f"Got result for {method}")
except Exception as e:
    self._logger.error(f"Request failed: {e}")
    raise

# Same pattern in mcp_fuzzer/transport/stdio.py
try:
    response = await self._read_response()
    if not response:
        self._logger.warning("Got empty response")
        return {}
    self._logger.debug(f"Read response: {response}")
except Exception as e:
    self._logger.error(f"Read failed: {e}")
    raise

Why This is Bad:

• Changes must be made in multiple places
• Easy to fix a bug in one place but forget others
• More code to maintain and test

✅ Better Approach (Extract Method + Decorator):

# Create reusable error handling decorator
def log_and_handle_errors(
    operation_name: str,
    empty_value_msg: str = "Empty result",
    success_msg: str | None = None
):
    """Decorator for consistent error handling and logging"""
    def decorator(func):
        @wraps(func)
        async def wrapper(self, *args, **kwargs):
            try:
                result = await func(self, *args, **kwargs)
                
                if not result:
                    self._logger.warning(f"{operation_name}: {empty_value_msg}")
                    return [] if isinstance(result, list) else {}
                
                if success_msg:
                    self._logger.info(success_msg)
                
                return result
                
            except Exception as e:
                self._logger.error(f"{operation_name} failed: {e}")
                raise
        return wrapper
    return decorator

# Use it everywhere - DRY!
class ToolClient:
    @log_and_handle_errors(
        operation_name="get_tools",
        empty_value_msg="Server returned empty tool list",
        success_msg="Found {len(result)} tools to fuzz"
    )
    async def get_tools(self):
        return await self.transport.get_tools()

class ProtocolClient:
    @log_and_handle_errors(
        operation_name="send_request",
        empty_value_msg="Server returned empty result"
    )
    async def send_request(self, method, params):
        return await self.transport.send_request(method, params)

---

Class-Level Smells

4. 🏢 Large Class (God Object)

📖 Definition

A class that knows too much or does too much. Usually has too many instance variables, methods, or lines of code.

📚 References

• Refactoring Guru - Large Class
• God Object Anti-Pattern

❌ VIOLATION: Multiple Large Classes

Files exceeding 200 lines:

• mcp_fuzzer/cli/args.py - 671 lines 🚨
• mcp_fuzzer/fuzz_engine/runtime/manager.py - 565 lines 🚨
• mcp_fuzzer/fuzz_engine/strategy/aggressive/protocol_type_strategy.py - 926 lines 🚨
• mcp_fuzzer/fuzz_engine/strategy/schema_parser.py - 775 lines 🚨

Let's focus on ProcessManager as an example:

File: mcp_fuzzer/fuzz_engine/runtime/manager.py (565 lines)

class ProcessManager:
    """
    VIOLATION: 565 lines, 25+ methods
    This class does EVERYTHING related to processes!
    """
    
    def __init__(self, config, config_dict=None):
        self.config = config
        self._processes: dict = {}  # Registry
        self.watchdog = ProcessWatchdog(self.config)  # Monitoring
        self._lock = None  # Synchronization
        # ... more state ...
    
    # Group 1: Process Lifecycle (50 lines each)
    async def start_process(self, config): ...
    async def stop_process(self, pid, force=False): ...
    async def stop_all_processes(self, force=False): ...
    
    # Group 2: Process Registry (30 lines each)
    def register_existing_process(self, pid, process): ...
    def unregister_process(self, pid): ...
    async def list_processes(self): ...
    
    # Group 3: Signal Handling (40 lines each)
    async def send_timeout_signal(self, pid, signal_type): ...
    async def _send_term_signal(self, pid, process, name): ...
    async def _send_kill_signal(self, pid, process, name): ...
    async def _send_interrupt_signal(self, pid, process, name): ...
    
    # Group 4: Monitoring (35 lines each)
    async def get_process_status(self, pid): ...
    async def wait_for_process(self, pid, timeout=None): ...
    async def is_process_registered(self, pid): ...
    
    # Group 5: Statistics (40 lines each)
    async def get_stats(self): ...
    async def cleanup_finished_processes(self): ...
    
    # Group 6: Activity Tracking (25 lines each)
    async def update_activity(self, pid): ...
    def _get_activity_timestamp(self): ...
    
    # ... 15 more methods ...

Why This is Bad:

• Violates Single Responsibility Principle
• Difficult to understand - too much to hold in your head
• Hard to test - need to test many behaviors
• High coupling - changes ripple through entire class
• Merge conflicts in version control

✅ Better Approach (Extract Class):

class ProcessLifecycleManager:
    """Focused: Start and stop processes (100 lines)"""
    async def start(self, config): ...
    async def stop(self, pid, force=False): ...
    async def stop_all(self, force=False): ...

class ProcessRegistry:
    """Focused: Track running processes (80 lines)"""
    def register(self, pid, process, metadata): ...
    def unregister(self, pid): ...
    def get(self, pid): ...
    def list_all(self): ...
    def is_registered(self, pid): ...

class ProcessSignalDispatcher:
    """Focused: Send signals (120 lines)"""
    async def send_timeout_signal(self, pid, signal_type): ...
    async def send_term(self, pid): ...
    async def send_kill(self, pid): ...
    async def send_interrupt(self, pid): ...

class ProcessHealthMonitor:
    """Focused: Monitor process health (100 lines)"""
    async def get_status(self, pid): ...
    async def wait_for(self, pid, timeout): ...
    async def update_activity(self, pid): ...

class ProcessStatisticsCollector:
    """Focused: Gather statistics (80 lines)"""
    async def get_stats(self): ...
    async def cleanup_finished(self): ...

class ProcessWatchdogCoordinator:
    """Focused: Coordinate watchdog (60 lines)"""
    def start_watchdog(self): ...
    def stop_watchdog(self): ...

# Facade to provide unified interface
class ProcessManager:
    """
    GOOD: Now just 50 lines coordinating focused components!
    """
    def __init__(self, config):
        self.lifecycle = ProcessLifecycleManager(config)
        self.registry = ProcessRegistry()
        self.signals = ProcessSignalDispatcher(self.registry)
        self.monitor = ProcessHealthMonitor(self.registry)
        self.stats = ProcessStatisticsCollector(self.registry)
        self.watchdog = ProcessWatchdogCoordinator(config, self.registry)
    
    async def start_process(self, config):
        """Delegate to lifecycle manager"""
        process = await self.lifecycle.start(config)
        self.registry.register(process.pid, process, {})
        return process
    
    async def stop_process(self, pid, force=False):
        """Coordinate lifecycle and registry"""
        await self.lifecycle.stop(pid, force)
        self.registry.unregister(pid)
    
    # ... other methods just delegate (1-2 lines each)

---

5. 🦥 Lazy Class (Speculative Generality)

📖 Definition

A class that doesn't do enough to justify its existence. Often created "for future needs" that never materialize.

📚 References

• Refactoring Guru - Lazy Class
• Speculative Generality

❌ VIOLATION: Classes with 1-2 Methods

Found several lazy classes:

# File: mcp_fuzzer/fuzz_engine/fuzzer/tool_fuzzer.py
class ToolFuzzer:
    """VIOLATION: Only 1 method - does this need to be a class?"""
    def __init__(self, max_concurrency=5, safety_system=None):
        self.max_concurrency = max_concurrency
        self.safety_system = safety_system
    
    async def fuzz_tool(self, tool, runs):
        """Only method in the class"""
        # ... implementation ...

# File: mcp_fuzzer/fuzz_engine/executor.py
class AsyncFuzzExecutor:
    """VIOLATION: Only 2 methods - is this needed?"""
    def __init__(self, max_concurrency=5):
        self._semaphore = None
        self.max_concurrency = max_concurrency
    
    async def execute(self, tasks):
        """First method"""
        # ... implementation ...
    
    async def _run_with_semaphore(self, task):
        """Second method - helper"""
        # ... implementation ...

# File: mcp_fuzzer/reports/formatters/csv_fmt.py
class CSVFormatter:
    """VIOLATION: Only 1 method"""
    def save_report(self, data, filename):
        """Only method"""
        # ... CSV writing ...

Why This is Bad:

• Adds unnecessary complexity
• More files to navigate
• Extra indirection without benefit
• Harder to find functionality

✅ Better Approach:

# Option 1: Make it a function if stateless
async def fuzz_tool(tool, runs, max_concurrency=5, safety_system=None):
    """Simple function - no class needed!"""
    # ... implementation ...

# Option 2: Merge with related class if stateful
class FuzzingCoordinator:
    """Combine related functionality"""
    def __init__(self, max_concurrency=5):
        self.max_concurrency = max_concurrency
    
    async def fuzz_tool(self, tool, runs):
        """Tool fuzzing"""
        pass
    
    async def fuzz_protocol(self, protocol_type, runs):
        """Protocol fuzzing"""
        pass
    
    async def fuzz_batch(self, items):
        """Batch fuzzing"""
        pass

# Option 3: Use module-level functions
# In csv_formatter.py module
def save_csv_report(data, filename):
    """Module function instead of single-method class"""
    with open(filename, 'w') as f:
        writer = csv.DictWriter(f, fieldnames=data[0].keys())
        writer.writeheader()
        writer.writerows(data)

---

6. 🎭 Middle Man

📖 Definition

A class that mostly delegates to other classes without adding value.

📚 References

• Refactoring Guru - Middle Man
• Remove Middle Man Refactoring

❌ VIOLATION: Delegation Without Value

File: mcp_fuzzer/reports/safety_reporter.py:426

class SafetyReporter:
    """VIOLATION: Just wrapping another object"""
    def __init__(self, safety_filter):
        self.safety_filter = safety_filter
    
    def export_safety_data(self, filename):
        """
        VIOLATION: Just calls the same method on another object.
        No added value - pure delegation!
        """
        try:
            return self.safety_filter.export_safety_data(filename)
        except Exception as e:
            # Same error handling the caller could do
            raise

    def get_blocked_operations(self):
        """VIOLATION: Another pure delegation"""
        return self.safety_filter.get_blocked_operations()
    
    def get_sanitized_values(self):
        """VIOLATION: Another pure delegation"""
        return self.safety_filter.get_sanitized_values()

Why This is Bad:

• Extra layer of indirection
• No added value
• More code to maintain
• Harder to navigate codebase

✅ Better Approach:

# Option 1: Remove the middle man entirely
class FuzzerReporter:
    def __init__(self, safety_filter):
        # Expose safety_filter directly instead of wrapping
        self.safety_filter = safety_filter
    
    def export_reports(self, dir_path):
        """Reporter adds value by coordinating multiple exports"""
        self.export_tool_report(dir_path / "tools.json")
        self.export_protocol_report(dir_path / "protocols.json")
        # Call safety_filter directly - no wrapper needed!
        self.safety_filter.export_safety_data(dir_path / "safety.json")

# Option 2: If wrapper adds value, make it meaningful
class SafetyReporter:
    """Adds value by enriching/transforming data"""
    def __init__(self, safety_filter):
        self.safety_filter = safety_filter
    
    def export_comprehensive_report(self, filename):
        """GOOD: Adds value by combining multiple data sources"""
        safety_data = self.safety_filter.export_safety_data()
        blocked_ops = self.safety_filter.get_blocked_operations()
        
        # Add value: enrich with statistics
        report = {
            "safety_data": safety_data,
            "blocked_operations": blocked_ops,
            "statistics": self._calculate_stats(safety_data),
            "recommendations": self._generate_recommendations(blocked_ops),
        }
        
        with open(filename, 'w') as f:
            json.dump(report, f, indent=2)

---

Data-Level Smells

7. 🔢 Magic Numbers/Strings

📖 Definition

Hardcoded numbers or strings that have no obvious meaning. They should be named constants.

📚 References

• Refactoring Guru - Magic Number
• Clean Code: "Replace Magic Numbers with Named Constants"

❌ VIOLATION: Magic Numbers Everywhere

# File: mcp_fuzzer/types.py
DEFAULT_TIMEOUT = 30.0  # What does 30 seconds mean? Why 30?
PREVIEW_LENGTH = 200  # Why 200? Why not 100 or 500?
HTTP_OK = 200  # This one is okay - standard HTTP code

# File: mcp_fuzzer/transport/stdio.py:27
def __init__(self, command: str, timeout: float = 30.0):
    # Magic: 30.0 - why this timeout?
    self.timeout = timeout

# File: mcp_fuzzer/transport/stdio.py:53
max_hang_time=self.timeout + 10.0
# Magic: 10.0 - why add 10 seconds?

# File: mcp_fuzzer/safety_system/reporting/events.py:96
if len(items) > 10:
    # Magic: 10 - why 10 items? Why not 5 or 20?
    pass

# File: mcp_fuzzer/fuzz_engine/strategy/strategy_manager.py:190
batch[0]["id"] = random.randint(1, 1000)
# Magic: 1000 - why this range?

# File: mcp_fuzzer/transport/streamable_http.py:255
max_attempts = 1000
# Magic: 1000 attempts - why this many?

# File: mcp_fuzzer/safety_system/safety.py:259
f"BLOCKED potentially dangerous {arg_name}: {value[:50]}..."
# Magic: 50 - why truncate at 50 characters?

Why This is Bad:

• Hard to understand what the number means
• Hard to change - need to find all occurrences
• Easy to use wrong value accidentally
• No context for why this specific value

✅ Better Approach:

# Define constants with descriptive names and documentation

# timeouts.py
class Timeouts:
    """Centralized timeout configuration"""
    # Default timeout for tool operations (chosen based on average tool execution time)
    DEFAULT_TOOL_TIMEOUT_SECONDS = 30.0
    
    # Buffer added to process timeout to allow graceful shutdown
    GRACEFUL_SHUTDOWN_BUFFER_SECONDS = 10.0
    
    # Maximum time to wait for process cleanup
    FORCE_KILL_TIMEOUT_SECONDS = 5.0

# limits.py
class DisplayLimits:
    """Limits for displaying data in UI/logs"""
    # Maximum number of items to show before truncation
    MAX_ITEMS_IN_PREVIEW = 10
    
    # Character limit for argument value preview in logs
    ARGUMENT_PREVIEW_LENGTH = 50
    
    # Character limit for full data previews
    DATA_PREVIEW_LENGTH = 200

class RetryLimits:
    """Retry configuration"""
    # Maximum connection attempts before giving up (based on typical network flakiness)
    MAX_CONNECTION_ATTEMPTS = 1000
    
    # Delay between retry attempts
    RETRY_DELAY_SECONDS = 0.1

class IDRanges:
    """ID generation ranges"""
    # Range for random request IDs (must be unique within session)
    MIN_REQUEST_ID = 1
    MAX_REQUEST_ID = 1000

# Usage - now crystal clear!
def __init__(self, command: str, timeout: float = Timeouts.DEFAULT_TOOL_TIMEOUT_SECONDS):
    self.timeout = timeout

max_hang_time = self.timeout + Timeouts.GRACEFUL_SHUTDOWN_BUFFER_SECONDS

if len(items) > DisplayLimits.MAX_ITEMS_IN_PREVIEW:
    items = items[:DisplayLimits.MAX_ITEMS_IN_PREVIEW]

batch[0]["id"] = random.randint(IDRanges.MIN_REQUEST_ID, IDRanges.MAX_REQUEST_ID)

f"BLOCKED {arg_name}: {value[:DisplayLimits.ARGUMENT_PREVIEW_LENGTH]}..."

---

8. 🧊 Data Clumps

📖 Definition

Groups of data items that always appear together. They should be encapsulated in their own class.

📚 References

• Refactoring Guru - Data Clumps
• Extract Class Refactoring

❌ VIOLATION: Timeout Parameters Always Together

Found in multiple files:

# File: mcp_fuzzer/client/base.py
class MCPFuzzerClient:
    def __init__(self, transport, tool_timeout=None, ...):
        self.tool_timeout = tool_timeout  # Clump member 1
    
    async def fuzz_tool(self, tool, runs=10, tool_timeout=None):
        effective_timeout = tool_timeout or self.tool_timeout  # Clump member 2
        return await self.tool_client.fuzz_tool(
            tool, 
            runs=runs, 
            tool_timeout=effective_timeout  # Passed together
        )

# File: mcp_fuzzer/client/tool_client.py  
class ToolClient:
    async def fuzz_all_tools(self, runs_per_tool=10, tool_timeout=None):
        # Same timeout parameters passed around together
        for tool in tools:
            await self.fuzz_tool(tool, runs_per_tool, tool_timeout=tool_timeout)

# File: mcp_fuzzer/cli/args.py
client_args["tool_timeout"] = getattr(args, "tool_timeout", None)

# These timeout values are ALWAYS used together - they're a data clump!

Why This is Bad:

• Parameters always passed together - should be one object
• Changes to timeouts require changing many method signatures
• Easy to pass wrong combination of values
• Indicates missing abstraction

✅ Better Approach (Introduce Parameter Object):

@dataclass
class TimeoutConfiguration:
    """Encapsulates all timeout-related data"""
    # Tool execution timeout
    tool_timeout: float | None = None
    
    # Overall operation timeout
    operation_timeout: float | None = None
    
    # Connection timeout
    connection_timeout: float = 30.0
    
    # Response timeout
    response_timeout: float = 60.0
    
    def effective_tool_timeout(self, default: float = 30.0) -> float:
        """Get effective tool timeout with fallback"""
        return self.tool_timeout or default
    
    def with_tool_timeout(self, timeout: float) -> "TimeoutConfiguration":
        """Create new config with different tool timeout"""
        return replace(self, tool_timeout=timeout)

# Usage - much cleaner!
class MCPFuzzerClient:
    def __init__(self, transport, timeouts: TimeoutConfiguration = None):
        self.timeouts = timeouts or TimeoutConfiguration()
    
    async def fuzz_tool(
        self, 
        tool, 
        runs=10, 
        timeouts: TimeoutConfiguration | None = None
    ):
        # Use provided timeouts or fall back to instance timeouts
        effective_timeouts = timeouts or self.timeouts
        return await self.tool_client.fuzz_tool(
            tool, 
            runs=runs, 
            timeouts=effective_timeouts  # Pass entire configuration
        )

# Creating and using is easy
timeouts = TimeoutConfiguration(
    tool_timeout=45.0,
    connection_timeout=10.0,
    response_timeout=120.0,
)

client = MCPFuzzerClient(transport, timeouts=timeouts)

# Easy to override for specific calls
quick_timeouts = timeouts.with_tool_timeout(5.0)
await client.fuzz_tool(my_tool, timeouts=quick_timeouts)

---

9. 🔤 Primitive Obsession

📖 Definition

Using primitive types (strings, ints, floats) instead of small objects for simple tasks.

📚 References

• Refactoring Guru - Primitive Obsession
• Value Object Pattern

❌ VIOLATION: Strings for Everything

# File: Throughout codebase
# Using strings for protocols
protocol: str = "stdio"  # Could be invalid!
protocol: str = "STDIO"  # Inconsistent casing!
protocol: str = "stdio-typo"  # Typo not caught!

# Using strings for phases
phase: str = "aggressive"  # Could be any string!
phase: str = "agressive"  # Typo!

# Using strings for modes
mode: str = "tool"  # What modes are valid?
mode: str = "tools"  # Plural vs singular!

# Using primitive dict for configuration
config: dict = {
    "timeout": 30,
    "endpoint": "http://localhost",
    "safety_enabled": True,
}
# No validation, no type safety, can contain anything!

# Using tuples for coordinates
point = (10, 20)  # Which is x and which is y?
dimensions = (100, 200, 300)  # What do these represent?

Why This is Bad:

• No validation - invalid values accepted
• No type safety - typos not caught
• No behavior - can't add methods
• Meaning unclear - what does this string represent?

✅ Better Approach (Value Objects and Enums):

from enum import Enum, auto
from typing import NewType

# Use Enums for fixed sets of values
class Protocol(Enum):
    """Valid protocol types"""
    STDIO = "stdio"
    HTTP = "http"
    HTTPS = "https"
    SSE = "sse"
    WEBSOCKET = "websocket"
    
    @classmethod
    def from_string(cls, value: str) -> "Protocol":
        """Parse from string with validation"""
        value_lower = value.lower()
        for protocol in cls:
            if protocol.value == value_lower:
                return protocol
        raise ValueError(f"Invalid protocol: {value}. Valid: {[p.value for p in cls]}")

class FuzzingPhase(Enum):
    """Valid fuzzing phases"""
    REALISTIC = "realistic"
    AGGRESSIVE = "aggressive"
    
    def is_aggressive(self) -> bool:
        """Add behavior to enum"""
        return self == FuzzingPhase.AGGRESSIVE

class FuzzingMode(Enum):
    """Valid fuzzing modes"""
    TOOL = "tool"
    PROTOCOL = "protocol"
    BOTH = "both"

# Use dataclasses for structured data
@dataclass(frozen=True)  # Immutable value object
class Endpoint:
    """Represents a server endpoint with validation"""
    host: str
    port: int
    path: str = "/"
    
    def __post_init__(self):
        """Validate on construction"""
        if not self.host:
            raise ValueError("Host cannot be empty")
        if not (1 <= self.port <= 65535):
            raise ValueError(f"Invalid port: {self.port}")
    
    @classmethod
    def from_url(cls, url: str) -> "Endpoint":
        """Parse from URL string"""
        parsed = urlparse(url)
        return cls(
            host=parsed.hostname or "localhost",
            port=parsed.port or 80,
            path=parsed.path or "/",
        )
    
    def to_url(self, protocol: Protocol) -> str:
        """Convert to URL"""
        return f"{protocol.value}://{self.host}:{self.port}{self.path}"

@dataclass(frozen=True)
class Point2D:
    """2D point - clear meaning"""
    x: int
    y: int
    
    def distance_to(self, other: "Point2D") -> float:
        """Add behavior"""
        return math.sqrt((self.x - other.x)**2 + (self.y - other.y)**2)

@dataclass(frozen=True)
class Dimensions3D:
    """3D dimensions - clear meaning"""
    width: int
    height: int
    depth: int
    
    def volume(self) -> int:
        """Add behavior"""
        return self.width * self.height * self.depth

# Usage - type-safe and validated!
protocol = Protocol.STDIO  # Enum - can't have invalid value!
protocol = Protocol.from_string("stdio")  # Validated parsing

phase = FuzzingPhase.AGGRESSIVE
if phase.is_aggressive():  # Can add behavior
    print("Using aggressive fuzzing")

mode = FuzzingMode.TOOL

endpoint = Endpoint.from_url("http://localhost:8080/api")
url = endpoint.to_url(Protocol.HTTP)

point = Point2D(x=10, y=20)  # Clear which is which
distance = point.distance_to(Point2D(x=30, y=40))

dims = Dimensions3D(width=100, height=200, depth=300)
vol = dims.volume()

---

Structural Smells

10. 💥 Feature Envy

📖 Definition

A method that uses data from another class more than its own data.

📚 References

• Refactoring Guru - Feature Envy
• Move Method Refactoring

❌ VIOLATION: Using Other Object's Data Heavily

# File: mcp_fuzzer/safety_system/safety.py:229
class SafetyFilter:
    def sanitize_arguments(self, tool_name: str, arguments: dict) -> dict:
        """
        VIOLATION: This method is OBSESSED with Match objects!
        Uses match.danger_type.value heavily.
        """
        for arg_name, arg_value in arguments.items():
            match = self.detector.check_dangerous_content(arg_value)
            if match:
                # Feature Envy: Digging into match's internals
                danger_label = match.danger_type.value.upper()
                
                # More feature envy
                self._logger.warning(
                    f"BLOCKED {match.danger_type.value.upper()} "
                    f"in '{arg_name}': {value[:50]}..."
                )
                
                # Even more feature envy
                sanitized[arg_name] = self._create_safe_value(
                    match.danger_type.value,
                    match.pattern
                )

Why This is Bad:

• Method should probably be in the other class
• Violates "Tell, Don't Ask" principle
• Creates tight coupling
• Changes to Match class affect this class

✅ Better Approach (Move Method):

@dataclass
class DangerMatch:
    """Move the behavior here where the data lives!"""
    danger_type: DangerType
    pattern: str
    matched_value: str
    
    def get_danger_label(self) -> str:
        """Behavior with data"""
        return self.danger_type.value.upper()
    
    def format_warning(self, arg_name: str, preview_len: int = 50) -> str:
        """Behavior with data"""
        preview = self.matched_value[:preview_len]
        return (
            f"BLOCKED {self.get_danger_label()} "
            f"in '{arg_name}': {preview}..."
        )
    
    def create_safe_replacement(self) -> Any:
        """Behavior with data"""
        if self.danger_type == DangerType.COMMAND_INJECTION:
            return "[COMMAND_BLOCKED]"
        elif self.danger_type == DangerType.PATH_TRAVERSAL:
            return "[PATH_BLOCKED]"
        else:
            return "[BLOCKED]"

# Now SafetyFilter just uses the Match's behavior
class SafetyFilter:
    def sanitize_arguments(self, tool_name: str, arguments: dict) -> dict:
        """
        GOOD: Just uses Match's methods - no feature envy!
        """
        sanitized = {}
        for arg_name, arg_value in arguments.items():
            match = self.detector.check_dangerous_content(arg_value)
            if match:
                # Tell, don't ask! Use object's behavior
                self._logger.warning(match.format_warning(arg_name))
                sanitized[arg_name] = match.create_safe_replacement()
            else:
                sanitized[arg_name] = arg_value
        return sanitized

---

11. 🚂 Message Chains (Law of Demeter Violation)

📖 Definition

Code that makes long chains of method calls: a.getB().getC().doSomething(). Violates Law of Demeter: "Only talk to your immediate friends."

📚 References

• Refactoring Guru - Message Chains
• Law of Demeter
• Tell, Don't Ask Principle

❌ VIOLATION: Deep Message Chains

# File: tests/unit/transport/test_stdio.py:63
self.transport.process_manager.register_existing_process.assert_called_once_with(...)
# Chain: transport -> process_manager -> register_existing_process

# File: tests/unit/transport/test_stdio.py:142
self.transport.stdin.write.assert_called_once_with(...)
# Chain: transport -> stdin -> write

# File: tests/unit/transport/test_stdio.py:145
self.transport.stdin.drain.assert_awaited_once()
# Chain: transport -> stdin -> drain

# File: mcp_fuzzer/safety_system/safety.py:229
danger_label = match.danger_type.value.upper()
# Chain: match -> danger_type -> value -> upper()

# File: tests/unit/reports/test_reporter.py:211
reporter.console_formatter.print_tool_summary.assert_called_once_with(results)
# Chain: reporter -> console_formatter -> print_tool_summary

# Real production code example
class FuzzCoordinator:
    def start_fuzzing(self):
        # VIOLATION: Digging through multiple objects
        if self.client.transport.process_manager.watchdog.is_running():
            timeout = self.client.transport.process_manager.config.timeout
            self.client.transport.process_manager.watchdog.update_timeout(timeout)

Why This is Bad:

• Creates tight coupling to internal structure
• Changes to intermediate objects break your code
• Hard to mock/test
• Exposes internal implementation details

✅ Better Approach:

# Option 1: Hide Chains (Delegate)
class StdioTransport:
    """Hide internal structure with delegation"""
    def write_message(self, message: str):
        """
        GOOD: Caller doesn't need to know about stdin!
        Law of Demeter: Only one dot after self!
        """
        self.stdin.write(message.encode())
        await self.stdin.drain()
    
    def register_process(self, pid, process, metadata):
        """GOOD: Hide process_manager existence"""
        self.process_manager.register_existing_process(pid, process, metadata)

# Usage - clean!
transport.write_message("hello")  # ONE dot
transport.register_process(123, process, {})  # ONE dot

# Option 2: Flatten Structure
class Reporter:
    """GOOD: Direct access to formatters"""
    def __init__(self, console_formatter, json_formatter):
        # Store formatters directly - no nesting
        self.console_formatter = console_formatter
        self.json_formatter = json_formatter
    
    def print_tool_summary(self, results):
        """GOOD: Only one level of access"""
        self.console_formatter.print_summary(results)

# Option 3: Use Facade
class FuzzingFacade:
    """Simplified interface hiding complexity"""
    def __init__(self, client):
        self._client = client
    
    def start_with_watchdog(self):
        """
        GOOD: Hides complex chain behind simple method.
        Internally coordinates multiple objects.
        """
        # Hide the complexity internally
        if self._is_watchdog_running():
            timeout = self._get_watchdog_timeout()
            self._update_watchdog_timeout(timeout)
    
    def _is_watchdog_running(self) -> bool:
        """Private helper encapsulates the chain"""
        return self._client.transport.process_manager.watchdog.is_running()
    
    def _get_watchdog_timeout(self) -> float:
        """Private helper encapsulates the chain"""
        return self._client.transport.process_manager.config.timeout
    
    def _update_watchdog_timeout(self, timeout: float):
        """Private helper encapsulates the chain"""
        self._client.transport.process_manager.watchdog.update_timeout(timeout)

---

12. 💉 Shotgun Surgery

📖 Definition

Making a single change requires making many small changes to many different classes.

📚 References

• Refactoring Guru - Shotgun Surgery
• Move Method Refactoring

❌ VIOLATION: Safety Concept Scattered Everywhere

62 files mention "safety"!

# Safety scattered across many files:
# - mcp_fuzzer/safety_system/__init__.py
# - mcp_fuzzer/safety_system/safety.py
# - mcp_fuzzer/safety_system/reporting/events.py
# - mcp_fuzzer/safety_system/filesystem/sanitizer.py
# - mcp_fuzzer/safety_system/detection/detector.py
# - mcp_fuzzer/client/base.py
# - mcp_fuzzer/client/tool_client.py
# - mcp_fuzzer/client/protocol_client.py
# - mcp_fuzzer/reports/safety_reporter.py
# - mcp_fuzzer/fuzz_engine/fuzzer/tool_fuzzer.py
# - tests/unit/safety_system/test_safety.py
# ... and 50+ more files!

# Example: To add a new safety check, you need to modify:
class MCPFuzzerClient:  # File 1
    def __init__(self, safety_system=None, safety_enabled=True):
        self.safety_system = safety_system
        if not safety_enabled:
            self.safety_system = None

class ToolClient:  # File 2
    def __init__(self, safety_system=None):
        self.safety_system = safety_system
    
    async def fuzz_tool(self, tool):
        if self.safety_system:
            args = self.safety_system.sanitize_arguments(tool.name, args)

class ProtocolClient:  # File 3
    def __init__(self, safety_system=None):
        self.safety_system = safety_system

class ToolFuzzer:  # File 4
    def __init__(self, safety_system=None):
        self.safety_system = safety_system

# Every client needs to know about safety!

Why This is Bad:

• Single concept (safety) spread across 60+ files
• Adding new safety feature requires changing many files
• High risk of missing a spot
• Hard to understand the full safety system

✅ Better Approach (Centralize + Facade):

# Centralize safety in one place
class SafetyCoordinator:
    """
    GOOD: Central point for ALL safety concerns.
    All safety operations go through here.
    """
    def __init__(self):
        self.detector = DangerDetector()
        self.sanitizer = ArgumentSanitizer()
        self.blocker = OperationBlocker()
        self.reporter = SafetyReporter()
        self.policy = SafetyPolicy()
    
    def check_and_sanitize_tool_call(
        self,
        tool_name: str,
        arguments: dict
    ) -> tuple[bool, dict]:
        """
        Single method that does all safety checks.
        Clients just call this - don't need to know internals.
        """
        # Check policy
        if self.policy.should_block_tool(tool_name):
            self.reporter.log_blocked_tool(tool_name)
            return False, {}
        
        # Detect dangers
        matches = self.detector.scan_arguments(arguments)
        if matches:
            self.reporter.log_detected_dangers(tool_name, matches)
        
        # Sanitize
        sanitized = self.sanitizer.sanitize(arguments, matches)
        
        # Block if needed
        if self.blocker.should_block(matches):
            return False, {}
        
        return True, sanitized
    
    def check_protocol_message(self, message: dict) -> tuple[bool, dict]:
        """Single entry point for protocol safety"""
        # All protocol safety logic here
        pass

# Now clients are simple
class ToolClient:
    def __init__(self, safety: SafetyCoordinator):
        # Just ONE safety object
        self.safety = safety
    
    async def fuzz_tool(self, tool):
        # Just ONE call
        allowed, safe_args = self.safety.check_and_sanitize_tool_call(
            tool.name,
            args
        )
        
        if not allowed:
            return {"error": "blocked_by_safety"}
        
        return await self.transport.call_tool(tool.name, safe_args)

# Benefits:
# 1. To add new safety feature: only modify SafetyCoordinator
# 2. All safety logic in one place
# 3. Clients don't need to know safety internals
# 4. Easy to test safety in isolation

---

13. 🌳 Deep Nesting

📖 Definition

Code with deeply nested if statements, loops, or callbacks. Makes code hard to read and understand.

📚 References

• Refactoring Guru - Nested Conditionals
• Cyclomatic Complexity
• Flattening Arrow Code

❌ VIOLATION: 4-5 Levels of Nesting

File: mcp_fuzzer/transport/sse.py:146-210

async def _stream_request(self, payload: dict):
    """
    VIOLATION: DEEPLY nested if statements (5 levels!)
    Hard to follow the logic flow.
    """
    async with self.client.stream("POST", self.url, json=payload) as response:
        # Level 1
        if response.status_code == 200:
            # Level 2
            if hasattr(chunks, "__aiter__"):
                # Level 3
                async for chunk in chunks:
                    # Level 4
                    if not chunk:
                        continue
                    
                    buffer += chunk.decode("utf-8")
                    lines = buffer.split("\n")
                    buffer = lines[-1]
                    
                    for line in lines[:-1]:
                        # Level 5
                        if line.strip():
                            # Level 6!
                            if line.startswith("data: "):
                                data_str = line[6:]
                                if buffer:
                                    # Level 7!!
                                    try:
                                        parsed = json.loads(buffer)
                                        if parsed is not None:
                                            # Level 8!!!
                                            yield parsed
                                    except json.JSONDecodeError:
                                        pass

Why This is Bad:

• Hard to read - arrow pattern >>>>>>
• Hard to understand control flow
• Hard to test each condition
• High cyclomatic complexity

✅ Better Approach (Guard Clauses + Extract Method):

async def _stream_request(self, payload: dict):
    """
    GOOD: Flattened with guard clauses and extracted methods.
    Maximum nesting: 2 levels.
    """
    # Guard clause - early return
    async with self.client.stream("POST", self.url, json=payload) as response:
        if response.status_code != 200:
            raise HTTPError(f"Bad status: {response.status_code}")
        
        # Guard clause
        if not hasattr(chunks, "__aiter__"):
            return
        
        # Delegate to focused method
        async for message in self._parse_sse_stream(chunks):
            yield message

async def _parse_sse_stream(self, chunks):
    """
    GOOD: Extracted method with single responsibility.
    Focused on parsing SSE stream.
    """
    buffer = ""
    async for chunk in chunks:
        # Guard clause
        if not chunk:
            continue
        
        buffer += chunk.decode("utf-8")
        
        # Delegate to another focused method
        messages, buffer = self._extract_messages(buffer)
        for message in messages:
            yield message

def _extract_messages(self, buffer: str) -> tuple[list[dict], str]:
    """
    GOOD: Extract and parse complete messages from buffer.
    Returns parsed messages and remaining buffer.
    """
    lines = buffer.split("\n")
    remaining_buffer = lines[-1]
    complete_lines = lines[:-1]
    
    messages = []
    for line in complete_lines:
        # Guard clause
        if not line.strip():
            continue
        
        # Guard clause
        if not line.startswith("data: "):
            continue
        
        # Delegate parsing
        message = self._parse_sse_line(line)
        if message:
            messages.append(message)
    
    return messages, remaining_buffer

def _parse_sse_line(self, line: str) -> dict | None:
    """
    GOOD: Single responsibility - parse one SSE line.
    Simple, testable, no nesting.
    """
    data_str = line[6:]  # Remove "data: " prefix
    
    try:
        return json.loads(data_str)
    except json.JSONDecodeError:
        self._logger.warning(f"Invalid JSON in SSE line: {line}")
        return None

---

14. 🔄 Switch Statements (Type Checking)

📖 Definition

Using isinstance() or type() checks instead of polymorphism. Often indicates missing abstraction.

📚 References

• Refactoring Guru - Switch Statements
• Replace Conditional with Polymorphism

❌ VIOLATION: Type Checking Instead of Polymorphism

# File: mcp_fuzzer/client/protocol_client.py:267-269
def _extract_params(self, data: Any) -> dict[str, Any]:
    """
    VIOLATION: Checking types instead of using polymorphism
    """
    if isinstance(data, dict):
        params = data.get("params", {})
        if isinstance(params, dict):
            return params
        return {}
    return {}

# File: mcp_fuzzer/transport/stdio.py:228
return result if isinstance(result, dict) else {"result": result}

# File: mcp_fuzzer/transport/stdio.py:265
if isinstance(line, bytes):
    line = line.decode("utf-8")

# File: mcp_fuzzer/fuzz_engine/runtime/manager.py:87-89
if isinstance(data, bytes):
    return data.decode("utf-8")
if isinstance(data, str):
    return data.encode("utf-8")

Why This is Bad:

• Violates Open/Closed Principle
• Adding new type requires modifying existing code
• Type checking scattered throughout codebase
• Misses opportunity for polymorphism

✅ Better Approach (Polymorphism + Protocol):

# Define protocol for objects that can be extracted
class ExtractableParams(Protocol):
    """Protocol for objects with extractable params"""
    def extract_params(self) -> dict[str, Any]:
        ...

# Different implementations
@dataclass
class DictMessage:
    """Dictionary message"""
    data: dict
    
    def extract_params(self) -> dict[str, Any]:
        """Extract from dict"""
        params = self.data.get("params", {})
        return params if isinstance(params, dict) else {}

@dataclass
class StringMessage:
    """String message"""
    data: str
    
    def extract_params(self) -> dict[str, Any]:
        """Parse string to extract params"""
        try:
            parsed = json.loads(self.data)
            return parsed.get("params", {})
        except json.JSONDecodeError:
            return {}

@dataclass
class ListMessage:
    """List message"""
    data: list
    
    def extract_params(self) -> dict[str, Any]:
        """Extract from list (e.g., first element)"""
        if self.data and isinstance(self.data[0], dict):
            return self.data[0].get("params", {})
        return {}

# Factory to create appropriate message type
class MessageFactory:
    @staticmethod
    def create(data: Any) -> ExtractableParams:
        """Create appropriate message type"""
        if isinstance(data, dict):
            return DictMessage(data)
        elif isinstance(data, str):
            return StringMessage(data)
        elif isinstance(data, list):
            return ListMessage(data)
        else:
            return DictMessage({})  # Default

# Now client code uses polymorphism - NO type checking!
class ProtocolClient:
    def _extract_params(self, data: Any) -> dict[str, Any]:
        """
        GOOD: No type checking!
        Uses polymorphism through protocol.
        """
        message = MessageFactory.create(data)
        return message.extract_params()
    
    # Or better yet, receive typed messages:
    def process_message(self, message: ExtractableParams):
        """GOOD: Accepts any type that implements protocol"""
        params = message.extract_params()
        # ... process params ...

---

15. 🗑️ Temporary Field

📖 Definition

Instance variables that are only set and used in certain circumstances. They're None or empty most of the time.

📚 References

• Refactoring Guru - Temporary Field
• Null Object Pattern

❌ VIOLATION: Fields Set to None

# File: mcp_fuzzer/transport/stdio.py:32-36
class StdioTransport:
    def __init__(self, command: str):
        self.command = command
        # VIOLATION: These are None until connection!
        self.process = None
        self.stdin = None
        self.stdout = None
        self.stderr = None
        self._lock = None  # Created lazily

# File: mcp_fuzzer/client/base.py:56
class MCPFuzzerClient:
    def __init__(self, safety_enabled=True, safety_system=None):
        # VIOLATION: Sometimes None, sometimes not
        if not safety_enabled:
            self.safety_system = None
        else:
            self.safety_system = safety_system or SafetyFilter()

# File: mcp_fuzzer/fuzz_engine/executor.py:28
class AsyncFuzzExecutor:
    def __init__(self):
        # VIOLATION: Lazy initialization
        self._semaphore = None

# Now every method must check if these are None!
async def write(self, data):
    if self.stdin is None:  # Defensive check everywhere
        raise RuntimeError("Not connected")
    await self.stdin.write(data)

Why This is Bad:

• Object in invalid state until fields initialized
• Must check for None everywhere
• Unclear when fields will be set
• Error-prone - easy to forget None check

✅ Better Approach:

# Option 1: Separate Unconnected and Connected States
class UnconnectedStdioTransport:
    """Transport before connection"""
    def __init__(self, command: str):
        self.command = command
    
    async def connect(self) -> "ConnectedStdioTransport":
        """Connect and return connected state"""
        process = await asyncio.create_subprocess_exec(
            self.command,
            stdin=subprocess.PIPE,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
        )
        return ConnectedStdioTransport(
            command=self.command,
            process=process,
            stdin=process.stdin,
            stdout=process.stdout,
            stderr=process.stderr,
        )

@dataclass
class ConnectedStdioTransport:
    """Transport after connection - all fields guaranteed present"""
    command: str
    process: asyncio.subprocess.Process
    stdin: asyncio.StreamWriter
    stdout: asyncio.StreamReader
    stderr: asyncio.StreamReader
    
    async def write(self, data: bytes):
        """GOOD: No None check needed - stdin always present!"""
        self.stdin.write(data)
        await self.stdin.drain()

# Usage
unconnected = UnconnectedStdioTransport("python server.py")
connected = await unconnected.connect()
await connected.write(b"hello")  # Type-safe, no None checks!

# Option 2: Null Object Pattern for Safety
class NoOpSafetySystem:
    """Null object - does nothing but avoids None checks"""
    def sanitize_arguments(self, tool_name: str, args: dict) -> dict:
        return args  # Pass through
    
    def should_skip_tool_call(self, tool_name: str, args: dict) -> bool:
        return False  # Never skip
    
    # ... implement all SafetyProvider methods as no-ops

class MCPFuzzerClient:
    def __init__(self, safety_enabled=True, safety_system=None):
        # GOOD: Never None! Use null object if disabled
        if not safety_enabled:
            self.safety_system = NoOpSafetySystem()
        else:
            self.safety_system = safety_system or SafetyFilter()
    
    async def fuzz_tool(self, tool):
        # GOOD: No None check needed!
        safe_args = self.safety_system.sanitize_arguments(tool.name, args)
        return await self.call_tool(tool.name, safe_args)

# Option 3: Lazy Initialization Property
class AsyncFuzzExecutor:
    def __init__(self, max_concurrency=5):
        self.max_concurrency = max_concurrency
        self._semaphore_value = None
    
    @property
    def semaphore(self) -> asyncio.Semaphore:
        """
        GOOD: Lazily create but hide None from callers.
        Always returns valid semaphore.
        """
        if self._semaphore_value is None:
            self._semaphore_value = asyncio.Semaphore(self.max_concurrency)
        return self._semaphore_value
    
    async def execute(self, task):
        # GOOD: No None check, property handles it
        async with self.semaphore:
            return await task()

---

Summary & Action Plan

📊 Code Smell Priority Matrix

Smell	Severity	Effort to Fix	Priority
Long Method (332 lines!)	🔴 Critical	Medium	HIGH
Large Class (565 lines!)	🔴 Critical	High	HIGH
Magic Numbers	🟡 Medium	Low	HIGH
Data Clumps	🟡 Medium	Medium	MEDIUM
Duplicate Code	🟡 Medium	Low	MEDIUM
Deep Nesting	🟡 Medium	Medium	MEDIUM
Shotgun Surgery	🔴 Critical	High	LOW (architectural)
Feature Envy	🟢 Low	Low	LOW
Primitive Obsession	🟡 Medium	Medium	LOW

🎯 Recommended Refactoring Order

1. Week 1-2: Quick Wins

   • Replace magic numbers with named constants
   • Extract long methods into smaller methods
   • Remove duplicate exception handling with decorators

2. Week 3-4: Structural Improvements

   • Flatten deep nesting with guard clauses
   • Introduce parameter objects for data clumps
   • Extract lazy classes into functions

3. Week 5-6: Major Refactoring

   • Split large classes (ProcessManager, CLI args)
   • Consolidate shotgun surgery (safety system)
   • Replace primitive obsession with value objects

4. Week 7-8: Polish

   • Fix feature envy by moving methods
   • Remove middle man classes
   • Implement null object pattern for temporary fields

📚 Learning Path for Your Junior

1. Day 1-2: Read about code smells

   • Refactoring Guru catalog
   • Martin Fowler's Refactoring book

2. Day 3-5: Practice on small examples

   • Pick one smell (e.g., Long Method)
   • Refactor one instance
   • Write tests to ensure behavior unchanged

3. Week 2: Learn refactoring techniques

   • Extract Method
   • Replace Temp with Query
   • Introduce Parameter Object
   • Replace Conditional with Polymorphism

4. Week 3+: Apply systematically

   • One smell type at a time
   • Always write tests first
   • Commit after each successful refactoring

Would you like me to:

1. Create detailed refactoring plans for specific files?
2. Generate more code examples for any smell?
3. Provide additional learning resources for specific topics?
4. Analyze other aspects of the codebase (performance, security, etc.)?

---

🧠 Learnings used

Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.