From abeb4b0eedd38ac0c29853b7bd2b996810bfd063 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sun, 18 Oct 2026 21:47:26 +0200 Subject: [PATCH] refactor: AbilityRegistrar nimmt AbilityGuard-Liste und ToolRefusal statt DiviToolGate/DiviLocked; DiviToolGate implementiert AbilityGuard, DiviLocked ToolRefusal; Test mit Fake-Guard und Fake-Refusal ohne Divi Co-Authored-By: Claude Sonnet 5.5 --- src/Abilities/AbilityGuard.php | 35 +++++ src/Abilities/AbilityRegistrar.php | 57 +++++-- src/Abilities/ToolRefusal.php | 20 +++ src/Integrations/Divi/Release/DiviLocked.php | 3 +- .../Divi/Release/DiviToolGate.php | 7 +- src/Plugin.php | 6 +- tests/Unit/Abilities/AbilityRegistrarTest.php | 147 +++++++++++++++++- .../Divi/Release/DiviToolGateTest.php | 8 +- 8 files changed, 257 insertions(+), 26 deletions(-) create mode 100644 src/Abilities/AbilityGuard.php create mode 100644 src/Abilities/ToolRefusal.php diff --git a/src/Abilities/AbilityGuard.php b/src/Abilities/AbilityGuard.php new file mode 100644 index 0000000..68988a1 --- /dev/null +++ b/src/Abilities/AbilityGuard.php @@ -0,0 +1,35 @@ + $input + */ + public function refusal(Ability $ability, array $input): ?WP_Error; + + /** The result of a finished call, possibly with added warnings. */ + public function annotate(Ability $ability, mixed $result): mixed; + + /** + * The output schema, possibly extended (e.g. by a warnings property). + * + * @param array|null $schema + * @return array|null + */ + public function outputSchema(Ability $ability, ?array $schema): ?array; +} diff --git a/src/Abilities/AbilityRegistrar.php b/src/Abilities/AbilityRegistrar.php index 37e7ca8..35a4626 100644 --- a/src/Abilities/AbilityRegistrar.php +++ b/src/Abilities/AbilityRegistrar.php @@ -5,8 +5,6 @@ declare(strict_types=1); namespace LIT\MCP\Abilities; use LIT\MCP\Access; -use LIT\MCP\Integrations\Divi\Release\DiviLocked; -use LIT\MCP\Integrations\Divi\Release\DiviToolGate; use LIT\MCP\Integrations\IntegrationRegistry; use LIT\MCP\Log; use LIT\MCP\Server\RequestContext; @@ -17,16 +15,20 @@ defined('ABSPATH') || exit; /** * Registers all abilities of available integrations with the Abilities API and wraps them - * with the role gate, the Divi release lock (DiviToolGate) and neutral error handling. + * with the role gate, the AbilityGuards it is given (e.g. the Divi release lock) and neutral error handling. + * It knows no integration: guards and ToolRefusal exceptions are the only seams. */ final class AbilityRegistrar { public const CATEGORY = 'lit-mcp'; + /** + * @param list $guards Applied in order: the first refusal wins, annotate() and outputSchema() chain. + */ public function __construct( private IntegrationRegistry $registry, private ?ExposurePolicy $policy = null, - private ?DiviToolGate $divi = null + private array $guards = [] ) { } @@ -55,7 +57,7 @@ final class AbilityRegistrar /** @return array */ public function argsFor(Ability $ability): array { - $divi = $this->divi ?? new DiviToolGate($this->registry->groupMap(...)); + $guards = $this->guards; $args = [ 'label' => $ability->label(), 'description' => $ability->description(), @@ -77,14 +79,22 @@ final class AbilityRegistrar return Errors::internal(); } }, - 'execute_callback' => static function (mixed $input = null) use ($ability, $divi): mixed { - $refusal = $divi->refusal($ability); - if ($refusal !== null) { - return $refusal; + 'execute_callback' => static function (mixed $input = null) use ($ability, $guards): mixed { + $input = is_array($input) ? $input : []; + foreach ($guards as $guard) { + $refusal = $guard->refusal($ability, $input); + if ($refusal !== null) { + return $refusal; + } } try { - return $divi->annotate($ability, $ability->execute(is_array($input) ? $input : [])); + $result = $ability->execute($input); + foreach ($guards as $guard) { + $result = $guard->annotate($ability, $result); + } + + return $result; } catch (Throwable $e) { Log::failure($ability->name(), $e); @@ -104,7 +114,10 @@ final class AbilityRegistrar ], ]; - $output = $divi->outputSchema($ability, $ability->outputSchema()); + $output = $ability->outputSchema(); + foreach ($guards as $guard) { + $output = $guard->outputSchema($ability, $output); + } if ($output !== null) { $args['output_schema'] = $output; } @@ -113,19 +126,31 @@ final class AbilityRegistrar } /** - * A partial write stays partial_write; otherwise a Divi release lock anywhere in the cause chain is its refusal - * (spec §5.5); a rolled-back write is write_failed; anything else internal_error. + * A partial write stays partial_write; otherwise the first ToolRefusal anywhere in the cause chain is the answer + * (e.g. the Divi release lock, spec §5.5); a rolled-back write is write_failed; anything else internal_error. */ private static function errorFor(Throwable $e): WP_Error { if ($e instanceof WriteFailed && ! $e->rolledBack) { return $e->toError(); } - $locked = DiviLocked::in($e); - if ($locked !== null) { - return $locked->toError(); + $refusal = self::refusalIn($e); + if ($refusal !== null) { + return $refusal->toError(); } return $e instanceof WriteFailed ? $e->toError() : Errors::internal(); } + + /** The first ToolRefusal in $e's chain of previous exceptions (callers wrap failures in WriteFailed). */ + private static function refusalIn(Throwable $e): ?ToolRefusal + { + for ($cause = $e; $cause !== null; $cause = $cause->getPrevious()) { + if ($cause instanceof ToolRefusal) { + return $cause; + } + } + + return null; + } } diff --git a/src/Abilities/ToolRefusal.php b/src/Abilities/ToolRefusal.php new file mode 100644 index 0000000..d13d1b1 --- /dev/null +++ b/src/Abilities/ToolRefusal.php @@ -0,0 +1,20 @@ +groups)()[$ability->name()] ?? '', self::GROUPS, true); } - public function refusal(Ability $ability): ?WP_Error + public function refusal(Ability $ability, array $input): ?WP_Error { if (! $this->covers($ability) || self::readsOnly($ability)) { return null; diff --git a/src/Plugin.php b/src/Plugin.php index 06724c7..e82bea8 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -144,7 +144,11 @@ final class Plugin DiviCheckCommand::register($diviCheck); $policy = ExposurePolicy::fromOption()->withGroups($registry->groupMap(...)); - (new AbilityRegistrar($registry, $policy, new DiviToolGate($registry->groupMap(...), $changeLog)))->register(); + (new AbilityRegistrar( + $registry, + $policy, + [new DiviToolGate($registry->groupMap(...), $changeLog)] + ))->register(); (new ServerRegistrar( $registry, new InstructionsBuilder($registry, static fn (): array => $schema->postTypeNames(), $policy), diff --git a/tests/Unit/Abilities/AbilityRegistrarTest.php b/tests/Unit/Abilities/AbilityRegistrarTest.php index 1bc6191..467e9d7 100644 --- a/tests/Unit/Abilities/AbilityRegistrarTest.php +++ b/tests/Unit/Abilities/AbilityRegistrarTest.php @@ -6,8 +6,11 @@ namespace LIT\MCP\Tests\Unit\Abilities; use Brain\Monkey\Functions; use LIT\MCP\Abilities\Ability; +use LIT\MCP\Abilities\AbilityGuard; use LIT\MCP\Abilities\AbilityRegistrar; +use LIT\MCP\Abilities\Errors; use LIT\MCP\Abilities\ExposurePolicy; +use LIT\MCP\Abilities\ToolRefusal; use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\Integrations\Divi\Release\DiviLocked; use LIT\MCP\Integrations\Divi\Release\DiviToolGate; @@ -20,6 +23,7 @@ use LIT\MCP\Server\RequestContext; use LIT\MCP\Tests\Unit\TestCase; use Mockery; use RuntimeException; +use Throwable; use WP_Error; final class AbilityRegistrarTest extends TestCase @@ -56,7 +60,7 @@ final class AbilityRegistrarTest extends TestCase $integration->allows('abilities')->andReturn([$ability]); $registry = new IntegrationRegistry($integration); - return new AbilityRegistrar($registry, null, new DiviToolGate($registry->groupMap(...))); + return new AbilityRegistrar($registry, null, [new DiviToolGate($registry->groupMap(...))]); } private function ability(bool $readonly = true): Mockery\MockInterface @@ -247,6 +251,147 @@ final class AbilityRegistrarTest extends TestCase self::assertToolError($this->executeLogged($ability), 'partial_write'); } + /** @param list $guards */ + private function guardedRegistrar(Ability $ability, array $guards): AbilityRegistrar + { + $integration = Mockery::mock(Integration::class); + $integration->allows('id')->andReturn('core'); + $integration->allows('isAvailable')->andReturn(true); + $integration->allows('abilities')->andReturn([$ability]); + + return new AbilityRegistrar(new IntegrationRegistry($integration), null, $guards); + } + + /** + * A guard without any Divi knowledge: refuses with $refusal (or lets through), appends $tag to the result's + * "trail" and to the schema's "trail" property; $seen collects the inputs it was asked about. + */ + private function guard(string $tag, ?WP_Error $refusal = null, array &$seen = []): AbilityGuard + { + return new class ($tag, $refusal, $seen) implements AbilityGuard { + public function __construct(private string $tag, private ?WP_Error $refusal, private array &$seen) + { + } + + public function refusal(Ability $ability, array $input): ?WP_Error + { + $this->seen[] = $input; + + return $this->refusal; + } + + public function annotate(Ability $ability, mixed $result): mixed + { + $result['trail'] = [...($result['trail'] ?? []), $this->tag]; + + return $result; + } + + public function outputSchema(Ability $ability, ?array $schema): ?array + { + $schema['properties']['trail'][] = $this->tag; + + return $schema; + } + }; + } + + private function refusalException(WP_Error $error, ?Throwable $previous = null): RuntimeException + { + return new class ($error, $previous) extends RuntimeException implements ToolRefusal { + public function __construct(private WP_Error $error, ?Throwable $previous) + { + parent::__construct('refused', 0, $previous); + } + + public function toError(): WP_Error + { + return $this->error; + } + }; + } + + public function testWithoutGuardsNothingIsCheckedOrAdded(): void + { + $ability = $this->ability(false); + $ability->expects('execute')->with(['a' => 1])->andReturn(['ok' => true]); + + $args = $this->registrarFor($ability)->argsFor($ability); + + self::assertSame(['ok' => true], ($args['execute_callback'])(['a' => 1])); + self::assertSame(['type' => 'object'], $args['output_schema']); + } + + public function testAGuardRefusesBeforeTheAbilityRuns(): void + { + $seen = []; + $ability = $this->ability(false); + $ability->expects('execute')->never(); + $registrar = $this->guardedRegistrar($ability, [$this->guard('a', Errors::forbidden('Nein.'), $seen)]); + + $result = ($registrar->argsFor($ability)['execute_callback'])(['x' => 1]); + + self::assertToolError($result, 'forbidden'); + self::assertSame([['x' => 1]], $seen); + } + + public function testTheFirstRefusalWinsAndLaterGuardsAreNotAsked(): void + { + $seenSecond = []; + $ability = $this->ability(false); + $ability->expects('execute')->never(); + $registrar = $this->guardedRegistrar($ability, [ + $this->guard('a'), + $this->guard('b', Errors::notFound('Weg.')), + $this->guard('c', Errors::forbidden('Nein.'), $seenSecond), + ]); + + self::assertToolError(($registrar->argsFor($ability)['execute_callback'])([]), 'not_found'); + self::assertSame([], $seenSecond); + } + + public function testGuardsAnnotateTheResultAndExtendTheSchemaInOrder(): void + { + $ability = $this->ability(false); + $ability->allows('execute')->andReturn(['ok' => true]); + $registrar = $this->guardedRegistrar($ability, [$this->guard('a'), $this->guard('b')]); + + $args = $registrar->argsFor($ability); + + self::assertSame(['ok' => true, 'trail' => ['a', 'b']], ($args['execute_callback'])([])); + self::assertSame(['a', 'b'], $args['output_schema']['properties']['trail']); + } + + public function testAToolRefusalThrownByAnAbilityIsItsErrorWithoutDivi(): void + { + $ability = $this->ability(); + $ability->allows('execute')->andThrow($this->refusalException(Errors::forbidden('Gesperrt.'))); + + self::assertToolError($this->executeLogged($ability), 'forbidden'); + } + + public function testAToolRefusalInsideARolledBackWriteIsItsErrorButPartialStaysPartial(): void + { + $refusal = $this->refusalException(Errors::notFound('Weg.')); + $rolledBack = $this->ability(); + $rolledBack->allows('execute')->andThrow(WriteFailed::rolledBack('Nicht gespeichert.', $refusal)); + $partial = $this->ability(); + $partial->allows('execute')->andThrow(WriteFailed::partial('Teilweise gespeichert.', $refusal)); + + self::assertToolError($this->executeLogged($rolledBack), 'not_found'); + self::assertToolError($this->executeLogged($partial), 'partial_write'); + } + + public function testTheFirstRefusalInTheCauseChainWins(): void + { + $inner = $this->refusalException(Errors::notFound('Innen.')); + $outer = $this->refusalException(Errors::forbidden('Außen.'), $inner); + $ability = $this->ability(); + $ability->allows('execute')->andThrow(WriteFailed::rolledBack('Nicht gespeichert.', $outer)); + + self::assertToolError($this->executeLogged($ability), 'forbidden'); + } + /** * Runs the execute callback with error_log() redirected (a PHP internal, not stubbable – like the tests above). */ diff --git a/tests/Unit/Integrations/Divi/Release/DiviToolGateTest.php b/tests/Unit/Integrations/Divi/Release/DiviToolGateTest.php index 5274a79..f8ca2e3 100644 --- a/tests/Unit/Integrations/Divi/Release/DiviToolGateTest.php +++ b/tests/Unit/Integrations/Divi/Release/DiviToolGateTest.php @@ -47,7 +47,7 @@ final class DiviToolGateTest extends TestCase { $gate = $this->gate(FakeChangeLog::forbidden()); - self::assertNull($gate->refusal($this->ability('lit/divi-insert', false))); + self::assertNull($gate->refusal($this->ability('lit/divi-insert', false), [])); self::assertSame(['ok' => true], $gate->annotate($this->ability('lit/divi-outline', true), ['ok' => true])); } @@ -58,10 +58,10 @@ final class DiviToolGateTest extends TestCase $gate = $this->gate($log); foreach (['lit/divi-insert', 'lit/library-save', 'lit/tb-create-template'] as $name) { - self::assertToolError($gate->refusal($this->ability($name, false)), 'divi_untested', $name); + self::assertToolError($gate->refusal($this->ability($name, false), []), 'divi_untested', $name); } - self::assertNull($gate->refusal($this->ability('lit/content-create', false)), 'not a Divi tool (adapter lock)'); - self::assertNull($gate->refusal($this->ability('lit/divi-outline', true)), 'reading stays allowed'); + self::assertNull($gate->refusal($this->ability('lit/content-create', false), []), 'not a Divi tool (adapter lock)'); + self::assertNull($gate->refusal($this->ability('lit/divi-outline', true), []), 'reading stays allowed'); self::assertSame(['refused', 'refused', 'refused'], array_column($log->records, 'result')); self::assertSame('Abgelehnt: Divi 5.3.0 ist nicht freigegeben.', $log->records[0]['summary']); }