Compare commits
No commits in common. "main" and "chore/issue-2-discovery-scan" have entirely different histories.
main
...
chore/issu
3 changed files with 0 additions and 346 deletions
|
|
@ -1,57 +0,0 @@
|
||||||
name: CI
|
|
||||||
|
|
||||||
on:
|
|
||||||
push:
|
|
||||||
branches: [main]
|
|
||||||
pull_request:
|
|
||||||
branches: [main]
|
|
||||||
|
|
||||||
jobs:
|
|
||||||
test:
|
|
||||||
name: PHP ${{ matrix.php }}
|
|
||||||
runs-on: ubuntu-latest
|
|
||||||
container:
|
|
||||||
image: lthn/build:php-${{ matrix.php }}
|
|
||||||
|
|
||||||
strategy:
|
|
||||||
fail-fast: true
|
|
||||||
matrix:
|
|
||||||
php: ["8.3", "8.4"]
|
|
||||||
|
|
||||||
steps:
|
|
||||||
- uses: actions/checkout@v4
|
|
||||||
|
|
||||||
- name: Clone sister packages
|
|
||||||
env:
|
|
||||||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
|
|
||||||
run: |
|
|
||||||
echo "Cloning php-framework into ../php-framework"
|
|
||||||
git clone --depth 1 \
|
|
||||||
"https://x-access-token:${GITHUB_TOKEN}@forge.lthn.ai/core/php-framework.git" \
|
|
||||||
../php-framework
|
|
||||||
ls -la ../php-framework/composer.json
|
|
||||||
|
|
||||||
- name: Configure path repositories
|
|
||||||
run: |
|
|
||||||
composer config repositories.core path ../php-framework --no-interaction
|
|
||||||
|
|
||||||
- name: Install dependencies
|
|
||||||
run: composer install --prefer-dist --no-interaction --no-progress
|
|
||||||
|
|
||||||
- name: Run Pint
|
|
||||||
run: |
|
|
||||||
if [ -f vendor/bin/pint ]; then
|
|
||||||
vendor/bin/pint --test
|
|
||||||
else
|
|
||||||
echo "Pint not installed, skipping"
|
|
||||||
fi
|
|
||||||
|
|
||||||
- name: Run tests
|
|
||||||
run: |
|
|
||||||
if [ -f vendor/bin/pest ]; then
|
|
||||||
vendor/bin/pest --ci --coverage
|
|
||||||
elif [ -f vendor/bin/phpunit ]; then
|
|
||||||
vendor/bin/phpunit --coverage-text
|
|
||||||
else
|
|
||||||
echo "No test runner found, skipping"
|
|
||||||
fi
|
|
||||||
236
FINDINGS.md
236
FINDINGS.md
|
|
@ -1,236 +0,0 @@
|
||||||
# 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,58 +1,5 @@
|
||||||
# Core-MCP TODO
|
# 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
|
## Testing & Quality Assurance
|
||||||
|
|
||||||
### High Priority
|
### High Priority
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue