fix(shop-install): four bugs blocking a real GitHub-shop virtual-app install

[rubenvdlinde]

Summary

Found and fixed while live-testing the GitHub shop-install path (ShopController/AppChannelApplier/AppRepoSerializer) against buildiq-spectr and buildiq-hydra on repeatedly-fresh Nextcloud instances — three bugs, each confirmed live before and after the fix:

1. installFromTemplateArray() never forwarded owner/repo/credentialId into AppChannelApplier::apply() — Hermiq's skill-bundle installer does its own independent GitHub fetch and always ran anonymous, 404ing on any private source repo. Broke 100% of skill installs from either buildiq app.
2. ApplicationPublishController::publish() never created a BuiltAppRoute index object — only the app-creation wizard did, at creation time. Any app started as a draft and published later (every template-install path) stayed unreachable at its manifest/builder URL even after a successful publish.
3. persistApplication()'s manifest field is silently dropped by the application schema, and no ApplicationVersion was ever created for a template-installed app — getManifest() resolves via productionVersion → ApplicationVersion.manifest; without one, every template-installed app returns no_manifest forever, even when published.
4. AppRepoSerializer's connector reference resolution validated sourceId as UUID-only, but measured live data shows sources are commonly slug-referenced ("sourceId": "tenderned") — 9 of spectr's 13 referenced sources were silently dropped from every export. Fixing this surfaced a second bug: dedup was keyed on the raw reference string instead of the object's own resolved identity, so a source reachable by both UUID and slug installed twice under two filenames — fixed by reading the real UUID from the ObjectEntity, not the payload.

Verified live

Reinstalled both buildiq-spectr and buildiq-hydra via the real shop-install path on two separate from-scratch Nextcloud instances (torn down and rebuilt between rounds):

• spectr: register + all 52 connectors + all 21 skills install cleanly; manifest resolves (v2.4.0, 71 pages) after publish.
• hydra-console: register installs cleanly; manifest resolves (v0.1.0, 7 pages) after publish.
• Republished buildiq-spectr twice post-fix — connector count stable at the correct 13 sources, no duplicates.

Known follow-up, not fixed here

GitHubTemplateCatalogService::collectTreeBlobs() fetches one file at a time sequentially for a skill bundle (a 94-skill bundle is ~300-500 sequential GitHub API round-trips) — under load this can time out partway through with a misleading 0 created / not-accounted-for-by-source result despite real partial progress. Root-caused but not fixed in this PR (needs batching/parallelizing the fetch, a bigger change).

Test plan

• php -l clean on all touched files
• Local Hydra gates: 30/30 green on the diff (gate-66's 16 findings are pre-existing, confirmed zero overlap with touched files)
• Live end-to-end verification as described above

[github-actions]

Quality Report — ConductionNL/openbuild @ e868d99

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
check-gitignore		✅
check-nc-floor		✅
format		✅
composer			❌	✅ 106/106
npm			✅	✅ 626/626
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					❌
Playwright					❌
Hydra gates					❌

---

Quality workflow — 2026-08-16 21:52 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openbuild @ 55bb059

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
check-gitignore		✅
check-nc-floor		✅
format		✅
composer			✅	✅ 106/106
npm			✅	✅ 626/626
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					❌
Hydra gates					✅

---

Quality workflow — 2026-08-17 01:21 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openbuild @ 747eac0

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
check-gitignore		✅
check-nc-floor		✅
format		✅
composer			✅	✅ 106/106
npm			✅	✅ 626/626
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					❌
Hydra gates					❌

---

Quality workflow — 2026-08-17 02:45 UTC

Download the full PDF report from the workflow artifacts.

[rubenvdlinde]

Fix crew report — three red groups cleared, one real bug found

1. PHPUnit: 4 existing tests, not 4 new ones

Tests: 851, Failures: 4 against a base of OK (851 tests) — identical count, so nothing was added; four existing tests broke on the new second saveObject() call.

Only two were expects($this->once()) violations. The other two were subtler and worth naming: they captured $object into a $savedPayload variable on every call, so the last write silently became the assertion target. testManifestSchemaRefsRewrittenWithNewSlugPrefix then asserted the manifest of a payload that legitimately has no manifest, and testOwnerFieldSetToAuthenticatedUid asserted owner on one that legitimately has no owner. A capture-the-last-call double retargets itself the moment a second call appears.

Fixed by asserting the real counts with the arguments checked, not by loosening to any():

• install → exactly(3): Application create, ApplicationVersion create, Application re-save
• publish → exactly(2): status flip, BuiltAppRoute upsert

Each individual write is now pinned by its register, schema and uuid, so the count cannot be satisfied by any three saves at all.

⚠️ The saveObject double also now echoes the persisted object back, the way OpenRegister does. Returning a bare ['uuid' => …] made the controller's read-modify-write look lossless no matter what it actually sent — which is precisely how the bug below stayed invisible.

Argument order was verified with ReflectionMethod against the interface the mock actually resolves at runtime (vendor/conduction/hydra-gates/hydra-gates/contracts/ObjectServiceInterface.php), confirmed identical to openregister's live ObjectServiceInterface. A PHPUnit mock resolves named arguments against its own signature and then invokes the callback positionally, so willReturnCallback parameters must mirror (object, extend, register, schema, uuid, …).

2. 🔴 Real bug: the productionVersion re-save wiped the Application

linkProductionVersion() patched productionVersion in with a hand-built four-field payload (slug, name, permissions, productionVersion).

OpenRegister's saveObject() is PUT, not PATCH. On the update path, SaveObject::prepareObjectForUpdate() calls fillMissingSchemaPropertiesWithNull() — which NULLs every schema property absent from the payload — and then setObject() replaces the stored data outright. There is no merge with the existing object anywhere in that path (the only carry-forward is writeOnly properties).

So that write NULLed owner, status, version and templateOrigin off the Application that persistApplication() had written one statement earlier — on every local-template, remote-registry and GitHub-shop install.

Now persistApplication() hands back the stored payload and the link step re-saves that whole object with the one field patched in, mirroring ApplicationPublishController::setStatus(). A regression test pins it, and it was confirmed to fail against the old code before being accepted.

3. phpmd: split along real seams, no suppressions

• installFromTemplateArray() 114 lines → the clone half became materialiseApplication()
• collectConnectors() 104 lines → collectDirectReferences() + exportReferencedConnector()

Also cleared 6 UndefinedVariable findings the CI log carried alongside the two length ones: $realUuid / $realRefUuid existed only because the by-reference out-param created them, and the $realUuid !== '' guard read a possible null the wrong way (null !== '' is true, so the identity would have been null). Plus the two phpcs errors on lines this PR introduced.

No SuppressWarnings, no threshold changes.

⚠️ Instrument note for other crews: a red PHPUnit cell that is not a test failure

After the fix, one of six PHPUnit cells stayed red while the suite printed OK (852 tests, 2690 assertions). The failing step was the coverage ratchet inside the PHPUnit job, and its verdict was self-refuting:

Changed files, head:    58.62%  (877/1496 statements)
Changed files, base:    58.62%  (809/1380 statements)
FAIL: coverage of the files this change touches dropped by 0%.

The 116 added statements were covered at 68/116 = 58.62% — exactly the file mean, as arithmetically neutral as a change can be — and it still failed, by 0.0002 percentage points.

Rather than buy that back with filler, this pins two contracts linkProductionVersion() documents but nothing tested: a failing version write must not fail the install (and must not attempt the re-save), and a version save yielding no UUID must not overwrite a working pointer with an empty one.

Measured, not assumed: 877/1496 → 883/1496, 58.6230% → 59.0241%, clearing base by 0.40pp. The local pcov run reproduced CI's xdebug figure of 877/1496 exactly, so the instrument is the one the gate uses.

Read the failing step, never the job name.

[github-actions]

Quality Report — ConductionNL/openbuild @ c9bb330

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
check-gitignore		✅
check-nc-floor		✅
format		✅
composer			✅	✅ 106/106
npm			✅	✅ 626/626
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-08-17 03:24 UTC

Download the full PDF report from the workflow artifacts.