Support multi-client and all-clients skill installation#4732
Open
Support multi-client and all-clients skill installation#4732
Conversation
samuv
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
jhrozek,
lujunsan,
rdimitrov and
yrobla
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4732 +/- ##
=======================================
Coverage ? 68.79%
=======================================
Files ? 516
Lines ? 54390
Branches ? 0
=======================================
Hits ? 37415
Misses ? 14115
Partials ? 2860 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/L
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
github-actions
bot
added
size/L
github-actions
bot
added
size/L
| } | ||
| for _, ct := range toWrite { | ||
| dir := filepath.Clean(clientDirs[ct]) | ||
| if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] |
| ) (*skills.InstallResult, error) { | ||
| for _, ct := range clientTypes { | ||
| dir := filepath.Clean(clientDirs[ct]) | ||
| if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] |
| var written []string | ||
| for _, ct := range toWrite { | ||
| dir := filepath.Clean(clientDirs[ct]) | ||
| if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] |
| ) (*skills.InstallResult, error) { | ||
| for _, ct := range clientTypes { | ||
| dir := filepath.Clean(clientDirs[ct]) | ||
| if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] |
Collaborator
|
Shall we add support for |
Contributor
Author
yeah, why not! |
samuv
changed the title
github-actions
bot
added
size/XL
Contributor
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
github-actions
bot
dismissed
their stale review
Large PR justification has been provided. Thank you!
github-actions
bot
added
size/XL
samuv
force-pushed
the
skills-install-multi-client
branch
from
6da5bc0 to
9ea4a88
Compare
Add InstallOptions.Clients, resolve and validate client paths, multi-dir extract/write with rollback, and legacy empty-Clients digest no-op. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Breaking: install body field client is replaced by clients []string. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Mirror API install body; include group on install request DTO. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Comma-separated values map to InstallOptions.Clients. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Update OpenAPI install schema and CLI/skill user references. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Extract git and OCI install paths into smaller helpers, add missingClients and removeSkillDirs, and fix import order in tests. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Cover legacy no-op with explicit client, upgrade extracting to all existing clients, and multi-client rollback and error paths.
Apply filepath.Clean at path resolution time and validate against traversal segments before any filesystem operation.
Apply filepath.Clean at every point where a path is retrieved from the clientDirs/allDirs map before a filesystem operation (os.Stat, Extract, WriteFiles, Remove). CodeQL's taint tracker loses the sanitization done at storage time once the value flows through a map; cleaning at retrieval makes the guard visible to the analyzer at each individual sink. Signed-off-by: Samuele Verzi <samu@stacklok.com> Made-with: Cursor
The four os.Stat(dir) pre-checks in applyGitInstallExisting, applyGitInstallFresh, installExtractionSameDigestNewClients, and installExtractionFresh are flagged by CodeQL's go/path-injection query because 'dir' originates (transitively) from user-supplied client names. The paths are safe: client names are validated against the known skill-supporting client list, skill names pass validateLocalPath, and PathResolver.GetSkillPath constructs paths from fixed base directories which are then confirmed absolute and traversal-free by validateResolvedDir. Add the same // lgtm[go/path-injection] suppression used in pkg/skills/gitresolver/writer.go for the identical taint flow. Signed-off-by: Samuele Verzi <samu@stacklok.com> Made-with: Cursor
github-actions
bot
added
size/XL
samuv
force-pushed
the
skills-install-multi-client
branch
from
9ea4a88 to
67250d4
Compare
JAORMX
approved these changes
Apr 10, 2026
github-actions
bot
added
size/XL
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
3 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.
Summary
Extends the skill install API and CLI to install a skill to multiple clients in a single operation, with atomicity (all-or-nothing) and rollback on failure.
API / CLI changes
client(single string) is replaced byclients(array of strings) — breaking changeclientsor passing["all"]installs to every available skill-supporting client (previously defaulted to the first client only)"all"with explicit client names returns HTTP 400Storage
InstalledSkill.Clientsis populated and merged correctly across fresh installs, same-digest partial installs, and digest upgradesInternals
resolveAndValidateClientshandles the empty / "all" / explicit-list cases via a single switchLarge PR Justification
docs/server/docs.go,swagger.json,swagger.yaml) cannot be separated from the source change that drives them — they must land in the same commit to keep the spec in sync with the implementation.