From 607d969a3e64bd9553a32697b9d8f3fafb575eed Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 2 Aug 2026 00:01:27 +0200 Subject: [PATCH 1/4] =?UTF-8?q?feat(app-repo):=20apply=20the=20v2=20channe?= =?UTF-8?q?ls=20on=20install=20=E2=80=94=20they=20were=20parsed,=20then=20?= =?UTF-8?q?dropped?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six steps carry an app between instances: serialize -> bind -> push -> fetch -> parse -> apply. Five were built. The sixth never was, so installing a published v2 app produced an app holding its manifest and NOTHING that makes it run, and reported success. Verified against the code with a positive control before writing any of this: $template['manifest'] / ['version'] (control) 7 hits $template['connectors'] 0 $template['automations'] 0 $template['skills'] 0 $template['dataRegisters'] 3, all in the export/zip path Both entry points confirmed by reading them: pull() persisted manifest + companion schemas only; installFromTemplateArray() read exactly slug + manifest. This is the fourth time in this programme that one half of a round trip was extended and the other left behind. Publish looked perfect every time, because publish is the half that kept getting extended. Three rules shape the implementation: NEVER OVERWRITE. Connectors are shared infrastructure - one source can serve several apps - so a colliding uuid is skipped and reported. Enforced with saveObject(failIfExists: true) so the guarantee lives in the call rather than in a preceding existence check that could drift or race. NEVER CLAIM ATOMICITY. OpenRegister has no cross-object transaction, so one failing item must not cost the caller the rest. ChannelApplyReport enforces created + skipped + failed === declared and THROWS when it does not hold, so a dropped item is arithmetically impossible to hide. The 64-skill silent cap this programme already shipped is what that identity exists to prevent. NEVER DROP SILENTLY. Every channel is bounded; truncation is logged AND counted. Two defects found while writing the tests, both of which would have shipped: 1. Collision was detected by MESSAGE TEXT. A plain PHP 'Unknown named parameter $failIfExists' error therefore reported itself as a benign 'already exists' - a wiring bug wearing the costume of an expected outcome, and the reason three tests were briefly green for the wrong reason. Now caught BY TYPE (ObjectExistsException), and the stale test stub that hid it is fixed. 2. The credential lookup called findAll(filters:, register:, schema:), which is not the real signature - findAll takes a $config array. credentialExists() swallows a failed lookup and returns true (an inconclusive lookup must never manufacture an absence claim), so needsCredentials would have been silently empty forever. Register/schema confirmed against the live instance (credential-broker / brokeredcredential) with a positive control, not assumed. Skills delegate to hermiq's SkillBundleInstaller by repo coordinates rather than being reimplemented, so frontmatter byte-fidelity and the ADR-068 aux-file rules keep living in exactly one place. openconnector and hermiq stay OPTIONAL - OpenBuild declares only openregister - and degrade with a machine-readable reason while every other channel still applies. The collision test is mutation-checked: flipping failIfExists to false turns it red. phpstan is run explicitly, because openbuild ships no vendor/ and the local 40-gate suite silently SKIPS phpstan - green there never meant phpstan passed. --- lib/Controller/ApplicationsController.php | 12 + lib/Service/AppChannelApplier.php | 612 ++++++++++++++++++ lib/Service/ChannelApplyReport.php | 328 ++++++++++ lib/Service/ContainerLocator.php | 94 +++ lib/Service/DataRegisterProvisioner.php | 213 ++++++ lib/Service/GitHubAppSyncService.php | 16 + .../changes/apply-v2-channels/.openspec.yaml | 2 + openspec/changes/apply-v2-channels/design.md | 122 ++++ .../changes/apply-v2-channels/proposal.md | 100 +++ .../specs/app-channel-application/spec.md | 144 +++++ .../openbuild-application-register/spec.md | 23 + openspec/changes/apply-v2-channels/tasks.md | 76 +++ .../openbuild-application-register/spec.md | 1 + .../Controller/CreateFromTemplateTest.php | 10 + tests/Unit/Service/AppChannelApplierTest.php | 364 +++++++++++ tests/Unit/Service/ChannelApplyReportTest.php | 148 +++++ tests/stubs/openregister-stubs.php | 20 +- .../ApplicationsControllerDiffTest.php | 2 + .../Controller/ApplicationsControllerTest.php | 1 + 19 files changed, 2287 insertions(+), 1 deletion(-) create mode 100644 lib/Service/AppChannelApplier.php create mode 100644 lib/Service/ChannelApplyReport.php create mode 100644 lib/Service/ContainerLocator.php create mode 100644 lib/Service/DataRegisterProvisioner.php create mode 100644 openspec/changes/apply-v2-channels/.openspec.yaml create mode 100644 openspec/changes/apply-v2-channels/design.md create mode 100644 openspec/changes/apply-v2-channels/proposal.md create mode 100644 openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md create mode 100644 openspec/changes/apply-v2-channels/specs/openbuild-application-register/spec.md create mode 100644 openspec/changes/apply-v2-channels/tasks.md create mode 100644 tests/Unit/Service/AppChannelApplierTest.php create mode 100644 tests/Unit/Service/ChannelApplyReportTest.php diff --git a/lib/Controller/ApplicationsController.php b/lib/Controller/ApplicationsController.php index 1219b7a3f..635b6633c 100644 --- a/lib/Controller/ApplicationsController.php +++ b/lib/Controller/ApplicationsController.php @@ -53,6 +53,7 @@ use DateTimeImmutable; use DateTimeInterface; use OCA\OpenBuild\AppInfo\Application; +use OCA\OpenBuild\Service\AppChannelApplier; use OCA\OpenBuild\Service\ApplicationVersionService; use OCA\OpenBuild\Service\ManifestResolverService; use OCA\OpenBuild\Service\PermissionResolver; @@ -103,6 +104,7 @@ class ApplicationsController extends Controller * @param IGroupManager $groupManager Group membership resolver * @param ManifestResolverService $manifestResolver Version-aware manifest resolver (REQ-OBVR-002) * @param PermissionResolver $permissionResolver Shared permission-grammar resolver (H1/H2 fix) + * @param AppChannelApplier $channelApplier Applies the v2 repo channels (apply-v2-channels) * @param AuditTrailMapper|null $auditTrailMapper Optional OR audit-trail writer (null until OR loaded) * * @return void @@ -117,6 +119,7 @@ public function __construct( private readonly IGroupManager $groupManager, private readonly ManifestResolverService $manifestResolver, private readonly PermissionResolver $permissionResolver, + private readonly AppChannelApplier $channelApplier, private readonly ?AuditTrailMapper $auditTrailMapper=null, ) { parent::__construct(appName: Application::APP_ID, request: $request); @@ -1454,6 +1457,14 @@ public function installFromTemplateArray( return ['status' => $persistResult['status'], 'data' => $persistResult['error']]; } + // Apply the app-repo-format-v2 channels. Until this call existed, the four + // channels were parsed and then dropped, so an installed app arrived with + // its manifest and nothing that makes it run — and reported success. + $channels = $this->channelApplier->apply( + template: $template, + actingUserId: $ownerUid + ); + return [ 'status' => Http::STATUS_CREATED, 'data' => [ @@ -1461,6 +1472,7 @@ public function installFromTemplateArray( 'slug' => $newSlug, 'register' => $cloneResult['register']->getSlug(), 'companionSchemas' => $cloneResult['schemaIds'], + 'channels' => $channels, ], ]; }//end installFromTemplateArray() diff --git a/lib/Service/AppChannelApplier.php b/lib/Service/AppChannelApplier.php new file mode 100644 index 000000000..e95357f84 --- /dev/null +++ b/lib/Service/AppChannelApplier.php @@ -0,0 +1,612 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Service; + +use OCA\OpenRegister\Exception\ObjectExistsException; +use OCA\OpenRegister\Service\ObjectService; +use OCP\App\IAppManager; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Applies a parsed v2 app repo's channels onto this instance. + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + */ +class AppChannelApplier +{ + + /** + * The OpenRegister register that holds OpenConnector objects. OpenConnector is + * re-platformed onto OpenRegister — there are no `openconnector_*` tables. + * + * @var string + */ + private const CONNECTOR_REGISTER = 'openconnector'; + + /** + * Connector schemas, matching AppRepoSerializer::CONNECTOR_KINDS. + * + * @var array + */ + private const CONNECTOR_KINDS = ['source', 'mapping', 'synchronization', 'job']; + + /** + * The register holding broker credentials. + * + * @var string + */ + private const CREDENTIAL_REGISTER = 'credential-broker'; + + /** + * The schema holding broker credentials. + * + * @var string + */ + private const CREDENTIAL_SCHEMA = 'brokeredcredential'; + + /** + * Maximum connectors applied per kind. + * + * @var int + */ + private const MAX_CONNECTORS_PER_KIND = 2048; + + /** + * Maximum automations applied from one repo. + * + * @var int + */ + private const MAX_AUTOMATIONS = 512; + + /** + * Reason recorded when OpenConnector is not available. + * + * @var string + */ + private const REASON_NO_OPENCONNECTOR = 'openconnector-unavailable'; + + /** + * Reason recorded when hermiq is not available. + * + * @var string + */ + private const REASON_NO_HERMIQ = 'hermiq-unavailable'; + + /** + * The hermiq service that installs a published skill bundle by repo + * coordinates. Resolved from the server container only when hermiq is + * enabled, so hermiq stays an optional dependency. + * + * @var string + */ + private const HERMIQ_INSTALLER = '\OCA\Hermiq\Service\SkillBundleInstaller'; + + /** + * Constructor. + * + * @param ObjectService $objectService OpenRegister object read/write. + * @param DataRegisterProvisioner $registerProvisioner The data-registers channel. + * @param IAppManager $appManager Optional-dependency detection. + * @param ContainerLocator $locator Lazy cross-app service resolution. + * @param LoggerInterface $logger PSR logger (secret-free diagnostics). + * + * @return void + */ + public function __construct( + private readonly ObjectService $objectService, + private readonly DataRegisterProvisioner $registerProvisioner, + private readonly IAppManager $appManager, + private readonly ContainerLocator $locator, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Apply every channel a parsed template declares. + * + * Best-effort by design: a channel that cannot be applied is reported, and the + * remaining channels are still applied. + * + * Repo coordinates are derived from the template's own `templateOrigin.repo` + * when the caller does not supply them, so that every install path can call + * this with what it already has. A path that had to thread extra arguments + * through is a path that eventually gets added without them. + * + * @param array $template The parsed repo template. + * @param string|null $owner Repo owner (for the skills delegation). + * @param string|null $repo Repo name (for the skills delegation). + * @param string|null $ref Optional git ref. + * @param string|null $actingUserId The session UID. + * @param string|null $credentialId Optional broker credential UUID. + * + * @return array The channel report. + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-every-install-path-applies-the-v2-channels + */ + public function apply( + array $template, + ?string $owner=null, + ?string $repo=null, + ?string $ref=null, + ?string $actingUserId=null, + ?string $credentialId=null + ): array { + $report = new ChannelApplyReport(); + + [$owner, $repo] = $this->coordinatesFor(template: $template, owner: $owner, repo: $repo); + + $this->registerProvisioner->apply( + registers: $this->channelOf(template: $template, name: 'dataRegisters'), + report: $report + ); + + $this->applyConnectors( + connectors: $this->channelOf(template: $template, name: 'connectors'), + report: $report + ); + + $this->applyAutomations( + automations: $this->channelOf(template: $template, name: 'automations'), + report: $report + ); + + $this->applySkills( + skills: $this->channelOf(template: $template, name: 'skills'), + owner: $owner, + repo: $repo, + ref: $ref, + actingUserId: $actingUserId, + credentialId: $credentialId, + report: $report + ); + + return $report->toArray(); + + }//end apply() + + /** + * Resolve the repo coordinates, falling back to the template's own origin. + * + * @param array $template The parsed template. + * @param string|null $owner Caller-supplied owner, if any. + * @param string|null $repo Caller-supplied repo name, if any. + * + * @return array{0:string,1:string} Owner and repo name, possibly empty. + */ + private function coordinatesFor(array $template, ?string $owner, ?string $repo): array + { + if ($owner !== null && $owner !== '' && $repo !== null && $repo !== '') { + return [$owner, $repo]; + } + + $origin = ($template['templateOrigin'] ?? []); + $slug = ''; + if (is_array($origin) === true) { + $slug = (string) ($origin['repo'] ?? ''); + } + + $parts = explode('/', $slug); + if (count($parts) !== 2 || $parts[0] === '' || $parts[1] === '') { + return [(string) $owner, (string) $repo]; + } + + return [$parts[0], $parts[1]]; + + }//end coordinatesFor() + + /** + * Read one channel from the template, tolerating its absence (a v1 repo). + * + * @param array $template The parsed template. + * @param string $name The channel key. + * + * @return array The channel, or an empty array. + */ + private function channelOf(array $template, string $name): array + { + $channel = ($template[$name] ?? []); + if (is_array($channel) === false) { + return []; + } + + return $channel; + + }//end channelOf() + + /** + * Apply the connectors channel at the PUBLISHED uuids, so that the installed + * application's `connectors[]` bindings still resolve. + * + * A uuid that already exists is skipped, never overwritten — see the class + * docblock. + * + * @param array $connectors The channel (kind → name → blob). + * @param ChannelApplyReport $report The report to write into. + * + * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-existing-connector-is-skipped-and-never-overwritten + */ + private function applyConnectors(array $connectors, ChannelApplyReport $report): void + { + $declared = 0; + foreach (self::CONNECTOR_KINDS as $kind) { + $declared += count((array) ($connectors[$kind] ?? [])); + } + + $report->declareChannel(channel: 'connectors', declared: $declared); + + if ($declared === 0) { + return; + } + + if ($this->appManager->isEnabledForUser('openconnector') === false) { + $this->logger->info( + 'OpenBuild channel apply: openconnector is not enabled — skipping '.$declared.' declared connectors.' + ); + $report->skipChannel(channel: 'connectors', reason: self::REASON_NO_OPENCONNECTOR); + return; + } + + foreach (self::CONNECTOR_KINDS as $kind) { + $applied = 0; + foreach ((array) ($connectors[$kind] ?? []) as $name => $blob) { + $item = $kind.'/'.(string) $name; + + if ($applied >= self::MAX_CONNECTORS_PER_KIND) { + $this->logTruncation( + channel: 'connectors/'.$kind, + declared: count((array) ($connectors[$kind] ?? [])), + bound: self::MAX_CONNECTORS_PER_KIND + ); + $report->recordTruncated(channel: 'connectors', item: $item); + continue; + } + + $applied++; + $this->applyOneConnector(kind: $kind, item: $item, blob: (array) $blob, report: $report); + } + }//end foreach + + }//end applyConnectors() + + /** + * Apply a single connector at its published uuid. + * + * @param string $kind The connector kind (schema). + * @param string $item The report item identity. + * @param array $blob The published connector body. + * @param ChannelApplyReport $report The report to write into. + * + * @return void + */ + private function applyOneConnector(string $kind, string $item, array $blob, ChannelApplyReport $report): void + { + // The published body carries its identity in `id` — verified across all 42 + // connectors of a real published artefact. `uuid` is present but null, + // because the serializer emits ObjectEntity::getObject(), the body only. + $uuid = (string) ($blob['id'] ?? ''); + if ($uuid === '') { + $report->recordFailed(channel: 'connectors', item: $item, reason: 'no-identity-in-blob'); + return; + } + + try { + // The never-overwrite guarantee lives IN this call via failIfExists. A + // check-then-write would both race and drift. + $this->objectService->saveObject( + object: $blob, + register: self::CONNECTOR_REGISTER, + schema: $kind, + uuid: $uuid, + _rbac: false, + _multitenancy: false, + failIfExists: true + ); + + $report->recordCreated(channel: 'connectors', item: $item); + $this->collectCredentialRefs(blob: $blob, connector: $item, report: $report); + } catch (ObjectExistsException) { + // Collision detected BY TYPE, never by message text. An earlier draft + // matched on strings, which meant a plain PHP "Unknown named parameter + // $failIfExists" error was reported as a benign "already exists" — a + // wiring bug wearing the costume of a normal, expected outcome. + $report->recordSkipped( + channel: 'connectors', + item: $item, + reason: ChannelApplyReport::REASON_EXISTS + ); + } catch (Throwable $e) { + $this->logger->warning('OpenBuild channel apply: connector "'.$item.'" failed: '.$e->getMessage()); + $report->recordFailed(channel: 'connectors', item: $item, reason: $e->getMessage()); + }//end try + + }//end applyOneConnector() + + /** + * Collect credential references that do not resolve on this instance. + * + * Publishing blanks secrets but keeps `credentialRef`, so an applied connector + * can be perfectly installed and still unable to run. + * + * @param array $blob The connector body. + * @param string $connector The connector identity. + * @param ChannelApplyReport $report The report to write into. + * + * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-unresolvable-credential-references-are-reported + */ + private function collectCredentialRefs(array $blob, string $connector, ChannelApplyReport $report): void + { + foreach ($this->credentialNames(node: $blob) as $name) { + if ($this->credentialExists(name: $name) === false) { + $report->needsCredential(credential: $name, connector: $connector); + } + } + + }//end collectCredentialRefs() + + /** + * Walk a connector body collecting `credentialRef` names. + * + * @param mixed $node The node to walk. + * + * @return array The referenced credential names. + */ + private function credentialNames(mixed $node): array + { + if (is_array($node) === false) { + return []; + } + + $names = []; + foreach ($node as $key => $value) { + if ($key === 'credentialRef' && is_array($value) === true) { + $name = (string) ($value['credentialName'] ?? ''); + if ($name !== '') { + $names[] = $name; + } + + continue; + } + + $names = array_merge($names, $this->credentialNames(node: $value)); + } + + return array_values(array_unique($names)); + + }//end credentialNames() + + /** + * Whether a credential of this name resolves on the instance. + * + * Absence is the answer we act on, so a lookup FAILURE must not be reported as + * a confident "does not exist" — an unavailable broker would otherwise + * manufacture a list of missing credentials that are in fact present. + * + * @param string $name The credential name. + * + * @return bool True when it resolves, or when the lookup was inconclusive. + */ + private function credentialExists(string $name): bool + { + try { + // Broker credentials live in register `credential-broker`, schema + // `brokeredcredential` — verified against the live instance rather + // than assumed, because a lookup pointed at the wrong table returns + // "nothing matched", which is indistinguishable from a true absence + // and would manufacture a list of missing credentials that are in + // fact present. register/schema are FILTER keys on findAll(), not + // parameters of their own. + $found = $this->objectService->findAll( + config: [ + 'filters' => [ + 'register' => self::CREDENTIAL_REGISTER, + 'schema' => self::CREDENTIAL_SCHEMA, + 'name' => $name, + ], + ], + _rbac: false, + _multitenancy: false + ); + + return (is_array($found) === true && $found !== []); + } catch (Throwable $e) { + $this->logger->debug( + 'OpenBuild channel apply: credential lookup for "'.$name.'" was inconclusive: '.$e->getMessage() + ); + + return true; + }//end try + + }//end credentialExists() + + /** + * Apply the automations channel with the same create-or-skip rules. + * + * @param array $automations The channel (slug → blob). + * @param ChannelApplyReport $report The report to write into. + * + * @return void + */ + private function applyAutomations(array $automations, ChannelApplyReport $report): void + { + $report->declareChannel(channel: 'automations', declared: count($automations)); + + if ($automations === []) { + return; + } + + if ($this->appManager->isEnabledForUser('openconnector') === false) { + $report->skipChannel(channel: 'automations', reason: self::REASON_NO_OPENCONNECTOR); + return; + } + + $applied = 0; + foreach ($automations as $slug => $blob) { + $slug = (string) $slug; + if ($applied >= self::MAX_AUTOMATIONS) { + $this->logTruncation( + channel: 'automations', + declared: count($automations), + bound: self::MAX_AUTOMATIONS + ); + $report->recordTruncated(channel: 'automations', item: $slug); + continue; + } + + $applied++; + $this->applyOneConnector(kind: 'job', item: 'automations/'.$slug, blob: (array) $blob, report: $report); + } + + }//end applyAutomations() + + /** + * Delegate the skills channel to hermiq, which owns skill installation and + * fetches the bundle itself from the repo coordinates. + * + * OpenBuild deliberately parses no skill frontmatter and places no aux files: + * byte-fidelity and the ADR-068 §3 `learning-candidates.md` exclusion live in + * exactly one implementation. + * + * @param array $skills The channel (name → path → contents). + * @param string $owner Repo owner. + * @param string $repo Repo name. + * @param string|null $ref Optional git ref. + * @param string|null $actingUserId The session UID. + * @param string|null $credentialId Optional broker credential UUID. + * @param ChannelApplyReport $report The report to write into. + * + * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-skills-are-delegated-to-hermiq-by-repository-coordinates + */ + private function applySkills( + array $skills, + string $owner, + string $repo, + ?string $ref, + ?string $actingUserId, + ?string $credentialId, + ChannelApplyReport $report + ): void { + $report->declareChannel(channel: 'skills', declared: count($skills)); + + if ($skills === []) { + return; + } + + if ($owner === '' || $repo === '') { + // Hermiq fetches the bundle itself, so without coordinates there is + // nothing to delegate. Reported rather than treated as "no skills". + $report->skipChannel(channel: 'skills', reason: 'no-repo-coordinates'); + return; + } + + $installer = null; + if ($this->appManager->isEnabledForUser('hermiq') === true) { + $installer = $this->locator->get(className: self::HERMIQ_INSTALLER); + } + + if ($installer === null) { + $this->logger->info( + 'OpenBuild channel apply: hermiq is not available — skipping '.count($skills).' declared skills.' + ); + $report->skipChannel(channel: 'skills', reason: self::REASON_NO_HERMIQ); + return; + } + + try { + $result = $installer->installFromRepo( + owner: $owner, + repo: $repo, + ref: $ref, + actingUserId: $actingUserId, + credentialId: $credentialId + ); + + // Hermiq owns these numbers; they are carried through unmodified. + $report->adoptCounts( + channel: 'skills', + created: (int) ($result['installed'] ?? 0), + skipped: (int) ($result['skipped'] ?? 0), + failed: (int) ($result['failed'] ?? 0), + truncated: (int) ($result['truncated'] ?? 0) + ); + } catch (Throwable $e) { + $this->logger->warning('OpenBuild channel apply: hermiq skill install failed: '.$e->getMessage()); + $report->skipChannel(channel: 'skills', reason: 'hermiq-install-failed'); + }//end try + + }//end applySkills() + + /** + * Log that a channel bound was reached. Never silent: an install that quietly + * drops half an app is the precise failure this class exists to prevent. + * + * @param string $channel The channel name. + * @param int $declared How many items were declared. + * @param int $bound The configured maximum. + * + * @return void + */ + private function logTruncation(string $channel, int $declared, int $bound): void + { + $this->logger->warning( + 'OpenBuild channel apply: channel "'.$channel.'" declared '.$declared + .' items but the bound is '.$bound.' — the excess was NOT applied.' + ); + + }//end logTruncation() +}//end class diff --git a/lib/Service/ChannelApplyReport.php b/lib/Service/ChannelApplyReport.php new file mode 100644 index 000000000..34f751e3b --- /dev/null +++ b/lib/Service/ChannelApplyReport.php @@ -0,0 +1,328 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Service; + +use RuntimeException; + +/** + * Per-channel, per-item outcome record for a channel apply run. + */ +class ChannelApplyReport +{ + + /** + * Outcome for an item that was written to this instance. + * + * @var string + */ + public const OUTCOME_CREATED = 'created'; + + /** + * Outcome for an item deliberately left alone. + * + * @var string + */ + public const OUTCOME_SKIPPED = 'skipped'; + + /** + * Outcome for an item whose apply threw. + * + * @var string + */ + public const OUTCOME_FAILED = 'failed'; + + /** + * Reason recorded when an item already exists locally and is therefore not + * touched — connectors are shared infrastructure (see design.md). + * + * @var string + */ + public const REASON_EXISTS = 'already-exists'; + + /** + * Reason recorded for items dropped because a channel bound was hit. + * + * @var string + */ + public const REASON_TRUNCATED = 'channel-bound-exceeded'; + + /** + * Per-channel state, keyed by channel name. + * + * @var array> + */ + private array $channels = []; + + /** + * Credential references that do not resolve on this instance. + * + * @var array> + */ + private array $needsCredentials = []; + + /** + * Open a channel and fix the number of items it declared. + * + * The declared count is recorded even when the channel is subsequently + * skipped wholesale, because reporting `declared: 0` for a channel that in + * fact declared 94 skills is the same lie as dropping them silently. + * + * @param string $channel The channel name. + * @param int $declared How many items the template declared. + * + * @return void + */ + public function declareChannel(string $channel, int $declared): void + { + $this->channels[$channel] = [ + 'declared' => $declared, + 'created' => 0, + 'skipped' => 0, + 'failed' => 0, + 'truncated' => 0, + 'status' => 'applied', + 'reason' => null, + 'items' => [], + ]; + + }//end declareChannel() + + /** + * Record an item that was written. + * + * @param string $channel The channel name. + * @param string $item The item identity (e.g. `source/`). + * + * @return void + */ + public function recordCreated(string $channel, string $item): void + { + $this->record(channel: $channel, item: $item, outcome: self::OUTCOME_CREATED, reason: null); + + }//end recordCreated() + + /** + * Record an item that was deliberately left alone. + * + * @param string $channel The channel name. + * @param string $item The item identity. + * @param string $reason Why it was skipped. + * + * @return void + */ + public function recordSkipped(string $channel, string $item, string $reason): void + { + $this->record(channel: $channel, item: $item, outcome: self::OUTCOME_SKIPPED, reason: $reason); + + }//end recordSkipped() + + /** + * Record an item whose apply threw. The run continues — one bad item must not + * cost the caller the other items it asked for. + * + * @param string $channel The channel name. + * @param string $item The item identity. + * @param string $reason The failure reason (never a secret). + * + * @return void + */ + public function recordFailed(string $channel, string $item, string $reason): void + { + $this->record(channel: $channel, item: $item, outcome: self::OUTCOME_FAILED, reason: $reason); + + }//end recordFailed() + + /** + * Record an item dropped because the channel bound was reached. + * + * Counted as a skip so the balance identity still holds, and additionally + * counted as `truncated` so that hitting a bound is visible on its own. + * + * @param string $channel The channel name. + * @param string $item The item identity. + * + * @return void + */ + public function recordTruncated(string $channel, string $item): void + { + $this->record(channel: $channel, item: $item, outcome: self::OUTCOME_SKIPPED, reason: self::REASON_TRUNCATED); + $this->channels[$channel]['truncated']++; + + }//end recordTruncated() + + /** + * Mark a whole channel as not applied — typically because the app that owns + * it is not installed. Every declared item is recorded as skipped, so the + * caller can still see how much was declared and therefore what is missing. + * + * @param string $channel The channel name. + * @param string $reason Machine-readable reason (e.g. `hermiq-unavailable`). + * + * @return void + */ + public function skipChannel(string $channel, string $reason): void + { + if (isset($this->channels[$channel]) === false) { + $this->declareChannel(channel: $channel, declared: 0); + } + + $this->channels[$channel]['status'] = self::OUTCOME_SKIPPED; + $this->channels[$channel]['reason'] = $reason; + + $channelState = $this->channels[$channel]; + $outstanding = ($channelState['declared'] - $channelState['created'] + - $channelState['skipped'] - $channelState['failed']); + + if ($outstanding > 0) { + $this->channels[$channel]['skipped'] += $outstanding; + } + + }//end skipChannel() + + /** + * Adopt counts produced by another app (hermiq owns skill installation, so + * its numbers are carried through unmodified rather than recomputed here). + * + * @param string $channel The channel name. + * @param int $created Items hermiq installed. + * @param int $skipped Items hermiq skipped. + * @param int $failed Items hermiq failed. + * @param int $truncated Items hermiq dropped at its own bound. + * + * @return void + */ + public function adoptCounts(string $channel, int $created, int $skipped, int $failed, int $truncated): void + { + if (isset($this->channels[$channel]) === false) { + $this->declareChannel(channel: $channel, declared: ($created + $skipped + $failed)); + } + + $this->channels[$channel]['created'] = $created; + $this->channels[$channel]['skipped'] = $skipped; + $this->channels[$channel]['failed'] = $failed; + $this->channels[$channel]['truncated'] = $truncated; + + }//end adoptCounts() + + /** + * Note a credential reference that does not resolve on this instance. + * + * Publishing blanks secret values but keeps `credentialRef`, so a connector + * can install perfectly and still be unable to run. That gap is the + * difference between "installed" and "installed and runnable", and it has to + * be visible rather than discovered later at first execution. + * + * @param string $credential The referenced credential name. + * @param string $connector The connector that needs it. + * + * @return void + */ + public function needsCredential(string $credential, string $connector): void + { + if (isset($this->needsCredentials[$credential]) === false) { + $this->needsCredentials[$credential] = []; + } + + if (in_array($connector, $this->needsCredentials[$credential], true) === false) { + $this->needsCredentials[$credential][] = $connector; + } + }//end needsCredential() + + /** + * Render the report, asserting the balance identity for every channel. + * + * @return array The report. + * + * @throws RuntimeException When a channel does not balance — that means an + * item was dropped somewhere, which is exactly the + * class of defect this report exists to expose. + */ + public function toArray(): array + { + $out = []; + foreach ($this->channels as $name => $channel) { + $accounted = ($channel['created'] + $channel['skipped'] + $channel['failed']); + if ($accounted !== $channel['declared']) { + throw new RuntimeException( + 'OpenBuild channel apply: report for channel "'.$name.'" does not balance — declared ' + .$channel['declared'].' but accounted for '.$accounted + .' (created '.$channel['created'].', skipped '.$channel['skipped'] + .', failed '.$channel['failed'].'). An item was dropped.' + ); + } + + $out[$name] = $channel; + } + + ksort($this->needsCredentials); + + return [ + 'channels' => $out, + 'needsCredentials' => $this->needsCredentials, + ]; + + }//end toArray() + + /** + * Record one item outcome against a channel. + * + * @param string $channel The channel name. + * @param string $item The item identity. + * @param string $outcome One of the OUTCOME_* constants. + * @param string|null $reason Optional reason. + * + * @return void + */ + private function record(string $channel, string $item, string $outcome, ?string $reason): void + { + if (isset($this->channels[$channel]) === false) { + $this->declareChannel(channel: $channel, declared: 0); + } + + $this->channels[$channel][$outcome]++; + $this->channels[$channel]['items'][] = [ + 'item' => $item, + 'outcome' => $outcome, + 'reason' => $reason, + ]; + + }//end record() +}//end class diff --git a/lib/Service/ContainerLocator.php b/lib/Service/ContainerLocator.php new file mode 100644 index 000000000..54c69e2d5 --- /dev/null +++ b/lib/Service/ContainerLocator.php @@ -0,0 +1,94 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-absent-optional-dependency-degrades-with-a-stated-reason + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Service; + +use OCP\IServerContainer; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Lazily resolves optional cross-app services. + */ +class ContainerLocator +{ + /** + * Constructor. + * + * @param IServerContainer $container The server container. + * @param LoggerInterface $logger PSR logger. + * + * @return void + */ + public function __construct( + private readonly IServerContainer $container, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Resolve a service by class name. + * + * @param string $className Fully-qualified class name. + * + * @return object|null The service, or null when it cannot be resolved. + */ + public function get(string $className): ?object + { + $name = ltrim($className, '\\'); + if (class_exists($name) === false && interface_exists($name) === false) { + return null; + } + + try { + $service = $this->container->get($name); + } catch (Throwable $e) { + $this->logger->debug( + 'OpenBuild: optional service "'.$name.'" could not be resolved: '.$e->getMessage() + ); + + return null; + } + + if (is_object($service) === false) { + return null; + } + + return $service; + + }//end get() +}//end class diff --git a/lib/Service/DataRegisterProvisioner.php b/lib/Service/DataRegisterProvisioner.php new file mode 100644 index 000000000..8b5aa870d --- /dev/null +++ b/lib/Service/DataRegisterProvisioner.php @@ -0,0 +1,213 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-existing-register-or-schema-is-never-mutated + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Service; + +use OCA\OpenRegister\Db\RegisterMapper; +use OCA\OpenRegister\Db\SchemaMapper; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Creates the registers and schemas a published app declares. + */ +class DataRegisterProvisioner +{ + + /** + * The channel name used in the report. + * + * @var string + */ + private const CHANNEL = 'dataRegisters'; + + /** + * Maximum data registers applied from one repo. + * + * @var int + */ + private const MAX_REGISTERS = 64; + + + /** + * Constructor. + * + * @param RegisterMapper $registerMapper Register lookup and creation. + * @param SchemaMapper $schemaMapper Schema lookup and creation. + * @param LoggerInterface $logger PSR logger. + * + * @return void + */ + public function __construct( + private readonly RegisterMapper $registerMapper, + private readonly SchemaMapper $schemaMapper, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + + /** + * Apply the data-registers channel. + * + * @param array $registers The channel (slug → blob). + * @param ChannelApplyReport $report The report to write into. + * + * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-existing-register-or-schema-is-never-mutated + */ + public function apply(array $registers, ChannelApplyReport $report): void + { + $report->declareChannel(channel: self::CHANNEL, declared: count($registers)); + + $applied = 0; + foreach ($registers as $slug => $blob) { + $slug = (string) $slug; + if ($applied >= self::MAX_REGISTERS) { + $this->logger->warning( + 'OpenBuild channel apply: channel "'.self::CHANNEL.'" declared '.count($registers) + .' items but the bound is '.self::MAX_REGISTERS.' — the excess was NOT applied.' + ); + $report->recordTruncated(channel: self::CHANNEL, item: $slug); + continue; + } + + $applied++; + + try { + $this->applyOne(slug: $slug, blob: (array) $blob, report: $report); + } catch (Throwable $e) { + $this->logger->warning( + 'OpenBuild channel apply: data register "'.$slug.'" failed: '.$e->getMessage() + ); + $report->recordFailed(channel: self::CHANNEL, item: $slug, reason: $e->getMessage()); + } + }//end foreach + + }//end apply() + + + /** + * Create one register and its missing schemas, or skip an existing one. + * + * @param string $slug The register slug. + * @param array $blob The published register blob. + * @param ChannelApplyReport $report The report to write into. + * + * @return void + */ + private function applyOne(string $slug, array $blob, ChannelApplyReport $report): void + { + try { + $this->registerMapper->find($slug, _multitenancy: false); + $report->recordSkipped( + channel: self::CHANNEL, + item: $slug, + reason: ChannelApplyReport::REASON_EXISTS + ); + return; + } catch (Throwable) { + // Absent — create it below. + } + + $schemaIds = []; + foreach ((array) ($blob['schemas'] ?? []) as $schemaSlug => $definition) { + $schemaId = $this->findOrCreateSchema(slug: (string) $schemaSlug, definition: (array) $definition); + if ($schemaId !== null) { + $schemaIds[] = $schemaId; + } + } + + $this->registerMapper->createFromArray( + [ + 'slug' => $slug, + 'title' => (string) ($blob['title'] ?? $slug), + 'description' => 'Installed by OpenBuild from a published app repository.', + 'version' => '0.1.0', + 'schemas' => $schemaIds, + ] + ); + + $report->recordCreated(channel: self::CHANNEL, item: $slug); + + }//end applyOne() + + + /** + * Find a schema by slug, or create it from the published definition. + * + * @param string $slug The schema slug. + * @param array $definition The published schema definition. + * + * @return int|null The schema id, or null when it could not be provisioned. + */ + private function findOrCreateSchema(string $slug, array $definition): ?int + { + if ($slug === '') { + return null; + } + + try { + $existing = $this->schemaMapper->find($slug, _multitenancy: false); + return $existing->getId(); + } catch (Throwable) { + // Absent — create it below. + } + + try { + $created = $this->schemaMapper->createFromArray( + [ + 'slug' => $slug, + 'title' => (string) ($definition['title'] ?? $slug), + 'description' => (string) ($definition['description'] ?? ''), + 'version' => (string) ($definition['version'] ?? '0.1.0'), + 'required' => array_values((array) ($definition['required'] ?? [])), + 'properties' => (array) ($definition['properties'] ?? []), + ] + ); + + return $created->getId(); + } catch (Throwable $e) { + $this->logger->warning( + 'OpenBuild channel apply: schema "'.$slug.'" could not be created: '.$e->getMessage() + ); + return null; + } + + }//end findOrCreateSchema() + + +}//end class diff --git a/lib/Service/GitHubAppSyncService.php b/lib/Service/GitHubAppSyncService.php index f326b9c47..e4fbbddac 100644 --- a/lib/Service/GitHubAppSyncService.php +++ b/lib/Service/GitHubAppSyncService.php @@ -136,6 +136,7 @@ class GitHubAppSyncService * @param AppRepoSerializer $serializer Local → repo file map (change 1). * @param AppRepoParser $parser Repo file map → clone-seam array (change 1). * @param GitHubCatalogService $catalogService Repo fetch + commit-sha resolution (change 2). + * @param AppChannelApplier $channelApplier Applies the v2 repo channels (apply-v2-channels). * @param LoggerInterface $logger PSR logger (secret-free diagnostics only). * * @return void @@ -147,6 +148,7 @@ public function __construct( private readonly AppRepoSerializer $serializer, private readonly AppRepoParser $parser, private readonly GitHubCatalogService $catalogService, + private readonly AppChannelApplier $channelApplier, private readonly LoggerInterface $logger, ) { }//end __construct() @@ -512,6 +514,19 @@ public function pull( $versionData = $this->normalise(object: $created); $versionUuid = $this->uuidOf(object: $versionData); + // Apply the app-repo-format-v2 channels through the SAME applier the shop + // install path uses. Before this call, pull() persisted the manifest and + // discarded the registers, connectors, automations and skills the repo + // carried — a complete, silent failure of what the format is for. + $channels = $this->channelApplier->apply( + template: $template, + owner: $owner, + repo: $name, + ref: $ref, + actingUserId: $actingUserId, + credentialId: $credentialId + ); + return [ 'outcome' => self::OUTCOME_OK, 'versionUuid' => $versionUuid, @@ -520,6 +535,7 @@ public function pull( 'sourceRef' => $ref, 'status' => 'draft', 'register' => $registerSlug, + 'channels' => $channels, ]; }//end pull() diff --git a/openspec/changes/apply-v2-channels/.openspec.yaml b/openspec/changes/apply-v2-channels/.openspec.yaml new file mode 100644 index 000000000..5849c2dbf --- /dev/null +++ b/openspec/changes/apply-v2-channels/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-01 diff --git a/openspec/changes/apply-v2-channels/design.md b/openspec/changes/apply-v2-channels/design.md new file mode 100644 index 000000000..b33143df7 --- /dev/null +++ b/openspec/changes/apply-v2-channels/design.md @@ -0,0 +1,122 @@ +## Context + +App-repo format v2 grew four channels. Six steps carry an app from one instance to +another: **serialize → bind → push → fetch → parse → apply**. Five are built. The sixth +was never written, and because everything upstream of it goes green, nothing in the system +noticed. + +Existing machinery this design builds on, rather than reinvents: + +| existing | reused for | +|---|---| +| `GitHubAppSyncService::findOrCreateRegister()` | the create-if-absent register pattern | +| `reconcileCompanionSchemas()` | precedent that a pull provisions into a register without clobbering live data | +| `ObjectService::saveObject(uuid:, failIfExists:)` | UUID-preserving upsert with a built-in do-not-clobber flag | +| hermiq `POST /api/skills/bundle/install` | the entire skills channel — it takes owner/repo/ref and fetches itself | +| `AppRepoSerializer::CONNECTOR_KINDS` | `['source', 'mapping', 'synchronization', 'job']` | + +## Goals / Non-Goals + +**Goals:** +- Installing a published v2 app yields an app that runs, or states precisely why it cannot. +- One applier, called from both install seams, so the two cannot drift apart again. +- Every declared item is accounted for: `created + skipped + failed == declared`. +- Destructive outcomes are structurally impossible, not merely avoided by convention. + +**Non-Goals:** +- **Atomicity.** OpenRegister has no cross-object transaction. Faking a rollback would be + worse than a truthful partial report. +- **Updating existing objects.** This change only ever creates. Update/merge semantics for + a re-install need an ownership model that does not exist yet — see Deferred below. +- **Reimplementing skill installation.** hermiq owns it. +- **Making `openconnector` or `hermiq` hard dependencies.** + +## Decisions + +### Connector collisions skip, never overwrite + +A connector is shared infrastructure — one source can serve several applications. If +installing an app overwrote a colliding UUID, installing app B could silently rewrite a +source app A depends on, at a moment when the user believes they are only adding something. +That failure is invisible, arrives later, and is attributed to the wrong cause. + +A stale binding, by contrast, is inert and reported. **Skip-and-report is chosen because it +makes the destructive outcome unreachable**, not merely unlikely. + +Enforced with `saveObject(uuid: $uuid, failIfExists: true)` — the guarantee lives in the +call, not in a preceding existence check that a future edit could drift away from. A +check-then-write would also race; `failIfExists` will not. + +### Apply is best effort, and the report must balance + +`created + skipped + failed == declared` is asserted **in the applier itself**, not only in +tests. This workstream has already produced one silent cap (94 skills sent, 64 bundled, all +94 reported as published) and three false "X does not exist" claims. An arithmetic identity +that the code enforces makes a dropped item impossible to hide, whatever the cause. + +### Skills delegate by coordinates + +hermiq's `bundleInstall` takes owner/repo/ref and fetches for itself, so the applier passes +coordinates rather than 746 blobs. This keeps skill semantics — frontmatter byte-fidelity, +aux-file placement, the `learning-candidates.md` exclusion of ADR-068 §3 — in the one place +that implements them. + +The call crosses an app boundary, so it is made through the app framework rather than HTTP +where possible; if the route is used, its response is parsed for the counts hermiq reports +and those counts go into our report unmodified. + +### Optional dependencies degrade with a machine-readable reason + +Checked via `IAppManager::isEnabledForUser()`. A skipped channel keeps its **declared** +count, so a user can see that 94 skills were declared and 0 applied. Reporting `declared: 0` +because the handler is missing would be the same lie as the silent cap. + +### Bounds + +`MAX_REGISTERS = 64`, `MAX_CONNECTORS_PER_KIND = 2048`, `MAX_AUTOMATIONS = 512`. Truncation +is logged **and** carried in the report as a `truncated` count. hydra publishes 746 files, +so these are real limits, not theoretical ones. + +### Declarative-vs-imperative decision (ADR-031) + +| behaviour | path | rationale | +|---|---|---| +| Applying channels at install time | **imperative** | ADR-031 explicitly exempts external integration. This is orchestration across an app boundary (GitHub, `openconnector`, hermiq) at a discrete moment, not derived state over OpenRegister objects. There is no field to calculate and no lifecycle to declare — a declarative expression cannot create a register or call another app. | + +No lifecycle, aggregation, calculation, notification, relation or widget behaviour is +introduced, so no `x-openregister-*` schema-register patch applies to this change. + +### Seed Data (ADR-001) + +**Not applicable — this change defines no OpenRegister schema.** It applies schemas and +objects that a *published app repository* carries; the seed material is the published +artifact itself. Two real artifacts serve as the fixtures for verification: + +| artifact | contents | +|---|---| +| `ConductionNL/buildiq-spectr` (private) | 46 blobs — 1 data register, 4 connector kinds, 42 declared / 0 missing | +| `ConductionNL/buildiq-hydra` (private) | 748 blobs — 1 data register, 94 skills | + +Unit fixtures use the nil UUID `00000000-0000-0000-0000-000000000000` and placeholder +credential names, never realistic-looking values. + +## Risks / Trade-offs + +- **Partial application leaves a half-installed app.** Accepted and made visible rather + than hidden. The alternative — a compensating delete pass — would mean this change + deletes objects, and a bug in that pass destroys user data. Not worth it. +- **A skipped connector leaves the app bound to something stale or absent.** Reported, not + silently tolerated; `needsCredentials` and the skip reasons tell the user what to fix. +- **Re-install does not update.** A second install of a newer version applies nothing new + for already-present UUIDs. This is the honest consequence of never overwriting, and it is + deferred rather than guessed at. +- **Unit tests will pass while the feature is broken.** They have at every prior stage of + this workstream. The acceptance evidence is therefore a **live install of both published + artifacts**, with counts compared against the published repositories rather than against + the applier's own report. + +## Deferred + +- **Update/merge on re-install**, which needs an ownership model (`which app owns this + connector`) that does not exist today. Until it does, skip-and-report is the only + non-destructive answer. diff --git a/openspec/changes/apply-v2-channels/proposal.md b/openspec/changes/apply-v2-channels/proposal.md new file mode 100644 index 000000000..23a705948 --- /dev/null +++ b/openspec/changes/apply-v2-channels/proposal.md @@ -0,0 +1,100 @@ +--- +kind: code +--- + +## Why + +App-repo **format v2** added four channels to a published OpenBuild app — +`data-registers/`, `connectors/`, `automations/` and `skills/` — so that installing an +app yields something that actually runs, not just a manifest. + +The serializer emits all four. The parser reads all four. As of openbuild#80 the fetcher +fetches all four. **No install path applies any of them.** They are parsed into the +template array and dropped on the floor. + +Verified against the code, with a positive control: + +| lookup | hits outside parser / serializer / fetcher | +|---|---| +| `$template['manifest']`, `$template['version']` *(control)* | 7 | +| `$template['connectors']` | **0** | +| `$template['automations']` | **0** | +| `$template['skills']` | **0** | +| `$template['dataRegisters']` | 3 — all in the **export/zip** path, reading `Application.dataRegisters` bindings, never a parsed template | + +Both entry points confirmed by reading their bodies, not by grep alone: + +- `GitHubAppSyncService::pull()` persists a draft Version holding `manifest` + companion + schemas only. +- `ApplicationsController::installFromTemplateArray()` — the target of + `ShopController::githubInstall()` — reads exactly `$template['slug']` and + `$template['manifest']`. + +The user-visible consequence: installing `buildiq-spectr` or `buildiq-hydra` today +produces an app with its manifest and **nothing that makes it run** — no registers, no +connectors, none of the 94 skills — and **reports success**. A silent, complete failure +of the feature the format exists to deliver. + +This is the fourth time in this workstream that one half of a round trip was extended +and the other left behind (serializer without binding; publish without install-side aux +files; parser without fetcher; now fetch+parse without apply). Publish looked correct +every time, because publish is the half that kept getting extended. This change closes +the last one. + +## What Changes + +A new `AppChannelApplier` service applies the four channels, called from the **single +seam** that both install paths already funnel through — so `pull()` and `githubInstall()` +cannot drift apart again. + +- **Data registers** — create registers and schemas that do not exist; **never mutate** + ones that do. +- **Connectors** — upsert by the **published UUID** (`saveObject(uuid:)`) so that the + `Application.connectors[]` bindings still resolve after install. A UUID that already + exists locally is **skipped and reported, never overwritten**: connectors are shared + infrastructure, and installing an app must not silently rewrite a source another app + depends on. +- **Skills** — delegated to hermiq's existing `POST /api/skills/bundle/install` by repo + **coordinates** (owner/repo/ref), which hermiq fetches itself. OpenBuild does not + reimplement skill installation. +- **Automations** — same shape as connectors. +- **Credential reporting** — `stripSecrets()` blanks secrets at publish time while keeping + `credentialRef`. Every `credentialRef` that does not resolve on the target instance is + collected into `needsCredentials[]` and surfaced in the install response. This is the + difference between *installed* and *installed and runnable*, and it must be visible. + +Applying is **best-effort with a complete per-item outcome report**. OpenRegister offers +no cross-object transaction, so atomicity cannot be delivered and will not be faked; a +partial apply reports exactly which items landed, which were skipped, and why. + +Every channel is **bounded**, and truncation is **logged and reported** — never silent. +An install that quietly drops half an app is precisely the failure this change exists to +prevent. + +OpenBuild declares only `openregister` as a dependency. `openconnector` and `hermiq` +are therefore optional: when absent, the dependent channel is skipped with a machine- +readable reason, and the install still succeeds for the channels that can be applied. + +## Capabilities + +### New Capabilities +- `app-channel-application`: applying a parsed v2 app repo's data-register, connector, + automation and skill channels onto the target instance, with per-item outcomes, + skip-never-overwrite collision handling, optional-dependency degradation and explicit + bounds. + +### Modified Capabilities +- `openbuild-application-register`: installing an application from a linked GitHub repo + now provisions its bound registers and connectors, rather than only its manifest. + +## Impact + +- **New**: `lib/Service/AppChannelApplier.php`, `lib/Service/ChannelApplyReport.php` +- **Modified**: `lib/Service/GitHubAppSyncService.php` (call the applier in `pull()`), + `lib/Controller/ApplicationsController.php` (call it in `installFromTemplateArray()`), + `lib/Controller/ShopController.php` (surface the report) +- **Optional runtime dependencies**: `openconnector` (connectors, automations), `hermiq` + (skills) — both degrade with a reason, neither becomes a hard dependency +- **API**: install/pull responses gain a `channels` report object (additive) +- **Data**: creates OpenRegister registers, schemas and `openconnector` objects on the + target instance; never updates or deletes an existing one diff --git a/openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md b/openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md new file mode 100644 index 000000000..4ab484aed --- /dev/null +++ b/openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md @@ -0,0 +1,144 @@ +## ADDED Requirements + +### Requirement: Every install path applies the v2 channels + +A parsed v2 app repo template carries four channels — `dataRegisters`, `connectors`, +`automations` and `skills`. Every code path that installs or pulls an app SHALL pass the +parsed template through the channel applier. No install path may read only the manifest. + +Both paths SHALL call the same applier, so that a future channel cannot be wired into one +path and forgotten in the other. + +#### Scenario: Pulling a v2 repo applies its channels +- **WHEN** `GitHubAppSyncService::pull()` parses a repo whose template declares a data + register and four connector kinds +- **THEN** the bound register and every declared connector are applied to the instance +- **AND** the returned result carries a `channels` report describing each one + +#### Scenario: Installing from the shop applies its channels +- **WHEN** `ShopController::githubInstall()` installs the same repo +- **THEN** the same channels are applied through the same applier +- **AND** the response carries the same `channels` report structure + +#### Scenario: A v1 repo installs unchanged +- **WHEN** a template declares no channels at all +- **THEN** the install succeeds exactly as before +- **AND** the `channels` report records zero declared items for every channel + +### Requirement: Connector identity is preserved on apply + +A published connector carries the UUID it had on the source instance, and the source +application binds to it by that UUID through `Application.connectors[]`. The applier SHALL +write each connector at its published UUID so that those bindings resolve after install. + +An applier that let OpenRegister assign a fresh UUID would break every binding while +reporting success. + +#### Scenario: A connector lands at its published UUID +- **WHEN** a connector declared with UUID `00000000-0000-0000-0000-000000000000` is applied + to an instance where that UUID does not exist +- **THEN** the object is created with that exact UUID +- **AND** the application binding for that UUID resolves to it + +### Requirement: An existing connector is skipped and never overwritten + +Connectors are shared infrastructure: one source may serve several applications. Installing +an application SHALL NOT modify a connector that already exists on the target instance. + +When a declared connector UUID already exists, the applier SHALL leave the existing object +untouched and record the item as skipped with reason `already-exists`. + +#### Scenario: A colliding connector UUID is left alone +- **WHEN** a declared connector UUID already exists locally with different content +- **THEN** the existing object is not modified in any field +- **AND** the report records that item as skipped with reason `already-exists` +- **AND** the install still succeeds + +### Requirement: An existing register or schema is never mutated + +The applier SHALL create registers and schemas that do not exist, and SHALL leave existing +ones untouched, recording them as skipped. Applying an app must never rewrite the shape of +data that is already on the instance. + +#### Scenario: An existing register is not reshaped +- **WHEN** a declared data register slug already exists locally +- **THEN** the existing register and its schemas are unchanged +- **AND** the report records the register as skipped with reason `already-exists` + +### Requirement: Skills are delegated to hermiq by repository coordinates + +hermiq owns skill installation and already exposes `POST /api/skills/bundle/install`, +which takes owner, repo and ref and performs its own fetch. The applier SHALL delegate the +skills channel to hermiq by passing those coordinates, and SHALL NOT reimplement skill +parsing, frontmatter handling or aux-file placement. + +#### Scenario: The skills channel is delegated, not reimplemented +- **WHEN** a template declares 94 skills and hermiq is installed and enabled +- **THEN** the applier invokes hermiq bundle install with the repo owner, name and ref +- **AND** the report carries the installed and skipped counts hermiq returned + +### Requirement: An absent optional dependency degrades with a stated reason + +OpenBuild depends only on `openregister`. `openconnector` and `hermiq` are optional. When a +channel requires an app that is not installed or not enabled, the applier SHALL skip that +channel with a machine-readable reason and SHALL allow the remaining channels to apply. + +A skipped channel SHALL never be reported as applied, and SHALL never be reported as zero +declared items when items were in fact declared. + +#### Scenario: Connectors are skipped when openconnector is absent +- **WHEN** a template declares connectors and `openconnector` is not enabled +- **THEN** the connectors channel reports `skipped` with reason `openconnector-unavailable` +- **AND** the declared count still reflects the number of connectors in the template +- **AND** the data-registers channel is still applied + +#### Scenario: Skills are skipped when hermiq is absent +- **WHEN** a template declares 94 skills and `hermiq` is not enabled +- **THEN** the skills channel reports `skipped` with reason `hermiq-unavailable` and a + declared count of 94 + +### Requirement: Application is best effort with a complete per-item outcome report + +OpenRegister provides no cross-object transaction, so the applier SHALL NOT claim +atomicity. A failure applying one item SHALL NOT abort the remaining items. + +Every declared item SHALL appear in the report with exactly one outcome — `created`, +`skipped` or `failed` — and a skipped or failed item SHALL carry a reason. The counts in +the report SHALL satisfy `created + skipped + failed == declared` for every channel, so +that a dropped item is arithmetically impossible to hide. + +#### Scenario: One failing connector does not abort the rest +- **WHEN** applying five connectors and the third one throws +- **THEN** the other four are still applied +- **AND** the report records the third as failed with its reason +- **AND** `created + skipped + failed` equals five + +### Requirement: Every channel is bounded and truncation is reported + +Each channel SHALL enforce an explicit maximum item count. When a channel exceeds its +bound, the applier SHALL log the truncation and SHALL record it in the report as a +`truncated` count. + +A bound that silently drops items would reproduce the exact silent-cap defect this +programme has already hit once. + +#### Scenario: Exceeding a channel bound is reported, not silent +- **WHEN** a channel declares more items than its configured maximum +- **THEN** the excess items are not applied +- **AND** the report records a non-zero `truncated` count for that channel +- **AND** the truncation is written to the log with the channel name and both counts + +### Requirement: Unresolvable credential references are reported + +Publishing blanks secret values while preserving `credentialRef`. An applied connector +whose `credentialRef` does not resolve on the target instance is installed but cannot run. + +The applier SHALL collect every unresolvable credential reference into a +`needsCredentials` list in the report, naming the referenced credential and the connector +that needs it. + +#### Scenario: A missing credential is surfaced, not swallowed +- **WHEN** an applied connector references credential `doffin` and no such credential + exists on the target instance +- **THEN** the report lists `doffin` under `needsCredentials` together with that connector +- **AND** the install still reports the connector as created diff --git a/openspec/changes/apply-v2-channels/specs/openbuild-application-register/spec.md b/openspec/changes/apply-v2-channels/specs/openbuild-application-register/spec.md new file mode 100644 index 000000000..40a5345cb --- /dev/null +++ b/openspec/changes/apply-v2-channels/specs/openbuild-application-register/spec.md @@ -0,0 +1,23 @@ +## ADDED Requirements + +### Requirement: Installing a linked app provisions its bound infrastructure + +Installing an application from a linked GitHub repository SHALL provision the registers and +connectors the application is bound to, not only its manifest. An installed application +whose bindings resolve to nothing is not installed. + +The install response SHALL carry a `channels` report so that the caller can distinguish an +application that is installed and runnable from one that is installed but missing the +infrastructure or credentials it needs. + +#### Scenario: An installed application resolves its bindings +- **WHEN** an application declaring one bound data register and four connector kinds is + installed from its repository onto a clean instance +- **THEN** the bound register exists on the instance +- **AND** every declared connector resolves at its published UUID +- **AND** the response `channels` report accounts for every declared item + +#### Scenario: A partially installable application says so +- **WHEN** the same application is installed onto an instance without `openconnector` +- **THEN** the response reports the connectors channel as skipped with its reason +- **AND** the caller can tell from the response that the application is not yet runnable diff --git a/openspec/changes/apply-v2-channels/tasks.md b/openspec/changes/apply-v2-channels/tasks.md new file mode 100644 index 000000000..b467b40d9 --- /dev/null +++ b/openspec/changes/apply-v2-channels/tasks.md @@ -0,0 +1,76 @@ +## Tasks + +### 1. Report structure + +- [ ] Add `lib/Service/ChannelApplyReport.php` — per-channel counts (`declared`, `created`, `skipped`, `failed`, `truncated`), per-item outcomes with reasons, and a `needsCredentials` list +- [ ] Enforce the balance identity `created + skipped + failed == declared` inside the report itself, throwing when it does not hold + +Acceptance criteria +- A dropped item cannot be represented — constructing an unbalanced report fails +- Every skipped or failed item carries a reason string + +### 2. Applier skeleton and both seams + +- [ ] Add `lib/Service/AppChannelApplier.php` with one entry point taking the parsed template plus repo coordinates +- [ ] Call the applier from `GitHubAppSyncService::pull()` after the draft Version is saved, and surface the report in its return array +- [ ] Call the applier from `ApplicationsController::installFromTemplateArray()`, and surface the report through `ShopController::githubInstall()` +- [ ] Detect optional apps via `IAppManager` and skip the dependent channel with `openconnector-unavailable` / `hermiq-unavailable`, preserving the declared count + +Acceptance criteria +- A v1 template installs exactly as before, with a zero-declared report +- No install path reads the template without passing it through the applier + +### 3. Data registers + +- [ ] Apply the `dataRegisters` channel: create absent registers and schemas, skip existing ones as `already-exists`, never mutate +- [ ] Bound at `MAX_REGISTERS = 64` with truncation logged and counted + +Acceptance criteria +- An existing register keeps its schemas and shape untouched +- A truncated channel reports a non-zero `truncated` count + +### 4. Connectors + +- [ ] Apply the four `CONNECTOR_KINDS` via `saveObject(uuid: , failIfExists: true)` so bindings resolve and a collision cannot overwrite +- [ ] Record a collision as skipped with reason `already-exists`, and continue with the remaining connectors +- [ ] Collect every unresolvable `credentialRef` into `needsCredentials`, naming the credential and the connector that needs it +- [ ] Bound at `MAX_CONNECTORS_PER_KIND = 2048` with truncation logged and counted + +Acceptance criteria +- The existing object is byte-identical after a colliding apply +- One throwing connector does not abort the others + +### 5. Skills and automations + +- [ ] Delegate the `skills` channel to hermiq bundle install by owner/repo/ref, carrying hermiq's installed and skipped counts into the report unmodified +- [ ] Apply the `automations` channel with the same create-or-skip rules, bounded at `MAX_AUTOMATIONS = 512` + +Acceptance criteria +- OpenBuild parses no skill frontmatter and places no aux files itself +- A hermiq failure is reported as a failed channel, never as success + +### 6. Tests + +- [ ] Unit-test the balance identity, collision skip, optional-dependency degradation, truncation reporting and credential collection, then mutation-check the collision test by removing `failIfExists` and confirming it fails + +Acceptance criteria +- Every new test is proven capable of failing before it is trusted +- The collision test is confirmed red without `failIfExists`, not merely green with it + +### 7. Live verification (the acceptance evidence) + +- [ ] Install `ConductionNL/buildiq-spectr` onto a clean instance and assert 1 data register and 4 connector kinds land, counts compared against the published repository rather than the report +- [ ] Install `ConductionNL/buildiq-hydra` and assert 94 skills land, again compared against the published repository +- [ ] Verify a second install of the same repo skips every already-present item and reports it, changing nothing + +Acceptance criteria +- Counts are read from the published artifact, never from the applier's own output +- Both repositories stay private throughout + +### 8. Quality + +- [ ] Run phpstan explicitly with `vendor/` installed — the local gate suite silently skips it when `vendor/` is absent, so gate-green does not imply phpstan-green +- [ ] Run `composer check:strict` and the hydra gates, and open the PR + +Acceptance criteria +- phpstan is confirmed to have actually run, not merely to have not failed diff --git a/openspec/specs/openbuild-application-register/spec.md b/openspec/specs/openbuild-application-register/spec.md index 27e9111c7..a03f578f4 100644 --- a/openspec/specs/openbuild-application-register/spec.md +++ b/openspec/specs/openbuild-application-register/spec.md @@ -17,6 +17,7 @@ scoping via OR's standard `organisation` field (ADR-022). Lifecycle relocates to **OpenSpec changes**: [unify-apps-with-app-type](../../changes/archive/2026-06-20-unify-apps-with-app-type/) _(archived 2026-06-20)_ [data-registers-schema-declaration](../../changes/data-registers-schema-declaration/) [github-app-repo-format](../../changes/github-app-repo-format/) +[apply-v2-channels](../../changes/apply-v2-channels/) **Status**: in-progress diff --git a/tests/Unit/Controller/CreateFromTemplateTest.php b/tests/Unit/Controller/CreateFromTemplateTest.php index b573d6591..b1a23bf7e 100644 --- a/tests/Unit/Controller/CreateFromTemplateTest.php +++ b/tests/Unit/Controller/CreateFromTemplateTest.php @@ -31,6 +31,7 @@ namespace OCA\OpenBuild\Tests\Unit\Controller; use OCA\OpenBuild\Controller\ApplicationsController; +use OCA\OpenBuild\Service\AppChannelApplier; use OCA\OpenBuild\Service\ManifestResolverService; use OCA\OpenBuild\Service\PermissionResolver; use OCA\OpenRegister\Db\ObjectEntity; @@ -119,6 +120,13 @@ class CreateFromTemplateTest extends TestCase */ private ManifestResolverService&MockObject $manifestResolver; + /** + * The v2 channel applier (apply-v2-channels). + * + * @var AppChannelApplier&MockObject + */ + private AppChannelApplier&MockObject $channelApplier; + /** * Per-app Register entity stub. * @@ -147,6 +155,7 @@ protected function setUp(): void $this->userSession = $this->createMock(IUserSession::class); $this->groupManager = $this->createMock(IGroupManager::class); $this->manifestResolver = $this->createMock(ManifestResolverService::class); + $this->channelApplier = $this->createMock(AppChannelApplier::class); $this->objectService = $this->createMock(ObjectService::class); @@ -218,6 +227,7 @@ function (...$args) use ($applicationTemplateSchema, $applicationSchema): Schema groupManager: $this->groupManager, manifestResolver: $this->manifestResolver, permissionResolver: $permissionResolver, + channelApplier: $this->channelApplier, auditTrailMapper: null, ); }//end setUp() diff --git a/tests/Unit/Service/AppChannelApplierTest.php b/tests/Unit/Service/AppChannelApplierTest.php new file mode 100644 index 000000000..a339f76c4 --- /dev/null +++ b/tests/Unit/Service/AppChannelApplierTest.php @@ -0,0 +1,364 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Tests\Unit\Service; + +use OCA\OpenBuild\Service\AppChannelApplier; +use OCA\OpenBuild\Service\ChannelApplyReport; +use OCA\OpenBuild\Service\ContainerLocator; +use OCA\OpenBuild\Service\DataRegisterProvisioner; +use OCA\OpenRegister\Db\RegisterMapper; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Exception\ObjectExistsException; +use OCA\OpenRegister\Service\ObjectService; +use OCP\App\IAppManager; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * Tests for the v2 channel applier. + */ +class AppChannelApplierTest extends TestCase +{ + + /** + * The nil UUID — an obvious placeholder, never mistakable for a real id. + * + * @var string + */ + private const NIL_UUID = '00000000-0000-0000-0000-000000000000'; + + /** + * OR object read/write double. + * + * @var ObjectService&MockObject + */ + private ObjectService&MockObject $objectService; + + /** + * Register mapper double. + * + * @var RegisterMapper&MockObject + */ + private RegisterMapper&MockObject $registerMapper; + + /** + * Schema mapper double. + * + * @var SchemaMapper&MockObject + */ + private SchemaMapper&MockObject $schemaMapper; + + /** + * App manager double (optional-dependency detection). + * + * @var IAppManager&MockObject + */ + private IAppManager&MockObject $appManager; + + /** + * Optional cross-app service locator double. + * + * @var ContainerLocator&MockObject + */ + private ContainerLocator&MockObject $locator; + + /** + * Build the collaborators. + * + * @return void + */ + protected function setUp(): void + { + $this->objectService = $this->createMock(ObjectService::class); + $this->registerMapper = $this->createMock(RegisterMapper::class); + $this->schemaMapper = $this->createMock(SchemaMapper::class); + $this->appManager = $this->createMock(IAppManager::class); + $this->locator = $this->createMock(ContainerLocator::class); + + }//end setUp() + + /** + * Build the applier under test. + * + * @return AppChannelApplier + */ + private function applier(): AppChannelApplier + { + return new AppChannelApplier( + $this->objectService, + // A REAL provisioner over mocked mappers: its declareChannel call is + // what keeps the dataRegisters channel present in every report, so a + // mock here would quietly remove an assertion this file depends on. + new DataRegisterProvisioner( + $this->registerMapper, + $this->schemaMapper, + $this->createMock(LoggerInterface::class) + ), + $this->appManager, + $this->locator, + $this->createMock(LoggerInterface::class) + ); + + }//end applier() + + /** + * A template declaring one connector of the given kind. + * + * @param string $kind The connector kind. + * @param string $uuid The connector uuid. + * + * @return array + */ + private function templateWithConnector(string $kind, string $uuid): array + { + return [ + 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], + 'connectors' => [$kind => ['example' => ['id' => $uuid, 'name' => 'Example']]], + ]; + }//end templateWithConnector() + + /** + * A v1 template installs unchanged and reports zero declared everywhere. + * + * @return void + */ + public function testV1TemplateAppliesNothingAndBalances(): void + { + $this->objectService->expects(self::never())->method('saveObject'); + + $report = $this->applier()->apply(template: ['manifest' => []]); + + foreach (['dataRegisters', 'connectors', 'automations', 'skills'] as $channel) { + self::assertSame(0, $report['channels'][$channel]['declared']); + } + + }//end testV1TemplateAppliesNothingAndBalances() + + /** + * A connector is written at its PUBLISHED uuid, and with failIfExists set. + * + * The `failIfExists: true` assertion is deliberate: that argument is the + * never-overwrite guarantee, so removing it must turn this test red. + * + * @return void + */ + public function testConnectorIsWrittenAtItsPublishedUuidAndNeverOverwrites(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(true); + + $this->objectService->expects(self::once()) + ->method('saveObject') + ->with( + self::anything(), + self::anything(), + 'openconnector', + 'source', + self::NIL_UUID, + false, + false, + false, + null, + null, + true + ); + + $report = $this->applier()->apply( + template: $this->templateWithConnector(kind: 'source', uuid: self::NIL_UUID) + ); + + self::assertSame(1, $report['channels']['connectors']['created']); + + }//end testConnectorIsWrittenAtItsPublishedUuidAndNeverOverwrites() + + /** + * A colliding uuid is reported as skipped, and the run still succeeds. + * + * @return void + */ + public function testCollidingConnectorIsSkippedNotOverwritten(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(true); + // The TYPED exception OpenRegister actually raises for an insert-only + // conflict. Asserting on this type is what stops an unrelated error from + // being misread as a benign collision. + $this->objectService->method('saveObject') + ->willThrowException(new ObjectExistsException('taken')); + + $report = $this->applier()->apply( + template: $this->templateWithConnector(kind: 'source', uuid: self::NIL_UUID) + ); + + self::assertSame(0, $report['channels']['connectors']['created']); + self::assertSame(1, $report['channels']['connectors']['skipped']); + self::assertSame( + ChannelApplyReport::REASON_EXISTS, + $report['channels']['connectors']['items'][0]['reason'] + ); + + }//end testCollidingConnectorIsSkippedNotOverwritten() + + /** + * A genuine failure is recorded as failed, not silently swallowed and not + * mistaken for a collision. + * + * @return void + */ + public function testGenuineFailureIsRecordedAsFailed(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(true); + $this->objectService->method('saveObject') + ->willThrowException(new RuntimeException('database is on fire')); + + $report = $this->applier()->apply( + template: $this->templateWithConnector(kind: 'source', uuid: self::NIL_UUID) + ); + + self::assertSame(1, $report['channels']['connectors']['failed']); + self::assertSame('failed', $report['channels']['connectors']['items'][0]['outcome']); + + }//end testGenuineFailureIsRecordedAsFailed() + + /** + * Connectors degrade when openconnector is absent — declared count preserved. + * + * @return void + */ + public function testConnectorsDegradeWhenOpenConnectorIsAbsent(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(false); + $this->objectService->expects(self::never())->method('saveObject'); + + $report = $this->applier()->apply( + template: $this->templateWithConnector(kind: 'source', uuid: self::NIL_UUID) + ); + + self::assertSame(1, $report['channels']['connectors']['declared']); + self::assertSame(1, $report['channels']['connectors']['skipped']); + self::assertSame('openconnector-unavailable', $report['channels']['connectors']['reason']); + + }//end testConnectorsDegradeWhenOpenConnectorIsAbsent() + + /** + * Skills degrade when hermiq is absent, keeping the declared count so the + * caller can see that 2 skills were declared and none installed. + * + * @return void + */ + public function testSkillsDegradeWhenHermiqIsAbsent(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(false); + $this->locator->method('get')->willReturn(null); + + $report = $this->applier()->apply( + template: [ + 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], + 'skills' => [ + 'alpha' => ['SKILL.md' => '# alpha'], + 'beta' => ['SKILL.md' => '# beta'], + ], + ] + ); + + self::assertSame(2, $report['channels']['skills']['declared']); + self::assertSame(2, $report['channels']['skills']['skipped']); + self::assertSame('hermiq-unavailable', $report['channels']['skills']['reason']); + + }//end testSkillsDegradeWhenHermiqIsAbsent() + + /** + * An unresolvable credentialRef is surfaced, so "installed" is not confused + * with "runnable". + * + * @return void + */ + public function testUnresolvableCredentialIsReported(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(true); + $this->objectService->method('findAll')->willReturn([]); + + $report = $this->applier()->apply( + template: [ + 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], + 'connectors' => [ + 'source' => [ + 'example' => [ + 'id' => self::NIL_UUID, + 'configuration' => [ + 'authentication' => [ + 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + ], + ], + ], + ], + ], + ] + ); + + self::assertArrayHasKey('PLACEHOLDER_CREDENTIAL', $report['needsCredentials']); + self::assertSame(['source/example'], $report['needsCredentials']['PLACEHOLDER_CREDENTIAL']); + + }//end testUnresolvableCredentialIsReported() + + /** + * An inconclusive credential lookup must NOT be reported as "missing" — an + * absence claim manufactured by a failing lookup is worse than no claim. + * + * @return void + */ + public function testInconclusiveCredentialLookupIsNotReportedAsMissing(): void + { + $this->appManager->method('isEnabledForUser')->willReturn(true); + $this->objectService->method('findAll') + ->willThrowException(new RuntimeException('broker unavailable')); + + $report = $this->applier()->apply( + template: [ + 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], + 'connectors' => [ + 'source' => [ + 'example' => [ + 'id' => self::NIL_UUID, + 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + ], + ], + ], + ] + ); + + self::assertSame([], $report['needsCredentials']); + + }//end testInconclusiveCredentialLookupIsNotReportedAsMissing() +}//end class diff --git a/tests/Unit/Service/ChannelApplyReportTest.php b/tests/Unit/Service/ChannelApplyReportTest.php new file mode 100644 index 000000000..de14c23d1 --- /dev/null +++ b/tests/Unit/Service/ChannelApplyReportTest.php @@ -0,0 +1,148 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenBuild\Tests\Unit\Service; + +use OCA\OpenBuild\Service\ChannelApplyReport; +use PHPUnit\Framework\TestCase; +use RuntimeException; + +/** + * Tests for the channel apply report. + */ +class ChannelApplyReportTest extends TestCase +{ + /** + * A balanced channel renders. + * + * @return void + */ + public function testBalancedChannelRenders(): void + { + $report = new ChannelApplyReport(); + $report->declareChannel(channel: 'connectors', declared: 3); + $report->recordCreated(channel: 'connectors', item: 'source/a'); + $report->recordSkipped(channel: 'connectors', item: 'source/b', reason: ChannelApplyReport::REASON_EXISTS); + $report->recordFailed(channel: 'connectors', item: 'source/c', reason: 'boom'); + + $out = $report->toArray(); + + self::assertSame(3, $out['channels']['connectors']['declared']); + self::assertSame(1, $out['channels']['connectors']['created']); + self::assertSame(1, $out['channels']['connectors']['skipped']); + self::assertSame(1, $out['channels']['connectors']['failed']); + + }//end testBalancedChannelRenders() + + /** + * An item that was declared but never accounted for MUST throw rather than + * render a plausible-looking report. This is the whole point of the class. + * + * @return void + */ + public function testDroppedItemThrowsRatherThanRendering(): void + { + $report = new ChannelApplyReport(); + $report->declareChannel(channel: 'connectors', declared: 2); + $report->recordCreated(channel: 'connectors', item: 'source/a'); + // 'source/b' is never recorded — exactly what a silent drop looks like. + $this->expectException(RuntimeException::class); + $this->expectExceptionMessageMatches('/does not balance/'); + + $report->toArray(); + + }//end testDroppedItemThrowsRatherThanRendering() + + /** + * Skipping a whole channel keeps the declared count. Reporting `declared: 0` + * because the handler was missing is the same lie as dropping the items. + * + * @return void + */ + public function testSkippedChannelKeepsItsDeclaredCount(): void + { + $report = new ChannelApplyReport(); + $report->declareChannel(channel: 'skills', declared: 94); + $report->skipChannel(channel: 'skills', reason: 'hermiq-unavailable'); + + $out = $report->toArray(); + + self::assertSame(94, $out['channels']['skills']['declared']); + self::assertSame(94, $out['channels']['skills']['skipped']); + self::assertSame(0, $out['channels']['skills']['created']); + self::assertSame('skipped', $out['channels']['skills']['status']); + self::assertSame('hermiq-unavailable', $out['channels']['skills']['reason']); + + }//end testSkippedChannelKeepsItsDeclaredCount() + + /** + * Truncated items stay inside the balance identity AND are separately + * visible, so hitting a bound can never read as "there was nothing more". + * + * @return void + */ + public function testTruncationIsCountedAndStaysBalanced(): void + { + $report = new ChannelApplyReport(); + $report->declareChannel(channel: 'automations', declared: 3); + $report->recordCreated(channel: 'automations', item: 'a'); + $report->recordTruncated(channel: 'automations', item: 'b'); + $report->recordTruncated(channel: 'automations', item: 'c'); + + $out = $report->toArray(); + + self::assertSame(2, $out['channels']['automations']['truncated']); + self::assertSame(2, $out['channels']['automations']['skipped']); + self::assertSame( + $out['channels']['automations']['declared'], + ($out['channels']['automations']['created'] + $out['channels']['automations']['skipped'] + $out['channels']['automations']['failed']) + ); + + }//end testTruncationIsCountedAndStaysBalanced() + + /** + * Credential gaps are collected per credential, de-duplicated per connector. + * + * @return void + */ + public function testNeedsCredentialsAreCollected(): void + { + $report = new ChannelApplyReport(); + $report->declareChannel(channel: 'connectors', declared: 0); + $report->needsCredential(credential: 'doffin', connector: 'source/a'); + $report->needsCredential(credential: 'doffin', connector: 'source/a'); + $report->needsCredential(credential: 'doffin', connector: 'source/b'); + + $out = $report->toArray(); + + self::assertSame(['source/a', 'source/b'], $out['needsCredentials']['doffin']); + + }//end testNeedsCredentialsAreCollected() +}//end class diff --git a/tests/stubs/openregister-stubs.php b/tests/stubs/openregister-stubs.php index 2fd61293c..5a8c53cf2 100644 --- a/tests/stubs/openregister-stubs.php +++ b/tests/stubs/openregister-stubs.php @@ -1125,7 +1125,7 @@ public function findAll(array $config=[], bool $_rbac=true, bool $_multitenancy= * * @return \OCA\OpenRegister\Db\ObjectEntity */ - public function saveObject(array|\OCA\OpenRegister\Db\ObjectEntity $object, ?array $extend=[], \OCA\OpenRegister\Db\Register|string|int|null $register=null, \OCA\OpenRegister\Db\Schema|string|int|null $schema=null, ?string $uuid=null, bool $_rbac=true, bool $_multitenancy=true, bool $silent=false, ?array $uploadedFiles=null, ?\OCP\IUser $currentUser=null): \OCA\OpenRegister\Db\ObjectEntity + public function saveObject(array|\OCA\OpenRegister\Db\ObjectEntity $object, ?array $extend=[], \OCA\OpenRegister\Db\Register|string|int|null $register=null, \OCA\OpenRegister\Db\Schema|string|int|null $schema=null, ?string $uuid=null, bool $_rbac=true, bool $_multitenancy=true, bool $silent=false, ?array $uploadedFiles=null, ?\OCP\IUser $currentUser=null, bool $failIfExists=false): \OCA\OpenRegister\Db\ObjectEntity { return new \OCA\OpenRegister\Db\ObjectEntity(); }//end saveObject() @@ -1886,6 +1886,24 @@ public function check(array $object, string $action, string $userId): GuardResul } } +namespace OCA\OpenRegister\Exception { + + if (class_exists('OCA\OpenRegister\Exception\ObjectExistsException', false) === false) { + /** + * Insert-only save conflict — thrown by ObjectService::saveObject() when + * $failIfExists is true and the identifier is already taken. + * + * Stubbed as a TYPE because OpenBuild's channel applier catches it by + * type. Matching on message text instead let a plain PHP "Unknown named + * parameter $failIfExists" error masquerade as a benign collision, so a + * wiring bug reported itself as "already exists" and the test went green. + */ + class ObjectExistsException extends \Exception + { + }//end class + } +} + namespace OCA\OpenRegister\Lifecycle { if (class_exists(GuardResult::class, autoload: false) === false) { diff --git a/tests/unit/Controller/ApplicationsControllerDiffTest.php b/tests/unit/Controller/ApplicationsControllerDiffTest.php index f412f7347..9768c89e2 100644 --- a/tests/unit/Controller/ApplicationsControllerDiffTest.php +++ b/tests/unit/Controller/ApplicationsControllerDiffTest.php @@ -127,6 +127,7 @@ protected function setUp(): void groupManager: $groupManager, manifestResolver: $this->createMock(ManifestResolverService::class), permissionResolver: $permissionResolver, + channelApplier: $this->createMock(\OCA\OpenBuild\Service\AppChannelApplier::class), ); }//end setUp() @@ -380,6 +381,7 @@ public function testDiffVersionsReturns403WhenCallerHasNoRole(): void groupManager: $noAdminGroupManager, manifestResolver: $this->createMock(ManifestResolverService::class), permissionResolver: $noAdminPermissionResolver, + channelApplier: $this->createMock(\OCA\OpenBuild\Service\AppChannelApplier::class), ); $result = $controller->diffVersions(slug: 'hello-world', from: 'draft', to: 'draft'); diff --git a/tests/unit/Controller/ApplicationsControllerTest.php b/tests/unit/Controller/ApplicationsControllerTest.php index 5b79d7d5e..975df21ae 100644 --- a/tests/unit/Controller/ApplicationsControllerTest.php +++ b/tests/unit/Controller/ApplicationsControllerTest.php @@ -173,6 +173,7 @@ static function (string $callerUid, string $gid) use ($uid, $isAdmin): bool { groupManager: $this->groupManager, manifestResolver: $this->manifestResolver, permissionResolver: $permissionResolver, + channelApplier: $this->createMock(\OCA\OpenBuild\Service\AppChannelApplier::class), auditTrailMapper: $this->auditTrailMapper, ); }//end buildController() From 55d819ad20463881616230635ad8b1a157c5ab66 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 2 Aug 2026 00:05:00 +0200 Subject: [PATCH 2/4] fix(gates): @spec on the new report methods, repoint archived anchors, declare persistApplication throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gate-16 wanted @spec on every new public method. gate-46 and gate-49 were PRE-EXISTING on development (36 identical dangling anchors; persistApplication untouched) and surfaced only because this change touches the file — fixed here per the repo rule rather than left for later. The 36 anchors pointed at retrofit-2026-05-24-annotate-openbuild, which has since been ARCHIVED; repointed to the archive path. persistApplication already wraps its save — the unguarded call is normaliseObject() AFTER it, so the contract is now declared rather than the behaviour quietly changed. --- lib/Controller/ApplicationsController.php | 78 ++++++++++++----------- lib/Service/ChannelApplyReport.php | 18 ++++++ lib/Service/ContainerLocator.php | 2 + 3 files changed, 62 insertions(+), 36 deletions(-) diff --git a/lib/Controller/ApplicationsController.php b/lib/Controller/ApplicationsController.php index 635b6633c..003a125e1 100644 --- a/lib/Controller/ApplicationsController.php +++ b/lib/Controller/ApplicationsController.php @@ -30,18 +30,18 @@ * * @link https://conduction.nl * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-45 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-46 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-49 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-51 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-69 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-70 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-45 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-46 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-49 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-51 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-69 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-70 * @spec openspec/specs/openbuild-runtime/spec.md#requirement-the-runtime-must-inject-the-current-user-s-group-context * @spec openspec/specs/openbuild-runtime/spec.md#requirement-menu-items-and-pages-must-be-filterable-by-permission */ @@ -166,8 +166,8 @@ public function __construct( * * @return JSONResponse The manifest blob (carrying `runtime.user.isOwner`), or a 404 envelope when not found * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-51 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-51 * @spec openspec/changes/openbuild-admin-settings-abstraction/specs/admin-settings-owner-gating/spec.md#requirement-owner-signal-is-derived-from-existing-openbuild-primitives */ #[NoAdminRequired] @@ -468,8 +468,8 @@ public function saveManifest(string $slug): JSONResponse * * @return JSONResponse 200 with manifest, or 404 when not found / not authorised. * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-69 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-70 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-69 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-70 */ private function resolveVersionedManifestResponse(string $slug, string $versionSlug): JSONResponse { @@ -535,7 +535,7 @@ private function resolveVersionedManifestResponse(string $slug, string $versionS * and the resolveVersionBlob() check on `applicationUuid` rejects snapshots * that do not belong to this Application. Mirrors getManifest()'s pattern. * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 */ #[NoAdminRequired] #[NoCSRFRequired] @@ -652,7 +652,7 @@ public function diffVersions(string $slug, string $from, string $to): JSONRespon * * @return array|null Blob or null if the version is missing. * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-58 */ private function resolveVersionBlob(string $token, array $application, string $applicationUuid): ?array { @@ -700,7 +700,7 @@ private function resolveVersionBlob(string $token, array $application, string $a * * @return JSONResponse|array{0: ObjectEntity|array, 1: array, 2: string} * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-50 */ private function resolveApplicationBySlug(string $slug): JSONResponse|array { @@ -781,9 +781,9 @@ private function resolveApplicationBySlug(string $slug): JSONResponse|array * * @return JSONResponse The filtered Application list * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-46 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-46 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 */ #[NoAdminRequired] #[NoCSRFRequired] @@ -865,7 +865,7 @@ public function listMine(): JSONResponse * * @return array{0: array>, 1: bool} [filtered list, adminBypassUsed]. * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-48 */ private function filterApplicationsByRole( array $results, @@ -1178,8 +1178,8 @@ private function injectPermissionsSignal(array $manifest, array $applicationArra * * @return JSONResponse|null Null on allow, 403 JSONResponse on deny * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-45 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-45 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-47 */ private function requirePermission( ?ObjectEntity $application, @@ -1240,7 +1240,7 @@ private function requirePermission( * * @return void * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-49 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-49 */ private function recordAdminBypass(?ObjectEntity $application, string $slug, string $actor): void { @@ -1303,8 +1303,8 @@ private function recordAdminBypass(?ObjectEntity $application, string $slug, str * * @return JSONResponse The new application's uuid + slug, or an error envelope * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ #[NoAdminRequired] #[UserRateLimit(limit: 10, period: 3600)] @@ -1526,7 +1526,7 @@ private function resolveSharedContext(): ?array * * @return array * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function buildClonedManifest(array $template, array $rewriteMap): array { @@ -1554,7 +1554,7 @@ private function buildClonedManifest(array $template, array $rewriteMap): array * * @return array{register:\OCA\OpenRegister\Db\Register,schemaIds:array}|array{error:array,status:int} * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function provisionPerAppArtifacts( string $newSlug, @@ -1596,7 +1596,13 @@ private function provisionPerAppArtifacts( * * @return array{uuid:string|null}|array{error:array,status:int} * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 + * @throws Throwable From normaliseObject() AFTER a successful save. Deliberately + * not folded into the envelope: at that point the Application + * record already exists, so returning `clone_failed` would tell + * the caller nothing was created when something was. Surfacing + * the real failure is the honest outcome. + * + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 */ private function persistApplication( string $name, @@ -1682,7 +1688,7 @@ private function validateCloneRequest(array $body): array * * @return array> * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function extractCompanionSchemas(array $template): array { @@ -1707,7 +1713,7 @@ private function extractCompanionSchemas(array $template): array * * @return array * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function buildRewriteMap(array $companions, string $newSlug): array { @@ -1731,7 +1737,7 @@ private function buildRewriteMap(array $companions, string $newSlug): array * * @return \OCA\OpenRegister\Db\Register * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function provisionPerAppRegister(string $newSlug, string $ownerUid): \OCA\OpenRegister\Db\Register { @@ -1846,7 +1852,7 @@ private function extractRegisterOwner(mixed $register): string * * @return array List of created schema IDs * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function cloneCompanionSchemas( array $companions, @@ -1889,7 +1895,7 @@ private function cloneCompanionSchemas( * * @return mixed The rewritten node * - * @spec openspec/changes/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 + * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-56 */ private function rewriteSchemaRefs(mixed $node, array $map): mixed { diff --git a/lib/Service/ChannelApplyReport.php b/lib/Service/ChannelApplyReport.php index 34f751e3b..f6c4aea56 100644 --- a/lib/Service/ChannelApplyReport.php +++ b/lib/Service/ChannelApplyReport.php @@ -109,6 +109,8 @@ class ChannelApplyReport * @param int $declared How many items the template declared. * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-absent-optional-dependency-degrades-with-a-stated-reason */ public function declareChannel(string $channel, int $declared): void { @@ -132,6 +134,8 @@ public function declareChannel(string $channel, int $declared): void * @param string $item The item identity (e.g. `source/`). * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-application-is-best-effort-with-a-complete-per-item-outcome-report */ public function recordCreated(string $channel, string $item): void { @@ -147,6 +151,8 @@ public function recordCreated(string $channel, string $item): void * @param string $reason Why it was skipped. * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-application-is-best-effort-with-a-complete-per-item-outcome-report */ public function recordSkipped(string $channel, string $item, string $reason): void { @@ -163,6 +169,8 @@ public function recordSkipped(string $channel, string $item, string $reason): vo * @param string $reason The failure reason (never a secret). * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-application-is-best-effort-with-a-complete-per-item-outcome-report */ public function recordFailed(string $channel, string $item, string $reason): void { @@ -180,6 +188,8 @@ public function recordFailed(string $channel, string $item, string $reason): voi * @param string $item The item identity. * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-every-channel-is-bounded-and-truncation-is-reported */ public function recordTruncated(string $channel, string $item): void { @@ -197,6 +207,8 @@ public function recordTruncated(string $channel, string $item): void * @param string $reason Machine-readable reason (e.g. `hermiq-unavailable`). * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-absent-optional-dependency-degrades-with-a-stated-reason */ public function skipChannel(string $channel, string $reason): void { @@ -228,6 +240,8 @@ public function skipChannel(string $channel, string $reason): void * @param int $truncated Items hermiq dropped at its own bound. * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-skills-are-delegated-to-hermiq-by-repository-coordinates */ public function adoptCounts(string $channel, int $created, int $skipped, int $failed, int $truncated): void { @@ -254,6 +268,8 @@ public function adoptCounts(string $channel, int $created, int $skipped, int $fa * @param string $connector The connector that needs it. * * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-unresolvable-credential-references-are-reported */ public function needsCredential(string $credential, string $connector): void { @@ -274,6 +290,8 @@ public function needsCredential(string $credential, string $connector): void * @throws RuntimeException When a channel does not balance — that means an * item was dropped somewhere, which is exactly the * class of defect this report exists to expose. + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-application-is-best-effort-with-a-complete-per-item-outcome-report */ public function toArray(): array { diff --git a/lib/Service/ContainerLocator.php b/lib/Service/ContainerLocator.php index 54c69e2d5..c4a115eff 100644 --- a/lib/Service/ContainerLocator.php +++ b/lib/Service/ContainerLocator.php @@ -66,6 +66,8 @@ public function __construct( * @param string $className Fully-qualified class name. * * @return object|null The service, or null when it cannot be resolved. + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-an-absent-optional-dependency-degrades-with-a-stated-reason */ public function get(string $className): ?object { From 604711cb247d6a26583511f64f48c2795f834702 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 2 Aug 2026 00:08:18 +0200 Subject: [PATCH 3/4] fix(gates): document persistApplication exception contract for gate-49 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The save is wrapped in catch(Throwable), which covers OpenRegister's ValidationException and DoesNotExistException — the gate's heuristic matches on named exceptions and cannot see that a Throwable catch subsumes them. Documenting the actual contract is what the gate is for, so it is now stated rather than the behaviour changed to suit the checker. --- lib/Controller/ApplicationsController.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/Controller/ApplicationsController.php b/lib/Controller/ApplicationsController.php index 003a125e1..c2accf7dd 100644 --- a/lib/Controller/ApplicationsController.php +++ b/lib/Controller/ApplicationsController.php @@ -1596,11 +1596,16 @@ private function provisionPerAppArtifacts( * * @return array{uuid:string|null}|array{error:array,status:int} * + * Exception contract: the save is wrapped in `catch (Throwable)`, which + * deliberately covers OpenRegister's ValidationException and + * DoesNotExistException as well as anything else the write path raises — all + * of them are translated into the `clone_failed` envelope rather than leaking + * out of the controller. + * * @throws Throwable From normaliseObject() AFTER a successful save. Deliberately - * not folded into the envelope: at that point the Application - * record already exists, so returning `clone_failed` would tell - * the caller nothing was created when something was. Surfacing - * the real failure is the honest outcome. + * NOT folded into the envelope: by then the Application record + * already exists, so returning `clone_failed` would tell the + * caller nothing was created when something was. * * @spec openspec/changes/archive/retrofit-2026-05-24-annotate-openbuild/tasks.md#task-55 */ From 765253e2f00a8024792d6466c0ce408a0f6eef09 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 2 Aug 2026 00:26:45 +0200 Subject: [PATCH 4/4] fix(app-repo): read channels where the parser ACTUALLY puts them, plus psalm/phpcs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit THE BUG THIS COMMIT FIXES WOULD HAVE MADE THE WHOLE CHANGE A NO-OP. AppRepoParser nests the v2 channels under `$payload['channels']`. The applier read them from the top level, so every channel resolved to [] and reported `declared: 0` — an install that does nothing and returns success, which is precisely the failure this change exists to end. Found by reading the parser while preparing the live probe, NOT by the unit tests: I had written the test fixtures in the same wrong shape, so tests and implementation agreed with each other while both disagreed with the real producer. The durable fix is the new test, not the one-line change: testAppliesTheChannelShapeTheParserActuallyProduces drives the REAL AppRepoParser over a v2 file map and feeds its output straight into the applier, so the two shapes cannot drift apart again without a red suite. Mutation-checked — reverting channelOf() to the top-level read turns it red. Lesson worth keeping: a hand-written fixture that mirrors the implementation's assumption cannot detect a shape mismatch with the real producer. It only ever tests that the code agrees with itself. Also: adoptCounts() now takes truncated as the BOOL it is (hermiq knows truncation happened, not how many items it missed) and absorbs any shortfall between our declared count and the source's outcomes as a NAMED skip, so the balance identity holds and the cause is stated rather than the difference silently disagreeing. psalm needed ObjectExistsException on the cross-app suppression list; two inline comments needed capitals. --- lib/Service/AppChannelApplier.php | 14 ++- lib/Service/AppRepoSerializer.php | 2 +- lib/Service/ChannelApplyReport.php | 40 ++++++--- lib/Service/DataRegisterProvisioner.php | 6 -- lib/Service/GitHubCatalogService.php | 2 +- psalm.xml | 5 ++ tests/Unit/Service/AppChannelApplierTest.php | 94 ++++++++++++++++---- 7 files changed, 126 insertions(+), 37 deletions(-) diff --git a/lib/Service/AppChannelApplier.php b/lib/Service/AppChannelApplier.php index e95357f84..f427df25c 100644 --- a/lib/Service/AppChannelApplier.php +++ b/lib/Service/AppChannelApplier.php @@ -254,7 +254,17 @@ private function coordinatesFor(array $template, ?string $owner, ?string $repo): */ private function channelOf(array $template, string $name): array { - $channel = ($template[$name] ?? []); + // AppRepoParser NESTS the v2 channels under `channels`, and adds the key + // only for a v2 repo. Reading them from the top level instead returns + // nothing for every channel — which is not an error, just a silent + // `declared: 0`, i.e. exactly the do-nothing-and-report-success failure + // this class exists to end. Verified against the parser, not assumed. + $channels = ($template['channels'] ?? []); + if (is_array($channels) === false) { + return []; + } + + $channel = ($channels[$name] ?? []); if (is_array($channel) === false) { return []; } @@ -582,7 +592,7 @@ private function applySkills( created: (int) ($result['installed'] ?? 0), skipped: (int) ($result['skipped'] ?? 0), failed: (int) ($result['failed'] ?? 0), - truncated: (int) ($result['truncated'] ?? 0) + truncated: (bool) ($result['truncated'] ?? false) ); } catch (Throwable $e) { $this->logger->warning('OpenBuild channel apply: hermiq skill install failed: '.$e->getMessage()); diff --git a/lib/Service/AppRepoSerializer.php b/lib/Service/AppRepoSerializer.php index ed4f55410..0dd18c1ec 100644 --- a/lib/Service/AppRepoSerializer.php +++ b/lib/Service/AppRepoSerializer.php @@ -416,7 +416,7 @@ private function findConnector(string $kind, string $uuid): ?array } try { - // find(), NOT findAll(filters: ['uuid' => …]): a uuid is OpenRegister + // Resolved with find(), NOT findAll(filters: ['uuid' => …]): a uuid is OpenRegister // METADATA, not an object property, so a filter on it matches nothing. // Resolved by UUID rather than slug because OpenConnector objects // overwhelmingly have no slug (measured live: 0 of 74 jobs, 1 of 291 diff --git a/lib/Service/ChannelApplyReport.php b/lib/Service/ChannelApplyReport.php index f6c4aea56..608b8976c 100644 --- a/lib/Service/ChannelApplyReport.php +++ b/lib/Service/ChannelApplyReport.php @@ -220,8 +220,7 @@ public function skipChannel(string $channel, string $reason): void $this->channels[$channel]['reason'] = $reason; $channelState = $this->channels[$channel]; - $outstanding = ($channelState['declared'] - $channelState['created'] - - $channelState['skipped'] - $channelState['failed']); + $outstanding = ($channelState['declared'] - $channelState['created'] - $channelState['skipped'] - $channelState['failed']); if ($outstanding > 0) { $this->channels[$channel]['skipped'] += $outstanding; @@ -233,26 +232,45 @@ public function skipChannel(string $channel, string $reason): void * Adopt counts produced by another app (hermiq owns skill installation, so * its numbers are carried through unmodified rather than recomputed here). * + * Any shortfall between what we declared and what the source accounted for is + * absorbed as a skip with a named cause, rather than left to break the balance + * identity. A source that returns fewer outcomes than we sent it items is a + * real event with a real explanation — usually its own truncation — and the + * report should say which, not throw or quietly disagree with itself. + * * @param string $channel The channel name. - * @param int $created Items hermiq installed. - * @param int $skipped Items hermiq skipped. - * @param int $failed Items hermiq failed. - * @param int $truncated Items hermiq dropped at its own bound. + * @param int $created Items the source installed. + * @param int $skipped Items the source skipped. + * @param int $failed Items the source failed. + * @param bool $truncated Whether the SOURCE truncated its own fetch. A flag, + * not a count: it knows truncation happened but not + * how many items it never read. * * @return void * * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-skills-are-delegated-to-hermiq-by-repository-coordinates */ - public function adoptCounts(string $channel, int $created, int $skipped, int $failed, int $truncated): void + public function adoptCounts(string $channel, int $created, int $skipped, int $failed, bool $truncated): void { if (isset($this->channels[$channel]) === false) { $this->declareChannel(channel: $channel, declared: ($created + $skipped + $failed)); } - $this->channels[$channel]['created'] = $created; - $this->channels[$channel]['skipped'] = $skipped; - $this->channels[$channel]['failed'] = $failed; - $this->channels[$channel]['truncated'] = $truncated; + $this->channels[$channel]['created'] = $created; + $this->channels[$channel]['skipped'] = $skipped; + $this->channels[$channel]['failed'] = $failed; + + if ($truncated === true) { + $this->channels[$channel]['reason'] = 'source-bundle-truncated'; + } + + $outstanding = ($this->channels[$channel]['declared'] - $created - $skipped - $failed); + if ($outstanding > 0) { + $this->channels[$channel]['skipped'] += $outstanding; + if ($this->channels[$channel]['reason'] === null) { + $this->channels[$channel]['reason'] = 'not-accounted-for-by-source'; + } + } }//end adoptCounts() diff --git a/lib/Service/DataRegisterProvisioner.php b/lib/Service/DataRegisterProvisioner.php index 8b5aa870d..41a255f20 100644 --- a/lib/Service/DataRegisterProvisioner.php +++ b/lib/Service/DataRegisterProvisioner.php @@ -61,7 +61,6 @@ class DataRegisterProvisioner */ private const MAX_REGISTERS = 64; - /** * Constructor. * @@ -78,7 +77,6 @@ public function __construct( ) { }//end __construct() - /** * Apply the data-registers channel. * @@ -119,7 +117,6 @@ public function apply(array $registers, ChannelApplyReport $report): void }//end apply() - /** * Create one register and its missing schemas, or skip an existing one. * @@ -165,7 +162,6 @@ private function applyOne(string $slug, array $blob, ChannelApplyReport $report) }//end applyOne() - /** * Find a schema by slug, or create it from the published definition. * @@ -208,6 +204,4 @@ private function findOrCreateSchema(string $slug, array $definition): ?int } }//end findOrCreateSchema() - - }//end class diff --git a/lib/Service/GitHubCatalogService.php b/lib/Service/GitHubCatalogService.php index 905cd3aaa..c00572862 100644 --- a/lib/Service/GitHubCatalogService.php +++ b/lib/Service/GitHubCatalogService.php @@ -408,7 +408,7 @@ public function fetchRepoFiles( } } - // app-repo-format-v2 channels. Without this the parser — which DOES know + // The app-repo-format-v2 channels. Without this the parser — which DOES know // how to read them — is handed a v1 file set, so every channel comes back // empty and a v2 repo installs as if it carried nothing but a manifest. // Verified against the real published artefacts: buildiq-spectr fetched diff --git a/psalm.xml b/psalm.xml index 9757ee8f9..d7bdb93f8 100644 --- a/psalm.xml +++ b/psalm.xml @@ -112,6 +112,11 @@ + + diff --git a/tests/Unit/Service/AppChannelApplierTest.php b/tests/Unit/Service/AppChannelApplierTest.php index a339f76c4..1d02ab859 100644 --- a/tests/Unit/Service/AppChannelApplierTest.php +++ b/tests/Unit/Service/AppChannelApplierTest.php @@ -34,6 +34,7 @@ namespace OCA\OpenBuild\Tests\Unit\Service; use OCA\OpenBuild\Service\AppChannelApplier; +use OCA\OpenBuild\Service\AppRepoParser; use OCA\OpenBuild\Service\ChannelApplyReport; use OCA\OpenBuild\Service\ContainerLocator; use OCA\OpenBuild\Service\DataRegisterProvisioner; @@ -146,10 +147,65 @@ private function templateWithConnector(string $kind, string $uuid): array { return [ 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], - 'connectors' => [$kind => ['example' => ['id' => $uuid, 'name' => 'Example']]], + 'channels' => [ + 'connectors' => [$kind => ['example' => ['id' => $uuid, 'name' => 'Example']]], + ], ]; }//end templateWithConnector() + + /** + * The applier reads the channels AppRepoParser actually produces. + * + * This is the load-bearing test of the file. The channels are nested under + * `channels`, and an applier that read them from the top level would find + * nothing, report `declared: 0` for everything, and still return success — + * the precise do-nothing-and-claim-victory failure this class exists to end. + * + * Hand-written fixtures cannot catch that: write them in the same shape the + * implementation assumes and the two agree with each other while both + * disagree with the real producer. So this test drives the REAL parser and + * feeds its output straight in, which is the only version of this assertion + * that can fail when the shapes drift apart. + * + * @return void + * + * @spec openspec/changes/apply-v2-channels/specs/app-channel-application/spec.md#requirement-every-install-path-applies-the-v2-channels + */ + public function testAppliesTheChannelShapeTheParserActuallyProduces(): void + { + $files = [ + 'openbuild-app.json' => json_encode( + [ + 'formatVersion' => '2.0', + 'appType' => 'virtual', + 'slug' => 'example-app', + 'version' => '1.0.0', + ] + ), + 'manifest.json' => json_encode(['version' => '1.0.0', 'pages' => []]), + 'connectors/source/example.json' => json_encode(['id' => self::NIL_UUID, 'name' => 'Example']), + 'data-registers/example.json' => json_encode(['slug' => 'example', 'title' => 'Example', 'schemas' => []]), + ]; + + $template = (new AppRepoParser())->parse( + files: $files, + repo: ['owner' => 'ConductionNL', 'name' => 'example-app'] + ); + + $this->appManager->method('isEnabledForUser')->willReturn(true); + $this->objectService->method('findAll')->willReturn([]); + $this->registerMapper->method('find')->willThrowException(new RuntimeException('absent')); + + $report = $this->applier()->apply(template: $template); + + // The whole point: NON-ZERO declared counts straight off the real parser. + self::assertSame(1, $report['channels']['connectors']['declared']); + self::assertSame(1, $report['channels']['dataRegisters']['declared']); + self::assertSame(1, $report['channels']['connectors']['created']); + + }//end testAppliesTheChannelShapeTheParserActuallyProduces() + /** * A v1 template installs unchanged and reports zero declared everywhere. * @@ -285,9 +341,11 @@ public function testSkillsDegradeWhenHermiqIsAbsent(): void $report = $this->applier()->apply( template: [ 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], - 'skills' => [ - 'alpha' => ['SKILL.md' => '# alpha'], - 'beta' => ['SKILL.md' => '# beta'], + 'channels' => [ + 'skills' => [ + 'alpha' => ['SKILL.md' => '# alpha'], + 'beta' => ['SKILL.md' => '# beta'], + ], ], ] ); @@ -312,13 +370,15 @@ public function testUnresolvableCredentialIsReported(): void $report = $this->applier()->apply( template: [ 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], - 'connectors' => [ - 'source' => [ - 'example' => [ - 'id' => self::NIL_UUID, - 'configuration' => [ - 'authentication' => [ - 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + 'channels' => [ + 'connectors' => [ + 'source' => [ + 'example' => [ + 'id' => self::NIL_UUID, + 'configuration' => [ + 'authentication' => [ + 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + ], ], ], ], @@ -347,11 +407,13 @@ public function testInconclusiveCredentialLookupIsNotReportedAsMissing(): void $report = $this->applier()->apply( template: [ 'templateOrigin' => ['repo' => 'ConductionNL/example-app'], - 'connectors' => [ - 'source' => [ - 'example' => [ - 'id' => self::NIL_UUID, - 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + 'channels' => [ + 'connectors' => [ + 'source' => [ + 'example' => [ + 'id' => self::NIL_UUID, + 'credentialRef' => ['credentialName' => 'PLACEHOLDER_CREDENTIAL'], + ], ], ], ],