From 266c1feba636cac1940f0d98effce6a2dfce0e53 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Tue, 6 Oct 2026 11:11:17 -0400 Subject: [PATCH] feat: add a destructive marker to #[Command] and refuse marked commands in mcp #[Command] takes a new `destructive: bool = false` argument. It is copied onto CommandDefinition (and the discovery cache, whose format version goes to 4) so any caller can tell a state-changing command from a read-only one without guessing from its name or options. Every shipped command that changes or deletes stored state is now marked: db:migrate, db:reset, db:rollback, db:rebuild, db:seed, cache:clear, page-cache:clear, page-cache:purge, queue:clear, log:clear, session:gc, auth:clear-tokens, discovery:clear, indexer:rebuild, admin-auth:permissions:sync and devai:install. The MCP run_console_command tool now refuses commands marked destructive unless mcp.console.allow_destructive is on, instead of inferring destructiveness from a declared `force` flag. That closes the gap where cache:clear, queue:clear, log:clear and page-cache:clear were guarded only by the allowlist. --force stays the CLI confirmation mechanism via DestructiveCommandGuard. A repo-level test fails when a shipped command named *:clear, *:reset, *:rollback, *:rebuild, *:fresh, *:seed, *:truncate, *:purge or *:clear-tokens, or one that depends on DestructiveCommandGuard, lacks the marker. Fixes #516 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KyjR4cJTfro54iFZ9CWkv1 --- .../src/Command/SyncPermissionsCommand.php | 1 + .../src/Command/ClearTokensCommand.php | 7 +- packages/cache/src/Command/ClearCommand.php | 2 +- .../src/Commands/RebuildIndexCommand.php | 2 +- packages/core/src/Attributes/Command.php | 4 + .../core/src/Command/CommandDefinition.php | 1 + .../core/src/Command/CommandDiscovery.php | 1 + .../src/Commands/DiscoveryClearCommand.php | 2 +- .../core/src/Discovery/DiscoveryCache.php | 5 +- .../tests/Command/CommandAttributeTest.php | 12 ++ .../tests/Command/CommandDefinitionTest.php | 19 +++ .../tests/Command/CommandDiscoveryTest.php | 77 ++++++++++++ .../Discovery/DiscoveryCacheModulesTest.php | 5 +- .../Unit/Discovery/DiscoveryCacheTest.php | 18 ++- .../src/Command/DestructiveCommandGuard.php | 5 + .../database/src/Command/MigrateCommand.php | 1 + .../database/src/Command/RebuildCommand.php | 7 +- .../database/src/Command/ResetCommand.php | 2 +- .../database/src/Command/RollbackCommand.php | 7 +- packages/database/src/Command/SeedCommand.php | 2 +- .../devai/src/Commands/InstallCommand.php | 1 + packages/docs-markdown/docs/packages/cli.md | 17 +++ packages/docs-markdown/docs/packages/core.md | 5 +- .../docs-markdown/docs/packages/database.md | 2 +- packages/docs-markdown/docs/packages/mcp.md | 7 +- packages/log/src/Command/ClearCommand.php | 2 +- .../Tools/Runtime/RunConsoleCommandTool.php | 11 +- .../mcp/tests/Feature/ContainerWiringTest.php | 11 +- .../Runtime/RunConsoleCommandToolTest.php | 49 +++++++- .../page-cache/src/Command/ClearCommand.php | 2 +- .../page-cache/src/Command/PurgeCommand.php | 2 +- .../queue/src/Command/QueueClearCommand.php | 2 +- .../src/Command/GarbageCollectCommand.php | 2 +- tests/DestructiveCommandMarkerTest.php | 116 ++++++++++++++++++ 34 files changed, 375 insertions(+), 34 deletions(-) create mode 100644 tests/DestructiveCommandMarkerTest.php diff --git a/packages/admin-auth/src/Command/SyncPermissionsCommand.php b/packages/admin-auth/src/Command/SyncPermissionsCommand.php index 3b16ec663..c47484b7c 100644 --- a/packages/admin-auth/src/Command/SyncPermissionsCommand.php +++ b/packages/admin-auth/src/Command/SyncPermissionsCommand.php @@ -39,6 +39,7 @@ name: 'admin-auth:permissions:sync', description: 'Write the registered admin permissions to the database (--prune deletes unregistered ones)', flags: ['prune', 'force'], + destructive: true, )] readonly class SyncPermissionsCommand implements CommandInterface { diff --git a/packages/authentication/src/Command/ClearTokensCommand.php b/packages/authentication/src/Command/ClearTokensCommand.php index 098096083..2fbf993b0 100644 --- a/packages/authentication/src/Command/ClearTokensCommand.php +++ b/packages/authentication/src/Command/ClearTokensCommand.php @@ -11,7 +11,12 @@ use Marko\Core\Command\Output; /** @noinspection PhpUnused */ -#[Command(name: 'auth:clear-tokens', description: 'Clear expired remember me tokens', flags: ['force'])] +#[Command( + name: 'auth:clear-tokens', + description: 'Clear expired remember me tokens', + flags: ['force'], + destructive: true, +)] readonly class ClearTokensCommand implements CommandInterface { public function __construct( diff --git a/packages/cache/src/Command/ClearCommand.php b/packages/cache/src/Command/ClearCommand.php index 8d57ddba6..67b9b5e9d 100644 --- a/packages/cache/src/Command/ClearCommand.php +++ b/packages/cache/src/Command/ClearCommand.php @@ -11,7 +11,7 @@ use Marko\Core\Command\Output; /** @noinspection PhpUnused */ -#[Command(name: 'cache:clear', description: 'Clear all cached items')] +#[Command(name: 'cache:clear', description: 'Clear all cached items', destructive: true)] readonly class ClearCommand implements CommandInterface { public function __construct( diff --git a/packages/codeindexer/src/Commands/RebuildIndexCommand.php b/packages/codeindexer/src/Commands/RebuildIndexCommand.php index 2bef59671..484aef5ad 100644 --- a/packages/codeindexer/src/Commands/RebuildIndexCommand.php +++ b/packages/codeindexer/src/Commands/RebuildIndexCommand.php @@ -11,7 +11,7 @@ use Marko\Core\Command\Input; use Marko\Core\Command\Output; -#[Command(name: 'indexer:rebuild', description: 'Rebuild the Marko code index cache')] +#[Command(name: 'indexer:rebuild', description: 'Rebuild the Marko code index cache', destructive: true)] class RebuildIndexCommand implements CommandInterface { private const string CACHE_FILE = '.marko/index.cache'; diff --git a/packages/core/src/Attributes/Command.php b/packages/core/src/Attributes/Command.php index 15559fc2c..7e675fff0 100644 --- a/packages/core/src/Attributes/Command.php +++ b/packages/core/src/Attributes/Command.php @@ -12,11 +12,15 @@ /** * @param list $aliases * @param list $flags Value-less options (e.g. `force`, `d`) that never consume the next token + * @param bool $destructive The command changes or deletes stored state (database rows, cache entries, queued + * jobs, log files...). Callers that run commands on someone else's behalf, such as the + * MCP run_console_command tool, refuse destructive commands unless explicitly allowed */ public function __construct( public string $name, public string $description = '', public array $aliases = [], public array $flags = [], + public bool $destructive = false, ) {} } diff --git a/packages/core/src/Command/CommandDefinition.php b/packages/core/src/Command/CommandDefinition.php index 9a8e5aef5..4869054e3 100644 --- a/packages/core/src/Command/CommandDefinition.php +++ b/packages/core/src/Command/CommandDefinition.php @@ -19,5 +19,6 @@ public function __construct( public string $description = '', public array $aliases = [], public array $flags = [], + public bool $destructive = false, ) {} } diff --git a/packages/core/src/Command/CommandDiscovery.php b/packages/core/src/Command/CommandDiscovery.php index ff7ff7e8b..8c9cd2089 100644 --- a/packages/core/src/Command/CommandDiscovery.php +++ b/packages/core/src/Command/CommandDiscovery.php @@ -90,6 +90,7 @@ private function discoverInDirectory( description: $attribute->description, aliases: $attribute->aliases, flags: $attribute->flags, + destructive: $attribute->destructive, ); } diff --git a/packages/core/src/Commands/DiscoveryClearCommand.php b/packages/core/src/Commands/DiscoveryClearCommand.php index 34c0e84a3..467f82a53 100644 --- a/packages/core/src/Commands/DiscoveryClearCommand.php +++ b/packages/core/src/Commands/DiscoveryClearCommand.php @@ -11,7 +11,7 @@ use Marko\Core\Discovery\DiscoveryCache; /** @noinspection PhpUnused */ -#[Command(name: 'discovery:clear', description: 'Remove the discovery cache')] +#[Command(name: 'discovery:clear', description: 'Remove the discovery cache', destructive: true)] readonly class DiscoveryClearCommand implements CommandInterface { public function __construct( diff --git a/packages/core/src/Discovery/DiscoveryCache.php b/packages/core/src/Discovery/DiscoveryCache.php index 03094cd0c..20a3dda2e 100644 --- a/packages/core/src/Discovery/DiscoveryCache.php +++ b/packages/core/src/Discovery/DiscoveryCache.php @@ -28,7 +28,7 @@ */ class DiscoveryCache { - public const int CACHE_VERSION = 3; + public const int CACHE_VERSION = 4; private const array REQUIRED_KEYS = [ 'fingerprint', @@ -161,6 +161,7 @@ public function write( 'description' => $d->description, 'aliases' => $d->aliases, 'flags' => $d->flags, + 'destructive' => $d->destructive, ], $payload['commands'], ), @@ -561,6 +562,7 @@ private function hydrateCommands( $this->assertStringField($path, $record, 'commands', $i, 'description'); $this->assertArrayField($path, $record, 'commands', $i, 'aliases'); $this->assertArrayField($path, $record, 'commands', $i, 'flags'); + $this->assertBoolField($path, $record, 'commands', $i, 'destructive'); $result[] = new CommandDefinition( commandClass: $record['commandClass'], @@ -568,6 +570,7 @@ private function hydrateCommands( description: $record['description'], aliases: $record['aliases'], flags: $record['flags'], + destructive: $record['destructive'], ); } diff --git a/packages/core/tests/Command/CommandAttributeTest.php b/packages/core/tests/Command/CommandAttributeTest.php index 88bc213eb..b9edd2353 100644 --- a/packages/core/tests/Command/CommandAttributeTest.php +++ b/packages/core/tests/Command/CommandAttributeTest.php @@ -81,6 +81,18 @@ class DescribedCommand {} expect($command->flags)->toBe([]); }); +it('accepts the destructive marker in Command attribute', function (): void { + $command = new Command(name: 'cache:clear', destructive: true); + + expect($command->destructive)->toBeTrue(); +}); + +it('defaults destructive to false when not provided', function (): void { + $command = new Command(name: 'test:cmd'); + + expect($command->destructive)->toBeFalse(); +}); + it('preserves existing Command attribute behavior without aliases', function (): void { $reflection = new ReflectionClass(TestCommand::class); $attributes = $reflection->getAttributes(Command::class); diff --git a/packages/core/tests/Command/CommandDefinitionTest.php b/packages/core/tests/Command/CommandDefinitionTest.php index a435c52ff..87e7114a9 100644 --- a/packages/core/tests/Command/CommandDefinitionTest.php +++ b/packages/core/tests/Command/CommandDefinitionTest.php @@ -72,6 +72,25 @@ expect($definition->flags)->toBe([]); }); +it('stores the destructive marker in CommandDefinition', function (): void { + $definition = new CommandDefinition( + commandClass: 'App\Command\TestCommand', + name: 'cache:clear', + destructive: true, + ); + + expect($definition->destructive)->toBeTrue(); +}); + +it('defaults the destructive marker to false', function (): void { + $definition = new CommandDefinition( + commandClass: 'App\Command\TestCommand', + name: 'test:command', + ); + + expect($definition->destructive)->toBeFalse(); +}); + it('marks CommandDefinition as readonly', function (): void { $reflection = new ReflectionClass(CommandDefinition::class); diff --git a/packages/core/tests/Command/CommandDiscoveryTest.php b/packages/core/tests/Command/CommandDiscoveryTest.php index dd2ca9505..9cadef657 100644 --- a/packages/core/tests/Command/CommandDiscoveryTest.php +++ b/packages/core/tests/Command/CommandDiscoveryTest.php @@ -432,6 +432,83 @@ public function execute( rmdir($tempDir); }); +it('discovers the destructive marker from the Command attribute', function (): void { + $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); + mkdir($tempDir . '/src', 0755, true); + + $destructiveCode = <<<'PHP' +discover([$manifest]) as $definition) { + $byName[$definition->name] = $definition; + } + + expect($byName)->toHaveCount(2) + ->and($byName['app:wipe']->destructive)->toBeTrue() + ->and($byName['app:status']->destructive)->toBeFalse(); + + // Cleanup + unlink($tempDir . '/src/WipeCommand.php'); + unlink($tempDir . '/src/StatusCommand.php'); + rmdir($tempDir . '/src'); + rmdir($tempDir); +}); + it('creates CommandDefinition with empty aliases when none specified', function (): void { $tempDir = sys_get_temp_dir() . '/marko_test_' . bin2hex(random_bytes(8)); mkdir($tempDir . '/src', 0755, true); diff --git a/packages/core/tests/Unit/Discovery/DiscoveryCacheModulesTest.php b/packages/core/tests/Unit/Discovery/DiscoveryCacheModulesTest.php index 58c309e68..3d41a36f0 100644 --- a/packages/core/tests/Unit/Discovery/DiscoveryCacheModulesTest.php +++ b/packages/core/tests/Unit/Discovery/DiscoveryCacheModulesTest.php @@ -175,7 +175,10 @@ function (): void { " 2, 'preferences' => [], 'plugins' => [], 'observers' => [], 'commands' => []];", ); - expect(fn () => $this->cache->load())->toThrow(DiscoveryCacheException::class, 'found 2, expected 3'); + expect(fn () => $this->cache->load())->toThrow( + DiscoveryCacheException::class, + 'found 2, expected ' . DiscoveryCache::CACHE_VERSION, + ); }); it('throws malformed when a module record or section is invalid', function (): void { diff --git a/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php b/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php index 4285816a5..b3001a13c 100644 --- a/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php +++ b/packages/core/tests/Unit/Discovery/DiscoveryCacheTest.php @@ -72,6 +72,7 @@ function samplePayload(): array description: 'Clears the cache', aliases: ['cc'], flags: ['force'], + destructive: true, ), ], ]; @@ -194,7 +195,8 @@ function (): void { ->and($loaded['commands'][0]->name)->toBe('cache:clear') ->and($loaded['commands'][0]->description)->toBe('Clears the cache') ->and($loaded['commands'][0]->aliases)->toBe(['cc']) - ->and($loaded['commands'][0]->flags)->toBe(['force']); + ->and($loaded['commands'][0]->flags)->toBe(['force']) + ->and($loaded['commands'][0]->destructive)->toBeTrue(); }, ); @@ -367,6 +369,20 @@ function (): void { ); expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + // Command destructive missing (cache written before the destructive marker existed) + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [], 'observers' => [], 'commands' => [['commandClass' => 'App\\\\Cmd', 'name' => 'cmd', 'description' => '', 'aliases' => [], 'flags' => []]]];", + ); + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + + // Command destructive as string instead of bool + file_put_contents( + $path, + " $version, 'preferences' => [], 'plugins' => [], 'observers' => [], 'commands' => [['commandClass' => 'App\\\\Cmd', 'name' => 'cmd', 'description' => '', 'aliases' => [], 'flags' => [], 'destructive' => 'yes']]];", + ); + expect(fn () => $cache->load())->toThrow(DiscoveryCacheException::class); + // Plugin beforeMethods as string file_put_contents( $path, diff --git a/packages/database/src/Command/DestructiveCommandGuard.php b/packages/database/src/Command/DestructiveCommandGuard.php index c9d5b8a04..da80d38cb 100644 --- a/packages/database/src/Command/DestructiveCommandGuard.php +++ b/packages/database/src/Command/DestructiveCommandGuard.php @@ -25,6 +25,11 @@ * per call with allowInProduction: production is then treated like staging, so it still needs * --force and still asks when someone can answer. confirmInDevelopment makes development and * testing ask for confirmation too when someone can answer. + * + * --force is the terminal confirmation only. A command that calls this guard must also be marked + * #[Command(destructive: true)]: callers that run commands for someone else (the MCP run_console_command + * tool) read that marker, not the presence of a force flag, and a repository test fails when a + * shipped command that depends on this guard is unmarked. */ readonly class DestructiveCommandGuard { diff --git a/packages/database/src/Command/MigrateCommand.php b/packages/database/src/Command/MigrateCommand.php index 60ab7c012..7f8e5a88e 100644 --- a/packages/database/src/Command/MigrateCommand.php +++ b/packages/database/src/Command/MigrateCommand.php @@ -47,6 +47,7 @@ name: 'db:migrate', description: 'Apply database migrations', flags: ['generate', 'no-generate', 'force', 'verbose', 'v'], + destructive: true, )] readonly class MigrateCommand implements CommandInterface { diff --git a/packages/database/src/Command/RebuildCommand.php b/packages/database/src/Command/RebuildCommand.php index cf35ca7a7..52da06305 100644 --- a/packages/database/src/Command/RebuildCommand.php +++ b/packages/database/src/Command/RebuildCommand.php @@ -12,7 +12,12 @@ use Marko\Database\Migration\Migrator; /** @noinspection PhpUnused */ -#[Command(name: 'db:rebuild', description: 'Reset and re-run all migrations (clean slate)', flags: ['force'])] +#[Command( + name: 'db:rebuild', + description: 'Reset and re-run all migrations (clean slate)', + flags: ['force'], + destructive: true, +)] readonly class RebuildCommand implements CommandInterface { public function __construct( diff --git a/packages/database/src/Command/ResetCommand.php b/packages/database/src/Command/ResetCommand.php index 8b8cccc78..2f414fd46 100644 --- a/packages/database/src/Command/ResetCommand.php +++ b/packages/database/src/Command/ResetCommand.php @@ -12,7 +12,7 @@ use Marko\Database\Migration\Migrator; /** @noinspection PhpUnused */ -#[Command(name: 'db:reset', description: 'Rollback all database migrations', flags: ['force'])] +#[Command(name: 'db:reset', description: 'Rollback all database migrations', flags: ['force'], destructive: true)] readonly class ResetCommand implements CommandInterface { public function __construct( diff --git a/packages/database/src/Command/RollbackCommand.php b/packages/database/src/Command/RollbackCommand.php index 37f2418f3..cd735c60e 100644 --- a/packages/database/src/Command/RollbackCommand.php +++ b/packages/database/src/Command/RollbackCommand.php @@ -12,7 +12,12 @@ use Marko\Database\Migration\Migrator; /** @noinspection PhpUnused */ -#[Command(name: 'db:rollback', description: 'Rollback the last batch of migrations', flags: ['force'])] +#[Command( + name: 'db:rollback', + description: 'Rollback the last batch of migrations', + flags: ['force'], + destructive: true, +)] readonly class RollbackCommand implements CommandInterface { public function __construct( diff --git a/packages/database/src/Command/SeedCommand.php b/packages/database/src/Command/SeedCommand.php index 2d9b3fd40..12aa191c8 100644 --- a/packages/database/src/Command/SeedCommand.php +++ b/packages/database/src/Command/SeedCommand.php @@ -15,7 +15,7 @@ use Marko\Database\Seed\SeederRunner; /** @noinspection PhpUnused */ -#[Command(name: 'db:seed', description: 'Run database seeders', flags: ['force'])] +#[Command(name: 'db:seed', description: 'Run database seeders', flags: ['force'], destructive: true)] readonly class SeedCommand implements CommandInterface { public function __construct( diff --git a/packages/devai/src/Commands/InstallCommand.php b/packages/devai/src/Commands/InstallCommand.php index 643ff6fa8..8c1347bf9 100644 --- a/packages/devai/src/Commands/InstallCommand.php +++ b/packages/devai/src/Commands/InstallCommand.php @@ -21,6 +21,7 @@ name: 'devai:install', description: 'Install Marko AI development tooling for selected agents', flags: ['force', 'update-gitignore', 'skip-lsp-deps', 'yes'], + destructive: true, )] readonly class InstallCommand implements CommandInterface { diff --git a/packages/docs-markdown/docs/packages/cli.md b/packages/docs-markdown/docs/packages/cli.md index f655136da..367512b2c 100644 --- a/packages/docs-markdown/docs/packages/cli.md +++ b/packages/docs-markdown/docs/packages/cli.md @@ -194,9 +194,26 @@ use Marko\Core\Attributes\Command; description: 'What it does', aliases: ['alias'], // Optional alternative names flags: ['force', 'f'], // Optional value-less options that never consume the next token + destructive: true, // Optional: the command changes or deletes stored state (default false) )] ``` +### Destructive Commands + +Mark a command that changes or deletes stored state (database rows or schema, cache entries, queued jobs, log files, sessions, tokens) with `destructive: true`: + +```php +use Marko\Core\Attributes\Command; + +#[Command(name: 'report:purge', description: 'Delete old reports', flags: ['force'], destructive: true)] +``` + +The marker is how callers that run commands on someone else's behalf tell a state-changing command from a read-only one, without guessing from its name or options. The [MCP `run_console_command` tool](/docs/packages/mcp/#run_console_command-safety) refuses every destructive command unless the server operator sets `mcp.console.allow_destructive`. The marker is copied onto the command's `CommandDefinition` as `$definition->destructive`, so your own tooling can read it from the `CommandRegistry`. + +The marker does not change how the command runs from a terminal. Asking for `--force` or a confirmation is still the command's job: database commands use [`DestructiveCommandGuard`](/docs/packages/database/#using-the-policy-in-your-own-commands), and any command can use [`ConfirmationPrompterInterface`](/docs/packages/core/#asking-for-confirmation). + +Every shipped command that changes or deletes stored state carries the marker: `db:migrate`, `db:reset`, `db:rollback`, `db:rebuild`, `db:seed`, `cache:clear`, `page-cache:clear`, `page-cache:purge`, `queue:clear`, `log:clear`, `session:gc`, `auth:clear-tokens`, `discovery:clear`, `indexer:rebuild`, `admin-auth:permissions:sync` and `devai:install`. A repository test fails when a shipped command named `*:clear`, `*:reset`, `*:rollback`, `*:rebuild`, `*:fresh`, `*:seed`, `*:truncate`, `*:purge` or `*:clear-tokens`, or one that uses `DestructiveCommandGuard`, is missing it. + ### Exceptions The CLI provides rich error messages with context and suggestions via `CliException` and its subclasses: diff --git a/packages/docs-markdown/docs/packages/core.md b/packages/docs-markdown/docs/packages/core.md index a21a1c002..73ff344f7 100644 --- a/packages/docs-markdown/docs/packages/core.md +++ b/packages/docs-markdown/docs/packages/core.md @@ -327,7 +327,7 @@ use Marko\Core\Command\ConfirmationPrompterInterface; use Marko\Core\Command\Input; use Marko\Core\Command\Output; -#[Command(name: 'billing:purge', description: 'Delete archived invoices', flags: ['force'])] +#[Command(name: 'billing:purge', description: 'Delete archived invoices', flags: ['force'], destructive: true)] class PurgeCommand implements CommandInterface { public function __construct( @@ -363,7 +363,7 @@ class PurgeCommand implements CommandInterface **`--no-interaction`** is accepted by every command; you don't declare it. When it is passed, `isInteractive()` is `false`, `$input->isInteractive()` is `false`, and `confirm()` asks nothing and returns `$default`. `isInteractive()` is also `false` when standard input is not a terminal (CI, a pipe), although `confirm()` still reads a piped answer such as `echo y | marko billing:purge`. -A command that must not go ahead without a person (a destructive action) checks `isInteractive()` first and refuses loudly with a flag such as `--force`, as above. Relying on the default is for optional offers that are safe to skip. +A command that must not go ahead without a person (a destructive action) checks `isInteractive()` first and refuses loudly with a flag such as `--force`, as above. Mark it `destructive: true` on `#[Command]` too, so callers that run commands for someone else, such as the MCP `run_console_command` tool, refuse it unless explicitly allowed (see [Destructive Commands](/docs/packages/cli/#destructive-commands)). Relying on the default is for optional offers that are safe to skip. `StdinConfirmationPrompter` is the default implementation, bound by `Application`. To replace it, bind `ConfirmationPrompterInterface` in your module's `module.php`. In tests, use [`FakeConfirmationPrompter`](/docs/packages/testing/#fakeconfirmationprompter) from `marko/testing`. @@ -593,6 +593,7 @@ if (!ErrorCapture::run($reason, fn (): bool => rename($tmp, $path))) { #[Observer(event: EventClass::class)] // React to events (synchronous) #[Observer(event: EventClass::class, async: true)] // Queue it (needs marko/queue; throws EventException without it) #[Command(name: 'cmd:name', description: '', aliases: [], flags: [])] // Register CLI command; flags never take a value +#[Command(name: 'cmd:wipe', destructive: true)] // Changes or deletes stored state (MCP refuses it by default) ``` ### Container diff --git a/packages/docs-markdown/docs/packages/database.md b/packages/docs-markdown/docs/packages/database.md index e62e4f8fe..5a2f7e37f 100644 --- a/packages/docs-markdown/docs/packages/database.md +++ b/packages/docs-markdown/docs/packages/database.md @@ -1692,7 +1692,7 @@ Production has no override. `SeederRunner` applies the same policy when you call #### Using the policy in your own commands -The `db:*` commands get this policy from `Marko\Database\Command\DestructiveCommandGuard`. Inject it into your own destructive command and call `check()` before doing anything. It returns `null` when the command may go ahead. Otherwise it returns the exit code to stop with: `1` when the environment refuses the command, or `0` when the person declines the confirmation. It writes the error or cancellation message itself. Declare `force` as a flag on the command so `--force` never consumes the next argument. +The `db:*` commands get this policy from `Marko\Database\Command\DestructiveCommandGuard`. Inject it into your own destructive command and call `check()` before doing anything. It returns `null` when the command may go ahead. Otherwise it returns the exit code to stop with: `1` when the environment refuses the command, or `0` when the person declines the confirmation. It writes the error or cancellation message itself. Declare `force` as a flag on the command so `--force` never consumes the next argument, and mark the command `destructive: true` so the [MCP `run_console_command` tool](/docs/packages/mcp/#run_console_command-safety) and other callers refuse it unless explicitly allowed (`#[Command(name: 'app:wipe', flags: ['force'], destructive: true)]`). Every shipped command that uses the guard carries the marker, and a repository test keeps it that way. ```php public function check( diff --git a/packages/docs-markdown/docs/packages/mcp.md b/packages/docs-markdown/docs/packages/mcp.md index 6a317c9f4..6a05164e9 100644 --- a/packages/docs-markdown/docs/packages/mcp.md +++ b/packages/docs-markdown/docs/packages/mcp.md @@ -65,7 +65,7 @@ Conditional tools (registered only when their dependency is present): An agent can be steered by content it reads, and it controls the command name and every argument, including `--force`. So the server operator, not the agent, decides what `run_console_command` may run: 1. **Allowlist.** Only commands named in `mcp.console.allowed_commands` run (an alias of an allowed command also runs). Every other command is refused before it is dispatched. The default list is read-only: `list`, `module:list`, `route:list`, `db:status`, `db:diff`, `cache:status`, `page-cache:status`, `queue:status` and `queue:failed`. -2. **Destructive commands.** A command whose `#[Command]` attribute declares a `force` flag (`db:reset`, `db:rollback`, `db:rebuild`, `db:seed`, `auth:clear-tokens`, ...) is refused even when it is allowlisted, unless `mcp.console.allow_destructive` is `true` (`MCP_ALLOW_DESTRUCTIVE=true`). Passing `--force` cannot enable it. +2. **Destructive commands.** A command marked `#[Command(destructive: true)]` is refused even when it is allowlisted, unless `mcp.console.allow_destructive` is `true` (`MCP_ALLOW_DESTRUCTIVE=true`). Passing `--force` cannot enable it. Every shipped command that changes or deletes stored state carries the marker: `db:migrate`, `db:reset`, `db:rollback`, `db:rebuild`, `db:seed`, `cache:clear`, `page-cache:clear`, `page-cache:purge`, `queue:clear`, `log:clear`, `session:gc`, `auth:clear-tokens`, `discovery:clear`, `indexer:rebuild`, `admin-auth:permissions:sync` and `devai:install`. The tool reads the marker from the command's definition; it does not guess from option names, so a destructive command without a `--force` flag (such as `cache:clear`) is refused too. Mark your own state-changing commands the same way (see [marko/cli](/docs/packages/cli/#destructive-commands)). To allow more commands, override the list in your app's `config/mcp.php`: @@ -73,11 +73,12 @@ To allow more commands, override the list in your app's `config/mcp.php`: return [ 'console' => [ 'allowed_commands' => ['list', 'module:list', 'route:list', 'db:status', 'cache:clear'], + 'allow_destructive' => true, ], ]; ``` -Commands that change state without declaring a `force` flag, such as `cache:clear` or `queue:clear`, are guarded only by the allowlist, so add them deliberately. +`cache:clear` is destructive, so it needs both settings: the allowlist entry and `allow_destructive`. Turning on `allow_destructive` enables only the destructive commands that are also in the allowlist. ## `query_database` Safety @@ -120,7 +121,7 @@ For the strongest guarantee, also point the MCP server at a database user that o |-----|-----|---------|---------| | `mcp.allow_production` | `MCP_ALLOW_PRODUCTION` | `false` | Let `mcp:serve` start when the app environment is production | | `mcp.console.allowed_commands` | — | read-only commands (see above) | Commands `run_console_command` may run, by name or alias | -| `mcp.console.allow_destructive` | `MCP_ALLOW_DESTRUCTIVE` | `false` | Let `run_console_command` run allowlisted commands that declare a `force` flag | +| `mcp.console.allow_destructive` | `MCP_ALLOW_DESTRUCTIVE` | `false` | Let `run_console_command` run allowlisted commands marked `#[Command(destructive: true)]` | | `mcp.database.allow_writes` | `MCP_ALLOW_WRITES` | `false` | Let `query_database` run writes when the agent passes `allowWrite` and `confirm` | ## Related Packages diff --git a/packages/log/src/Command/ClearCommand.php b/packages/log/src/Command/ClearCommand.php index 1a8e1e408..95c13ac6a 100644 --- a/packages/log/src/Command/ClearCommand.php +++ b/packages/log/src/Command/ClearCommand.php @@ -12,7 +12,7 @@ use Psr\Clock\ClockInterface; /** @noinspection PhpUnused */ -#[Command(name: 'log:clear', description: 'Clear old log files')] +#[Command(name: 'log:clear', description: 'Clear old log files', destructive: true)] readonly class ClearCommand implements CommandInterface { public function __construct( diff --git a/packages/mcp/src/Tools/Runtime/RunConsoleCommandTool.php b/packages/mcp/src/Tools/Runtime/RunConsoleCommandTool.php index b54eee3bc..6340e2359 100644 --- a/packages/mcp/src/Tools/Runtime/RunConsoleCommandTool.php +++ b/packages/mcp/src/Tools/Runtime/RunConsoleCommandTool.php @@ -13,9 +13,10 @@ * The run_console_command MCP tool. * * The agent can be steered by content it reads, so the server operator decides what it may run. A command runs only - * when its name or alias is in the mcp.console.allowed_commands config list. A command whose definition declares a - * `force` flag is destructive (db:reset, db:rollback...), and also needs mcp.console.allow_destructive - * (MCP_ALLOW_DESTRUCTIVE): the agent can pass --force itself, so the flag alone guards nothing. + * when its name or alias is in the mcp.console.allowed_commands config list. A command marked + * #[Command(destructive: true)] (db:reset, cache:clear, queue:clear...) also needs mcp.console.allow_destructive + * (MCP_ALLOW_DESTRUCTIVE). The marker is read from the command's definition, never inferred from its options: the + * agent can pass --force itself, so a --force flag alone guards nothing. */ readonly class RunConsoleCommandTool implements ToolHandlerInterface { @@ -99,8 +100,8 @@ private function refusalFor( . "mcp.console.allowed_commands config key. Allowed: $allowed"; } - if ($definition !== null && in_array('force', $definition->flags, true) && ! $this->allowDestructive) { - return "Command '$command' is destructive (it declares a --force flag) and is disabled. The server " + if ($definition !== null && $definition->destructive && ! $this->allowDestructive) { + return "Command '$command' is destructive (#[Command(destructive: true)]) and is disabled. The server " . 'operator must set the mcp.console.allow_destructive config key to true (MCP_ALLOW_DESTRUCTIVE=true) ' . 'to run it; passing --force cannot enable it.'; } diff --git a/packages/mcp/tests/Feature/ContainerWiringTest.php b/packages/mcp/tests/Feature/ContainerWiringTest.php index 7ecf6f4c9..f432325c9 100644 --- a/packages/mcp/tests/Feature/ContainerWiringTest.php +++ b/packages/mcp/tests/Feature/ContainerWiringTest.php @@ -162,7 +162,14 @@ function wiredRunConsoleCommandTool( $registry = new CommandRegistry(); $registry->register(new CommandDefinition('Marko\\Mcp\\Tests\\MissingRouteListCommand', 'route:list')); - $registry->register(new CommandDefinition('Marko\\Mcp\\Tests\\MissingResetCommand', 'db:reset', flags: ['force'])); + $registry->register( + new CommandDefinition( + 'Marko\\Mcp\\Tests\\MissingResetCommand', + 'db:reset', + flags: ['force'], + destructive: true, + ), + ); $container->instance(CommandRegistry::class, $registry); $tools = new ReflectionClass(McpServer::class) @@ -181,7 +188,7 @@ function wiredRunConsoleCommandTool( ->and($result['content'][0]['text'])->toContain("Command 'db:reset' is not allowed"); }); -it('refuses wired force-flag commands unless mcp.console.allow_destructive', function (): void { +it('refuses wired destructive commands unless mcp.console.allow_destructive', function (): void { $refused = wiredRunConsoleCommandTool([ 'mcp.console.allowed_commands' => ['db:reset'], 'mcp.console.allow_destructive' => false, diff --git a/packages/mcp/tests/Unit/Tools/Runtime/RunConsoleCommandToolTest.php b/packages/mcp/tests/Unit/Tools/Runtime/RunConsoleCommandToolTest.php index a4e103527..87008b56c 100644 --- a/packages/mcp/tests/Unit/Tools/Runtime/RunConsoleCommandToolTest.php +++ b/packages/mcp/tests/Unit/Tools/Runtime/RunConsoleCommandToolTest.php @@ -108,9 +108,9 @@ function okResult(): array ->and($dispatcher->dispatched->getArrayCopy())->toBe(['status']); }); -it('refuses an allowlisted command that declares a force flag even when the agent passes --force', function (): void { +it('refuses an allowlisted destructive command even when the agent passes --force', function (): void { $dispatcher = makeDispatcher(okResult(), [ - new CommandDefinition('ResetCommand', 'db:reset', flags: ['force']), + new CommandDefinition('ResetCommand', 'db:reset', flags: ['force'], destructive: true), ]); $result = RunConsoleCommandTool::definition($dispatcher, allowedCommands: ['db:reset']) @@ -122,9 +122,48 @@ function okResult(): array ->and($dispatcher->dispatched->getArrayCopy())->toBe([]); }); -it('runs an allowlisted force-flag command when allow_destructive is enabled', function (): void { +it('refuses an allowlisted destructive command that has no force flag', function (): void { $dispatcher = makeDispatcher(okResult(), [ - new CommandDefinition('ResetCommand', 'db:reset', flags: ['force']), + new CommandDefinition('CacheClearCommand', 'cache:clear', destructive: true), + ]); + + $result = RunConsoleCommandTool::definition($dispatcher, allowedCommands: ['cache:clear']) + ->handler->handle(['command' => 'cache:clear']); + + expect($result['isError'] ?? false)->toBeTrue() + ->and($result['content'][0]['text'])->toContain("Command 'cache:clear' is destructive") + ->and($result['content'][0]['text'])->toContain('mcp.console.allow_destructive') + ->and($dispatcher->dispatched->getArrayCopy())->toBe([]); +}); + +it('refuses a destructive command reached through an allowlisted alias', function (): void { + $dispatcher = makeDispatcher(okResult(), [ + new CommandDefinition('CacheClearCommand', 'cache:clear', aliases: ['cc'], destructive: true), + ]); + + $result = RunConsoleCommandTool::definition($dispatcher, allowedCommands: ['cc']) + ->handler->handle(['command' => 'cc']); + + expect($result['isError'] ?? false)->toBeTrue() + ->and($result['content'][0]['text'])->toContain('destructive') + ->and($dispatcher->dispatched->getArrayCopy())->toBe([]); +}); + +it('runs an allowlisted command that declares a force flag but is not marked destructive', function (): void { + $dispatcher = makeDispatcher(okResult(), [ + new CommandDefinition('ScaffoldCommand', 'app:scaffold', flags: ['force']), + ]); + + $result = RunConsoleCommandTool::definition($dispatcher, allowedCommands: ['app:scaffold']) + ->handler->handle(['command' => 'app:scaffold', 'args' => ['--force']]); + + expect($result['isError'] ?? false)->toBeFalse() + ->and($dispatcher->dispatched->getArrayCopy())->toBe(['app:scaffold']); +}); + +it('runs an allowlisted destructive command when allow_destructive is enabled', function (): void { + $dispatcher = makeDispatcher(okResult(), [ + new CommandDefinition('ResetCommand', 'db:reset', flags: ['force'], destructive: true), ]); $result = RunConsoleCommandTool::definition( @@ -139,7 +178,7 @@ function okResult(): array it('does not run a destructive command just because allow_destructive is enabled', function (): void { $dispatcher = makeDispatcher(okResult(), [ - new CommandDefinition('ResetCommand', 'db:reset', flags: ['force']), + new CommandDefinition('ResetCommand', 'db:reset', flags: ['force'], destructive: true), ]); $result = RunConsoleCommandTool::definition( diff --git a/packages/page-cache/src/Command/ClearCommand.php b/packages/page-cache/src/Command/ClearCommand.php index 109fe086a..67e3fc72c 100644 --- a/packages/page-cache/src/Command/ClearCommand.php +++ b/packages/page-cache/src/Command/ClearCommand.php @@ -11,7 +11,7 @@ use Marko\PageCache\Contracts\PageCacheInterface; /** @noinspection PhpUnused */ -#[Command(name: 'page-cache:clear', description: 'Clear the full-page cache')] +#[Command(name: 'page-cache:clear', description: 'Clear the full-page cache', destructive: true)] readonly class ClearCommand implements CommandInterface { public function __construct( diff --git a/packages/page-cache/src/Command/PurgeCommand.php b/packages/page-cache/src/Command/PurgeCommand.php index c4104bd95..151a5ad84 100644 --- a/packages/page-cache/src/Command/PurgeCommand.php +++ b/packages/page-cache/src/Command/PurgeCommand.php @@ -11,7 +11,7 @@ use Marko\PageCache\Contracts\PageCacheInterface; /** @noinspection PhpUnused */ -#[Command(name: 'page-cache:purge', description: 'Purge a page cache entry by URL or tag')] +#[Command(name: 'page-cache:purge', description: 'Purge a page cache entry by URL or tag', destructive: true)] readonly class PurgeCommand implements CommandInterface { public function __construct( diff --git a/packages/queue/src/Command/QueueClearCommand.php b/packages/queue/src/Command/QueueClearCommand.php index 0a519bd78..294730887 100644 --- a/packages/queue/src/Command/QueueClearCommand.php +++ b/packages/queue/src/Command/QueueClearCommand.php @@ -11,7 +11,7 @@ use Marko\Queue\QueueInterface; /** @noinspection PhpUnused */ -#[Command(name: 'queue:clear', description: 'Clear all jobs from queue')] +#[Command(name: 'queue:clear', description: 'Clear all jobs from queue', destructive: true)] readonly class QueueClearCommand implements CommandInterface { public function __construct( diff --git a/packages/session/src/Command/GarbageCollectCommand.php b/packages/session/src/Command/GarbageCollectCommand.php index e9e067bff..f814e089f 100644 --- a/packages/session/src/Command/GarbageCollectCommand.php +++ b/packages/session/src/Command/GarbageCollectCommand.php @@ -12,7 +12,7 @@ use Marko\Session\Contracts\SessionHandlerInterface; /** @noinspection PhpUnused */ -#[Command(name: 'session:gc', description: 'Run session garbage collection')] +#[Command(name: 'session:gc', description: 'Run session garbage collection', destructive: true)] readonly class GarbageCollectCommand implements CommandInterface { public function __construct( diff --git a/tests/DestructiveCommandMarkerTest.php b/tests/DestructiveCommandMarkerTest.php new file mode 100644 index 000000000..b00308eb1 --- /dev/null +++ b/tests/DestructiveCommandMarkerTest.php @@ -0,0 +1,116 @@ + + */ +function shippedCommands(): array +{ + $commands = []; + $iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator(dirname(__DIR__) . '/packages', FilesystemIterator::SKIP_DOTS), + ); + + foreach ($iterator as $file) { + $path = $file->getPathname(); + + if (!str_ends_with($path, '.php') || !preg_match('#/packages/[^/]+/src/#', $path)) { + continue; + } + + $source = (string) file_get_contents($path); + + if (!str_contains($source, '#[Command(') + || !preg_match('/^namespace\s+([^;]+);/m', $source, $namespace) + || !preg_match('/^(?:readonly\s+|abstract\s+)*class\s+(\w+)/m', $source, $class) + ) { + continue; + } + + /** @var class-string $className */ + $className = $namespace[1] . '\\' . $class[1]; + $attributes = (new ReflectionClass($className))->getAttributes(Command::class); + + if ($attributes === []) { + continue; + } + + $attribute = $attributes[0]->newInstance(); + $commands[$attribute->name] = ['class' => $className, 'attribute' => $attribute]; + } + + ksort($commands); + + return $commands; +} + +it('finds the shipped commands', function (): void { + expect(shippedCommands())->toHaveKeys(['cache:clear', 'db:reset', 'list', 'queue:clear']); +}); + +it('marks every command named like a state-wiping command as destructive', function (): void { + $unmarked = []; + + foreach (shippedCommands() as $name => $command) { + $wipesState = array_any( + DESTRUCTIVE_COMMAND_SUFFIXES, + fn (string $suffix): bool => str_ends_with($name, $suffix), + ); + + if ($wipesState && !$command['attribute']->destructive) { + $unmarked[] = "$name ({$command['class']})"; + } + } + + expect($unmarked)->toBe([], 'Add destructive: true to the #[Command] attribute of: ' . implode(', ', $unmarked)); +}); + +it('marks every command guarded by DestructiveCommandGuard as destructive', function (): void { + $unmarked = []; + + foreach (shippedCommands() as $name => $command) { + $constructor = new ReflectionClass($command['class'])->getConstructor(); + $guarded = array_any( + $constructor?->getParameters() ?? [], + fn (ReflectionParameter $parameter): bool => $parameter->getType() instanceof ReflectionNamedType + && $parameter->getType()->getName() === DestructiveCommandGuard::class, + ); + + if ($guarded && !$command['attribute']->destructive) { + $unmarked[] = "$name ({$command['class']})"; + } + } + + expect($unmarked)->toBe([], 'Add destructive: true to the #[Command] attribute of: ' . implode(', ', $unmarked)); +}); + +it('keeps read-only commands unmarked', function (): void { + $commands = shippedCommands(); + + foreach (['list', 'module:list', 'cache:status', 'db:status', 'queue:status', 'route:list'] as $name) { + expect($commands[$name]['attribute']->destructive)->toBeFalse(); + } +});