From 0dffa383eeccf77c930831802da253a7c5d910fe Mon Sep 17 00:00:00 2001 From: knobik Date: Fri, 13 Feb 2026 13:14:54 +0100 Subject: [PATCH] - Remove `AgentResponse` contract in favor of `Data` namespace refactor - Simplify `ToolRegistry` by removing unused methods and tests for tool count, clearing, and removing tools - Replace `runtimeSchema` introspection with pre-defined schema logic via `Introspector` - Use `TextAnalyzer` utilities for text preparation to streamline search strategies - Refactor `RunSqlTool` to encapsulate mutable states and provide accessors --- README.md | 2 +- composer.json | 2 - src/Agent/SqlAgent.php | 12 ++- src/Agent/ToolRegistry.php | 38 +------- src/Contracts/Agent.php | 1 + src/{Contracts => Data}/AgentResponse.php | 4 +- src/Data/BusinessRuleData.php | 3 +- src/Data/ConnectionConfig.php | 4 +- src/Data/Context.php | 3 +- src/Data/EvaluationReport.php | 3 +- src/Data/GradeResult.php | 4 +- src/Data/QueryPatternData.php | 4 +- src/Data/TableSchema.php | 4 +- src/Data/TestResult.php | 5 +- .../Strategies/MysqlFullTextStrategy.php | 47 +--------- .../Strategies/PostgresFullTextStrategy.php | 47 +--------- .../Strategies/SqlServerFullTextStrategy.php | 40 ++------- src/Search/Strategies/SqliteLikeStrategy.php | 61 +++---------- src/Services/ContextBuilder.php | 10 ++- src/Services/SchemaIntrospector.php | 30 ++----- src/Tools/IntrospectSchemaTool.php | 87 ++----------------- src/Tools/RunSqlTool.php | 31 ++++++- tests/Feature/RunEvalsCommandTest.php | 2 +- tests/Unit/CustomToolRegistrationTest.php | 2 +- tests/Unit/EvaluationTest.php | 2 +- tests/Unit/RelayToolRegistrationTest.php | 4 +- tests/Unit/TokenUsageTrackingTest.php | 2 +- tests/Unit/Tools/RunSqlToolTest.php | 12 +-- tests/Unit/Tools/ToolRegistryTest.php | 29 ------- 29 files changed, 102 insertions(+), 393 deletions(-) rename src/{Contracts => Data}/AgentResponse.php (91%) diff --git a/README.md b/README.md index 46b40cd..9f36585 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ When testing code that uses SqlAgent, you can mock the facade: ```php use Knobik\SqlAgent\Facades\SqlAgent; -use Knobik\SqlAgent\Contracts\AgentResponse; +use Knobik\SqlAgent\Data\AgentResponse; public function test_it_handles_sql_agent_response(): void { diff --git a/composer.json b/composer.json index 47f91da..3ff7de6 100644 --- a/composer.json +++ b/composer.json @@ -13,8 +13,6 @@ "illuminate/support": "^11.0|^12.0", "illuminate/database": "^11.0|^12.0", "illuminate/console": "^11.0|^12.0", - "spatie/laravel-data": "^4.0", - "doctrine/dbal": "^3.0|^4.0", "prism-php/prism": "^0.99" }, "require-dev": { diff --git a/src/Agent/SqlAgent.php b/src/Agent/SqlAgent.php index c58f242..732b160 100644 --- a/src/Agent/SqlAgent.php +++ b/src/Agent/SqlAgent.php @@ -6,7 +6,7 @@ use Generator; use Knobik\SqlAgent\Contracts\Agent; -use Knobik\SqlAgent\Contracts\AgentResponse; +use Knobik\SqlAgent\Data\AgentResponse; use Knobik\SqlAgent\Llm\StreamChunk; use Knobik\SqlAgent\Services\ConnectionRegistry; use Knobik\SqlAgent\Services\ContextBuilder; @@ -236,9 +236,9 @@ protected function syncFromRunSqlTool(array $tools): void { foreach ($tools as $tool) { if ($tool instanceof RunSqlTool) { - $this->lastSql = $tool->lastSql; - $this->lastResults = $tool->lastResults; - $this->allQueries = $tool->executedQueries; + $this->lastSql = $tool->getLastSql(); + $this->lastResults = $tool->getLastResults(); + $this->allQueries = $tool->getExecutedQueries(); return; } @@ -336,9 +336,7 @@ protected function reset(): void // Reset tool state foreach ($this->toolRegistry->all() as $tool) { if ($tool instanceof RunSqlTool) { - $tool->lastSql = null; - $tool->lastResults = null; - $tool->executedQueries = []; + $tool->reset(); } } } diff --git a/src/Agent/ToolRegistry.php b/src/Agent/ToolRegistry.php index 9ec41b7..562fc31 100644 --- a/src/Agent/ToolRegistry.php +++ b/src/Agent/ToolRegistry.php @@ -27,6 +27,8 @@ public function register(Tool $tool): self /** * Register a single tool, throwing if a tool with the same name already exists. * + * @internal + * * @throws InvalidArgumentException */ public function registerStrict(Tool $tool): self @@ -95,40 +97,4 @@ public function names(): array { return array_keys($this->tools); } - - /** - * Remove a tool by name. - */ - public function remove(string $name): self - { - unset($this->tools[$name]); - - return $this; - } - - /** - * Clear all registered tools. - */ - public function clear(): self - { - $this->tools = []; - - return $this; - } - - /** - * Get the number of registered tools. - */ - public function count(): int - { - return count($this->tools); - } - - /** - * Check if the registry is empty. - */ - public function isEmpty(): bool - { - return empty($this->tools); - } } diff --git a/src/Contracts/Agent.php b/src/Contracts/Agent.php index caf3303..2ed0772 100644 --- a/src/Contracts/Agent.php +++ b/src/Contracts/Agent.php @@ -3,6 +3,7 @@ namespace Knobik\SqlAgent\Contracts; use Generator; +use Knobik\SqlAgent\Data\AgentResponse; interface Agent { diff --git a/src/Contracts/AgentResponse.php b/src/Data/AgentResponse.php similarity index 91% rename from src/Contracts/AgentResponse.php rename to src/Data/AgentResponse.php index 64cb980..27b6a7f 100644 --- a/src/Contracts/AgentResponse.php +++ b/src/Data/AgentResponse.php @@ -1,6 +1,8 @@ */ diff --git a/src/Data/GradeResult.php b/src/Data/GradeResult.php index 4e7e462..2ee4c83 100644 --- a/src/Data/GradeResult.php +++ b/src/Data/GradeResult.php @@ -4,9 +4,7 @@ namespace Knobik\SqlAgent\Data; -use Spatie\LaravelData\Data; - -class GradeResult extends Data +class GradeResult { public function __construct( public bool $passed, diff --git a/src/Data/QueryPatternData.php b/src/Data/QueryPatternData.php index ddd41e2..80ef2e3 100644 --- a/src/Data/QueryPatternData.php +++ b/src/Data/QueryPatternData.php @@ -4,9 +4,7 @@ namespace Knobik\SqlAgent\Data; -use Spatie\LaravelData\Data; - -class QueryPatternData extends Data +class QueryPatternData { public function __construct( public string $name, diff --git a/src/Data/TableSchema.php b/src/Data/TableSchema.php index 3cafb14..0fe7329 100644 --- a/src/Data/TableSchema.php +++ b/src/Data/TableSchema.php @@ -4,9 +4,7 @@ namespace Knobik\SqlAgent\Data; -use Spatie\LaravelData\Data; - -class TableSchema extends Data +class TableSchema { public function __construct( public string $tableName, diff --git a/src/Data/TestResult.php b/src/Data/TestResult.php index 2f9f138..72c403e 100644 --- a/src/Data/TestResult.php +++ b/src/Data/TestResult.php @@ -4,10 +4,7 @@ namespace Knobik\SqlAgent\Data; -use Knobik\SqlAgent\Contracts\AgentResponse; -use Spatie\LaravelData\Data; - -class TestResult extends Data +class TestResult { public const STATUS_PASS = 'pass'; diff --git a/src/Search/Strategies/MysqlFullTextStrategy.php b/src/Search/Strategies/MysqlFullTextStrategy.php index 33f00a5..0a9b05f 100644 --- a/src/Search/Strategies/MysqlFullTextStrategy.php +++ b/src/Search/Strategies/MysqlFullTextStrategy.php @@ -6,6 +6,7 @@ use Illuminate\Database\Eloquent\Builder; use Knobik\SqlAgent\Contracts\FullTextSearchStrategy; +use Knobik\SqlAgent\Support\TextAnalyzer; /** * MySQL MATCH...AGAINST full-text search strategy. @@ -21,7 +22,7 @@ public function __construct( public function apply(Builder $query, string $searchTerm, array $columns, int $limit): Builder { - $searchTerm = $this->prepareSearchTerm($searchTerm); + $searchTerm = TextAnalyzer::prepareSearchTerm($searchTerm); if (empty($searchTerm)) { return $query->limit($limit); @@ -41,48 +42,4 @@ public function getName(): string { return 'mysql_fulltext'; } - - /** - * Prepare the search term for MySQL full-text search. - */ - protected function prepareSearchTerm(string $term): string - { - // Extract keywords and filter stop words - $keywords = $this->extractKeywords($term); - - return implode(' ', $keywords); - } - - /** - * Extract keywords from a search term. - * - * @return array - */ - protected function extractKeywords(string $text): array - { - $stopWords = [ - 'a', 'an', 'the', 'is', 'are', 'was', 'were', 'be', 'been', 'being', - 'have', 'has', 'had', 'do', 'does', 'did', 'will', 'would', 'could', - 'should', 'may', 'might', 'must', 'can', 'to', 'of', 'in', 'for', - 'on', 'with', 'at', 'by', 'from', 'as', 'into', 'through', 'during', - 'before', 'after', 'above', 'below', 'between', 'under', 'again', - 'further', 'then', 'once', 'here', 'there', 'when', 'where', 'why', - 'how', 'all', 'each', 'few', 'more', 'most', 'other', 'some', 'such', - 'no', 'nor', 'not', 'only', 'own', 'same', 'so', 'than', 'too', - 'very', 'just', 'and', 'but', 'if', 'or', 'because', 'until', 'while', - 'what', 'which', 'who', 'whom', 'this', 'that', 'these', 'those', - 'show', 'get', 'find', 'list', 'give', 'tell', 'many', 'much', - ]; - - $words = preg_split('/[^a-zA-Z0-9]+/', strtolower($text), -1, PREG_SPLIT_NO_EMPTY); - - if ($words === false) { - return []; - } - - return array_values(array_filter( - $words, - fn (string $word) => strlen($word) > 2 && ! in_array($word, $stopWords) - )); - } } diff --git a/src/Search/Strategies/PostgresFullTextStrategy.php b/src/Search/Strategies/PostgresFullTextStrategy.php index c463445..0e64d12 100644 --- a/src/Search/Strategies/PostgresFullTextStrategy.php +++ b/src/Search/Strategies/PostgresFullTextStrategy.php @@ -6,6 +6,7 @@ use Illuminate\Database\Eloquent\Builder; use Knobik\SqlAgent\Contracts\FullTextSearchStrategy; +use Knobik\SqlAgent\Support\TextAnalyzer; /** * PostgreSQL to_tsvector/to_tsquery full-text search strategy. @@ -21,7 +22,7 @@ public function __construct( public function apply(Builder $query, string $searchTerm, array $columns, int $limit): Builder { - $searchTerm = $this->prepareSearchTerm($searchTerm); + $searchTerm = TextAnalyzer::prepareSearchTerm($searchTerm); if (empty($searchTerm)) { return $query->limit($limit); @@ -50,48 +51,4 @@ public function getName(): string { return 'postgres_fulltext'; } - - /** - * Prepare the search term for PostgreSQL full-text search. - */ - protected function prepareSearchTerm(string $term): string - { - // Extract keywords and filter stop words - $keywords = $this->extractKeywords($term); - - return implode(' ', $keywords); - } - - /** - * Extract keywords from a search term. - * - * @return array - */ - protected function extractKeywords(string $text): array - { - $stopWords = [ - 'a', 'an', 'the', 'is', 'are', 'was', 'were', 'be', 'been', 'being', - 'have', 'has', 'had', 'do', 'does', 'did', 'will', 'would', 'could', - 'should', 'may', 'might', 'must', 'can', 'to', 'of', 'in', 'for', - 'on', 'with', 'at', 'by', 'from', 'as', 'into', 'through', 'during', - 'before', 'after', 'above', 'below', 'between', 'under', 'again', - 'further', 'then', 'once', 'here', 'there', 'when', 'where', 'why', - 'how', 'all', 'each', 'few', 'more', 'most', 'other', 'some', 'such', - 'no', 'nor', 'not', 'only', 'own', 'same', 'so', 'than', 'too', - 'very', 'just', 'and', 'but', 'if', 'or', 'because', 'until', 'while', - 'what', 'which', 'who', 'whom', 'this', 'that', 'these', 'those', - 'show', 'get', 'find', 'list', 'give', 'tell', 'many', 'much', - ]; - - $words = preg_split('/[^a-zA-Z0-9]+/', strtolower($text), -1, PREG_SPLIT_NO_EMPTY); - - if ($words === false) { - return []; - } - - return array_values(array_filter( - $words, - fn (string $word) => strlen($word) > 2 && ! in_array($word, $stopWords) - )); - } } diff --git a/src/Search/Strategies/SqlServerFullTextStrategy.php b/src/Search/Strategies/SqlServerFullTextStrategy.php index 70f6e4b..e688965 100644 --- a/src/Search/Strategies/SqlServerFullTextStrategy.php +++ b/src/Search/Strategies/SqlServerFullTextStrategy.php @@ -6,6 +6,7 @@ use Illuminate\Database\Eloquent\Builder; use Knobik\SqlAgent\Contracts\FullTextSearchStrategy; +use Knobik\SqlAgent\Support\TextAnalyzer; /** * SQL Server CONTAINSTABLE full-text search strategy. @@ -53,10 +54,14 @@ public function getName(): string /** * Prepare the search term for SQL Server full-text search. * Uses FORMSOF and OR for broader matching. + * + * Safety: keywords are interpolated into FORMSOF() expressions. This is safe because + * TextAnalyzer::extractKeywords() only passes through [a-zA-Z0-9] characters. + * If that filter changes, this method must sanitize keywords. */ protected function prepareSearchTerm(string $term): string { - $keywords = $this->extractKeywords($term); + $keywords = TextAnalyzer::extractKeywords($term); if (empty($keywords)) { return ''; @@ -70,37 +75,4 @@ protected function prepareSearchTerm(string $term): string return implode(' OR ', $expressions); } - - /** - * Extract keywords from a search term. - * - * @return array - */ - protected function extractKeywords(string $text): array - { - $stopWords = [ - 'a', 'an', 'the', 'is', 'are', 'was', 'were', 'be', 'been', 'being', - 'have', 'has', 'had', 'do', 'does', 'did', 'will', 'would', 'could', - 'should', 'may', 'might', 'must', 'can', 'to', 'of', 'in', 'for', - 'on', 'with', 'at', 'by', 'from', 'as', 'into', 'through', 'during', - 'before', 'after', 'above', 'below', 'between', 'under', 'again', - 'further', 'then', 'once', 'here', 'there', 'when', 'where', 'why', - 'how', 'all', 'each', 'few', 'more', 'most', 'other', 'some', 'such', - 'no', 'nor', 'not', 'only', 'own', 'same', 'so', 'than', 'too', - 'very', 'just', 'and', 'but', 'if', 'or', 'because', 'until', 'while', - 'what', 'which', 'who', 'whom', 'this', 'that', 'these', 'those', - 'show', 'get', 'find', 'list', 'give', 'tell', 'many', 'much', - ]; - - $words = preg_split('/[^a-zA-Z0-9]+/', strtolower($text), -1, PREG_SPLIT_NO_EMPTY); - - if ($words === false) { - return []; - } - - return array_values(array_filter( - $words, - fn (string $word) => strlen($word) > 2 && ! in_array($word, $stopWords) - )); - } } diff --git a/src/Search/Strategies/SqliteLikeStrategy.php b/src/Search/Strategies/SqliteLikeStrategy.php index cb9699b..5b1269a 100644 --- a/src/Search/Strategies/SqliteLikeStrategy.php +++ b/src/Search/Strategies/SqliteLikeStrategy.php @@ -6,48 +6,26 @@ use Illuminate\Database\Eloquent\Builder; use Knobik\SqlAgent\Contracts\FullTextSearchStrategy; +use Knobik\SqlAgent\Support\TextAnalyzer; /** * SQLite and fallback LIKE-based search strategy. */ class SqliteLikeStrategy implements FullTextSearchStrategy { - /** - * Common stop words to filter out from search terms. - * - * @var array - */ - protected array $stopWords = [ - 'a', 'an', 'the', 'is', 'are', 'was', 'were', 'be', 'been', 'being', - 'have', 'has', 'had', 'do', 'does', 'did', 'will', 'would', 'could', - 'should', 'may', 'might', 'must', 'can', 'to', 'of', 'in', 'for', - 'on', 'with', 'at', 'by', 'from', 'as', 'into', 'through', 'during', - 'before', 'after', 'above', 'below', 'between', 'under', 'again', - 'further', 'then', 'once', 'here', 'there', 'when', 'where', 'why', - 'how', 'all', 'each', 'few', 'more', 'most', 'other', 'some', 'such', - 'no', 'nor', 'not', 'only', 'own', 'same', 'so', 'than', 'too', - 'very', 'just', 'and', 'but', 'if', 'or', 'because', 'until', 'while', - 'what', 'which', 'who', 'whom', 'this', 'that', 'these', 'those', - 'am', 'i', 'me', 'my', 'myself', 'we', 'our', 'ours', 'ourselves', - 'you', 'your', 'yours', 'yourself', 'yourselves', 'he', 'him', 'his', - 'himself', 'she', 'her', 'hers', 'herself', 'it', 'its', 'itself', - 'they', 'them', 'their', 'theirs', 'themselves', 'show', 'get', 'find', - 'list', 'give', 'tell', 'many', 'much', - ]; - public function apply(Builder $query, string $searchTerm, array $columns, int $limit): Builder { - $keywords = $this->extractKeywords($searchTerm); + $keywords = TextAnalyzer::extractKeywords($searchTerm); if (empty($keywords)) { return $query->limit($limit); } // Build a scoring expression using CASE statements - $scoreExpression = $this->buildScoreExpression($columns, $keywords); + ['expression' => $scoreExpression, 'bindings' => $scoreBindings] = $this->buildScoreExpression($columns, $keywords); return $query - ->selectRaw('*, ('.$scoreExpression.') as search_score') + ->selectRaw('*, ('.$scoreExpression.') as search_score', $scoreBindings) ->where(function ($q) use ($keywords, $columns) { foreach ($keywords as $keyword) { $term = '%'.strtolower($keyword).'%'; @@ -67,42 +45,29 @@ public function getName(): string return 'sqlite_like'; } - /** - * Extract keywords from a search term. - * - * @return array - */ - protected function extractKeywords(string $text): array - { - $words = preg_split('/[^a-zA-Z0-9]+/', strtolower($text), -1, PREG_SPLIT_NO_EMPTY); - - if ($words === false) { - return []; - } - - return array_values(array_filter( - $words, - fn (string $word) => strlen($word) > 2 && ! in_array($word, $this->stopWords) - )); - } - /** * Build a SQL score expression using CASE statements for keyword matching. * * @param array $columns * @param array $keywords + * @return array{expression: string, bindings: array} */ - protected function buildScoreExpression(array $columns, array $keywords): string + protected function buildScoreExpression(array $columns, array $keywords): array { $cases = []; + $bindings = []; foreach ($keywords as $keyword) { $term = '%'.strtolower($keyword).'%'; foreach ($columns as $column) { - $cases[] = "CASE WHEN LOWER({$column}) LIKE '{$term}' THEN 1 ELSE 0 END"; + $cases[] = "CASE WHEN LOWER({$column}) LIKE ? THEN 1 ELSE 0 END"; + $bindings[] = $term; } } - return implode(' + ', $cases); + return [ + 'expression' => implode(' + ', $cases), + 'bindings' => $bindings, + ]; } } diff --git a/src/Services/ContextBuilder.php b/src/Services/ContextBuilder.php index 4d35164..92fb19f 100644 --- a/src/Services/ContextBuilder.php +++ b/src/Services/ContextBuilder.php @@ -35,6 +35,8 @@ public function build(string $question): Context /** * Build context with custom options. + * + * @internal */ public function buildWithOptions( string $question, @@ -50,12 +52,14 @@ public function buildWithOptions( businessRules: $includeBusinessRules ? $this->rulesLoader->format() : '', queryPatterns: $includeQueryPatterns ? $this->searchQueryPatterns($question, $queryPatternLimit) : collect(), learnings: $includeLearnings ? $this->searchLearnings($question, $learningLimit) : collect(), - customKnowledge: $includeQueryPatterns ? $this->searchCustomIndexes($question) : collect(), + customKnowledge: $this->searchCustomIndexes($question), ); } /** * Build minimal context (just schema, no search). + * + * @internal */ public function buildMinimal(): Context { @@ -146,6 +150,8 @@ protected function buildSemanticModel(): string /** * Get the semantic model loader. + * + * @internal */ public function getSemanticLoader(): SemanticModelLoader { @@ -154,6 +160,8 @@ public function getSemanticLoader(): SemanticModelLoader /** * Get the business rules loader. + * + * @internal */ public function getRulesLoader(): BusinessRulesLoader { diff --git a/src/Services/SchemaIntrospector.php b/src/Services/SchemaIntrospector.php index 661b522..92a3eff 100644 --- a/src/Services/SchemaIntrospector.php +++ b/src/Services/SchemaIntrospector.php @@ -23,8 +23,6 @@ public function __construct( */ public function getAllTables(?string $connection = null, ?string $connectionName = null): Collection { - $connection = $this->resolveConnection($connection); - try { $tableNames = $this->getTableNames($connection, $connectionName); } catch (Throwable $e) { @@ -47,8 +45,6 @@ public function introspectTable(string $tableName, ?string $connection = null, ? return null; } - $connection = $this->resolveConnection($connection); - try { if (! $this->tableExists($tableName, $connection)) { return null; @@ -67,8 +63,6 @@ public function introspectTable(string $tableName, ?string $connection = null, ? */ public function getRelevantSchema(string $question, ?string $connection = null, ?string $connectionName = null): ?string { - $connection = $this->resolveConnection($connection); - // Extract potential table names from the question $potentialTables = $this->extractPotentialTableNames($question, $connection, $connectionName); @@ -163,7 +157,7 @@ protected function buildTableSchema(string $tableName, ?string $connection, ?str * * @return array */ - protected function getPrimaryKeyColumns(array $indexes): array + public function getPrimaryKeyColumns(array $indexes): array { foreach ($indexes as $index) { if ($index['primary'] ?? false) { @@ -179,7 +173,7 @@ protected function getPrimaryKeyColumns(array $indexes): array * * @return array */ - protected function buildForeignKeyMap(array $foreignKeys): array + public function buildForeignKeyMap(array $foreignKeys): array { $map = []; @@ -204,7 +198,7 @@ protected function buildForeignKeyMap(array $foreignKeys): array * * @return array */ - protected function getForeignKeys(string $tableName, ?string $connection): array + public function getForeignKeys(string $tableName, ?string $connection): array { try { return Schema::connection($connection)->getForeignKeys($tableName); @@ -216,7 +210,7 @@ protected function getForeignKeys(string $tableName, ?string $connection): array /** * Get table comment if supported by the database. */ - protected function getTableComment(string $tableName, ?string $connection): ?string + public function getTableComment(string $tableName, ?string $connection): ?string { try { $tables = $this->getTablesForConnection($connection); @@ -236,7 +230,7 @@ protected function getTableComment(string $tableName, ?string $connection): ?str /** * Format default value for display. */ - protected function formatDefaultValue(mixed $default): ?string + public function formatDefaultValue(mixed $default): ?string { if ($default === null) { return null; @@ -256,8 +250,6 @@ protected function formatDefaultValue(mixed $default): ?string */ protected function extractPotentialTableNames(string $question, ?string $connection = null, ?string $connectionName = null): array { - $connection = $this->resolveConnection($connection); - try { $allTables = $this->getTableNames($connection, $connectionName); } catch (Throwable) { @@ -310,8 +302,6 @@ protected function extractPotentialTableNames(string $question, ?string $connect */ public function getTableNames(?string $connection = null, ?string $connectionName = null): array { - $connection = $this->resolveConnection($connection); - try { $tables = $this->getTablesForConnection($connection); } catch (Throwable $e) { @@ -330,8 +320,6 @@ public function getTableNames(?string $connection = null, ?string $connectionNam */ public function tableExists(string $tableName, ?string $connection = null): bool { - $connection = $this->resolveConnection($connection); - try { return Schema::connection($connection)->hasTable($tableName); } catch (Throwable) { @@ -391,12 +379,4 @@ protected function getTablesForConnection(?string $connection): array array_filter($tables, fn (array $table) => ($table['schema'] ?? null) === $databaseName) ); } - - /** - * Resolve the connection name. - */ - protected function resolveConnection(?string $connection): ?string - { - return $connection; - } } diff --git a/src/Tools/IntrospectSchemaTool.php b/src/Tools/IntrospectSchemaTool.php index 569a7e4..41daadc 100644 --- a/src/Tools/IntrospectSchemaTool.php +++ b/src/Tools/IntrospectSchemaTool.php @@ -11,7 +11,6 @@ use Knobik\SqlAgent\Services\TableAccessControl; use Prism\Prism\Tool; use RuntimeException; -use Throwable; class IntrospectSchemaTool extends Tool { @@ -90,10 +89,10 @@ protected function inspectTable(string $tableName, bool $includeSampleData, ?str $dbColumns = $schemaBuilder->getColumns($tableName); $indexes = $schemaBuilder->getIndexes($tableName); - $foreignKeys = $this->getForeignKeys($tableName, $connection); + $foreignKeys = $this->introspector->getForeignKeys($tableName, $connection); - $primaryKeyColumns = $this->getPrimaryKeyColumns($indexes); - $foreignKeyMap = $this->buildForeignKeyMap($foreignKeys); + $primaryKeyColumns = $this->introspector->getPrimaryKeyColumns($indexes); + $foreignKeyMap = $this->introspector->buildForeignKeyMap($foreignKeys); $hiddenColumns = $this->tableAccessControl->getHiddenColumns($tableName, $connectionName); @@ -114,7 +113,7 @@ protected function inspectTable(string $tableName, bool $includeSampleData, ?str 'primary_key' => in_array($columnName, $primaryKeyColumns), 'foreign_key' => $fkInfo !== null, 'references' => $fkInfo !== null ? "{$fkInfo['table']}.{$fkInfo['column']}" : null, - 'default' => $this->formatDefaultValue($column['default'] ?? null), + 'default' => $this->introspector->formatDefaultValue($column['default'] ?? null), 'description' => $column['comment'] ?? null, ]; } @@ -129,7 +128,7 @@ protected function inspectTable(string $tableName, bool $includeSampleData, ?str ]; } - $tableComment = $this->getTableComment($tableName, $connection); + $tableComment = $this->introspector->getTableComment($tableName, $connection); $result = [ 'table' => $tableName, @@ -145,82 +144,6 @@ protected function inspectTable(string $tableName, bool $includeSampleData, ?str return $result; } - /** - * @return array - */ - protected function getPrimaryKeyColumns(array $indexes): array - { - foreach ($indexes as $index) { - if ($index['primary'] ?? false) { - return $index['columns'] ?? []; - } - } - - return []; - } - - /** - * @return array - */ - protected function buildForeignKeyMap(array $foreignKeys): array - { - $map = []; - - foreach ($foreignKeys as $fk) { - $localColumns = $fk['columns'] ?? []; - $foreignColumns = $fk['foreign_columns'] ?? []; - $foreignTable = $fk['foreign_table'] ?? null; - - foreach ($localColumns as $index => $columnName) { - $map[$columnName] = [ - 'table' => $foreignTable, - 'column' => $foreignColumns[$index] ?? 'id', - ]; - } - } - - return $map; - } - - protected function getForeignKeys(string $tableName, ?string $connection): array - { - try { - return Schema::connection($connection)->getForeignKeys($tableName); - } catch (Throwable) { - return []; - } - } - - protected function getTableComment(string $tableName, ?string $connection): ?string - { - try { - $tables = Schema::connection($connection)->getTables(); - } catch (Throwable) { - return null; - } - - foreach ($tables as $table) { - if ($table['name'] === $tableName) { - return $table['comment'] ?? null; - } - } - - return null; - } - - protected function formatDefaultValue(mixed $default): ?string - { - if ($default === null) { - return null; - } - - if (is_bool($default)) { - return $default ? 'true' : 'false'; - } - - return (string) $default; - } - protected function getSampleData(string $tableName, ?string $connection, ?string $connectionName = null): array { $hiddenColumns = $this->tableAccessControl->getHiddenColumns($tableName, $connectionName); diff --git a/src/Tools/RunSqlTool.php b/src/Tools/RunSqlTool.php index 9e16746..bcaad2a 100644 --- a/src/Tools/RunSqlTool.php +++ b/src/Tools/RunSqlTool.php @@ -20,11 +20,11 @@ class RunSqlTool extends Tool protected ConnectionRegistry $connectionRegistry; - public ?string $lastSql = null; + private ?string $lastSql = null; - public ?array $lastResults = null; + private ?array $lastResults = null; - public array $executedQueries = []; + private array $executedQueries = []; public function __construct() { @@ -105,6 +105,31 @@ public function getQuestion(): ?string return $this->question; } + public function getLastSql(): ?string + { + return $this->lastSql; + } + + public function getLastResults(): ?array + { + return $this->lastResults; + } + + /** + * @return array + */ + public function getExecutedQueries(): array + { + return $this->executedQueries; + } + + public function reset(): void + { + $this->lastSql = null; + $this->lastResults = null; + $this->executedQueries = []; + } + protected function resolveConnection(?string $logicalName): ?string { return $this->connectionRegistry->resolveConnection($logicalName); diff --git a/tests/Feature/RunEvalsCommandTest.php b/tests/Feature/RunEvalsCommandTest.php index b1552a3..6f0ee22 100644 --- a/tests/Feature/RunEvalsCommandTest.php +++ b/tests/Feature/RunEvalsCommandTest.php @@ -1,7 +1,7 @@ has('fake_custom'))->toBeTrue(); - expect($registry->count())->toBe(6); + expect($registry->all())->toHaveCount(6); }); it('resolves custom tools with constructor dependencies from the container', function () { diff --git a/tests/Unit/EvaluationTest.php b/tests/Unit/EvaluationTest.php index e8e2e3a..c820e04 100644 --- a/tests/Unit/EvaluationTest.php +++ b/tests/Unit/EvaluationTest.php @@ -1,7 +1,7 @@ count())->toBe(7); + expect($registry->all())->toHaveCount(7); expect($registry->has('run_sql'))->toBeTrue(); expect($registry->has('fake_custom'))->toBeTrue(); expect($registry->has('mcp_calculator'))->toBeTrue(); @@ -98,7 +98,7 @@ $registry = app(ToolRegistry::class); // Only built-in tools - expect($registry->count())->toBe(5); + expect($registry->all())->toHaveCount(5); }); }); diff --git a/tests/Unit/TokenUsageTrackingTest.php b/tests/Unit/TokenUsageTrackingTest.php index 598eca8..130d015 100644 --- a/tests/Unit/TokenUsageTrackingTest.php +++ b/tests/Unit/TokenUsageTrackingTest.php @@ -1,7 +1,7 @@ lastSql)->toBe('SELECT * FROM test_users'); - expect($tool->lastResults)->toHaveCount(2); + expect($tool->getLastSql())->toBe('SELECT * FROM test_users'); + expect($tool->getLastResults())->toHaveCount(2); }); it('executes WITH statements', function () { @@ -107,10 +107,10 @@ $tool('SELECT * FROM test_users'); $tool('SELECT name FROM test_users WHERE id = 1'); - expect($tool->executedQueries)->toHaveCount(2); - expect($tool->executedQueries[0]['sql'])->toBe('SELECT * FROM test_users'); - expect($tool->executedQueries[0]['connection'])->toBeNull(); - expect($tool->executedQueries[1]['sql'])->toBe('SELECT name FROM test_users WHERE id = 1'); + expect($tool->getExecutedQueries())->toHaveCount(2); + expect($tool->getExecutedQueries()[0]['sql'])->toBe('SELECT * FROM test_users'); + expect($tool->getExecutedQueries()[0]['connection'])->toBeNull(); + expect($tool->getExecutedQueries()[1]['sql'])->toBe('SELECT name FROM test_users WHERE id = 1'); }); it('can set and get question', function () { diff --git a/tests/Unit/Tools/ToolRegistryTest.php b/tests/Unit/Tools/ToolRegistryTest.php index abdcce3..61317be 100644 --- a/tests/Unit/Tools/ToolRegistryTest.php +++ b/tests/Unit/Tools/ToolRegistryTest.php @@ -43,7 +43,6 @@ new SaveLearningTool, ]); - expect($registry->count())->toBe(2); expect($registry->has('run_sql'))->toBeTrue(); expect($registry->has('save_learning'))->toBeTrue(); }); @@ -80,17 +79,6 @@ $registry->get('non_existent'); })->throws(InvalidArgumentException::class, "Tool 'non_existent' is not registered."); - it('can remove a tool', function () { - $registry = new ToolRegistry; - $registry->register(new RunSqlTool); - - expect($registry->has('run_sql'))->toBeTrue(); - - $registry->remove('run_sql'); - - expect($registry->has('run_sql'))->toBeFalse(); - }); - it('silently overwrites on register by default', function () { $registry = new ToolRegistry; $tool1 = new RunSqlTool; @@ -99,7 +87,6 @@ $registry->register($tool1); $registry->register($tool2); - expect($registry->count())->toBe(1); expect($registry->get('run_sql'))->toBe($tool2); }); @@ -115,21 +102,5 @@ $registry->registerStrict(new RunSqlTool); expect($registry->has('run_sql'))->toBeTrue(); - expect($registry->count())->toBe(1); - }); - - it('can clear all tools', function () { - $registry = new ToolRegistry; - $registry->registerMany([ - new RunSqlTool, - new SaveLearningTool, - ]); - - expect($registry->count())->toBe(2); - - $registry->clear(); - - expect($registry->isEmpty())->toBeTrue(); - expect($registry->count())->toBe(0); }); });