# Taskmaster Execution Plan: mcilspy Code Review Fixes

## Overview

33 issues identified in hater-hat code review, organized into 4 parallel workstreams.

```
┌─────────────────────────────────────────────────────────────────┐
│                    MERGE ORDER (Sequential)                      │
│  security → architecture → performance → testing                 │
└─────────────────────────────────────────────────────────────────┘
         │              │              │              │
         ▼              ▼              ▼              ▼
    ┌─────────┐   ┌─────────┐   ┌─────────┐   ┌─────────┐
    │SECURITY │   │  ARCH   │   │  PERF   │   │ TESTING │
    │ 4 issues│   │ 8 issues│   │ 8 issues│   │ 7 issues│
    │ P1-CRIT │   │ P2-HIGH │   │ P3-MED  │   │ P4-LOW  │
    └─────────┘   └─────────┘   └─────────┘   └─────────┘
```

---

## Domain 1: SECURITY (Priority 1 - Critical)

**Branch**: `fix/security`
**Estimated Effort**: 2-3 hours
**Blocks**: All other domains should wait for security review patterns

### Issues to Fix

| ID | Issue | File:Line | Fix |
|----|-------|-----------|-----|
| S1 | Path traversal - no validation | `server.py:*`, `ilspy_wrapper.py:132` | Add `_validate_assembly_path()` helper that checks: exists, is file, has .dll/.exe extension, resolves to absolute path, optionally check for PE signature |
| S2 | Temp directory race condition | `ilspy_wrapper.py:150-153` | Replace `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory()` context manager |
| S3 | Unbounded subprocess output | `ilspy_wrapper.py:104-107` | Add `MAX_OUTPUT_BYTES = 50_000_000` constant, truncate stdout/stderr if exceeded |
| S4 | No file size limit before loading | `metadata_reader.py:113-115` | Add `MAX_ASSEMBLY_SIZE_MB = 500` check before `dnfile.dnPE()` |

### Acceptance Criteria
- [ ] All user-provided paths validated before use
- [ ] No temp file leaks possible
- [ ] Memory exhaustion attacks mitigated
- [ ] Unit tests for validation functions

---

## Domain 2: ARCHITECTURE (Priority 2 - High)

**Branch**: `fix/architecture`
**Estimated Effort**: 3-4 hours
**Depends On**: Security (for validation patterns)

### Issues to Fix

| ID | Issue | File:Line | Fix |
|----|-------|-----------|-----|
| A1 | Duplicated PATH discovery | `server.py:99-123`, `ilspy_wrapper.py:41-75` | Extract to `src/mcilspy/utils.py` → `find_ilspycmd_path()` |
| A2 | Mixed dataclass/Pydantic models | `metadata_reader.py` vs `models.py` | Convert metadata_reader dataclasses to Pydantic models in `models.py` |
| A3 | Fragile lifespan context access | `server.py:51-80` | Simplify to module-level `_wrapper: ILSpyWrapper | None = None` with proper locking |
| A4 | Stateless wrapper pretending to be stateful | `ilspy_wrapper.py` | Keep as-is but document why (caches ilspycmd_path lookup) OR convert to module functions |
| A5 | Inconsistent async/sync | `metadata_reader.py` | Document as "CPU-bound sync" in docstring, add note about thread pool for heavy loads |
| A6 | Magic numbers scattered | Throughout | Create `src/mcilspy/constants.py` with all limits/timeouts |
| A7 | Repeated regex compilation | `server.py` (6 places) | Add `_compile_search_pattern(pattern, case_sensitive, use_regex)` helper |
| A8 | Language version validation missing | `server.py:578` | Add try/except with helpful error listing valid versions |

### Acceptance Criteria
- [ ] Single source of truth for PATH discovery
- [ ] Consistent data model layer (all Pydantic)
- [ ] All magic numbers in constants.py
- [ ] Helper functions reduce code duplication

---

## Domain 3: PERFORMANCE (Priority 3 - Medium)

**Branch**: `fix/performance`
**Estimated Effort**: 4-5 hours
**Depends On**: Architecture (for new constants/utils)

### Issues to Fix

| ID | Issue | File:Line | Fix |
|----|-------|-----------|-----|
| P1 | search_strings decompiles entire assembly | `server.py:948-954` | Use dnfile's `pe.net.user_strings` to search string heap directly - 100x faster |
| P2 | No result pagination | All list_* tools | Add `max_results: int = 1000` and `offset: int = 0` params, return `has_more` flag |
| P3 | List conversion instead of generators | `metadata_reader.py:289` | Use `enumerate()` directly on iterator where possible |
| P4 | No caching of decompilation | `ilspy_wrapper.py` | Add optional LRU cache keyed by (path, mtime, type_name) |
| P5 | Silent failures in platform detection | `server.py:195-230` | Log warnings for permission errors, return reason in result |
| P6 | Generic exception catches | Throughout | Replace with specific exceptions, preserve stack traces |
| P7 | MetadataReader __exit__ type mismatch | `metadata_reader.py:595-597` | Fix return type, remove unnecessary `return False` |
| P8 | No assembly validation | `ilspy_wrapper.py` | Add PE signature check (MZ header + PE\0\0) before subprocess |

### Acceptance Criteria
- [ ] search_strings uses string heap (benchmark: 10x improvement)
- [ ] All list tools support pagination
- [ ] No silent swallowed exceptions
- [ ] PE validation prevents wasted subprocess calls

---

## Domain 4: TESTING (Priority 4 - Enhancement)

**Branch**: `fix/testing`
**Estimated Effort**: 5-6 hours
**Depends On**: All other domains (tests should cover new code)

### Issues to Fix

| ID | Issue | Fix |
|----|-------|-----|
| T1 | No integration tests | Create `tests/integration/` with real assembly tests. Use a small test .dll checked into repo |
| T2 | No MCP tool tests | Add `tests/test_server_tools.py` testing each `@mcp.tool()` function |
| T3 | No error path tests | Add tests for: regex compilation failure, ilspycmd not found, ctx.info() failure |
| T4 | No concurrency tests | Add `tests/test_concurrency.py` with parallel tool invocations |
| T5 | Missing docstring validation | Add test that all public functions have docstrings (using AST) |
| T6 | No cancel/progress tests | Test timeout behavior, verify progress reporting works |
| T7 | Add test .NET assembly | Create minimal C# project, compile to .dll, check into `tests/fixtures/` |

### Acceptance Criteria
- [ ] Integration tests with real ilspycmd calls
- [ ] 80%+ code coverage (currently ~40% estimated)
- [ ] All error paths tested
- [ ] CI runs integration tests

---

## Deferred (Won't Fix This Sprint)

These issues are valid but lower priority:

| ID | Issue | Reason to Defer |
|----|-------|-----------------|
| D1 | No cancel/abort for decompilation | Requires MCP protocol support for cancellation |
| D2 | No progress reporting | Needs ilspycmd changes or parsing stdout in real-time |
| D3 | No resource extraction | Feature request, not bug - add to backlog |
| D4 | install_ilspy sudo handling | Edge case - document limitation instead |
| D5 | No dry-run for installation | Nice-to-have, not critical |
| D6 | Error messages expose paths | Low risk for MCP (local tool) |

---

## Execution Commands

### Setup Worktrees (Run Once)

```bash
# Create worktrees for parallel development
git worktree add ../mcilspy-security fix/security -b fix/security
git worktree add ../mcilspy-arch fix/architecture -b fix/architecture
git worktree add ../mcilspy-perf fix/performance -b fix/performance
git worktree add ../mcilspy-testing fix/testing -b fix/testing
```

### Task Master Commands

```bash
# Security Task Master
cd ../mcilspy-security
claude -p "You are the SECURITY Task Master. Read docs/taskmaster/PLAN.md and implement all S1-S4 issues. Update status.json when done."

# Architecture Task Master (can start in parallel)
cd ../mcilspy-arch
claude -p "You are the ARCHITECTURE Task Master. Read docs/taskmaster/PLAN.md and implement all A1-A8 issues. Check status.json for security patterns first."

# Performance Task Master
cd ../mcilspy-perf
claude -p "You are the PERFORMANCE Task Master. Read docs/taskmaster/PLAN.md and implement all P1-P8 issues."

# Testing Task Master (runs last)
cd ../mcilspy-testing
claude -p "You are the TESTING Task Master. Read docs/taskmaster/PLAN.md and implement all T1-T7 issues. Requires other domains complete first."
```

### Merge Protocol

```bash
# After all complete, merge in order:
git checkout main
git merge --no-ff fix/security -m "security: path validation, temp cleanup, output limits"
git merge --no-ff fix/architecture -m "refactor: consolidate utils, constants, models"
git merge --no-ff fix/performance -m "perf: string heap search, pagination, caching"
git merge --no-ff fix/testing -m "test: integration tests, tool coverage, fixtures"

# Cleanup
git worktree remove ../mcilspy-security
git worktree remove ../mcilspy-arch
git worktree remove ../mcilspy-perf
git worktree remove ../mcilspy-testing
```

---

## Priority Matrix

```
                    IMPACT
            Low     Medium    High
         ┌────────┬─────────┬─────────┐
    High │   A5   │ P2,P3   │ S1-S4   │  ← Fix First
  EFFORT │   A8   │ A6,A7   │ A1-A4   │
  Medium │   T5   │ P5-P8   │ P1,P4   │
         │   D4   │ T3,T4   │ T1,T2   │
    Low  │ D5,D6  │   T6    │   T7    │  ← Quick Wins
         └────────┴─────────┴─────────┘
```

**Quick Wins** (do first within each domain):
- S2: Temp directory fix (5 min)
- A6: Constants file (15 min)
- P7: __exit__ fix (2 min)
- T7: Add test assembly (30 min)

---

## Definition of Done

- [ ] All issues in domain addressed
- [ ] Tests pass: `uv run pytest`
- [ ] Lint passes: `uv run ruff check src/`
- [ ] Types pass: `uv run ruff check src/ --select=ANN`
- [ ] status.json updated to "ready"
- [ ] PR draft created (not merged)