From dfd712845d5a18a1457030d03cdfb1a8582674cd Mon Sep 17 00:00:00 2001 From: darbs-claude Date: Sat, 21 Feb 2026 00:37:35 +0000 Subject: [PATCH] docs(phase-0): add environment assessment findings and update TODO - Add FINDINGS.md with full Phase 0 assessment: - Composer install blocked by missing private host-uk/core package - Test baseline inventory (10 test files, 6 coverage gaps identified) - Architecture review: event-driven registration, defence-in-depth SQL validation, workspace tenant isolation, tier-based resource limits - Security observations with severity ratings - Phased work recommendations (Phase 1-4) - Update TODO.md with Phase 0 environment blockers and Phase 1 critical architecture tasks (tool extraction from monolithic command, missing QueryDatabase/AuditLogService tests, empty integration test suite) Closes #1 Co-Authored-By: Clotho --- FINDINGS.md | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++++ TODO.md | 53 ++++++++++++ 2 files changed, 289 insertions(+) create mode 100644 FINDINGS.md diff --git a/FINDINGS.md b/FINDINGS.md new file mode 100644 index 0000000..906c85c --- /dev/null +++ b/FINDINGS.md @@ -0,0 +1,236 @@ +# Phase 0 Findings: Environment Assessment + +**Date:** 2026-02-21 +**Branch:** feat/phase-0-assessment +**Issue:** #1 + +--- + +## 1. Environment Assessment + +### Composer Install + +**Result:** FAILED + +``` +Your requirements could not be resolved to an installable set of packages. +Problem 1 + - Root composer.json requires host-uk/core, it could not be found in any version +``` + +`host-uk/core` is a private proprietary package (the Core PHP framework). It is not +published to Packagist or a configured private registry accessible in this environment. + +**Impact:** Tests, lint, and static analysis cannot be executed without the vendor +directory. All tooling assessment below is based on static code review. + +--- + +## 2. Test Baseline + +**Status:** Unable to run (no vendor directory) + +**Configured test runner:** Pest (`vendor/bin/pest`) + +**Test file inventory:** + +| File | Suite | Status | +|------|-------|--------| +| `tests/Unit/SqlQueryValidatorTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/WorkspaceContextSecurityTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/ToolAnalyticsServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/McpQuotaServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/QueryAuditServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/QueryExecutionServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/ToolDependencyServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/ToolVersionServiceTest.php` | Unit | Present | +| `src/Mcp/Tests/Unit/ValidateWorkspaceContextMiddlewareTest.php` | Unit | Present | +| `src/Mcp/Tests/UseCase/ApiKeyManagerBasic.php` | UseCase | Present | + +**Notable gaps:** +- No test for `QueryDatabase` tool (the primary entry point) +- No test for `ToolRegistry` / `AgentToolRegistry` service +- No test for `McpAgentServerCommand` (stdio MCP server) +- No test for `AuditLogService` (tamper-evidence verification) +- No test for `CircuitBreaker` service +- No integration tests at all (`tests/Feature/` is empty) + +--- + +## 3. Lint Baseline + +**Tool:** `vendor/bin/pint` +**Status:** Unable to run (no vendor directory) + +**Static observation:** All reviewed files contain `declare(strict_types=1)` at the top +and follow PSR-12 conventions. Consistent UK English spelling observed throughout +(colour, organisation, licence, sanitise, normalise). + +--- + +## 4. Static Analysis Baseline + +**Tool:** `vendor/bin/phpstan` (level unknown — not declared in composer.json) +**Status:** Unable to run (no vendor directory) + +**Observations from code review:** + +### Type Safety +- All public methods have complete parameter and return type hints +- Private methods are consistently typed +- `SqlQueryValidator::$whitelist` is `array` — could be `array` for PHPStan level 5+ +- `Boot::$listens` uses `array` — correct + +### Potential PHPStan Issues (estimated level 5) +1. `QueryDatabase::getWorkspaceId()` calls `workspace()` global helper — not declared in stubs +2. `QueryDatabase::getUserId()` calls `auth()->id()` — return type is `int|string|null`, cast to int without null check +3. `QueryDatabase::interpretExplain()` accesses `$rowArray['type']` on `array` — likely needs type narrowing +4. `QueryDatabase::handleExplain()` passes `$explainResults` (array of stdClass) to `interpretExplain()` typed as `array` — needs `array` +5. `Boot::onMcpTools()` has empty body — PHPStan will warn about unused parameter + +--- + +## 5. Architecture Review + +### Package Structure + +``` +src/Mcp/ # Core\Mcp namespace (103 PHP files) +├── Boot.php # ServiceProvider + event-driven registration +├── Console/ # 5 Artisan commands +├── Context/ # WorkspaceContext value object +├── Controllers/ # McpApiController (REST) +├── Dependencies/ # Tool dependency system (interface + DTO + enum) +├── DTO/ # ToolStats data transfer object +├── Events/ # ToolExecuted domain event +├── Exceptions/ # 6 custom exceptions (typed hierarchy) +├── Lang/en_GB/ # UK English translations +├── Listeners/ # RecordToolExecution event listener +├── Middleware/ # 5 middleware (auth, quota, workspace context, deps) +├── Migrations/ # 5 database migrations +├── Models/ # 8 Eloquent models +├── Resources/ # AppConfig, ContentResource, DatabaseSchema +├── Routes/ # admin.php route file +├── Services/ # 18 business logic services +├── Tests/ # Unit tests co-located with package +├── Tools/ # 10 MCP tool implementations +└── View/ # 12 Blade templates + 9 Livewire components + +src/Website/Mcp/ # Core\Website\Mcp namespace +└── ... # Web-facing UI module +``` + +### Key Architectural Patterns + +**1. Event-Driven Module Registration** +`Boot.php` uses a `$listens` static array to subscribe to framework lifecycle events +(`AdminPanelBooting`, `ConsoleBooting`, `McpToolsRegistering`). This enables lazy-loading +of admin UI, commands, and tool registrations without booting the full framework. + +**2. Tool Contract** +Tools extend `Laravel\Mcp\Server\Tool` and implement: +- `$description` property +- `schema(JsonSchema $schema): array` — declares MCP input schema +- `handle(Request $request): Response` — executes tool logic + +**3. Defence-in-Depth SQL Validation** (`SqlQueryValidator`) +Four sequential layers: +1. Dangerous pattern check on raw query (before comment stripping) +2. Comment stripping (removes `--`, `#`, `/* */`, `/*!` obfuscation) +3. Blocked keyword check (write/admin/export operations) +4. Whitelist regex matching (only known-safe SELECT structures pass) + +**4. Workspace Tenant Isolation** +`RequiresWorkspaceContext` trait + `ValidateWorkspaceContext` middleware enforce per-request +tenant scoping. `MissingWorkspaceContextException` is thrown for unauthenticated context. + +**5. Tier-Based Resource Limits** +`McpQuotaService` and `QueryExecutionService` apply different limits per subscription tier: +- free / starter / pro / business / enterprise / unlimited +- Limits cover: row count, query timeout, daily request quota + +**6. Singleton Service Container** +All 8 core services registered as singletons in `Boot::register()`. Each is independently +testable and injected via Laravel's container. + +### Notable Issues + +**Issue A — `onMcpTools` is a stub** +`Boot::onMcpTools()` contains only a comment: +```php +public function onMcpTools(McpToolsRegistering $event): void +{ + // MCP tool handlers will be registered here once extracted + // from the monolithic McpAgentServerCommand +} +``` +This means MCP tools are registered inside `McpAgentServerCommand` rather than being +injected via the service container. Refactoring this is P1 work. + +**Issue B — `McpAgentServerCommand` is monolithic** +The stdio MCP server command handles tool registration, JSON-RPC dispatch, and tool +execution in a single command class. This makes it untestable in isolation. + +**Issue C — `ListTables` tool exists but Schema Exploration is listed as TODO** +`src/Mcp/Tools/ListTables.php` exists but the TODO.md item "Schema Exploration Tools" +lists adding `ListTables` as pending. This is already implemented. + +**Issue D — No `composer.lock`** +No `composer.lock` file is present. Dependency versions are not pinned, which creates +reproducibility risk in CI/CD. + +**Issue E — `phpunit.xml` references `vendor/phpunit/phpunit`** +The test runner is configured for PHPUnit XML format but Pest is the stated test runner. +This is compatible (Pest uses PHPUnit under the hood) but the XML namespace warning will +appear until `composer.lock` is generated. + +**Issue F — `tests/Feature/` is empty** +No feature/integration tests exist. All tests are unit tests that mock the database. +End-to-end request-response flows have no test coverage. + +--- + +## 6. Security Observations + +| Finding | Severity | Status | +|---------|----------|--------| +| SQL injection prevention (multi-layer) | GOOD | Implemented | +| Read-only connection enforcement | GOOD | Implemented | +| Workspace tenant isolation | GOOD | Implemented | +| Audit trail with HMAC verification | GOOD | Implemented | +| Tier-based resource limits | GOOD | Implemented | +| Circuit breaker for external calls | GOOD | Implemented | +| Tool registration outside DI container | MEDIUM | Issue A above | +| No integration tests for auth flow | MEDIUM | Issue F above | +| Missing `composer.lock` | LOW | Issue D above | +| `INFORMATION_SCHEMA` access blocked | GOOD | Implemented | +| System table access blocked | GOOD | Implemented | + +--- + +## 7. Phased Work Recommendations + +### Phase 1 — Unblock Testing (Prerequisite) + +1. Resolve `host-uk/core` dependency access (private registry credentials or mock stubs) +2. Generate `composer.lock` after successful install +3. Run `vendor/bin/pest` to establish a numerical test baseline + +### Phase 2 — Critical Gaps + +1. **Extract tools from `McpAgentServerCommand`** into the `McpToolsRegistering` event + handler in `Boot::onMcpTools()` — makes the command testable +2. **Write `QueryDatabase` tool tests** — primary public surface has zero test coverage +3. **Write `AuditLogService` tests** — tamper-evident logging is security-critical +4. **Write integration tests** for the full HTTP → tool → response flow + +### Phase 3 — Code Quality + +1. Fix estimated PHPStan level 5 type errors (see §4) +2. Add `phpstan.neon` configuration file (currently absent) +3. Add `pint.json` configuration file (currently absent) +4. Resolve TODO.md items marked medium priority + +### Phase 4 — Features + +Refer to `TODO.md` for the full backlog. diff --git a/TODO.md b/TODO.md index 7590ee0..9aabf08 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,58 @@ # Core-MCP TODO +> See [FINDINGS.md](FINDINGS.md) for the full Phase 0 environment assessment report. + +## Phase 0 — Environment Blockers (February 2026) + +- [ ] **Resolve `host-uk/core` dependency access** + - Package is not available via Packagist; private registry credentials needed + - Blocks: `composer install`, all tests, lint, and static analysis + - **Action:** Configure private composer repository or provide mock stubs + +- [ ] **Generate `composer.lock`** after successful install + - Currently absent — dependency versions are unpinned + - Reproducibility risk in CI/CD + +- [ ] **Establish numeric test baseline** + - Run `vendor/bin/pest` and record pass/fail counts + - Targeted after dependency access is resolved + +- [ ] **Run PHPStan analysis** + - `vendor/bin/phpstan analyse --memory-limit=512M` + - No `phpstan.neon` config file present — needs creating + +- [ ] **Run lint baseline** + - `vendor/bin/pint --test` + - No `pint.json` config file present — needs creating + +## Phase 1 — Critical Architecture (Prerequisite) + +- [ ] **Refactor: Extract tools from `McpAgentServerCommand`** + - MCP tools are registered inside the command, not via DI container + - Implement `Boot::onMcpTools()` handler (currently a stub) + - Enables unit testing of individual tools in isolation + - **Files:** `src/Mcp/Boot.php`, `src/Mcp/Console/Commands/McpAgentServerCommand.php` + - **Estimated effort:** 4-6 hours + +- [ ] **Test Coverage: QueryDatabase Tool** — primary public surface has zero tests + - Test SELECT execution, EXPLAIN analysis, connection validation + - Test blocked keywords and injection prevention end-to-end + - Test tier-based row limit truncation + - Test timeout handling + - **Files:** `tests/Unit/QueryDatabaseTest.php` + - **Estimated effort:** 3-4 hours + +- [ ] **Test Coverage: AuditLogService** — security-critical, no tests exist + - Test HMAC tamper-evident logging + - Test log verification command (`mcp:verify-audit-log`) + - **Files:** `src/Mcp/Tests/Unit/AuditLogServiceTest.php` + - **Estimated effort:** 2-3 hours + +- [ ] **Add integration tests** — `tests/Feature/` is currently empty + - Test full HTTP → tool → response flow + - Test authentication and quota enforcement via middleware stack + - **Estimated effort:** 4-5 hours + ## Testing & Quality Assurance ### High Priority -- 2.45.3