Skip to content

Split skill unpack into a dedicated function #1165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gorkem merged 2 commits into from
May 11, 2026
+93 −76
Merged

Split skill unpack into a dedicated function #1165

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d56ea44
Split skill unpack into a dedicated function
gorkem Apr 18, 2026
cc56894
Guard UnpackModelKit against nil options
gorkem Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 93 additions & 76 deletions pkg/lib/filesystem/unpack/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@ import (

// UnpackModelKit performs the core unpacking logic for a ModelKit.
func UnpackModelKit(ctx context.Context, opts *UnpackOptions) error {
if opts == nil {
return fmt.Errorf("unpack options must not be nil")
}
// If an unpack directory is provided, temporarily change the working directory
// so that unpack operations that are relative to CWD behave as expected.
// This centralizes tar -C semantics inside the unpack library.
if opts != nil && opts.UnpackDir != "" {
if opts.UnpackDir != "" {
// Ensure the directory exists
if err := os.MkdirAll(opts.UnpackDir, 0o755); err != nil {
return fmt.Errorf("failed to create unpack directory %s: %w", opts.UnpackDir, err)
Expand All @@ -66,6 +69,9 @@ func UnpackModelKit(ctx context.Context, opts *UnpackOptions) error {
}
}()
}
if opts.SkillOptions != nil {
return unpackSkill(ctx, opts)
}
Comment on lines +72 to +74

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnpackModelKit guards opts != nil above, but then unconditionally dereferences opts.SkillOptions. If opts is nil (which this function currently appears to allow), this will panic. Add an early opts == nil check (return a clear error) or change the condition to if opts != nil && opts.SkillOptions != nil before calling unpackSkill/unpackRecursive.

Copilot uses AI. Check for mistakes.

@gorkem gorkem Apr 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a check

return unpackRecursive(ctx, opts, []string{})
}

Expand Down Expand Up @@ -121,10 +127,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
// through the config's relevant field to get the correct path for unpacking
// We need to support older ModelKits (that were packed without diffIDs and digest
// in the config) for now, so we need to continue using the old structure.
// Skill-mode tracking
var skillErrors []error
var promptsFiltered, skillsFound int

var modelPartIdx, codeIdx, datasetIdx, docsIdx, promptIdx int
for _, layerDesc := range manifest.Layers {
// This variable supports older-format tar layers (that don't include the
Expand All @@ -144,9 +146,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
var layerInfo *artifact.LayerInfo
switch mediaType.Base() {
case mediatype.ModelBaseType:
if opts.SkillOptions != nil {
continue
}
entry := config.Model
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
continue
Expand All @@ -155,10 +154,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
output.Infof("Unpacking model %s to %s", config.Model.Name, config.Model.Path)

case mediatype.ModelPartBaseType:
if opts.SkillOptions != nil {
modelPartIdx += 1
continue
}
entry := config.Model.Parts[modelPartIdx]
modelPartIdx += 1
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
Expand All @@ -175,37 +170,9 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
continue
}

// Skill mode: install as agent skill
if opts.SkillOptions != nil {
promptsFiltered++
result, sErr := installPromptAsSkill(ctx, store, layerDesc, entry, mediaType.Compression(), opts)
if sErr != nil {
output.Logf(output.LogLevelWarn, "Error reading prompt %q: %s", entry.Path, sErr)
skillErrors = append(skillErrors, fmt.Errorf("prompt %q: %s", entry.Path, sErr))
} else if result != nil {
skillsFound++
for _, ar := range result.Agents {
if ar.Err != nil {
output.Infof("Failed to install skill '%s' for %s: %s", result.SkillName, ar.Agent, ar.Err)
skillErrors = append(skillErrors, fmt.Errorf("skill '%s' for %s: %s", result.SkillName, ar.Agent, ar.Err))
} else if ar.Skipped {
output.Infof("Skipped skill '%s' for %s: already exists (use -o to overwrite)", result.SkillName, ar.Agent)
} else {
output.Infof("Installed skill '%s' for %s → %s", result.SkillName, ar.Agent, ar.Path)
}
}
}
continue
}

layerInfo, layerPath = entry.LayerInfo, entry.Path
output.Infof("Unpacking prompt to %s", entry.Path)
} else {
if opts.SkillOptions != nil {
codeIdx += 1
continue
}
entry := config.Code[codeIdx]
codeIdx += 1
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
Expand All @@ -216,17 +183,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
}

case mediatype.DatasetBaseType:
if opts.SkillOptions != nil {
// Skip datasets but still advance the index past non-remote datasets
for idx := datasetIdx; idx < len(config.DataSets); idx++ {
if config.DataSets[idx].RemotePath != "" {
continue
}
datasetIdx = idx + 1
break
}
continue
}
// Since some datasets may be remote, we need to search the Kitfile for the next non-remote dataset
var entry *artifact.DataSet
for idx := datasetIdx; idx < len(config.DataSets); idx++ {
Expand All @@ -248,10 +204,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
output.Infof("Unpacking dataset %s to %s", entry.Name, entry.Path)

case mediatype.DocsBaseType:
if opts.SkillOptions != nil {
docsIdx += 1
continue
}
entry := config.Docs[docsIdx]
docsIdx += 1
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
Expand All @@ -269,11 +221,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
output.Logf(output.LogLevelWarn, "Unknown media type %s: skipping unpack", layerDesc.MediaType)
}

// In skill mode, prompts are handled above; nothing else to unpack
if opts.SkillOptions != nil {
continue
}

if layerInfo != nil {
if layerInfo.Digest != layerDesc.Digest.String() {
return fmt.Errorf("digest in config and manifest do not match in %s", mediaType.UserString())
Expand All @@ -292,22 +239,6 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
}
}

// Skill mode: end-of-loop summary
if opts.SkillOptions != nil {
if promptsFiltered == 0 {
output.Infof("No prompt layers matched the specified filters")
return nil
}
if skillsFound == 0 && len(skillErrors) == 0 {
output.Infof("No agent skills found in modelkit. Prompt layers must contain a SKILL.md file to be installed as skills.")
return nil
}
if len(skillErrors) > 0 {
return fmt.Errorf("failed to install %d skill(s)", len(skillErrors))
}
return nil
}

// Handle remotely stored files: first build a list so we can show a warning if remote files are skipped
remoteFiles := map[string]s3api.S3ObjectReference{}
for _, dataset := range config.DataSets {
Expand Down Expand Up @@ -369,6 +300,92 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
return nil
}

// unpackSkill handles the --as-skill flow: prompt layers containing SKILL.md
// are installed as agent skills and all other layer types are ignored.
// Unlike unpackRecursive, it does not unpack the Kitfile, traverse parent
// modelkits, or touch the filesystem outside the resolved skills directories.
func unpackSkill(ctx context.Context, opts *UnpackOptions) error {
ref := opts.ModelRef
store, err := getStoreForRef(ctx, opts)
if err != nil {
ref := artifact.FormatRepositoryForDisplay(opts.ModelRef.String())
return fmt.Errorf("failed to find reference %s: %s", ref, err)
}
manifestDesc, err := store.Resolve(ctx, ref.Reference)
if err != nil {
return fmt.Errorf("failed to resolve reference: %w", err)
}
manifest, err := util.GetManifest(ctx, store, manifestDesc)
if err != nil {
return fmt.Errorf("failed to read manifest: %s", err)
}
config, err := util.GetKitfileForManifest(ctx, store, manifest)
if err != nil {
if errors.Is(err, util.ErrNoKitfile) {
output.Infof("No Kitfile found in modelkit; no prompt layers to install as skills")
return nil
}
return err
}

var promptsFiltered, skillsFound, skillErrorCount, promptIdx int

for _, layerDesc := range manifest.Layers {
mediaType, err := mediatype.ParseMediaType(layerDesc.MediaType)
if err != nil {
output.Logf(output.LogLevelWarn, "Unknown media type %s: skipping", layerDesc.MediaType)
continue
}
if mediaType.Base() != mediatype.CodeBaseType {
continue
}
if layerDesc.Annotations[constants.LayerSubtypeAnnotation] != constants.LayerSubtypePrompt {
continue
}

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unpackSkill indexes config.Prompts[promptIdx] without checking bounds. If the Kitfile prompt list is missing entries or is inconsistent with the manifest prompt layers, this will panic. Consider validating promptIdx < len(config.Prompts) (and returning a descriptive error) before indexing, similar to how other mismatches are handled.

Suggested change
if promptIdx >= len(config.Prompts) {
return fmt.Errorf("manifest contains more prompt layers than kitfile entries: prompt index %d out of range (prompts=%d)", promptIdx, len(config.Prompts))
}

Copilot uses AI. Check for mistakes.

@gorkem gorkem Apr 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manifest's prompt layers and config.Prompts are produced 1:1 by the pack step from the same source, so promptIdx can't exceed len(config.Prompts) unless the modelkit is corrupt.

entry := config.Prompts[promptIdx]
promptIdx++
if !kitfile.LayerMatchesAnyFilter(entry, opts.FilterConfs) {
continue
}

promptsFiltered++
result, sErr := installPromptAsSkill(ctx, store, layerDesc, entry, mediaType.Compression(), opts)
if sErr != nil {
output.Logf(output.LogLevelWarn, "Error reading prompt %q: %s", entry.Path, sErr)
skillErrorCount++
continue
}
if result == nil {
continue
}
skillsFound++
for _, ar := range result.Agents {
if ar.Err != nil {
output.Infof("Failed to install skill '%s' for %s: %s", result.SkillName, ar.Agent, ar.Err)
skillErrorCount++
} else if ar.Skipped {
output.Infof("Skipped skill '%s' for %s: already exists (use -o to overwrite)", result.SkillName, ar.Agent)
} else {
output.Infof("Installed skill '%s' for %s → %s", result.SkillName, ar.Agent, ar.Path)
}
}
}

if promptsFiltered == 0 {
output.Infof("No prompt layers matched the specified filters")
return nil
}
if skillsFound == 0 && skillErrorCount == 0 {
output.Infof("No agent skills found in modelkit. Prompt layers must contain a SKILL.md file to be installed as skills.")
return nil
}
if skillErrorCount > 0 {
return fmt.Errorf("failed to install %d skill(s)", skillErrorCount)
}
return nil
}

func unpackParent(ctx context.Context, ref string, optsIn *UnpackOptions, visitedRefs []string) error {
if idx := getIndex(visitedRefs, ref); idx != -1 {
cycleStr := fmt.Sprintf("[%s=>%s]", strings.Join(visitedRefs[idx:], "=>"), ref)
Expand Down