fix: Task 6 review round 1 - admin label overlap, heading default level, structure deny-list, row widths (S41)

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

src/Integrations/Divi/BlockTree/AttrMap.php

@@ -22,6 +22,12 @@ final class AttrMap
         'builderVersion', 'modulePreset', 'groupPreset', 'globalModule', 'globalParent', 'localAttrs',
     ];
 
+    /** Whether two attribute paths reach the same value: equal, or one lies below the other. */
+    public static function overlaps(string $a, string $b): bool
+    {
+        return $a === $b || str_starts_with($a, $b . '.') || str_starts_with($b, $a . '.');
+    }
+
     /**
      * @param array<string, mixed> $attrs
      * @return array<string, mixed> Path => breakpoint map (or the raw value of an odd leaf).

src/Integrations/Divi/Compose/AttrWriter.php

@@ -42,6 +42,17 @@ final class AttrWriter
     /** Text-family kinds from laxest to strictest (ruling S33a); url stands apart. */
     private const STRICTNESS = ['code' => 0, 'html' => 1, 'text' => 2];
 
+    /**
+     * Structure attributes (ruling S41), refused for everyone and whatever the catalog says:
+     * the section type (regular/fullwidth/specialty) and a column's specialty columns decide
+     * which children are valid – StructureRules would be bypassed. Sections get their type
+     * through "section_type" when they are inserted.
+     */
+    private const STRUCTURE = [
+        'divi/section' => ['module.advanced.type'],
+        'divi/column' => ['module.advanced.specialtyColumns'],
+    ];
+
     private const BUILDER_ONLY = '/(^|\.)(decoration\.attributes|advanced\.html|advanced\.customCss|css)(\.|$)/';
 
     public function __construct(
@@ -65,6 +76,12 @@ final class AttrWriter
         foreach ($changes as $path => $map) {
             $path = (string) $path;
             $key = $field . '.' . $path;
+            if ($this->isStructure($module, $path)) {
+                $errors[$key] = 'Struktur des Abschnitts bzw. der Spalte; nur im Divi-Builder änderbar '
+                    . '(Abschnittsart beim Einfügen über section_type)';
+                continue;
+            }
+
             if (! $this->catalog->isKnownPath($module, $path)) {
                 $suggestions = $this->catalog->suggestions($module, $path);
                 $errors[$key] = 'unbekannter Attributpfad'
@@ -147,6 +164,17 @@ final class AttrWriter
         return $normalized;
     }
 
+    private function isStructure(string $module, string $path): bool
+    {
+        foreach (self::STRUCTURE[$module] ?? [] as $structure) {
+            if (AttrMap::overlaps($path, $structure)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * Rejects Divi's HTML/attribute/CSS fields for users without unfiltered_html (S7/S26); also
      * for removals, which the short form does without cleanValue().

src/Integrations/Divi/Compose/Composer.php

@@ -46,6 +46,8 @@ final class Composer
 
     private const BACKGROUND = 'module.decoration.background';
 
+    private const DEFAULT_LEVEL = 'h2';
+
     /** Advice of the S40 overlap error for new nodes. */
     private const ONE_FORM = 'für diesen Pfad nur Kurzform oder attrs angeben';
 
@@ -122,8 +124,13 @@ final class Composer
         }
 
         $set = $change['set'] ?? null;
+        $short = is_array($set) ? $this->shortPaths($block->shortName(), $set, 'set.') : [];
+        if (array_key_exists('label', $change)) {
+            // Ruling S41: the admin label is a short-form value too.
+            $short['label'] = self::LABEL_PATH;
+        }
         $this->overlaps(
-            is_array($set) ? $this->shortPaths($block->shortName(), $set, 'set.') : [],
+            $short,
             $change['attrs'] ?? null,
             $field,
             'bitte als zwei Änderungen senden'
@@ -156,10 +163,7 @@ final class Composer
         foreach (array_keys($full) as $path) {
             $path = (string) $path;
             foreach ($short as $name => $shortPath) {
-                if (
+                if (AttrMap::overlaps($path, $shortPath)) {
-                    $path === $shortPath || str_starts_with($path, $shortPath . '.')
-                    || str_starts_with($shortPath, $path . '.')
-                ) {
                     $this->errors[$field . '.attrs.' . $path] = sprintf(
                         'derselbe Attributpfad wird auch über %s geändert; %s',
                         $name,
@@ -171,6 +175,47 @@ final class Composer
         }
     }
 
+    /** @param array<array-key, mixed> $full "attrs" input. */
+    private function touches(array $full, string $path): bool
+    {
+        foreach (array_keys($full) as $given) {
+            if (AttrMap::overlaps((string) $given, $path)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * Ruling S41: the default level of a new heading stays when attrs replaced the desktop font
+     * value it lives in (attrs replace a breakpoint value whole) – unless attrs name a level.
+     *
+     * @param array<string, mixed> $attrs
+     * @return array<string, mixed>
+     */
+    private function defaultLevel(string $short, array $attrs): array
+    {
+        $field = ShortformMap::fields($short)['level'] ?? null;
+        if ($field === null || $field['sub'] === null) {
+            return $attrs;
+        }
+
+        $value = AttrMap::desktop($attrs, (string) $field['path']);
+        if ($value !== null && ! is_array($value)) {
+            return $attrs;
+        }
+
+        if (is_array($value) && array_key_exists((string) $field['sub'], $value)) {
+            return $attrs;
+        }
+
+        $current = AttrMap::get($attrs, (string) $field['path'] . '.desktop.value');
+        $merged = array_merge([(string) $field['sub'] => self::DEFAULT_LEVEL], is_array($current) ? $current : []);
+
+        return AttrMap::set($attrs, (string) $field['path'] . '.desktop.value', $merged);
+    }
+
     /**
      * Paths of the short-form fields a client gave, keyed by their input name ($prefix . field).
      *
@@ -346,8 +391,9 @@ final class Composer
         // of the same name (contact-field type, social network label) go through lit-divi-update "set".
         $values = array_intersect_key($spec, array_flip(array_diff($fields, self::COMMON)));
         $this->overlaps($this->shortPaths($short, $values, ''), $spec['attrs'] ?? null, $path, self::ONE_FORM);
-        if ($name === 'divi/heading' && ! array_key_exists('level', $values)) {
+        $defaultLevel = $name === 'divi/heading' && ! array_key_exists('level', $values);
-            $values['level'] = 'h2';
+        if ($defaultLevel) {
+            $values['level'] = self::DEFAULT_LEVEL;
         }
 
         $attrs = ['builderVersion' => $this->builderVersion];
@@ -357,7 +403,12 @@ final class Composer
             $this->errors += $result['errors'];
         }
 
-        $block = Block::create($name, $this->common($name, $attrs, $spec, $path, $this->autoLabel($name, $context)));
+        $attrs = $this->common($name, $attrs, $spec, $path, $this->autoLabel($name, $context));
+        if ($defaultLevel) {
+            $attrs = $this->defaultLevel($short, $attrs);
+        }
+
+        $block = Block::create($name, $attrs);
         if ($hasItems) {
             $this->children($block, $spec['items'] ?? [], $path . '.items', $context);
         } elseif ($name === 'divi/group') {
@@ -377,6 +428,14 @@ final class Composer
      */
     private function common(string $name, array $attrs, array $spec, string $path, ?string $autoLabel): array
     {
+        // Ruling S41: "label" and attrs on the admin label is an error; attrs alone replace the auto label.
+        $full = $spec['attrs'] ?? null;
+        if (array_key_exists('label', $spec) && $spec['label'] !== null) {
+            $this->overlaps(['label' => self::LABEL_PATH], $full, $path, self::ONE_FORM);
+        } elseif (is_array($full) && $this->touches($full, self::LABEL_PATH)) {
+            $autoLabel = null;
+        }
+
         $attrs = $this->presetAndAttrs($name, $attrs, $spec, $path);
         $label = array_key_exists('label', $spec) && $spec['label'] !== null ? $spec['label'] : $autoLabel;
         if ($label === null || $label === '') {
@@ -574,6 +633,31 @@ final class Composer
         return $widths;
     }
 
+    /**
+     * Ruling S41: the desktop flexType values of all columns of a row (in order), checked with
+     * the rules of "columns" – valid Divi widths that fill the row exactly. Null when valid,
+     * else the German problem.
+     *
+     * @param list<mixed> $flexTypes
+     */
+    public function rowWidthsError(array $flexTypes): ?string
+    {
+        $saved = $this->errors;
+        $this->errors = [];
+        $valid = $flexTypes !== [] && array_filter(
+            $flexTypes,
+            static fn (mixed $width): bool => is_string($width) && preg_match('/^\d{1,2}_\d{1,2}$/D', $width) === 1
+        ) === $flexTypes;
+        if ($valid) {
+            $this->widths(implode(',', $flexTypes), 'w');
+        }
+
+        $error = $valid ? ($this->errors['w'] ?? null) : 'Spaltenbreiten wie "8_24" erwartet';
+        $this->errors = $saved;
+
+        return $error;
+    }
+
     /**
      * Whether the fractions add up to exactly 1: numerators scaled to the least common
      * denominator (denominators are divisors of 24 or 5, 7, 9, 10, 11 – at most 27720).

src/Integrations/Divi/Pages/DiviUpdateAbility.php

@@ -7,7 +7,9 @@ namespace LIT\MCP\Integrations\Divi\Pages;
 use LIT\MCP\Abilities\Ability;
 use LIT\MCP\Abilities\Errors;
 use LIT\MCP\Abilities\ToolResult;
+use LIT\MCP\Integrations\Divi\BlockTree\AttrMap;
 use LIT\MCP\Integrations\Divi\BlockTree\Block;
+use LIT\MCP\Integrations\Divi\Compose\Composer;
 use WP_Error;
 
 /**
@@ -20,6 +22,8 @@ use WP_Error;
  */
 final class DiviUpdateAbility implements Ability
 {
+    private const SIZING = 'module.decoration.sizing';
+
     public function __construct(private DiviWriter $writer)
     {
     }
@@ -117,7 +121,7 @@ final class DiviUpdateAbility implements Ability
         $invalidPaths = false;
         /** @var array<string, int> $seen id => position of its first change */
         $seen = [];
-        /** @var list<array{0: Block, 1: array<string, mixed>}> $updates */
+        /** @var list<array{0: Block, 1: array<string, mixed>, 2: string}> $updates */
         $updates = [];
         foreach ($changes as $position => $change) {
             $field = sprintf('changes[%d]', $position);
@@ -150,7 +154,11 @@ final class DiviUpdateAbility implements Ability
             $result = $composer->update($block, $change, $field);
             $errors += $result['errors'];
             $invalidPaths = $invalidPaths || $result['invalid_paths'] !== [];
-            $updates[] = [$block, $result['attrs']];
+            $updates[] = [$block, $result['attrs'], $field];
+        }
+
+        if ($errors === []) {
+            $errors = $this->rowWidths($page, $composer, $updates);
         }
 
         if ($errors !== []) {
@@ -172,4 +180,55 @@ final class DiviUpdateAbility implements Ability
 
         return $this->writer->save($this->name(), $page, $change, $input);
     }
+
+    /**
+     * Ruling S41: a column whose desktop width changes must leave its row filled exactly – the
+     * rules of "columns" (Composer::rowWidthsError()), checked over the row after all changes.
+     *
+     * @param list<array{0: Block, 1: array<string, mixed>, 2: string}> $updates
+     * @return array<string, string>
+     */
+    private function rowWidths(DiviPage $page, Composer $composer, array $updates): array
+    {
+        $new = [];
+        $rows = [];
+        foreach ($updates as [$block, $attrs, $field]) {
+            if ($block->name() !== 'divi/column' || self::width($attrs) === self::width($block->attrs())) {
+                continue;
+            }
+
+            $new[spl_object_id($block)] = $attrs;
+            $row = $page->index->parentOf($block);
+            if ($row instanceof Block) {
+                $rows[spl_object_id($row)]['row'] = $row;
+                $rows[spl_object_id($row)]['fields'][] = $field;
+            }
+        }
+
+        $errors = [];
+        foreach ($rows as ['row' => $row, 'fields' => $fields]) {
+            $widths = [];
+            foreach ($row->children() as $column) {
+                $widths[] = self::width($new[spl_object_id($column)] ?? $column->attrs());
+            }
+
+            $error = $composer->rowWidthsError($widths);
+            if ($error !== null) {
+                foreach ($fields as $field) {
+                    $errors[$field . '.attrs.' . self::SIZING] = $error
+                        . ' (Breiten aller Spalten der Zeile zusammen ändern)';
+                }
+            }
+        }
+
+        return $errors;
+    }
+
+    /** @param array<string, mixed> $attrs */
+    private static function width(array $attrs): mixed
+    {
+        $sizing = AttrMap::desktop($attrs, self::SIZING);
+
+        return is_array($sizing) ? ($sizing['flexType'] ?? null) : null;
+    }
 }

tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php

@@ -510,5 +510,24 @@ final class AttrWriterTest extends TestCase
             self::assertSame($reference, $deep['attrs']['button']['innerContent']['desktop']['value']['text']);
         }
     }
-}
 
+    /** Ruling S41: structure attributes are refused whatever the catalog says (also for admins, also null). */
+    public function testStructureAttributesAreRefused(): void
+    {
+        foreach ([
+            ['divi/section', 'module.advanced.type', ['desktop' => 'specialty']],
+            ['divi/section', 'module.advanced.type', null],
+            ['divi/section', 'module.advanced', ['desktop' => ['type' => 'fullwidth']]],
+            ['divi/column', 'module.advanced.specialtyColumns', ['desktop' => '2']],
+        ] as [$module, $path, $map]) {
+            $result = ComposeFixture::attrWriter(true)->apply($module, [], [$path => $map], 'attrs');
+
+            self::assertStringContainsString('Struktur', $result['errors']['attrs.' . $path] ?? '', $module . ' ' . $path);
+            self::assertSame([], $result['invalid_paths'], $path);
+            self::assertSame([], $result['attrs'], $path);
+        }
+
+        $heading = ComposeFixture::attrWriter()->apply('divi/heading', [], ['module.meta.adminLabel' => ['desktop' => 'x']], 'attrs');
+        self::assertSame([], $heading['errors'], 'only section type and specialty columns are structure');
+    }
+}

tests/Unit/Integrations/Divi/Compose/ComposerTest.php

@@ -231,6 +231,72 @@ final class ComposerTest extends TestCase
         self::assertSame([], $separate['errors'], 'different paths, and the default heading level, may be combined');
     }
 
+    /** Ruling S41: an admin label given in attrs is kept (no auto label over it); label + attrs on it is an error. */
+    public function testAdminLabelInAttrs(): void
+    {
+        $kept = ComposeFixture::composer()->compose([[
+            'type' => 'heading',
+            'text' => 'x',
+            'attrs' => ['module.meta.adminLabel' => ['desktop' => 'Mein Label', 'tablet' => 'Tablet']],
+        ]], Block::create('divi/column'), 'nodes', 'Hero');
+
+        self::assertSame([], $kept['errors']);
+        self::assertSame(
+            ['desktop' => ['value' => 'Mein Label'], 'tablet' => ['value' => 'Tablet']],
+            $kept['blocks'][0]->attrs()['module']['meta']['adminLabel']
+        );
+
+        $both = ComposeFixture::composer()->compose([[
+            'type' => 'heading',
+            'text' => 'x',
+            'label' => 'B',
+            'attrs' => ['module.meta.adminLabel' => ['desktop' => 'A', 'tablet' => 'T']],
+        ]], Block::create('divi/column'), 'nodes', 'Hero');
+        self::assertStringContainsString('label', $both['errors']['nodes[0].attrs.module.meta.adminLabel']);
+
+        $update = ComposeFixture::composer()->update(Block::create('divi/heading'), [
+            'label' => 'B',
+            'attrs' => ['module.meta.adminLabel' => ['tablet' => 'T']],
+        ], 'changes[0]');
+        self::assertStringContainsString('label', $update['errors']['changes[0].attrs.module.meta.adminLabel']);
+    }
+
+    /** Ruling S41: the default heading level survives attrs on the same font value. */
+    public function testDefaultHeadingLevelIsMergedIntoAttrs(): void
+    {
+        $result = ComposeFixture::composer()->compose([[
+            'type' => 'heading',
+            'text' => 'x',
+            'attrs' => ['title.decoration.font.font' => ['desktop' => ['color' => '#111111']]],
+        ]], Block::create('divi/column'), 'nodes');
+
+        self::assertSame([], $result['errors']);
+        self::assertSame(
+            ['headingLevel' => 'h2', 'color' => '#111111'],
+            $result['blocks'][0]->attrs()['title']['decoration']['font']['font']['desktop']['value']
+        );
+
+        $explicit = ComposeFixture::composer()->compose([[
+            'type' => 'heading',
+            'text' => 'x',
+            'attrs' => ['title.decoration.font.font' => ['desktop' => ['headingLevel' => 'h3']]],
+        ]], Block::create('divi/column'), 'nodes');
+        self::assertSame('h3', $explicit['blocks'][0]->attrs()['title']['decoration']['font']['font']['desktop']['value']['headingLevel']);
+    }
+
+    /** Ruling S41: column widths on update follow the rules of "columns". */
+    public function testRowWidthsAreChecked(): void
+    {
+        $composer = ComposeFixture::composer();
+
+        self::assertNull($composer->rowWidthsError(['8_24', '16_24']));
+        self::assertNull($composer->rowWidthsError(['1_5', '1_5', '1_5', '1_5', '1_5']));
+        self::assertNotNull($composer->rowWidthsError(['12_24', '16_24']));
+        self::assertNotNull($composer->rowWidthsError(['1_13', '12_13']));
+        self::assertNotNull($composer->rowWidthsError(['x', '24_24']));
+        self::assertNotNull($composer->rowWidthsError([]));
+    }
+
     /** Ruling S13: labels go through the choke point; a label that fails is an error, never stored raw. */
     public function testLabelsAreCleanedOrReported(): void
     {

tests/Unit/Integrations/Divi/Pages/DiviWriteAbilitiesTest.php

@@ -297,6 +297,27 @@ final class DiviWriteAbilitiesTest extends TestCase
         self::assertSame([], $this->saved);
     }
 
+    /** Ruling S41: a column width change must still fill its row exactly. */
+    public function testUpdateChecksTheRowWidths(): void
+    {
+        $sizing = static fn (string $width): array => ['module.decoration.sizing' => ['desktop' => ['flexType' => $width]]];
+
+        $broken = $this->update()->execute(['post_id' => 7, 'tree_rev' => $this->treeRev(), 'changes' => [
+            ['id' => 's0.r0.c0', 'attrs' => $sizing('12_24')],
+        ]]);
+        self::assertSame('lit_validation_failed', $broken->get_error_code());
+        self::assertArrayHasKey('changes[0].attrs.module.decoration.sizing', self::clientError($broken)['fields']);
+        self::assertSame([], $this->saved);
+
+        $result = $this->update()->execute(['post_id' => 7, 'tree_rev' => $this->treeRev(), 'changes' => [
+            ['id' => 's0.r0.c0', 'attrs' => $sizing('12_24')],
+            ['id' => 's0.r0.c1', 'attrs' => $sizing('12_24')],
+        ]]);
+        self::assertTrue($result['ok'], (string) json_encode($result));
+        self::assertCount(1, $this->saved);
+        self::assertStringContainsString('"flexType":"12_24"}}}}},"builderVersion":"5.2.1"} --><!-- wp:divi/image', $this->saved[0]);
+    }
+
     /** Ruling S7: builder-only paths are refused for users without unfiltered_html – visible to the client. */
     public function testBuilderOnlyPathsAreRefusedForEditors(): void
     {