fix: settings compare against the sanitized value, fail via WriteFailed

- isStored() compares the stored option with sanitize_option() of the new value
  (core esc_html()s blogname/blogdescription): setting "Müller & Söhne" again
  no longer looks like a failed write; lit/wp-settings-get and previous return
  those two decoded, as the user would type them.
- The change log runs as FollowUp (warning on failure); a failed write throws
  WriteFailed (write_failed / partial_write with keys), partial writes are
  logged with result partial.
- permalink_structure needs a real %tag% (WordPress' own /%[^/%]+%/ rule).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7

src/Integrations/WordPress/Settings/SettingsWhitelist.php

@@ -39,9 +39,17 @@ final class SettingsWhitelist
 
     private const INTEGER_KEYS = ['page_on_front', 'page_for_posts', 'posts_per_page', 'site_icon'];
 
+    /** Stored esc_html()'d by sanitize_option(); read back as the user would type them. */
+    private const HTML_ESCAPED_KEYS = ['blogname', 'blogdescription'];
+
+    /** The value as a client should see and send it (not WordPress' storage form). */
     public function current(string $key): mixed
     {
         $value = get_option($key);
+        if (in_array($key, self::HTML_ESCAPED_KEYS, true)) {
+            return wp_specialchars_decode((string) $value, ENT_QUOTES);
+        }
+
         if ($key === 'WPLANG') {
             return $value === '' || $value === false ? 'en_US' : (string) $value;
         }
@@ -201,10 +209,11 @@ final class SettingsWhitelist
             return null;
         }
 
+        // WordPress' own rule for a structure tag (wp-admin/options-permalink.php): /%[^\/%]+%/.
         if (
             is_string($value)
             && str_starts_with($value, '/')
-            && str_contains($value, '%')
+            && preg_match('/%[^\/%]+%/', $value) === 1
             && mb_strlen($value) <= 200
             && preg_match('/\s/', $value) !== 1
         ) {

src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php

@@ -6,8 +6,10 @@ namespace LIT\MCP\Integrations\WordPress\Settings;
 
 use LIT\MCP\Abilities\Ability;
 use LIT\MCP\Abilities\Errors;
+use LIT\MCP\Abilities\FollowUp;
 use LIT\MCP\Abilities\JsonMap;
 use LIT\MCP\Abilities\ToolResult;
+use LIT\MCP\Abilities\WriteFailed;
 use LIT\MCP\ChangeLog\ChangeLog;
 use RuntimeException;
 use Throwable;
@@ -126,8 +128,10 @@ final class WpSettingsUpdateAbility implements Ability
      * previous values so the call stays all-or-nothing.
      *
      * @param array<string, mixed> $settings
+     * @return array<string, mixed>
+     * @throws WriteFailed When a write fails (after restoring what it could).
      */
-    private function write(array $settings): array|WP_Error
+    private function write(array $settings): array
     {
         $previous = [];
         foreach ($settings as $key => $value) {
@@ -146,7 +150,7 @@ final class WpSettingsUpdateAbility implements Ability
                 $changed[] = $key;
             }
         } catch (Throwable $e) {
-            return $this->fail($changed, $previous, $e);
+            $this->fail($changed, $previous, $e);
         }
 
         if ($changed === []) {
@@ -155,15 +159,26 @@ final class WpSettingsUpdateAbility implements Ability
                 ->toArray();
         }
 
+        // The settings are saved – a failure of the change log from here on is a warning only.
         $summary = 'Einstellungen geändert: ' . implode(', ', $changed);
-        $logId = $this->log->record($this->name(), 'option', implode(',', $changed), $summary);
+        $warnings = [];
+        $logId = FollowUp::run(
+            fn () => $this->log->record($this->name(), 'option', implode(',', $changed), $summary),
+            $this->name() . ' change log',
+            'Änderungsprotokoll konnte nicht geschrieben werden; die Einstellungen sind gespeichert.',
+            $warnings
+        );
 
         $result = ToolResult::ok($summary . '.');
         foreach ($changed as $key) {
             $result->changed($key, 'option', 'updated', SettingsWhitelist::SETTINGS[$key][2]);
         }
 
-        return $result->logId($logId)->with('previous', JsonMap::of($previous))->toArray();
+        return $result
+            ->logId(is_int($logId) ? $logId : null)
+            ->warnings($warnings)
+            ->with('previous', JsonMap::of($previous))
+            ->toArray();
     }
 
     private function writeOne(string $key, mixed $value): void
@@ -185,26 +200,32 @@ final class WpSettingsUpdateAbility implements Ability
         }
     }
 
-    /** Whether option $key already holds $normalized, compared like WordPress compares scalars. */
+    /**
+     * Whether option $key already holds $normalized in the form WordPress stores it:
+     * update_option() runs sanitize_option() first (blogname/blogdescription are esc_html()'d),
+     * so the stored value is compared with the sanitized one, like WordPress compares scalars.
+     */
     private function isStored(string $key, mixed $normalized): bool
     {
         $stored = get_option($key);
-        if (is_scalar($stored) && is_scalar($normalized)) {
+        $expected = sanitize_option($key, $normalized);
-            return (string) $stored === (string) $normalized;
+        if (is_scalar($stored) && is_scalar($expected)) {
+            return (string) $stored === (string) $expected;
         }
 
-        return $stored == $normalized; // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual
+        return $stored == $expected; // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual
     }
 
     /**
-     * A write failed; restores the keys already written to their previous values and
+     * A write failed; restores the keys already written to their previous values and throws
-     * reports write_failed (fully restored) or partial_write (some keys could not be
+     * WriteFailed: write_failed (fully restored) or partial_write (some keys could not be
-     * restored – the client is told which, so it can check and correct them).
+     * restored – the client is told which, so it can check and correct them; logged as partial).
      *
      * @param list<string>          $changed  Keys written so far, in write order.
      * @param array<string, mixed>  $previous Their values before this call.
+     * @throws WriteFailed Always.
      */
-    private function fail(array $changed, array $previous, Throwable $cause): WP_Error
+    private function fail(array $changed, array $previous, Throwable $cause): never
     {
         $stuck = [];
         foreach (array_reverse($changed) as $key) {
@@ -215,23 +236,27 @@ final class WpSettingsUpdateAbility implements Ability
             }
         }
 
-        error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
+        $reason = 'Eine Einstellung konnte nicht gespeichert werden.';
-            sprintf('[wp-lit-mcp] wp-settings-update failed: %s', $cause)
-        );
-
         if ($stuck === []) {
-            return Errors::writeFailed('Eine Einstellung konnte nicht gespeichert werden.');
+            throw new WriteFailed($reason, true, null, null, $cause);
         }
 
-        return Errors::partialWrite(
+        $this->log->recordPartial(
-            'Eine Einstellung konnte nicht gespeichert werden.',
+            $this->name(),
+            'option',
+            implode(',', $stuck),
+            'Einstellungen nur teilweise zurückgenommen: ' . implode(', ', $stuck)
+        );
+        throw new WriteFailed(
+            $reason,
+            false,
             null,
             null,
-            sprintf(
+            $cause,
-                'Möglicherweise auf dem neuen, nicht vollständigen Stand: %s.',
+            $cause->getMessage() . '; not restored: ' . implode(', ', $stuck),
-                implode(', ', $stuck)
+            sprintf('Möglicherweise auf dem neuen, nicht vollständigen Stand: %s.', implode(', ', $stuck)),
-            ),
+            'Aktuelle Werte über lit/wp-settings-get prüfen und ggf. erneut auf die gewünschten Werte setzen.',
-            'Aktuelle Werte über lit-wp-settings-get prüfen und ggf. erneut auf die gewünschten Werte setzen.'
+            ['keys' => $stuck]
         );
     }
 

tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php

@@ -64,6 +64,9 @@ final class SettingsWhitelistTest extends TestCase
         self::assertNull($whitelist->validate('permalink_structure', '/%postname%/', []));
         self::assertNull($whitelist->validate('permalink_structure', '', []));
         self::assertNotNull($whitelist->validate('permalink_structure', 'postname', []));
+        self::assertNull($whitelist->validate('permalink_structure', '/blog/%year%/%postname%/', []));
+        self::assertNotNull($whitelist->validate('permalink_structure', '/%%/', []));
+        self::assertNotNull($whitelist->validate('permalink_structure', '/%/', []));
         self::assertNotNull($whitelist->validate('posts_per_page', 0, []));
         self::assertNotNull($whitelist->validate('default_comment_status', 'maybe', []));
         self::assertNull($whitelist->validate('site_icon', 33, []));

tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php

@@ -5,15 +5,99 @@ declare(strict_types=1);
 namespace LIT\MCP\Tests\Unit\Integrations\WordPress\Settings;
 
 use Brain\Monkey\Functions;
+use LIT\MCP\Abilities\WriteFailed;
 use LIT\MCP\ChangeLog\ChangeLog;
 use LIT\MCP\Integrations\WordPress\Settings\SettingsWhitelist;
 use LIT\MCP\Integrations\WordPress\Settings\WpSettingsGetAbility;
 use LIT\MCP\Integrations\WordPress\Settings\WpSettingsUpdateAbility;
 use LIT\MCP\Tests\Unit\TestCase;
 use Mockery;
+use RuntimeException;
+use WP_Error;
 
 final class WpSettingsAbilitiesTest extends TestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // Like core: blogname/blogdescription are stored esc_html()'d, the rest as given here.
+        Functions\when('sanitize_option')->alias(static fn (string $key, $value) => in_array($key, ['blogname', 'blogdescription'], true)
+            ? htmlspecialchars((string) $value, ENT_QUOTES)
+            : $value);
+        Functions\when('wp_specialchars_decode')->alias(static fn (string $text, $quotes = ENT_NOQUOTES): string => htmlspecialchars_decode($text, ENT_QUOTES));
+    }
+
+    /**
+     * A stateful option store: update_option() sanitizes like core and returns false when
+     * nothing changes.
+     *
+     * @param array<string, mixed> $options
+     */
+    private function optionStore(array &$options): void
+    {
+        Functions\when('get_option')->alias(static function (string $key) use (&$options) {
+            return $options[$key];
+        });
+        Functions\when('update_option')->alias(static function (string $key, $value) use (&$options): bool {
+            $value = in_array($key, ['blogname', 'blogdescription'], true) ? htmlspecialchars((string) $value, ENT_QUOTES) : $value;
+            if ($options[$key] === $value) {
+                return false;
+            }
+            $options[$key] = $value;
+
+            return true;
+        });
+    }
+
+    public function testTextWithAnAmpersandCanBeSetTwiceAlongsideAnotherKey(): void
+    {
+        $options = ['blogname' => 'Alt', 'blogdescription' => 'Praxis'];
+        $this->optionStore($options);
+        $log = Mockery::mock(ChangeLog::class);
+        $log->allows('record')->andReturn(40);
+        $ability = new WpSettingsUpdateAbility(new SettingsWhitelist(), $log);
+        $input = ['settings' => ['blogname' => 'Müller & Söhne', 'blogdescription' => 'Zahnärzte']];
+
+        $first = $ability->execute($input);
+        $second = $ability->execute($input);
+
+        self::assertSame('Müller &amp; Söhne', $options['blogname']);
+        self::assertSame(['blogname', 'blogdescription'], array_column($first['changed'], 'id'));
+        self::assertTrue($second['ok']);
+        self::assertSame([], $second['changed']);
+        self::assertSame(['blogname' => 'Müller & Söhne', 'blogdescription' => 'Zahnärzte'], $second['previous']);
+    }
+
+    public function testGetReturnsTextAsTheUserWouldTypeIt(): void
+    {
+        Functions\when('get_option')->alias(static fn (string $key) => $key === 'blogname' ? 'Müller &amp; Söhne &quot;A&quot;' : '1');
+
+        $settings = (new WpSettingsGetAbility(new SettingsWhitelist()))->execute(['group' => 'general'])['settings'];
+
+        self::assertSame('Müller & Söhne "A"', $settings[0]['value']);
+    }
+
+    public function testAFailingChangeLogIsOnlyAWarning(): void
+    {
+        $options = ['blogname' => 'Alt'];
+        $this->optionStore($options);
+        $log = Mockery::mock(ChangeLog::class);
+        $log->expects('record')->andThrow(new RuntimeException('log kaputt'));
+        $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log');
+        $previous = ini_set('error_log', $file);
+
+        try {
+            $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => ['blogname' => 'Neu']]);
+        } finally {
+            ini_set('error_log', (string) $previous);
+            unlink($file);
+        }
+
+        self::assertTrue($result['ok']);
+        self::assertNull($result['log_id']);
+        self::assertCount(1, $result['warnings']);
+    }
+
     protected function tearDown(): void
     {
         unset($GLOBALS['wp_rewrite']);
@@ -129,20 +213,14 @@ final class WpSettingsAbilitiesTest extends TestCase
         Functions\expect('update_option')->once()->with('blogname', 'Alt')->andReturn(true);
         $log = Mockery::mock(ChangeLog::class);
         $log->shouldNotReceive('record');
-        $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log');
+        $log->shouldNotReceive('recordPartial');
-        $previous = ini_set('error_log', $file);
 
-        try {
+        $error = $this->failure(new WpSettingsUpdateAbility(new SettingsWhitelist(), $log), [
-            $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => [
+            'blogname' => 'Neu',
-                'blogname' => 'Neu',
+            'blogdescription' => 'Auch neu',
-                'blogdescription' => 'Auch neu',
+        ]);
-            ]]);
-        } finally {
-            ini_set('error_log', (string) $previous);
-            unlink($file);
-        }
 
-        self::assertSame('lit_write_failed', $result->get_error_code());
+        self::assertSame('lit_write_failed', $error->get_error_code());
     }
 
     public function testReportsPartialWriteWhenRestoringAlsoFails(): void
@@ -166,21 +244,28 @@ final class WpSettingsAbilitiesTest extends TestCase
         Functions\expect('update_option')->once()->with('blogname', 'Alt')->andReturn(false);
         $log = Mockery::mock(ChangeLog::class);
         $log->shouldNotReceive('record');
-        $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log');
+        $log->expects('recordPartial')->with('lit/wp-settings-update', 'option', 'blogname', Mockery::type('string'));
-        $previous = ini_set('error_log', $file);
 
+        $error = $this->failure(new WpSettingsUpdateAbility(new SettingsWhitelist(), $log), [
+            'blogname' => 'Neu',
+            'blogdescription' => 'Auch neu',
+        ]);
+
+        self::assertSame('lit_partial_write', $error->get_error_code());
+        self::assertStringContainsString('blogname', $error->get_error_message());
+        self::assertStringContainsString('lit/wp-settings-get', $error->get_error_data()['hint']);
+    }
+
+    /** @param array<string, mixed> $settings */
+    private function failure(WpSettingsUpdateAbility $ability, array $settings): WP_Error
+    {
         try {
-            $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => [
+            $ability->execute(['settings' => $settings]);
-                'blogname' => 'Neu',
+        } catch (WriteFailed $e) {
-                'blogdescription' => 'Auch neu',
+            return $e->toError();
-            ]]);
-        } finally {
-            ini_set('error_log', (string) $previous);
-            unlink($file);
         }
 
-        self::assertSame('lit_partial_write', $result->get_error_code());
+        self::fail('WriteFailed expected');
-        self::assertStringContainsString('blogname', $result->get_error_message());
     }
 
     public function testSettingsToolsNeedManageOptions(): void