From dcaad39868246233da3b229c03ed21969a484096 Mon Sep 17 00:00:00 2001 From: Rati Rukhadze Date: Tue, 11 Aug 2026 16:14:13 +0400 Subject: [PATCH] feat: harden the mcp stream, check drafts before they reach disk, and publish rule reasoning --- README.md | 29 +++- skills/difflock/SKILL.md | 26 ++- src/Console/Commands/McpCommand.php | 38 +++++ src/Mcp/Server.php | 55 +++++++ src/Mcp/Tools/LintMigration.php | 142 +++++++++++++--- src/Mcp/Tools/Rules.php | 129 +++++++++++++++ tests/Feature/AgentAlignmentTest.php | 232 +++++++++++++++++++++++++++ tests/Feature/McpTest.php | 2 +- 8 files changed, 625 insertions(+), 28 deletions(-) create mode 100644 src/Mcp/Tools/Rules.php create mode 100644 tests/Feature/AgentAlignmentTest.php diff --git a/README.md b/README.md index 7afc146..8302928 100644 --- a/README.md +++ b/README.md @@ -592,18 +592,41 @@ Difflock ships an MCP server that closes the loop. // .mcp.json — Claude Code, Cursor, Laravel Boost, anything speaking MCP { "mcpServers": { - "difflock": { "command": "php", "args": ["artisan", "difflock:mcp"] } + "difflock": { + "command": "php", + "args": ["-d", "display_errors=stderr", "artisan", "difflock:mcp"] + } } } ``` -Three tools, in the order a careful developer would use them: +Four tools, in the order a careful developer would use them: | Tool | Answers | | --- | --- | | `difflock_table_context` | What does this table look like — rows, columns, indexes, foreign keys? | -| `difflock_lint_migration` | I just wrote this migration; what is wrong with it? | +| `difflock_lint_migration` | What is wrong with this migration — **including one not written yet**? | | `difflock_schema_drift` | Has this database already diverged from the baseline? | +| `difflock_rules` | What does this rule actually check, in this project? | + +### Check the draft, not the file + +`difflock_lint_migration` takes `source` as well as `path`. An agent can validate the +migration it is *holding* — against real row counts and real indexes — fix it, and +write once. Checking after writing means every intermediate mistake lands in the +repository first. + +### Why `-d display_errors=stderr` + +On this transport **STDOUT carries the protocol and nothing else**. A single PHP +deprecation notice printed during bootstrap lands ahead of the handshake, the client +cannot parse it, and Difflock's tools appear not to exist — with nothing in the error +to suggest why. I hit exactly this on a live application whose `config/database.php` +used `PDO::MYSQL_ATTR_SSL_CA` on PHP 8.5. + +The flag redirects PHP's diagnostics to STDERR, where MCP clients collect server +logs, so you still see them. Difflock also seals STDOUT around every request itself, +so a `dd()` left in a model cannot corrupt the stream either. It is a **standalone stdio server**, not a Boost plugin. Boost publishes no documented API for third-party tool registration, and writing against an undocumented internal is how a package breaks on someone else's patch release. This works with Boost and with everything else. diff --git a/skills/difflock/SKILL.md b/skills/difflock/SKILL.md index cbcbd05..86b0225 100644 --- a/skills/difflock/SKILL.md +++ b/skills/difflock/SKILL.md @@ -17,12 +17,24 @@ wrong is not a style problem; it is how production columns get dropped. ``` 1. difflock_table_context → what am I dealing with? -2. write the migration -3. difflock_lint_migration → what's wrong with it? -4. fix and repeat until nothing is above `low` -5. show the user, quoting anything that remains +2. draft the migration (do not write it yet) +3. difflock_lint_migration with `source` → what's wrong with it? +4. fix the draft, repeat 3, until nothing is above `low` +5. write the file +6. show the user, quoting anything that remains ``` +**Check the draft with `source` before writing it to disk.** `difflock_lint_migration` +takes the migration code directly, and analyses it against the real database — real +row counts, real indexes — even though the file does not exist yet. Checking after +writing means every intermediate mistake lands in the user's repository first. + +``` +difflock_lint_migration { "source": "protectTheStream(); + $server = new Server([ new TableContext($contexts), new LintMigration($analyzer), new SchemaDrift($checkup), + new Rules($this->laravel->make(RuleRegistry::class), $this->laravel), ]); $server->serve(STDIN, STDOUT); return self::SUCCESS; } + + /** + * Stop the host application writing into the protocol stream. + * + * PHP sends warnings, notices and deprecations to STDOUT when `display_errors` + * is on, which is the default in most local setups. On this transport that is + * fatal rather than untidy: one deprecation from a config file lands ahead of + * the handshake, the client cannot parse it, and Difflock's tools appear not to + * exist. There is nothing in the error to suggest the cause. + * + * Diagnostics are not lost — they are redirected to STDERR, where MCP clients + * collect server logs. So the operator still sees the deprecation; the agent + * still gets clean JSON. + * + * This covers everything from here on. Output emitted *earlier*, while the + * framework booted, is already gone by the time any command runs, which is why + * the documented invocation passes `-d display_errors=stderr` to PHP itself. + */ + private function protectTheStream(): void + { + ini_set('display_errors', 'stderr'); + ini_set('log_errors', '0'); + + if (ob_get_level() > 0) { + // Something has been buffering since before this command. Flushing it to + // STDOUT now would be the exact corruption this method exists to prevent. + $pending = ob_get_clean(); + + if (is_string($pending) && trim($pending) !== '') { + fwrite(STDERR, "difflock: discarded output buffered before the server started.\n"); + } + } + } } diff --git a/src/Mcp/Server.php b/src/Mcp/Server.php index f8320ee..6e2449b 100644 --- a/src/Mcp/Server.php +++ b/src/Mcp/Server.php @@ -86,6 +86,50 @@ public function serve(mixed $input, mixed $output): void } } + /** + * Run the handler with STDOUT sealed off. + * + * This is the difference between a server that works and one that dies silently + * on somebody else's application. **STDOUT carries the protocol**, and anything + * else written to it — a `dd()` left in a model, a deprecation notice from a + * config file, a package that echoes during boot — lands in the middle of the + * JSON-RPC stream. The client sees malformed JSON, gives up, and the failure + * presents as the tools simply not existing. Nobody debugs that quickly. + * + * A real example, found on a live application: `config/database.php` referencing + * `PDO::MYSQL_ATTR_SSL_CA` on PHP 8.5 emits a deprecation notice to STDOUT, and + * that alone was enough to make the server mute. + * + * So every handler runs inside an output buffer. Whatever it prints is captured + * and thrown away rather than corrupting the stream, and the protocol frame is + * written afterwards by the caller. Output produced *before* this point — during + * framework bootstrap — cannot be caught here; {@see \Difflock\Console\Commands\McpCommand} + * handles that end. + * + * @template TReturn + * + * @param callable(): TReturn $handler + * @return TReturn + */ + private function guarded(callable $handler): mixed + { + ob_start(); + + try { + return $handler(); + } finally { + $stray = ob_get_clean(); + + if (is_string($stray) && trim($stray) !== '') { + // Not silently discarded: an operator debugging a quiet server needs + // to know something is writing where it should not. + fwrite(STDERR, 'difflock: discarded '.strlen($stray).' bytes written to STDOUT during a ' + .'request. Something in this application prints to standard output, which corrupts ' + ."the MCP stream.\n"); + } + } + } + /** * Handle one line of input, returning the response to write, or null for a * notification. @@ -93,6 +137,17 @@ public function serve(mixed $input, mixed $output): void * @return array|null */ public function dispatch(string $line): ?array + { + // The guard lives here rather than in serve() so that it protects every entry + // point, not just the one the production transport happens to use. A caller + // that dispatches directly deserves the same guarantee. + return $this->guarded(fn (): ?array => $this->route($line)); + } + + /** + * @return array|null + */ + private function route(string $line): ?array { try { $message = json_decode($line, true, 32, JSON_THROW_ON_ERROR); diff --git a/src/Mcp/Tools/LintMigration.php b/src/Mcp/Tools/LintMigration.php index c5fc614..efa82ff 100644 --- a/src/Mcp/Tools/LintMigration.php +++ b/src/Mcp/Tools/LintMigration.php @@ -7,19 +7,34 @@ use Difflock\Contracts\MigrationAnalyzer; use Difflock\Mcp\Tool; use Difflock\Migration\MigrationFinding; +use Difflock\Migration\MigrationReport; use Difflock\Migration\MigrationScope; +use Throwable; /** - * "I just wrote this migration — what's wrong with it?" + * "Is this migration safe?" — asked of a file, or of a draft that does not exist yet. * - * The tool an agent should reach for immediately after writing a migration, and the - * reason this server exists. It analyses one file against the live database, so the - * answer accounts for how many rows the table actually holds and what is actually - * built on the column — the things that separate a harmless drop from an incident, - * and exactly the things a model writing code cannot see. + * The `source` argument is the important one and it changes the shape of the work. + * Given only `path`, an agent has to write the file before it can find out the + * migration is wrong, then edit it, then check again — and every intermediate + * mistake is on disk in the user's repository. Given `source`, it validates the + * draft it is holding, fixes it, and writes once. + * + * The analysis is identical either way: the same rules against the same live + * database, so the row counts and existing indexes are real even though the + * migration is not yet. */ final readonly class LintMigration implements Tool { + /** + * How many findings come back before the response starts counting instead. + * + * A directory of two hundred migrations can produce hundreds of findings, and an + * agent that receives all of them has spent its context on a wall of text it + * cannot act on. The count is always exact; only the list is bounded. + */ + private const int LIMIT = 25; + public function __construct(private MigrationAnalyzer $analyzer) {} public function name(): string @@ -29,12 +44,14 @@ public function name(): string public function description(): string { - return 'Analyse a Laravel migration file for destructive or risky schema operations, using ' - .'the live database for table sizes and existing indexes. Call this immediately after ' - .'writing or editing a migration, before showing it to the user. Returns findings with a ' - .'risk level (safe, low, medium, high, critical), whether each operation is destructive ' - .'and reversible, and a concrete remediation. An empty findings list means nothing was ' - .'found; check the warnings field, which lists anything the analysis could not read.'; + return 'Analyse a Laravel migration for destructive or risky schema operations, using the ' + .'live database for table sizes and existing indexes. Pass "source" with the migration ' + .'code to check a draft BEFORE writing it to disk — do this while composing a migration, ' + .'then fix and re-check until nothing is above low. Pass "path" instead to check a file ' + .'or directory that already exists. Returns findings with a risk level (safe, low, ' + .'medium, high, critical), whether each is destructive and reversible, and a remediation. ' + .'An empty findings list does not mean safe: read "warnings", which lists what the ' + .'analysis could not read, such as table names built from config or raw DB::statement().'; } public function schema(): array @@ -42,22 +59,39 @@ public function schema(): array return [ 'type' => 'object', 'properties' => [ + 'source' => [ + 'type' => 'string', + 'description' => 'The full PHP source of a migration, including the opening tag. ' + .'Use this to check a draft before it is written to disk.', + ], 'path' => [ 'type' => 'string', - 'description' => 'Path to the migration file, absolute or relative to the ' - .'application root. A directory analyses every migration in it.', + 'description' => 'Path to an existing migration file or a directory of them, ' + .'absolute or relative to the application root.', ], ], - 'required' => ['path'], + 'oneOf' => [ + ['required' => ['source']], + ['required' => ['path']], + ], ]; } public function handle(array $arguments): array { + $source = $arguments['source'] ?? null; + + if (is_string($source) && trim($source) !== '') { + return $this->draft($source); + } + $path = $arguments['path'] ?? null; if (! is_string($path) || $path === '') { - return ['error' => 'A path is required.']; + return [ + 'error' => 'Pass either "source" with the migration code, or "path" to a file.', + 'next' => 'To check a migration you are drafting, pass its full PHP source as "source".', + ]; } $report = $this->analyzer->analyze(MigrationScope::All, [$path]); @@ -65,20 +99,88 @@ public function handle(array $arguments): array if ($report->migrations === []) { return [ 'error' => 'No migration was found at '.$path.'.', - 'hint' => 'Give a path to a .php migration file or a directory of them.', + 'next' => 'Give a path to a .php migration file or a directory of them, or pass ' + .'"source" instead to check code that is not on disk yet.', ]; } + return $this->result($report); + } + + /** + * Analyse source that is not on disk. + * + * Written to a temporary file rather than parsed in memory, because that is the + * one way the draft goes through *exactly* the path a real migration does — the + * same locator, parser, rules and database context — instead of a parallel one + * that could drift out of agreement with it. The file is named the way Laravel + * names migrations so the locator recognises it, lives in the system temporary + * directory rather than the user's repository, and is removed on every path out. + * + * @return array + */ + private function draft(string $source): array + { + $directory = sys_get_temp_dir().DIRECTORY_SEPARATOR.'difflock-draft-'.bin2hex(random_bytes(8)); + $file = $directory.DIRECTORY_SEPARATOR.'2000_01_01_000000_draft.php'; + + if (! @mkdir($directory, 0o700, true) && ! is_dir($directory)) { + return ['error' => 'Difflock could not create a temporary directory to analyse the draft.']; + } + + try { + file_put_contents($file, $source); + + $report = $this->analyzer->analyze(MigrationScope::All, [$file]); + $parsed = $report->migrations[0] ?? null; + + // A draft with no schema statements is not a clean bill of health — it is + // source Difflock could not read as a migration at all. Reporting "no + // findings" there is the one answer that would make an agent confidently + // tell the user something unchecked is fine. + if ($parsed === null || $parsed->statements === []) { + return [ + 'error' => 'No schema operations were found in that source.', + 'next' => 'Send the whole file, including the opening $parsed->warnings ?? [], + ]; + } + + return ['analysed_from' => 'source'] + $this->result($report); + } catch (Throwable $exception) { + return ['error' => 'Difflock could not analyse the draft: '.$exception->getMessage()]; + } finally { + // Both removed on every path, including the exception one. A draft is the + // user's unreleased code and has no business outliving the question. + @unlink($file); + @rmdir($directory); + } + } + + /** + * @return array + */ + private function result(MigrationReport $report): array + { + $findings = $report->findings; + $shown = array_slice($findings, 0, self::LIMIT); + return [ 'analysed' => count($report->migrations), 'risk' => $report->highestRisk()->value, 'counts' => $report->summary()->counts, + 'total_findings' => count($findings), + 'showing' => count($shown), + 'truncated' => count($shown) < count($findings), 'findings' => array_map( static fn (MigrationFinding $finding): array => $finding->toArray(), - $report->findings, + $shown, ), - // Never omitted. A clean result over a file the parser could only half read - // is the one case where an agent would confidently tell the user it is fine. + // Never omitted, and never empty-by-default. A clean findings list over a + // file the parser could only half read is the one case where an agent + // would confidently tell the user the migration is fine. 'warnings' => $report->warnings(), 'database_available' => $report->databaseAvailable, ]; diff --git a/src/Mcp/Tools/Rules.php b/src/Mcp/Tools/Rules.php new file mode 100644 index 0000000..c741b61 --- /dev/null +++ b/src/Mcp/Tools/Rules.php @@ -0,0 +1,129 @@ + 'object', + 'properties' => [ + 'rule' => [ + 'type' => 'string', + 'description' => 'A single rule identifier, such as unindexed-foreign-key. ' + .'Omit to list them all.', + ], + ], + 'required' => [], + ]; + } + + public function handle(array $arguments): array + { + $wanted = $arguments['rule'] ?? null; + $wanted = is_string($wanted) && $wanted !== '' ? $wanted : null; + + $rules = []; + + foreach ($this->registry->resolve($this->container) as $rule) { + if ($wanted !== null && $rule->identifier() !== $wanted) { + continue; + } + + $rules[] = [ + 'rule' => $rule->identifier(), + 'class' => $rule::class, + 'built_in' => str_starts_with($rule::class, 'Difflock\\Migration\\Rules\\'), + 'explains' => $this->documentation($rule), + ]; + } + + if ($wanted !== null && $rules === []) { + return [ + 'error' => 'No rule called '.$wanted.' is registered.', + 'next' => 'Call this tool with no arguments to see which rules this application has.', + ]; + } + + return ['rules' => $rules]; + } + + /** + * The rule's own class documentation, as prose. + * + * Read from the docblock rather than duplicated into a table, so it cannot drift + * away from the rule it describes: changing a rule's reasoning changes what an + * agent is told about it, with nothing to keep in step. + */ + private function documentation(MigrationRule $rule): string + { + try { + $comment = (new ReflectionClass($rule))->getDocComment(); + } catch (ReflectionException) { + return ''; + } + + if ($comment === false) { + return ''; + } + + $lines = []; + + foreach (explode("\n", $comment) as $line) { + $line = trim($line); + $line = preg_replace('#^/\*\*+|^\*+/?|\*/$#', '', $line); + $line = is_string($line) ? trim($line) : ''; + + // Annotations are for the reader of the source, not for an agent asking + // what the rule is about. + if (str_starts_with($line, '@')) { + continue; + } + + $lines[] = $line; + } + + return trim(preg_replace('/\n{3,}/', "\n\n", implode("\n", $lines)) ?? ''); + } +} diff --git a/tests/Feature/AgentAlignmentTest.php b/tests/Feature/AgentAlignmentTest.php new file mode 100644 index 0000000..b290801 --- /dev/null +++ b/tests/Feature/AgentAlignmentTest.php @@ -0,0 +1,232 @@ +id(); + $table->unsignedBigInteger('customer_id'); + }); + + config()->set('difflock.migrations.paths', [fixtures()]); +}); + +/** + * @return array + */ +function lintDraft(string $source): array +{ + return (new LintMigration(app(MigrationAnalyzer::class)))->handle(['source' => $source]); +} + +function draftSource(string $body): string +{ + return << \$t->dropColumn('customer_id'));", + )); + + expect($result['analysed_from'])->toBe('source') + ->and($result['risk'])->toBe('critical') + ->and($result['findings'][0]['rule'])->toBe('drop-column') + ->and($result['findings'][0]['column'])->toBe('customer_id'); + }); + + it('judges a draft against the real database, not a guess', function (): void { + DB::table('orders')->insert(array_map( + static fn (int $i): array => ['customer_id' => $i], + range(1, 5), + )); + + $result = lintDraft(draftSource( + " Schema::table('orders', fn (Blueprint \$t) => \$t->string('status'));", + )); + + // High, not medium: the rule saw actual rows in the actual table. + expect($result['findings'][0]['rule'])->toBe('add-not-null-column') + ->and($result['findings'][0]['risk'])->toBe('high') + ->and($result['findings'][0]['context'])->toContain('5 rows'); + }); + + it('leaves nothing behind on disk', function (): void { + $before = glob(sys_get_temp_dir().DIRECTORY_SEPARATOR.'difflock-draft-*') ?: []; + + lintDraft(draftSource(" Schema::drop('orders');")); + + expect(glob(sys_get_temp_dir().DIRECTORY_SEPARATOR.'difflock-draft-*') ?: [])->toBe($before); + }); + + it('says what to do when the source is not a migration', function (): void { + $result = lintDraft('toContain('No schema operations were found') + ->and($result['next'])->toContain('opening handle([]); + + expect($result['error'])->toContain('either "source"') + ->and($result['next'])->toContain('drafting'); + }); +}); + +describe('bounded responses', function (): void { + /** + * An agent that receives four hundred findings has spent its context on a wall of + * text it cannot act on. The count stays exact; only the list is capped. + */ + it('caps the findings it returns but never the count', function (): void { + $result = (new LintMigration(app(MigrationAnalyzer::class)))->handle(['path' => fixtures()]); + + expect($result['showing'])->toBeLessThanOrEqual(25) + ->and($result['total_findings'])->toBeGreaterThanOrEqual($result['showing']) + ->and($result)->toHaveKey('truncated') + ->and($result['counts'])->toHaveKeys(['safe', 'low', 'medium', 'high', 'critical']); + }); +}); + +describe('difflock_rules', function (): void { + it('publishes what each rule checks, from the rule itself', function (): void { + $result = (new Rules(app(RuleRegistry::class), app()))->handle([]); + + $identifiers = array_column($result['rules'], 'rule'); + + expect($identifiers)->toContain('drop-column', 'unindexed-foreign-key', 'sensitive-column'); + + $dropColumn = collect($result['rules'])->firstWhere('rule', 'drop-column'); + + expect($dropColumn['built_in'])->toBeTrue() + ->and($dropColumn['explains'])->toContain('dropColumn') + ->and($dropColumn['explains'])->toContain('down()') + ->and($dropColumn['explains'])->not->toContain('@param') + ->and($dropColumn['explains'])->not->toContain('/**'); + }); + + it('answers about one rule', function (): void { + $result = (new Rules(app(RuleRegistry::class), app()))->handle(['rule' => 'unindexed-foreign-key']); + + expect($result['rules'])->toHaveCount(1) + ->and($result['rules'][0]['explains'])->toContain('PostgreSQL'); + }); + + it('says so rather than inventing a rule that does not exist', function (): void { + $result = (new Rules(app(RuleRegistry::class), app()))->handle(['rule' => 'invented-rule']); + + expect($result['error'])->toContain('invented-rule') + ->and($result['next'])->toContain('no arguments'); + }); +}); + +describe('protocol integrity', function (): void { + /** + * The failure that motivated this: a live application emitted a PHP deprecation + * to STDOUT from `config/database.php`, which landed ahead of the handshake and + * made the whole server appear not to exist. A tool that prints must not be able + * to do that. + */ + it('survives a tool that writes to stdout', function (): void { + $noisy = new class implements Tool + { + public function name(): string + { + return 'noisy'; + } + + public function description(): string + { + return 'Writes where it should not.'; + } + + public function schema(): array + { + return ['type' => 'object', 'properties' => [], 'required' => []]; + } + + public function handle(array $arguments): array + { + echo "this would corrupt the stream\n"; + print_r(['and', 'so', 'would', 'this']); + + return ['ok' => true]; + } + }; + + $server = new Server([$noisy]); + + ob_start(); + $response = $server->dispatch('{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"noisy"}}'); + $leaked = ob_get_clean(); + + expect($leaked)->toBe('') + ->and($response['result']['content'][0]['text'])->toContain('"ok": true'); + }); + + it('keeps the buffer balanced when a tool throws', function (): void { + $throwing = new class implements Tool + { + public function name(): string + { + return 'throwing'; + } + + public function description(): string + { + return 'Fails.'; + } + + public function schema(): array + { + return ['type' => 'object', 'properties' => [], 'required' => []]; + } + + public function handle(array $arguments): array + { + echo 'noise before the failure'; + + throw new RuntimeException('deliberate'); + } + }; + + $level = ob_get_level(); + + $response = (new Server([$throwing])) + ->dispatch('{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"throwing"}}'); + + expect(ob_get_level())->toBe($level) + ->and($response['result']['isError'])->toBeTrue() + ->and($response['result']['content'][0]['text'])->toContain('deliberate'); + }); +}); diff --git a/tests/Feature/McpTest.php b/tests/Feature/McpTest.php index 9a49a1d..e8be4e2 100644 --- a/tests/Feature/McpTest.php +++ b/tests/Feature/McpTest.php @@ -104,7 +104,7 @@ function callTool(string $name, array $arguments = []): array 'arguments' => ['path' => 12345], ])['result']; - expect($result['content'][0]['text'])->toContain('A path is required'); + expect($result['content'][0]['text'])->toContain('Pass either'); }); });