fix(schema): stop ImportHandler grafting an INT $ref onto array properties, and drop any $ref Opis cannot use

lib/Service/Configuration/ImportHandler.php

@@ -1490,11 +1490,34 @@ public function importSchema(
                         }
                     }
 
+                    // 🔴 Both branches write to `items.$ref`. The second one used to
+                    // write to `$property['$ref']` — the ARRAY's own ref — which did
+                    // two wrong things at once: it left `items.$ref` as an unmapped
+                    // slug, and it grafted a schema ID (an INT) onto the array
+                    // property as a top-level `$ref`.
+                    //
+                    // That second effect is not cosmetic. `ValidateObject` strips a
+                    // top-level `$ref` from string-typed properties and from
+                    // `items`, but never from an array-typed property, so the int
+                    // survived into the schema handed to Opis. Opis parses a
+                    // property's subschema lazily — only when that property is
+                    // PRESENT in the written data — and then throws
+                    // `$ref must be a non-empty string` because an int is not a
+                    // string. The message names neither the property nor the
+                    // schema, so it reads like a broken register.
+                    //
+                    // Only reachable on the `schemasMap` fallback, i.e. when the
+                    // referenced slug was not part of this import's own
+                    // slug->id map. That is the FIRST install of a configuration
+                    // whose cross-references resolve against already-present
+                    // schemas — which is why it never showed on a long-lived
+                    // instance and surfaced on every clean one (hermiq's `agent`
+                    // schema: skillInstalls / contextRefs / delegationAllowlist).
                     if (($property['items']['$ref'] ?? null) !== null) {
                         if (($slugsAndIdsMap[$property['items']['$ref']] ?? null) !== null) {
                             $property['items']['$ref'] = $slugsAndIdsMap[$property['items']['$ref']];
                         } else if (($this->schemasMap[$property['items']['$ref']] ?? null) !== null) {
-                            $property['$ref'] = $this->schemasMap[$property['items']['$ref']]->getId();
+                            $property['items']['$ref'] = $this->schemasMap[$property['items']['$ref']]->getId();
                         }
                     }
 

lib/Service/Object/ValidateObject.php

@@ -468,6 +468,27 @@ private function transformOpenRegisterObjectConfigurations(object $schemaObject)
      */
     private function transformPropertyForOpenRegister(object $propertySchema): void
     {
+        // 🔴 First, drop any `$ref` that CANNOT be a JSON Schema reference.
+        //
+        // OpenRegister stores a `$ref` as a relation marker, and every branch
+        // below already removes it before the schema reaches Opis — but only for
+        // the shapes those branches recognise. A `$ref` that is an int, a null,
+        // an array or an empty string is not one of them, survives, and makes
+        // Opis throw `$ref must be a non-empty string` from
+        // RefKeywordParser::parse().
+        //
+        // That exception fires LAZILY, when the offending property is present in
+        // the written data, so it names neither the property nor the schema and
+        // reads like a broken register rather than a stored schema defect. It is
+        // also unrecoverable for the caller: no payload shape fixes a schema.
+        //
+        // Registers imported before openregister#2321 already carry exactly this
+        // (an int `$ref` grafted onto an array property by ImportHandler), so
+        // this normalisation is what heals them without a migration. A VALID
+        // reference — a non-empty string — is untouched here and handled by the
+        // branches below exactly as before.
+        $this->dropUnusableRef(schema: $propertySchema);
+
         // UUID pattern for related object references.
         $uuidPat = '^([a-z]+-)?([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-f]{32}|[0-9]+)$';
 
@@ -539,6 +560,9 @@ private function transformPropertyForOpenRegister(object $propertySchema): void
         if ($isArrayType === true && $hasItems === true && is_object($propertySchema->items) === true) {
             $itemsSchema = $propertySchema->items;
 
+            // Same normalisation as the property itself — see dropUnusableRef().
+            $this->dropUnusableRef(schema: $itemsSchema);
+
             // Handle inversedBy relationships for array items.
             // TODO: Move writeBack, removeAfterWriteBack, and inversedBy from items to config.
             if (($itemsSchema->inversedBy ?? null) !== null) {
@@ -598,6 +622,37 @@ private function transformPropertyForOpenRegister(object $propertySchema): void
         }
     }//end transformPropertyForOpenRegister()
 
+    /**
+     * Remove a `$ref` that cannot be a JSON Schema reference.
+     *
+     * A JSON Schema `$ref` MUST be a non-empty string; Opis enforces that in
+     * `RefKeywordParser::parse()` and throws `$ref must be a non-empty string`
+     * for anything else. OpenRegister only ever uses `$ref` as a relation
+     * marker, never for validation-time resolution, so removing an unusable one
+     * loses nothing and turns an opaque 500 back into a normal validation pass.
+     *
+     * Valid refs (non-empty strings) are deliberately left alone — the
+     * surrounding transform branches own those.
+     *
+     * @param object $schema The property or items schema to normalise in place.
+     *
+     * @return void
+     */
+    private function dropUnusableRef(object $schema): void
+    {
+        if (property_exists($schema, '$ref') === false) {
+            return;
+        }
+
+        $ref = $schema->{'$ref'};
+        if (is_string($ref) === true && $ref !== '') {
+            return;
+        }
+
+        unset($schema->{'$ref'});
+
+    }//end dropUnusableRef()
+
     /**
      * Transforms object properties based on OpenRegister object configuration.
      *

tests/Unit/Service/Configuration/ImportHandlerCoverageTest.php

@@ -367,9 +367,22 @@ public function testGetDuplicateRegisterInfoShowsUnknownWhenNoCreatedDate(): voi
     // =========================================================================
 
     /**
-     * importSchema resolves items.$ref from schemasMap.
+     * importSchema resolves items.$ref from schemasMap — INTO `items`, and
+     * without inventing a top-level `$ref` on the array property.
+     *
+     * 🔴 This used to assert only `instanceof Schema`, which is true whatever
+     * the branch writes, so it exercised the defect and reported green. The
+     * branch wrote the resolved id to `$property['$ref']` instead of
+     * `$property['items']['$ref']`, which left the items ref an unmapped slug
+     * AND grafted an INT `$ref` onto an array property. Opis rejects a
+     * non-string `$ref` with `$ref must be a non-empty string` the moment such
+     * a property appears in a written object — the 500 hermiq's clean-install
+     * e2e hit on `PUT /api/agents/{id}/tool-grants`.
+     *
+     * The assertions are on the payload handed to the mapper, because that is
+     * what gets persisted.
      */
-    public function testImportSchemaResolvesItemsRefFromSchemasMap(): void
+    public function testImportSchemaResolvesItemsRefIntoItemsAndNotOntoTheProperty(): void
     {
         $refSchema = $this->makeSchema(42, 'related-schema');
         $this->setProperty($this->handler, 'schemasMap', ['related-schema' => $refSchema]);
@@ -388,19 +401,40 @@ public function testImportSchemaResolvesItemsRefFromSchemasMap(): void
         ];
 
         $createdSchema = $this->makeSchema(100, 'items-ref-schema');
+        $persisted     = null;
 
         $this->schemaMapper->method('find')
             ->willThrowException(new \OCP\AppFramework\Db\DoesNotExistException(''));
         $this->schemaMapper->method('createFromArray')
-            ->willReturn($createdSchema);
+            ->willReturnCallback(
+                function (array $payload) use (&$persisted, $createdSchema): Schema {
+                    $persisted = $payload;
+                    return $createdSchema;
+                }
+            );
         $this->schemaMapper->method('update')
             ->willReturnArgument(0);
 
         $result = $this->handler->importSchema($data, [], null, null, '1.0.0');
 
         $this->assertInstanceOf(Schema::class, $result);
+        $this->assertIsArray($persisted, 'the schema payload must reach the mapper');
+
+        $property = $persisted['properties']['items_prop'];
+
+        $this->assertSame(
+            42,
+            $property['items']['$ref'],
+            'the items $ref must be resolved to the referenced schema id'
+        );
+        $this->assertArrayNotHasKey(
+            '$ref',
+            $property,
+            'an ARRAY property must never gain a top-level $ref from its items — '
+            .'a non-string $ref makes Opis throw "$ref must be a non-empty string"'
+        );
 
-    }//end testImportSchemaResolvesItemsRefFromSchemasMap()
+    }//end testImportSchemaResolvesItemsRefIntoItemsAndNotOntoTheProperty()
 
 
     // =========================================================================

tests/Unit/Service/Object/ValidateObjectUnusableRefTest.php

@@ -0,0 +1,218 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * ValidateObject unusable-`$ref` Unit Tests.
+ *
+ * A JSON Schema `$ref` MUST be a non-empty string. Opis enforces that in
+ * `RefKeywordParser::parse()` and throws
+ * `Opis\JsonSchema\Exceptions\InvalidKeywordException: $ref must be a non-empty string`
+ * for anything else.
+ *
+ * OpenRegister stores `$ref` as a RELATION marker, never as something Opis
+ * should resolve, and `ValidateObject` strips it before validation — but only
+ * for the shapes its transform branches recognise: string-typed properties,
+ * `items`, self-references and object properties carrying an
+ * `objectConfiguration.handling`. A `$ref` that is an INT, null, an array or an
+ * empty string matches none of them, survives into the schema handed to Opis,
+ * and blows up.
+ *
+ * That is not hypothetical. `ImportHandler::importSchema()` wrote a resolved
+ * schema ID (an int) to `$property['$ref']` when it meant
+ * `$property['items']['$ref']`, so any array-of-relations property imported
+ * through the `schemasMap` fallback carried an int `$ref`. Measured on hermiq's
+ * clean-install e2e (run 30865280923): `PUT /api/agents/{id}/tool-grants`
+ * answered 500 with `$ref must be a non-empty string` from
+ * `ObjectService::saveObject()`.
+ *
+ * The import side is fixed, but every database imported before that fix still
+ * carries the bad value, so this normalisation is what heals them — and these
+ * tests are what stop it regressing.
+ *
+ * @category Tests
+ * @package  OCA\OpenRegister\Tests\Unit\Service\Object
+ *
+ * @author    Conduction Development Team <info@conduction.nl>
+ * @copyright 2026 Conduction B.V.
+ * @license   EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * @link https://OpenRegister.app
+ */
+
+namespace OCA\OpenRegister\Tests\Unit\Service\Object;
+
+use OCA\OpenRegister\Db\MagicMapper;
+use OCA\OpenRegister\Db\Schema;
+use OCA\OpenRegister\Db\SchemaMapper;
+use OCA\OpenRegister\Service\Object\ValidateObject;
+use OCP\IAppConfig;
+use OCP\IURLGenerator;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Locks the behaviour of an unusable `$ref` on a stored schema.
+ */
+class ValidateObjectUnusableRefTest extends TestCase
+{
+
+    /**
+     * The subject under test.
+     *
+     * @var ValidateObject
+     */
+    private ValidateObject $handler;
+
+
+    /**
+     * Build a ValidateObject with mocked collaborators.
+     *
+     * @return void
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $urlGenerator = $this->createMock(IURLGenerator::class);
+        $urlGenerator->method('getBaseUrl')->willReturn('http://localhost:8080');
+
+        $this->handler = new ValidateObject(
+            $this->createMock(IAppConfig::class),
+            $this->createMock(MagicMapper::class),
+            $this->createMock(SchemaMapper::class),
+            $urlGenerator,
+            $this->createMock(LoggerInterface::class)
+        );
+
+    }//end setUp()
+
+
+    /**
+     * A Schema entity with the given slug.
+     *
+     * @param string $slug Schema slug.
+     *
+     * @return Schema
+     */
+    private function schema(string $slug='agent'): Schema
+    {
+        $schema = new Schema();
+        $schema->setSlug($slug);
+        $schema->setTitle('Agent');
+        return $schema;
+
+    }//end schema()
+
+
+    /**
+     * A schema object with one array property carrying the given top-level
+     * `$ref` — the exact shape a pre-fix import left behind.
+     *
+     * @param mixed $ref The stored `$ref` value.
+     *
+     * @return object
+     */
+    private function schemaWithPropertyRef(mixed $ref): object
+    {
+        return json_decode(
+            json_encode(
+                [
+                    'type'       => 'object',
+                    'properties' => [
+                        'delegationAllowlist' => [
+                            'type'  => 'array',
+                            '$ref'  => $ref,
+                            'items' => [
+                                'type'   => 'string',
+                                'format' => 'uuid',
+                            ],
+                        ],
+                    ],
+                ]
+            )
+        );
+
+    }//end schemaWithPropertyRef()
+
+
+    /**
+     * An INT `$ref` — the ImportHandler shape — validates instead of throwing.
+     *
+     * The property must be PRESENT in the object: Opis parses a property's
+     * subschema lazily, which is why this endpoint looked healthy for months
+     * on instances whose optional relation arrays were never written.
+     *
+     * @return void
+     */
+    public function testIntegerRefOnAnArrayPropertyDoesNotThrow(): void
+    {
+        $object = ['delegationAllowlist' => ['550e8400-e29b-41d4-a716-446655440000']];
+
+        $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(4365));
+
+        $this->assertTrue(
+            $result->isValid(),
+            'an int $ref must be dropped, not handed to Opis as a JSON Schema reference'
+        );
+
+    }//end testIntegerRefOnAnArrayPropertyDoesNotThrow()
+
+
+    /**
+     * An EMPTY-STRING `$ref` is equally unusable and equally dropped.
+     *
+     * @return void
+     */
+    public function testEmptyStringRefDoesNotThrow(): void
+    {
+        $object = ['delegationAllowlist' => ['11111111-1111-1111-1111-111111111111']];
+
+        $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(''));
+
+        $this->assertTrue($result->isValid(), 'an empty-string $ref must be dropped');
+
+    }//end testEmptyStringRefDoesNotThrow()
+
+
+    /**
+     * A NULL `$ref` is dropped too — and a null property value, which survives
+     * OpenRegister's own empty-value filter, is what actually reaches Opis.
+     *
+     * @return void
+     */
+    public function testNullRefWithANullValueDoesNotThrow(): void
+    {
+        $object = ['delegationAllowlist' => null];
+
+        $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(null));
+
+        $this->assertTrue($result->isValid(), 'a null $ref must be dropped');
+
+    }//end testNullRefWithANullValueDoesNotThrow()
+
+
+    /**
+     * 🔑 NEGATIVE CONTROL. Dropping the unusable `$ref` must not switch
+     * validation off for the property — a value of the wrong TYPE still fails.
+     *
+     * Without this, all three tests above would pass just as well if the fix
+     * had removed the property from validation altogether.
+     *
+     * @return void
+     */
+    public function testDroppingTheRefStillLeavesTheTypeEnforced(): void
+    {
+        $object = ['delegationAllowlist' => 'not an array at all'];
+
+        $result = $this->handler->validateObject($object, $this->schema(), $this->schemaWithPropertyRef(4365));
+
+        $this->assertFalse(
+            $result->isValid(),
+            'a string where the schema declares an array must still fail validation'
+        );
+
+    }//end testDroppingTheRefStillLeavesTheTypeEnforced()
+
+
+}//end class