Merge pull request 'docs(phase-0): environment assessment, findings and phased TODO' (#3) from feat/phase-0-assessment into main
This commit is contained in:
commit
ef1debf7a9
2 changed files with 289 additions and 0 deletions
236
FINDINGS.md
Normal file
236
FINDINGS.md
Normal file
|
|
@ -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<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:
|
||||
```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.
|
||||
53
TODO.md
53
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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue