fix: address 18 CRITICAL+HIGH PR review comments

**Critical Fixes (7 issues):** - Fix GraphQL schema field names in users tool (role→roles, remove email) - Fix GraphQL mutation signatures (addUserInput, deleteUser input) - Fix dict(None) TypeError guards in users tool (use `or {}` pattern) - Fix FastAPI version constraint (0.116.1→0.115.0) - Fix WebSocket SSL context handling (support CA bundles, bool, and None) - Fix critical disk threshold treated as warning (split counters) **High Priority Fixes (11 issues):** - Fix Docker update/remove action response field mapping - Fix path traversal vulnerability in log validation (normalize paths) - Fix deleteApiKeys validation (check response before success) - Fix rclone create_remote validation (check response) - Fix keys input_data type annotation (dict[str, Any]) - Fix VM domain/domains fallback restoration **Changes by file:** - unraid_mcp/tools/docker.py: Response field mapping - unraid_mcp/tools/info.py: Split critical/warning counters - unraid_mcp/tools/storage.py: Path normalization for traversal protection - unraid_mcp/tools/users.py: GraphQL schema + null handling - unraid_mcp/tools/keys.py: Validation + type annotations - unraid_mcp/tools/rclone.py: Response validation - unraid_mcp/tools/virtualization.py: Domain fallback - unraid_mcp/subscriptions/manager.py: SSL context creation - pyproject.toml: FastAPI version fix - tests/*: New tests for all fixes **Review threads resolved:** PRRT_kwDOO6Hdxs5uu70L, PRRT_kwDOO6Hdxs5uu70O, PRRT_kwDOO6Hdxs5uu70V, PRRT_kwDOO6Hdxs5uu70e, PRRT_kwDOO6Hdxs5uu70i, PRRT_kwDOO6Hdxs5uu7zn, PRRT_kwDOO6Hdxs5uu7z_, PRRT_kwDOO6Hdxs5uu7sI, PRRT_kwDOO6Hdxs5uu7sJ, PRRT_kwDOO6Hdxs5uu7sK, PRRT_kwDOO6Hdxs5uu7Tk, PRRT_kwDOO6Hdxs5uu7Tn, PRRT_kwDOO6Hdxs5uu7Tr, PRRT_kwDOO6Hdxs5uu7Ts, PRRT_kwDOO6Hdxs5uu7Tu, PRRT_kwDOO6Hdxs5uu7Tv, PRRT_kwDOO6Hdxs5uu7Tw, PRRT_kwDOO6Hdxs5uu7Tx All tests passing. Co-authored-by: docker-fixer <agent@pr-fixes> Co-authored-by: info-fixer <agent@pr-fixes> Co-authored-by: storage-fixer <agent@pr-fixes> Co-authored-by: users-fixer <agent@pr-fixes> Co-authored-by: config-fixer <agent@pr-fixes> Co-authored-by: websocket-fixer <agent@pr-fixes> Co-authored-by: keys-rclone-fixer <agent@pr-fixes> Co-authored-by: vm-fixer <agent@pr-fixes>
2026-02-15 16:42:58 -05:00
parent 2697c269a3
commit 184b8aca1c
35 changed files with 9360 additions and 588 deletions
--- a/.plan.md
+++ b/.plan.md
@@ -0,0 +1,544 @@
+# Implementation Plan: mcporter Integration Tests + Destructive Action Gating
+
+**Date:** 2026-02-15
+**Status:** Awaiting Approval
+**Estimated Effort:** 8-12 hours
+
+## Overview
+
+Implement comprehensive integration testing using mcporter CLI to validate all 86 tool actions (after removing 4 destructive array operations) against live Unraid servers, plus add environment variable gates for remaining destructive actions to prevent accidental operations.
+
+## Requirements
+
+1. **Remove destructive array operations** - start, stop, shutdown, reboot should not be exposed via MCP
+2. **Add per-tool environment variable gates** - UNRAID_ALLOW_*_DESTRUCTIVE flags for remaining destructive actions
+3. **Build mcporter test suite** - Real end-to-end testing of all 86 actions against live servers (tootie/shart)
+4. **Document all actions** - Comprehensive action catalog with test specifications
+
+## Architecture Changes
+
+### 1. Settings Infrastructure (Pydantic-based)
+
+**File:** `unraid_mcp/config/settings.py`
+
+- Migrate from simple `os.getenv()` to Pydantic `BaseSettings`
+- Add 7 destructive action gate flags (all default to False for safety):
+  - `allow_docker_destructive` (docker remove)
+  - `allow_vm_destructive` (vm force_stop, reset)
+  - `allow_notifications_destructive` (delete, delete_archived)
+  - `allow_rclone_destructive` (delete_remote)
+  - `allow_users_destructive` (user delete)
+  - `allow_keys_destructive` (key delete)
+  - `allow_array_destructive` (REMOVED - no longer needed after task 1)
+- Add `get_config_summary()` method showing gate status
+- Maintain backwards compatibility via module-level exports
+
+**Dependencies:** Add `pydantic-settings` to `pyproject.toml`
+
+### 2. Tool Implementation Pattern
+
+**Pattern for all tools with destructive actions:**
+
+```python
+from ..config.settings import settings
+
+# In tool function:
+if action in DESTRUCTIVE_ACTIONS:
+    # Check 1: Environment variable gate (first line of defense)
+    if not settings.allow_{tool}_destructive:
+        raise ToolError(
+            f"Destructive {tool} action '{action}' is disabled. "
+            f"Set UNRAID_ALLOW_{TOOL}_DESTRUCTIVE=true to enable. "
+            f"This is a safety gate to prevent accidental operations."
+        )
+
+    # Check 2: Runtime confirmation (second line of defense)
+    if not confirm:
+        raise ToolError(f"Action '{action}' is destructive. Set confirm=True to proceed.")
+```
+
+**Tools requiring updates:**
+- `unraid_mcp/tools/docker.py` (1 action: remove)
+- `unraid_mcp/tools/virtualization.py` (2 actions: force_stop, reset)
+- `unraid_mcp/tools/notifications.py` (2 actions: delete, delete_archived)
+- `unraid_mcp/tools/rclone.py` (1 action: delete_remote)
+- `unraid_mcp/tools/users.py` (1 action: delete)
+- `unraid_mcp/tools/keys.py` (1 action: delete)
+
+### 3. mcporter Integration Test Suite
+
+**New Directory Structure:**
+
+```
+tests/integration/
+├── helpers/
+│   ├── mcporter.sh          # mcporter wrapper (call_tool, call_destructive, get_field)
+│   ├── validation.sh        # Response validation (assert_fields, assert_equals, assert_success)
+│   └── reporting.sh         # Test reporting (init_report, record_test, generate_summary)
+├── tools/
+│   ├── test_health.sh       # 3 actions
+│   ├── test_info.sh         # 19 actions
+│   ├── test_storage.sh      # 6 actions
+│   ├── test_docker.sh       # 15 actions
+│   ├── test_vm.sh           # 9 actions
+│   ├── test_notifications.sh # 9 actions
+│   ├── test_rclone.sh       # 4 actions
+│   ├── test_users.sh        # 8 actions
+│   ├── test_keys.sh         # 5 actions
+│   └── test_array.sh        # 8 actions (after removal)
+├── run-all.sh               # Master test runner (parallel/sequential)
+├── run-tool.sh              # Single tool runner
+└── README.md                # Integration test documentation
+```
+
+**mcporter Configuration:** `config/mcporter.json`
+
+```json
+{
+  "mcpServers": {
+    "unraid-tootie": {
+      "command": "uv",
+      "args": ["run", "unraid-mcp-server"],
+      "env": {
+        "UNRAID_API_URL": "https://myunraid.net:31337/graphql",
+        "UNRAID_API_KEY": "${UNRAID_TOOTIE_API_KEY}",
+        "UNRAID_VERIFY_SSL": "false",
+        "UNRAID_MCP_TRANSPORT": "stdio"
+      },
+      "cwd": "/home/jmagar/workspace/unraid-mcp"
+    },
+    "unraid-shart": {
+      "command": "uv",
+      "args": ["run", "unraid-mcp-server"],
+      "env": {
+        "UNRAID_API_URL": "http://100.118.209.1/graphql",
+        "UNRAID_API_KEY": "${UNRAID_SHART_API_KEY}",
+        "UNRAID_VERIFY_SSL": "false",
+        "UNRAID_MCP_TRANSPORT": "stdio"
+      },
+      "cwd": "/home/jmagar/workspace/unraid-mcp"
+    }
+  }
+}
+```
+
+## Implementation Tasks
+
+### Task 1: Remove Destructive Array Operations
+
+**Files:**
+- `unraid_mcp/tools/array.py`
+- `tests/test_array.py`
+
+**Changes:**
+1. Remove from `MUTATIONS` dict:
+   - `start` (lines 24-28)
+   - `stop` (lines 29-33)
+   - `shutdown` (lines 69-73)
+   - `reboot` (lines 74-78)
+2. Remove from `DESTRUCTIVE_ACTIONS` set (line 81) - set becomes empty `{}`
+3. Remove from `ARRAY_ACTIONS` Literal type (lines 85-86)
+4. Update docstring removing these 4 actions (lines 105-106, 115-116)
+5. Remove tests for these actions in `tests/test_array.py`
+
+**Acceptance:**
+- ✅ Array tool has 8 actions (down from 12)
+- ✅ `DESTRUCTIVE_ACTIONS` is empty set
+- ✅ Tests pass for remaining actions
+- ✅ Removed mutations are not callable
+
+### Task 2: Add Pydantic Settings with Destructive Gates
+
+**Files:**
+- `unraid_mcp/config/settings.py`
+- `pyproject.toml`
+- `.env.example`
+
+**Changes:**
+
+1. **Add dependency:** `pydantic-settings>=2.12` in `pyproject.toml` dependencies
+
+2. **Update settings.py:**
+   - Import `BaseSettings` from `pydantic_settings`
+   - Create `UnraidSettings` class with all config fields
+   - Add 6 destructive gate fields (all default to False):
+     - `allow_docker_destructive: bool = Field(default=False, ...)`
+     - `allow_vm_destructive: bool = Field(default=False, ...)`
+     - `allow_notifications_destructive: bool = Field(default=False, ...)`
+     - `allow_rclone_destructive: bool = Field(default=False, ...)`
+     - `allow_users_destructive: bool = Field(default=False, ...)`
+     - `allow_keys_destructive: bool = Field(default=False, ...)`
+   - Add `get_config_summary()` method including gate status
+   - Instantiate global `settings = UnraidSettings()`
+   - Keep backwards compatibility exports
+
+3. **Update .env.example:** Add section documenting all destructive gates
+
+**Acceptance:**
+- ✅ `settings` instance loads successfully
+- ✅ All gate fields default to False
+- ✅ `get_config_summary()` shows gate status
+- ✅ Backwards compatibility maintained (existing code still works)
+
+### Task 3: Update Tools with Environment Variable Gates
+
+**Files to update:**
+- `unraid_mcp/tools/docker.py`
+- `unraid_mcp/tools/virtualization.py`
+- `unraid_mcp/tools/notifications.py`
+- `unraid_mcp/tools/rclone.py`
+- `unraid_mcp/tools/users.py`
+- `unraid_mcp/tools/keys.py`
+
+**Pattern for each tool:**
+
+1. Add import: `from ..config.settings import settings`
+2. Add gate check before confirm check in destructive action handler:
+   ```python
+   if action in DESTRUCTIVE_ACTIONS:
+       if not settings.allow_{tool}_destructive:
+           raise ToolError(
+               f"Destructive {tool} action '{action}' is disabled. "
+               f"Set UNRAID_ALLOW_{TOOL}_DESTRUCTIVE=true to enable."
+           )
+       if not confirm:
+           raise ToolError(f"Action '{action}' is destructive. Set confirm=True to proceed.")
+   ```
+3. Update tool docstring documenting security requirements
+
+**Acceptance (per tool):**
+- ✅ Destructive action fails with clear error when env var not set
+- ✅ Destructive action still requires confirm=True when env var is set
+- ✅ Both checks must pass for execution
+- ✅ Error messages guide user to correct env var
+
+### Task 4: Update Test Suite with Settings Mocking
+
+**Files:**
+- `tests/conftest.py`
+- `tests/test_docker.py`
+- `tests/test_vm.py`
+- `tests/test_notifications.py`
+- `tests/test_rclone.py`
+- `tests/test_users.py`
+- `tests/test_keys.py`
+
+**Changes:**
+
+1. **Add fixtures to conftest.py:**
+   ```python
+   @pytest.fixture
+   def mock_settings():
+       # All gates disabled
+
+   @pytest.fixture
+   def mock_settings_all_enabled(mock_settings):
+       # All gates enabled
+   ```
+
+2. **Update each test file:**
+   - Add `mock_settings` parameter to fixtures
+   - Wrap tool calls with `with patch("unraid_mcp.tools.{tool}.settings", mock_settings):`
+   - Add 3 destructive action tests:
+     - Test gate check (env var not set, confirm=True → fails)
+     - Test confirm check (env var set, confirm=False → fails)
+     - Test success (env var set, confirm=True → succeeds)
+
+**Acceptance:**
+- ✅ All 150 existing tests pass
+- ✅ New gate tests cover all destructive actions
+- ✅ Tests verify correct error messages
+- ✅ Tests use mocked settings (don't rely on actual env vars)
+
+### Task 5: Create mcporter Configuration
+
+**Files:**
+- `config/mcporter.json` (new)
+- `tests/integration/README.md` (new)
+
+**Changes:**
+
+1. Create `config/mcporter.json` with tootie and shart server configs
+2. Document how to use mcporter with the server in README
+3. Include instructions for loading credentials from `~/workspace/homelab/.env`
+
+**Acceptance:**
+- ✅ `mcporter list unraid-tootie` shows all tools
+- ✅ `mcporter call unraid-tootie.unraid_health action=test_connection` succeeds
+- ✅ Configuration works for both servers
+
+### Task 6: Build mcporter Helper Libraries
+
+**Files to create:**
+- `tests/integration/helpers/mcporter.sh`
+- `tests/integration/helpers/validation.sh`
+- `tests/integration/helpers/reporting.sh`
+
+**Functions to implement:**
+
+**mcporter.sh:**
+- `call_tool <tool> <action> [params...]` - Call tool via mcporter, return JSON
+- `call_destructive <tool> <action> <env_var> [params...]` - Safe destructive call
+- `get_field <json> <jq_path>` - Extract field from JSON
+- `is_success <json>` - Check if response indicates success
+- `get_error <json>` - Extract error message
+
+**validation.sh:**
+- `assert_fields <json> <field>...` - Verify required fields exist
+- `assert_equals <json> <field> <expected>` - Field value equality
+- `assert_matches <json> <field> <pattern>` - Field matches regex
+- `assert_success <json>` - Response indicates success
+- `assert_failure <json> [pattern]` - Response indicates failure (negative test)
+
+**reporting.sh:**
+- `init_report <tool>` - Initialize JSON report file
+- `record_test <report> <action> <status> [error]` - Record test result
+- `generate_summary` - Generate console summary from all reports
+
+**Acceptance:**
+- ✅ Helper functions work correctly
+- ✅ Error handling is robust
+- ✅ Functions are reusable across all tool tests
+
+### Task 7: Implement Tool Test Scripts
+
+**Files to create:**
+- `tests/integration/tools/test_health.sh` (3 actions)
+- `tests/integration/tools/test_info.sh` (19 actions)
+- `tests/integration/tools/test_storage.sh` (6 actions)
+- `tests/integration/tools/test_docker.sh` (15 actions)
+- `tests/integration/tools/test_vm.sh` (9 actions)
+- `tests/integration/tools/test_notifications.sh` (9 actions)
+- `tests/integration/tools/test_rclone.sh` (4 actions)
+- `tests/integration/tools/test_users.sh` (8 actions)
+- `tests/integration/tools/test_keys.sh` (5 actions)
+- `tests/integration/tools/test_array.sh` (8 actions)
+
+**Per-script implementation:**
+
+1. Source helper libraries
+2. Initialize report
+3. Implement test functions for each action:
+   - Basic functionality test
+   - Response structure validation
+   - Parameter validation
+   - Destructive action gate tests (if applicable)
+4. Run all tests and record results
+5. Return exit code based on failures
+
+**Priority order (implement in this sequence):**
+1. `test_health.sh` - Simplest (3 actions, no destructive)
+2. `test_info.sh` - Large but straightforward (19 query actions)
+3. `test_storage.sh` - Moderate (6 query actions)
+4. `test_docker.sh` - Complex (15 actions, 1 destructive)
+5. `test_vm.sh` - Complex (9 actions, 2 destructive)
+6. `test_notifications.sh` - Moderate (9 actions, 2 destructive)
+7. `test_rclone.sh` - Simple (4 actions, 1 destructive)
+8. `test_users.sh` - Moderate (8 actions, 1 destructive)
+9. `test_keys.sh` - Simple (5 actions, 1 destructive)
+10. `test_array.sh` - Moderate (8 actions, no destructive after removal)
+
+**Acceptance:**
+- ✅ Each script tests all actions for its tool
+- ✅ Tests validate response structure
+- ✅ Destructive action gates are tested
+- ✅ Scripts generate JSON reports
+- ✅ Exit code indicates success/failure
+
+### Task 8: Build Test Runners
+
+**Files to create:**
+- `tests/integration/run-all.sh`
+- `tests/integration/run-tool.sh`
+
+**run-all.sh features:**
+- Load credentials from `~/workspace/homelab/.env`
+- Support sequential and parallel execution modes
+- Run all 10 tool test scripts
+- Generate summary report
+- Return exit code based on any failures
+
+**run-tool.sh features:**
+- Accept tool name as argument
+- Load credentials
+- Execute single tool test script
+- Pass through exit code
+
+**Acceptance:**
+- ✅ `run-all.sh` executes all tool tests
+- ✅ Parallel mode works correctly (no race conditions)
+- ✅ Summary report shows pass/fail/skip counts
+- ✅ `run-tool.sh health` runs only health tests
+- ✅ Exit codes are correct
+
+### Task 9: Document Action Catalog
+
+**File to create:**
+- `docs/testing/action-catalog.md`
+
+**Content:**
+- Table of all 86 actions across 10 tools
+- For each action:
+  - Tool name
+  - Action name
+  - Type (query/mutation/compound)
+  - Required parameters
+  - Optional parameters
+  - Destructive? (yes/no + env var if yes)
+  - Expected response structure
+  - Example mcporter call
+  - Validation criteria
+
+**Acceptance:**
+- ✅ All 86 actions documented
+- ✅ Specifications are detailed and accurate
+- ✅ Examples are runnable
+- ✅ Becomes source of truth for test implementation
+
+### Task 10: Integration Documentation
+
+**Files to create/update:**
+- `tests/integration/README.md`
+- `docs/testing/integration-tests.md`
+- `docs/testing/test-environments.md`
+- `README.md` (add integration test section)
+
+**Content:**
+- How to run integration tests
+- How to configure mcporter
+- Server setup (tootie/shart)
+- Environment variable gates
+- Destructive action testing
+- CI/CD integration
+- Troubleshooting
+
+**Acceptance:**
+- ✅ Clear setup instructions
+- ✅ Examples for common use cases
+- ✅ Integration with existing pytest docs
+- ✅ CI/CD pipeline documented
+
+## Testing Strategy
+
+### Unit Tests (pytest - existing)
+- **150 tests** across 10 tool modules
+- Mock GraphQL responses
+- Fast, isolated, offline
+- Cover edge cases and error paths
+
+### Integration Tests (mcporter - new)
+- **86 tests** (one per action)
+- Real Unraid server calls
+- Slow, dependent, online
+- Validate actual API behavior
+
+### Test Matrix
+
+| Tool | Actions | pytest Tests | mcporter Tests | Destructive |
+|------|---------|--------------|----------------|-------------|
+| health | 3 | 10 | 3 | 0 |
+| info | 19 | 98 | 19 | 0 |
+| storage | 6 | 11 | 6 | 0 |
+| docker | 15 | 28 | 15 | 1 |
+| vm | 9 | 25 | 9 | 2 |
+| notifications | 9 | 7 | 9 | 2 |
+| rclone | 4 | (pending) | 4 | 1 |
+| users | 8 | (pending) | 8 | 1 |
+| keys | 5 | (pending) | 5 | 1 |
+| array | 8 | 26 | 8 | 0 |
+| **TOTAL** | **86** | **~150** | **86** | **8** |
+
+## Validation Checklist
+
+### Code Changes
+- [ ] Array tool has 8 actions (removed start/stop/shutdown/reboot)
+- [ ] Settings class with 6 destructive gate flags
+- [ ] All 6 tools updated with environment variable gates
+- [ ] All 6 tool tests updated with gate test cases
+- [ ] All existing 150 pytest tests pass
+- [ ] `pydantic-settings` added to dependencies
+- [ ] `.env.example` updated with gate documentation
+
+### Integration Tests
+- [ ] mcporter configuration works for both servers
+- [ ] All 3 helper libraries implemented
+- [ ] All 10 tool test scripts implemented
+- [ ] Test runners (run-all, run-tool) work correctly
+- [ ] All 86 actions have test coverage
+- [ ] Destructive action gates are tested
+- [ ] Reports generate correctly
+
+### Documentation
+- [ ] Action catalog documents all 86 actions
+- [ ] Integration test README is clear
+- [ ] Environment setup documented
+- [ ] CI/CD integration documented
+- [ ] Project README updated
+
+## Success Criteria
+
+1. **Safety:** Destructive actions require both env var AND confirm=True
+2. **Coverage:** All 86 actions have integration tests
+3. **Quality:** Clear error messages guide users to correct env vars
+4. **Automation:** Test suite runs via single command
+5. **Documentation:** Complete action catalog and testing guide
+
+## Risks & Mitigations
+
+### Risk: Breaking existing deployments
+**Impact:** HIGH - Users suddenly can't execute destructive actions
+**Mitigation:**
+- Clear error messages with exact env var to set
+- Document migration in release notes
+- Default to disabled (safe) but guide users to enable
+
+### Risk: Integration tests are flaky
+**Impact:** MEDIUM - CI/CD unreliable
+**Mitigation:**
+- Test against stable servers (tootie/shart)
+- Implement retry logic for network errors
+- Skip destructive tests if env vars not set (not failures)
+
+### Risk: mcporter configuration complexity
+**Impact:** LOW - Difficult for contributors to run tests
+**Mitigation:**
+- Clear setup documentation
+- Example .env template
+- Helper script to validate setup
+
+## Dependencies
+
+- `pydantic-settings>=2.12` (Python package)
+- `mcporter` (npm package - user must install)
+- `jq` (system package for JSON parsing in bash)
+- Access to tootie/shart servers (for integration tests)
+- Credentials in `~/workspace/homelab/.env`
+
+## Timeline Estimate
+
+| Task | Estimated Time |
+|------|---------------|
+| 1. Remove array ops | 30 min |
+| 2. Add settings infrastructure | 1 hour |
+| 3. Update tools with gates | 2 hours |
+| 4. Update test suite | 2 hours |
+| 5. mcporter config | 30 min |
+| 6. Helper libraries | 1.5 hours |
+| 7. Tool test scripts | 4 hours |
+| 8. Test runners | 1 hour |
+| 9. Action catalog | 2 hours |
+| 10. Documentation | 1.5 hours |
+| **Total** | **~12 hours** |
+
+## Notes
+
+- Integration tests complement (not replace) existing pytest suite
+- Tests validate actual Unraid API behavior, not just our code
+- Environment variable gates provide defense-in-depth security
+- mcporter enables real-world validation impossible with mocked tests
+- Action catalog becomes living documentation for all tools
+
+---
+
+**Plan Status:** Awaiting user approval
+**Next Step:** Review plan, make adjustments, then execute via task list