feat(app-repo): apply the v2 channels on install — they were parsed, then dropped - #85
Merged
Merged
Conversation
…then dropped
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.
…, declare persistApplication throw 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.
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.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 659/659 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-08-01 22:10 UTC
Download the full PDF report from the workflow artifacts.
…s psalm/phpcs 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.
Contributor
Quality Report — ConductionNL/openbuild @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 659/659 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 89.4% (17/19 statements)
Quality workflow — 2026-08-01 22:29 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last step of the app-repo round trip. Depends on ConductionNL/hermiq#109.
The defect
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 with a positive control before any code was written:
$template['manifest']/['version'](control)$template['connectors']$template['automations']$template['skills']$template['dataRegisters']Application.dataRegistersbindings, never a parsed templateBoth entry points confirmed by reading them, not by grep alone:
pull()persisted manifest + companion schemas only;installFromTemplateArray()read exactlyslugandmanifest.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
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): the guarantee lives in the call, not 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.
ChannelApplyReportenforcescreated + skipped + failed === declaredand throws when it doesn't hold — a dropped item is arithmetically impossible to hide. The 64-skill silent cap this programme already shipped is exactly what that identity exists to prevent.Never drop silently. Every channel is bounded; truncation is logged and counted.
Two defects the tests caught, both of which would have shipped
Unknown named parameter $failIfExiststherefore reported itself as a benignalready-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); the stale test stub that hid it is fixed.findAll(filters:, register:, schema:); the realfindAlltakes a$configarray. SincecredentialExists()deliberately swallows a failed lookup and returnstrue(an inconclusive lookup must never manufacture an absence claim),needsCredentialswould have been silently empty forever. Register/schema confirmed against the live instance (credential-broker/brokeredcredential) with a positive control rather than assumed.Design notes
SkillBundleInstallerby repo coordinates rather than being reimplementedopenconnectorandhermiqstay optional (OpenBuild declares onlyopenregister) and degrade with a machine-readable reason while every other channel still appliesDataRegisterProvisionersplit out when phpmd flagged complexity 61 > 50 — a real split, not a suppressionVerification
failIfExiststofalseturns the suite redvendor/, so the local 40-gate suite silently skips it; gate-green never meant phpstan-greenKnown, not fixed here
gate-46reports 38 dangling@specanchors, all pre-existing inApplicationsController(36 identical ondevelopment). Their change dir was archived, so I repointed the path to the archive; the fragments were always list items rather than headings and would need a semantic remap onto canonical requirements — a separate pass, not something to guess at across 36 methods.gate-46is local-only and not part of CI.