refactor(skills): extract SkillBundleInstaller so bundle install is callable off the HTTP path - #109
Merged
Conversation
…allable off the HTTP path Bundle install lived as a PRIVATE method on SkillController, which meant the only way to install a bundle was to be an HTTP request. OpenBuild needs to install the skills/ channel of a published app repo (apply-v2-channels) and must NOT reimplement skill installation to do it — frontmatter byte-fidelity, aux-file placement and the ADR-068 §3 learning-candidates.md exclusion have to keep living in exactly one implementation. SkillController now delegates, so the route and the cross-app caller run identical code rather than two copies that drift. No behaviour change. The controller test composes a REAL SkillBundleInstaller over the same mocks rather than stubbing it: a mock there would silently gut every bundle-install count assertion in that file, which is precisely the per-skill path they exist to check. Assertions rose 4308 -> 4362, confirming the path is still exercised.
Contributor
Quality Report — ConductionNL/hermiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 117/117 | |||
| npm | ✅ | ✅ 657/657 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-08-01 22:10 UTC
Download the full PDF report from the workflow artifacts.
…tion phpstan caught the `?? 0` on a non-nullable key, which exposed the real issue: fetchBundle's `truncated` is a bool — it knows truncation happened but not how many blobs it never read. Coercing it to (int) would have claimed a precise 'one skill dropped' that nobody measured. Passed through as the bool it is, matching the HTTP route's existing contract exactly.
Contributor
Quality Report — ConductionNL/hermiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 117/117 | |||
| npm | ✅ | ✅ 657/657 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ |
Coverage: 85.3% (93/109 statements)
Quality workflow — 2026-08-01 22:29 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 2, 2026
…ill (#112) * feat(skills): make bundle install idempotent — it duplicated every skill installFromSource() called saveObject() with no uuid and no existence check, so every install created a new object. Re-installing an app duplicated every skill it ships. Not hypothetical — already true on the shared instance before anyone re-installed anything: Meeting Summariser active (approved, 07-05) + quarantined shadow (07-26) Nightly Backup Helper two active copies, five minutes apart on 07-05 The first is the shape that matters: an approved skill with a quarantined twin competing with it. Re-installing buildiq-hydra would have added 94 more, 101 -> 195. IDENTITY is a canonical URL — https://<host>/<owner>/<repo>/skills/<bundleName>. No git ref: a branch is not identity, and pinning one would make the same skill on two branches two different skills. Mirror hosts are normalised first, because these repos are mirrored and without it the same skill from two hosts is two objects — the same defect returning through a side door. Existing skills carry no identity, so resolution is: exact sourceUrl -> ONE-TIME name fallback restricted to skills with NO sourceUrl -> stamp. The restriction is load-bearing: a name collision against a skill that already carries a DIFFERENT url is two genuinely different skills, and merging them would silently lose one. UPDATE, don't duplicate. Content (body/frontmatter/files/description) is replaced; maturity, target level, evidence, agent installations, createdBy, publishedAt, archivedAt and lastAcceptedVersionAt are carried forward explicitly — saveObject is PUT-semantic, so a key merely omitted would be nulled on write. RE-QUARANTINE on any content change, not just a worse scan verdict. An approval is a statement about specific content; once that content changes the statement no longer applies whatever a scanner says. Cost of re-quarantining is an unnecessary review; cost of the alternative is unreviewed content running under an old approval. LOCAL LEARNINGS ARE NEVER OVERWRITTEN. When learnings were accepted locally since the last sync AND the incoming learnings.md differs, the local file is kept and the rest of the update still lands. A warning issued after an overwrite is worthless and a confirmation prompt puts the destructive default one click away; keeping the file makes the loss unreachable. The clock is sourceUpdatedAt, NOT publishedAt. SkillConsolidationService::isBehind() compares against publishedAt, which is right for deciding whether to REPUBLISH — but publishedAt is stamped only when this instance publishes TO a remote, so on an instance that only installs it is empty and the comparison is silently always false. Building the guard on it would produce something that reviews as correct and never fires once. There is a dedicated test pinning this, with publishedAt deliberately set later than the accepted learnings. All three mutation checks confirmed red before being trusted: remove the identity match -> 2 failures clock switched to publishedAt -> 2 failures state preserved across change -> 1 failure 1415 tests OK, phpcs 0, phpmd 0, phpstan 0, psalm 0 — each run explicitly on PHP 8.3. * fix(gates): @SPEC on resolver helpers; correct the skill-bundle-publish anchor gate-16 wanted @SPEC on the three new public helpers. gate-46 caught a @SPEC path I introduced in #109 that never existed — the change files its specs under specs/skills-marketplace/, not specs/skill-bundle-publish/.
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
1 participant
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.
Prerequisite for OpenBuild's
apply-v2-channels(ConductionNL/openbuild#82). No behaviour change.Why
Bundle install lived as a private method on
SkillController, so the only way to install a bundle was to be an HTTP request. OpenBuild needs to install theskills/channel of a published app repo, and must not reimplement skill installation to do it — frontmatter byte-fidelity, aux-file placement and the ADR-068 §3 rule thatlearning-candidates.mdnever leaves the instance all have to keep living in exactly one implementation.What
SkillBundleInstallerwithinstallFromRepo(owner, repo, ref, …)andinstallParsed(parsed, createdBy)SkillController::installBundleSkills()now delegates — the HTTP route and the cross-app caller run identical code rather than two copies that driftOn the test wiring
SkillControllerTestcomposes a realSkillBundleInstallerover the same mocks rather than stubbing it. A mock there would have silently gutted every bundle-install count assertion in that file — the per-skill install path is precisely what they exist to check.Assertions went 4308 → 4362 across the same 1402 tests, confirming the path is still genuinely exercised.
Verification