9.9 KiB
9.9 KiB
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 |
| 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)
# 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
# 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
# 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)