Split skill unpack into a dedicated function#1165
Split skill unpack into a dedicated function#1165gorkem wants to merge 2 commits intokitops-ml:mainfrom
Conversation
Pulls --as-skill handling out of unpackRecursive, which was threaded with opts.SkillOptions guards across every layer case. unpackSkill now only walks prompt layers, skipping Kitfile write, parent-ref traversal, and remote-dataset handling that don't apply in skill mode. Signed-off-by: Gorkem Ercan <gorkem.ercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
Refactors --as-skill handling by extracting prompt-layer skill installation logic out of unpackRecursive into a dedicated unpackSkill function, simplifying control flow in the main unpack path.
Changes:
- Route
UnpackModelKittounpackSkillwhenopts.SkillOptionsis set. - Remove skill-mode conditionals, counters, and summary logic from
unpackRecursive. - Add
unpackSkillimplementation that scans prompt layers and installs skills fromSKILL.md.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if opts.SkillOptions != nil { | ||
| return unpackSkill(ctx, opts) | ||
| } |
There was a problem hiding this comment.
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.
| if layerDesc.Annotations[constants.LayerSubtypeAnnotation] != constants.LayerSubtypePrompt { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| 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)) | |
| } |
There was a problem hiding this comment.
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.
Callers always pass non-nil *UnpackOptions, but the existing L `opts != nil && opts.UnpackDir != ""` guard implied nil was possible while the rest of the function would dereference it unchecked. Return an explicit error at the API boundary instead. Signed-off-by: Gorkem Ercan <gorkem.ercan@gmail.com>
What
Extracts
--as-skillhandling out ofunpackRecursiveinto a dedicatedunpackSkillfunction inpkg/lib/filesystem/unpack/core.go.Why
opts.SkillOptionsguards were threaded through every layer case inunpackRecursive, making the control flow hard to follow. Skill unpack only cares about prompt layers — it doesn't write a Kitfile, traverse parent refs, or handle remote datasets. Separating the two paths removes dead branches from each function.