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/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/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/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'); + } } 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']);