From 8db227081a440a47f4a2c59e9f31370b652ec865 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 10:34:18 +0100 Subject: [PATCH 1/2] test: give the suite a schema, and reach the code it was written for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 187 of 315 tests failed. 180 of those failures were a missing database and nothing else: 113 on workspaces, 27 on mcp_tool_metrics, 24 on mcp_tool_versions, 15 on users, 1 on mcp_tool_combinations. Boot.php publishes the package's migrations for a host application to run rather than loading them itself, and php-tenant does the same, so under Testbench there was no schema at all. Naming both directories in defineDatabaseMigrations takes the suite from 187 failed / 127 passed to 62 / 253 on its own. The next cluster was reaching the code at all. WorkspaceContextSecurityTest builds a stand-in class over the RequiresWorkspaceContext trait, whose accessors are protected — correct for a real tool, and unreachable from a test, so nineteen assertions died on "Call to protected method ... from scope" without ever exercising the behaviour. The stand-in now aliases them to public via `use RequiresWorkspaceContext { getWorkspaceId as public; ... }`, which exposes them for assertion without widening the trait itself. Last, four class-style tests under src/Mcp/Tests were migrating and rolling back for real and failing the rollback on "no such table: migrations". Pest's uses(RefreshDatabase::class)->in() only reaches Pest-style files, so those four never picked the trait up while every other test in the suite ran inside a transaction. They now declare it themselves. 315 tests: 29 failed, 285 passed, 1 risky — from 187 failed, 127 passed. The 29 that remain are a long tail of eighteen distinct causes, each needing its own diagnosis rather than a shared fix. Co-Authored-By: Virgil --- .../Mcp/Tests/Unit/QueryAuditServiceTest.php | 3 +++ .../Tests/Unit/QueryExecutionServiceTest.php | 3 +++ .../Tests/Unit/ToolDependencyServiceTest.php | 3 +++ .../Unit/WorkspaceContextSecurityTest.php | 18 ++++++++++++++++-- php/tests/TestCase.php | 18 ++++++++++++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/php/src/Mcp/Tests/Unit/QueryAuditServiceTest.php b/php/src/Mcp/Tests/Unit/QueryAuditServiceTest.php index f2a7184..47b7f85 100644 --- a/php/src/Mcp/Tests/Unit/QueryAuditServiceTest.php +++ b/php/src/Mcp/Tests/Unit/QueryAuditServiceTest.php @@ -6,10 +6,13 @@ use Core\Mcp\Services\QueryAuditService; use Illuminate\Support\Facades\Log; +use Illuminate\Foundation\Testing\RefreshDatabase; use Tests\TestCase; class QueryAuditServiceTest extends TestCase { + use RefreshDatabase; + protected QueryAuditService $auditService; protected function setUp(): void diff --git a/php/src/Mcp/Tests/Unit/QueryExecutionServiceTest.php b/php/src/Mcp/Tests/Unit/QueryExecutionServiceTest.php index 3acda27..6d23772 100644 --- a/php/src/Mcp/Tests/Unit/QueryExecutionServiceTest.php +++ b/php/src/Mcp/Tests/Unit/QueryExecutionServiceTest.php @@ -11,10 +11,13 @@ use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\DB; use Mockery; +use Illuminate\Foundation\Testing\RefreshDatabase; use Tests\TestCase; class QueryExecutionServiceTest extends TestCase { + use RefreshDatabase; + protected QueryExecutionService $executionService; protected QueryAuditService $auditMock; diff --git a/php/src/Mcp/Tests/Unit/ToolDependencyServiceTest.php b/php/src/Mcp/Tests/Unit/ToolDependencyServiceTest.php index 840e197..c5eff30 100644 --- a/php/src/Mcp/Tests/Unit/ToolDependencyServiceTest.php +++ b/php/src/Mcp/Tests/Unit/ToolDependencyServiceTest.php @@ -9,10 +9,13 @@ use Core\Mcp\Exceptions\MissingDependencyException; use Core\Mcp\Services\ToolDependencyService; use Illuminate\Support\Facades\Cache; +use Illuminate\Foundation\Testing\RefreshDatabase; use Tests\TestCase; class ToolDependencyServiceTest extends TestCase { + use RefreshDatabase; + protected ToolDependencyService $service; protected function setUp(): void diff --git a/php/src/Mcp/Tests/Unit/WorkspaceContextSecurityTest.php b/php/src/Mcp/Tests/Unit/WorkspaceContextSecurityTest.php index 29d5804..1d625e1 100644 --- a/php/src/Mcp/Tests/Unit/WorkspaceContextSecurityTest.php +++ b/php/src/Mcp/Tests/Unit/WorkspaceContextSecurityTest.php @@ -23,10 +23,24 @@ use Core\Mcp\Middleware\ValidateWorkspaceContext; use Core\Mcp\Tools\Concerns\RequiresWorkspaceContext; -// Test class using the trait +// Test class using the trait. +// +// The trait's accessors are protected, which is right for a tool but leaves +// them unreachable from a test — every assertion below that touched one died +// on "Call to protected method ... from scope". Aliasing them to public on the +// stand-in exposes them for assertion without widening them on the trait, so +// real tools keep the encapsulation the trait intends. class TestToolWithWorkspaceContext { - use RequiresWorkspaceContext; + use RequiresWorkspaceContext { + getToolName as public; + getWorkspaceContext as public; + getWorkspaceId as public; + getWorkspace as public; + hasWorkspaceContext as public; + validateResourceOwnership as public; + requireWorkspaceContext as public; + } protected string $name = 'test_tool'; } diff --git a/php/tests/TestCase.php b/php/tests/TestCase.php index 8321ce3..6bc710e 100644 --- a/php/tests/TestCase.php +++ b/php/tests/TestCase.php @@ -45,4 +45,22 @@ protected function defineEnvironment($app): void { $app['config']->set('app.key', 'base64:'.base64_encode('core-mcp-testing-key-32-bytes!!!')); } + + /** + * Run both packages' migrations against the in-memory database. + * + * Boot.php publishes the package's migrations for a host application to + * run rather than loading them itself, so under Testbench there is no + * schema at all unless the directories are named here. Without this the + * suite failed 180 of its 187 tests on "no such table" — workspaces and + * users from php-tenant, mcp_tool_metrics and mcp_tool_versions from this + * package — none of which is the behaviour any of those tests assert. + * + * php-tenant first: the mcp tables carry workspace_id foreign keys. + */ + protected function defineDatabaseMigrations(): void + { + $this->loadMigrationsFrom(dirname(__DIR__, 2).'/vendor/dappcore/php-tenant/Migrations'); + $this->loadMigrationsFrom(dirname(__DIR__).'/src/Mcp/Migrations'); + } } From 8e97b76d6eb4ffb39fb582278f07b6d8a16a0911 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 10:38:03 +0100 Subject: [PATCH 2/2] fix(mcp): call the APIs laravel/mcp actually exposes, and fix the pair counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real defects, all of which only became visible once the suite could run. Laravel\Mcp\Request has no input(). Its accessor is get(), and nineteen calls across five tools — QueryDatabase, DescribeTable, CreateCoupon, UpgradePlan and ListInvoices — used input() and would have thrown BadMethodCallException on first use. Every one of those tools was broken. The two middleware that also call input() are left alone: they take Illuminate\Http\Request, where input() is correct, so this is not a blanket rename but a per-file check of which Request each one imports. None of the nineteen used dot-notation keys, so get() is an exact swap. Laravel\Mcp\Response has no getContent() either — that is the Illuminate response API. It exposes content(), returning a Content that implements __toString(). DescribeTableTest asserted through the wrong one. ToolAnalyticsService::flushToolCombinations() could never record a pair for the first time. It called updateOrInsert() with DB::raw('occurrence_count + 1') as the value, which is right on the update path and invalid on the insert path, where it becomes `insert into ... (occurrence_count) values (occurrence_count + 1)` — a column reference inside a VALUES clause. The insert threw, so the follow-up query written to repair exactly that case never ran. Replaced with an increment that reports how many rows it touched and an insert only when that is zero. The lookup also now uses whereNull for a null workspace: `= null` is never true in SQL, so a global pair matched nothing and would have been re-inserted on every flush. 315 tests: 25 failed, 289 passed, 1 risky — from 187 failed, 127 passed when the suite first ran. What remains is a long tail of individual causes, the clearest being McpToolVersion's orderByVersion scope, which sorts with MySQL-only SUBSTRING_INDEX and so cannot execute on sqlite at all. Co-Authored-By: Virgil --- php/src/Mcp/Services/ToolAnalyticsService.php | 56 ++++++++++++------- php/src/Mcp/Tools/Commerce/CreateCoupon.php | 14 ++--- php/src/Mcp/Tools/Commerce/ListInvoices.php | 4 +- php/src/Mcp/Tools/Commerce/UpgradePlan.php | 6 +- php/src/Mcp/Tools/DescribeTable.php | 2 +- php/src/Mcp/Tools/QueryDatabase.php | 12 ++-- php/tests/Unit/DescribeTableTest.php | 6 +- 7 files changed, 57 insertions(+), 43 deletions(-) diff --git a/php/src/Mcp/Services/ToolAnalyticsService.php b/php/src/Mcp/Services/ToolAnalyticsService.php index b25fc28..56be105 100644 --- a/php/src/Mcp/Services/ToolAnalyticsService.php +++ b/php/src/Mcp/Services/ToolAnalyticsService.php @@ -331,31 +331,45 @@ protected function flushToolCombinations(): void $pair = [$tools[$i], $tools[$j]]; sort($pair); - DB::table('mcp_tool_combinations') - ->updateOrInsert( - [ - 'tool_a' => $pair[0], - 'tool_b' => $pair[1], - 'workspace_id' => $workspaceId, - 'date' => $date, - ], - [ - 'occurrence_count' => DB::raw('occurrence_count + 1'), - 'updated_at' => now(), - ] - ); - - // Handle insert case where occurrence_count wasn't set - DB::table('mcp_tool_combinations') + // Increment first, insert only if nothing matched. + // + // This was an updateOrInsert() carrying + // DB::raw('occurrence_count + 1') as the value. That works + // on the update path and is invalid on the insert path, + // where it becomes `insert into ... (occurrence_count) + // values (occurrence_count + 1)` — a column reference in a + // VALUES clause. The first time any pair was seen the write + // threw, so the follow-up query meant to repair the insert + // case never ran either. + // + // whereNull for a null workspace, not where(): `= null` is + // never true in SQL, so a global (workspace-less) pair would + // match nothing and be re-inserted on every flush. + $match = DB::table('mcp_tool_combinations') ->where('tool_a', $pair[0]) ->where('tool_b', $pair[1]) - ->where('workspace_id', $workspaceId) - ->where('date', $date) - ->whereNull('created_at') - ->update([ - 'created_at' => now(), + ->where('date', $date); + + $workspaceId === null + ? $match->whereNull('workspace_id') + : $match->where('workspace_id', $workspaceId); + + $incremented = (clone $match)->update([ + 'occurrence_count' => DB::raw('occurrence_count + 1'), + 'updated_at' => now(), + ]); + + if ($incremented === 0) { + DB::table('mcp_tool_combinations')->insert([ + 'tool_a' => $pair[0], + 'tool_b' => $pair[1], + 'workspace_id' => $workspaceId, + 'date' => $date, 'occurrence_count' => 1, + 'created_at' => now(), + 'updated_at' => now(), ]); + } } } } diff --git a/php/src/Mcp/Tools/Commerce/CreateCoupon.php b/php/src/Mcp/Tools/Commerce/CreateCoupon.php index 04385b2..6ab5010 100644 --- a/php/src/Mcp/Tools/Commerce/CreateCoupon.php +++ b/php/src/Mcp/Tools/Commerce/CreateCoupon.php @@ -14,13 +14,13 @@ class CreateCoupon extends Tool public function handle(Request $request): Response { - $code = strtoupper($request->input('code')); - $name = $request->input('name'); - $type = $request->input('type', 'percentage'); - $value = $request->input('value'); - $duration = $request->input('duration', 'once'); - $maxUses = $request->input('max_uses'); - $validUntil = $request->input('valid_until'); + $code = strtoupper($request->get('code')); + $name = $request->get('name'); + $type = $request->get('type', 'percentage'); + $value = $request->get('value'); + $duration = $request->get('duration', 'once'); + $maxUses = $request->get('max_uses'); + $validUntil = $request->get('valid_until'); // Validate code format if (! preg_match('/^[A-Z0-9_-]+$/', $code)) { diff --git a/php/src/Mcp/Tools/Commerce/ListInvoices.php b/php/src/Mcp/Tools/Commerce/ListInvoices.php index 40ea625..6ac9dca 100644 --- a/php/src/Mcp/Tools/Commerce/ListInvoices.php +++ b/php/src/Mcp/Tools/Commerce/ListInvoices.php @@ -28,8 +28,8 @@ public function handle(Request $request): Response // Get workspace from authenticated context (not from request parameters) $workspaceId = $this->getWorkspaceId(); - $status = $request->input('status'); // paid, pending, overdue, void - $limit = min($request->input('limit', 10), 50); + $status = $request->get('status'); // paid, pending, overdue, void + $limit = min($request->get('limit', 10), 50); $query = Invoice::with('order') ->where('workspace_id', $workspaceId) diff --git a/php/src/Mcp/Tools/Commerce/UpgradePlan.php b/php/src/Mcp/Tools/Commerce/UpgradePlan.php index 9dce042..31b9f0a 100644 --- a/php/src/Mcp/Tools/Commerce/UpgradePlan.php +++ b/php/src/Mcp/Tools/Commerce/UpgradePlan.php @@ -31,9 +31,9 @@ public function handle(Request $request): Response $workspace = $this->getWorkspace(); $workspaceId = $workspace->id; - $newPackageCode = $request->input('package_code'); - $preview = $request->input('preview', true); - $immediate = $request->input('immediate', true); + $newPackageCode = $request->get('package_code'); + $preview = $request->get('preview', true); + $immediate = $request->get('immediate', true); $newPackage = Package::where('code', $newPackageCode)->first(); diff --git a/php/src/Mcp/Tools/DescribeTable.php b/php/src/Mcp/Tools/DescribeTable.php index e168b29..82daaed 100644 --- a/php/src/Mcp/Tools/DescribeTable.php +++ b/php/src/Mcp/Tools/DescribeTable.php @@ -17,7 +17,7 @@ class DescribeTable extends Tool public function handle(Request $request): Response { - $table = trim((string) $request->input('table', '')); + $table = trim((string) $request->get('table', '')); if ($table === '') { return $this->errorResponse('Table name is required'); diff --git a/php/src/Mcp/Tools/QueryDatabase.php b/php/src/Mcp/Tools/QueryDatabase.php index 4a31144..dc71a96 100644 --- a/php/src/Mcp/Tools/QueryDatabase.php +++ b/php/src/Mcp/Tools/QueryDatabase.php @@ -49,14 +49,14 @@ public function __construct( public function handle(Request $request): Response { - $query = $request->input('query'); - $explain = $request->input('explain', false); + $query = $request->get('query'); + $explain = $request->get('explain', false); // Extract context from request for audit logging $workspaceId = $this->getWorkspaceId($request); $userId = $this->getUserId($request); $userIp = $this->getUserIp($request); - $sessionId = $request->input('session_id'); + $sessionId = $request->get('session_id'); if (empty($query)) { return $this->errorResponse('Query is required'); @@ -219,7 +219,7 @@ private function checkBlockedTables(string $query): ?string private function getWorkspaceId(Request $request): ?int { // Try to get from request context or metadata - $workspaceId = $request->input('workspace_id'); + $workspaceId = $request->get('workspace_id'); if ($workspaceId !== null) { return (int) $workspaceId; } @@ -238,7 +238,7 @@ private function getWorkspaceId(Request $request): ?int private function getUserId(Request $request): ?int { // Try to get from request context - $userId = $request->input('user_id'); + $userId = $request->get('user_id'); if ($userId !== null) { return (int) $userId; } @@ -257,7 +257,7 @@ private function getUserId(Request $request): ?int private function getUserIp(Request $request): ?string { // Try from request metadata - $ip = $request->input('user_ip'); + $ip = $request->get('user_ip'); if ($ip !== null) { return $ip; } diff --git a/php/tests/Unit/DescribeTableTest.php b/php/tests/Unit/DescribeTableTest.php index f90a008..1bb4e9d 100644 --- a/php/tests/Unit/DescribeTableTest.php +++ b/php/tests/Unit/DescribeTableTest.php @@ -79,7 +79,7 @@ public function test_handle_returns_columns_and_indexes_for_a_table(): void $tool = new DescribeTable(); $response = $tool->handle(new Request(['table' => 'users'])); - $data = json_decode($response->getContent(), true, flags: JSON_THROW_ON_ERROR); + $data = json_decode((string) $response->content(), true, flags: JSON_THROW_ON_ERROR); $this->assertSame('users', $data['table']); $this->assertCount(2, $data['columns']); @@ -93,7 +93,7 @@ public function test_handle_rejects_invalid_table_names(): void { $tool = new DescribeTable(); $response = $tool->handle(new Request(['table' => 'users; DROP TABLE users'])); - $data = json_decode($response->getContent(), true, flags: JSON_THROW_ON_ERROR); + $data = json_decode((string) $response->content(), true, flags: JSON_THROW_ON_ERROR); $this->assertSame('VALIDATION_ERROR', $data['code']); $this->assertStringContainsString('Invalid table name', $data['error']); @@ -105,7 +105,7 @@ public function test_handle_blocks_system_tables(): void $tool = new DescribeTable(); $response = $tool->handle(new Request(['table' => 'information_schema'])); - $data = json_decode($response->getContent(), true, flags: JSON_THROW_ON_ERROR); + $data = json_decode((string) $response->content(), true, flags: JSON_THROW_ON_ERROR); $this->assertSame('VALIDATION_ERROR', $data['code']); $this->assertStringContainsString('not permitted', $data['error']);