php-mcp/FINDINGS.md
darbs-claude dfd712845d
Some checks are pending
CI / Assets (pull_request) Waiting to run
CI / PHP 8.2 (pull_request) Waiting to run
CI / PHP 8.3 (pull_request) Waiting to run
CI / PHP 8.4 (pull_request) Waiting to run
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 <clotho@lthn.ai>
2026-02-21 00:37:35 +00:00

9.5 KiB

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<int, string> for PHPStan level 5+
  • Boot::$listens uses array<class-string, string> — 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<string, mixed> — likely needs type narrowing
  4. QueryDatabase::handleExplain() passes $explainResults (array of stdClass) to interpretExplain() typed as array — needs array<int, object>
  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:

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.