diff --git a/docs/taskmaster/PLAN.md b/docs/taskmaster/PLAN.md new file mode 100644 index 0000000..95dba8e --- /dev/null +++ b/docs/taskmaster/PLAN.md @@ -0,0 +1,221 @@ +# 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) diff --git a/docs/taskmaster/status.json b/docs/taskmaster/status.json new file mode 100644 index 0000000..ef4f547 --- /dev/null +++ b/docs/taskmaster/status.json @@ -0,0 +1,11 @@ +{ + "project": "mcilspy-code-review-fixes", + "created": "2025-02-08T00:00:00Z", + "domains": { + "security": { "status": "pending", "branch": "fix/security", "priority": 1 }, + "architecture": { "status": "pending", "branch": "fix/architecture", "priority": 2 }, + "performance": { "status": "pending", "branch": "fix/performance", "priority": 3 }, + "testing": { "status": "pending", "branch": "fix/testing", "priority": 4 } + }, + "merge_order": ["security", "architecture", "performance", "testing"] +}