Skip to content

Commit 72fc15f

Browse files
gorkemgorkem
committed
Refine skill name sanitization fallback behavior
Make sanitizeName return empty for invalid/blank input and move "unnamed-skill" fallback into DeriveSkillName so fallback priority is preserved before defaulting. Signed-off-by: Gorkem Ercan <gorkem.ercan@gmail.com>
1 parent c337717 commit 72fc15f

3 files changed

Lines changed: 65 additions & 137 deletions

File tree

‎pkg/lib/filesystem/unpack/core.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -525,10 +525,7 @@ func installPromptAsSkill(ctx context.Context, store content.Storage, desc ocisp
525525
}
526526

527527
skillName := skill.DeriveSkillName(frontmatterName, entry, opts.ModelRef)
528-
result, err := skill.InstallSkill(entries, skillName, entry, opts.SkillOptions)
529-
if err != nil {
530-
return nil, err
531-
}
528+
result := skill.InstallSkill(entries, skillName, entry, opts.SkillOptions)
532529
return &result, nil
533530
}
534531

‎pkg/lib/skill/install.go‎

Lines changed: 53 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -82,97 +82,48 @@ func resolveSkillsDir(agentName, projectDir string) (string, error) {
8282
return GetGlobalSkillsDir(agentName)
8383
}
8484

85-
// normalizePath converts a path to use forward slashes and cleans it. Tar
86-
// entries always use forward slashes, but prompt paths from Kitfiles packed on
87-
// Windows may contain backslashes. Replacing backslashes with forward slashes
88-
// before cleaning ensures consistent comparison on all platforms.
89-
func normalizePath(p string) string {
90-
return path.Clean(strings.ReplaceAll(p, "\\", "/"))
91-
}
92-
93-
// stripPromptPrefix removes the prompt's path prefix from tar entry names so
94-
// that files are installed directly under the skill directory instead of nested
95-
// under the original directory structure from the pack context.
85+
// relativeEntryName computes the path an entry should be written to relative
86+
// to the skill directory. It strips the prompt path prefix so that files land
87+
// at the skill root instead of being nested under the original pack structure.
9688
//
97-
// For example, if the prompt path is "skills/docx/" and a tar entry is
98-
// "skills/docx/SKILL.md", the entry name becomes "SKILL.md".
99-
// For a single-file prompt like "SKILL.md", the entry name is unchanged.
89+
// Tar entries always use forward slashes. Prompt paths from Kitfiles packed on
90+
// Windows may contain backslashes, so both are normalized to forward slashes.
10091
//
101-
// Returns an error if stripping would produce zero entries (indicates a
102-
// separator mismatch or path alignment bug).
103-
func stripPromptPrefix(entries []TarEntry, promptPath string) ([]TarEntry, error) {
104-
if len(entries) == 0 {
105-
return entries, nil
92+
// Returns empty string for entries that should be skipped (e.g. the directory
93+
// entry matching the prefix itself).
94+
func relativeEntryName(entryName, promptPath string) string {
95+
normalize := func(p string) string {
96+
return path.Clean(strings.ReplaceAll(p, "\\", "/"))
10697
}
10798

108-
// Normalize to forward slashes so comparisons work regardless of
109-
// whether the Kitfile was packed on Windows or Unix.
110-
prefix := normalizePath(promptPath)
99+
prefix := normalize(promptPath)
100+
cleaned := normalize(entryName)
111101

112-
// "." means the entire context directory is the prompt — entries are
113-
// already at root, no prefix to strip.
102+
// "." prefix means entries are already at root
114103
if prefix == "." {
115-
return entries, nil
104+
return cleaned
116105
}
117106

118-
// Single file prompt — one entry whose name matches the prefix
119-
if len(entries) == 1 && entries[0].Header.Typeflag != tar.TypeDir && normalizePath(entries[0].Header.Name) == prefix {
120-
entry := entries[0]
121-
entry.Header = cloneHeader(entry.Header)
122-
entry.Header.Name = path.Base(prefix)
123-
return []TarEntry{entry}, nil
107+
// Single file: entry name matches the prefix exactly — use just the filename
108+
if cleaned == prefix {
109+
return path.Base(cleaned)
124110
}
125111

126-
// Directory prompt — strip the normalized prefix from all descendant entries.
127-
// Since both sides use forward slashes, do this by removing prefix + "/"
128-
// from matching paths rather than using filepath-specific path handling.
129-
result := make([]TarEntry, 0, len(entries))
130-
for _, entry := range entries {
131-
cleaned := normalizePath(entry.Header.Name)
132-
// If cleaned == prefix, it's the directory entry itself — skip it.
133-
// Otherwise, if cleaned starts with prefix + "/", take everything after it.
134-
if cleaned == prefix {
135-
continue
136-
}
137-
trimmed := strings.TrimPrefix(cleaned, prefix+"/")
138-
if trimmed == cleaned {
139-
// Entry is outside the prefix — skip
140-
continue
141-
}
142-
if trimmed == "" || strings.HasPrefix(trimmed, "..") {
143-
continue
144-
}
145-
newEntry := TarEntry{
146-
Header: cloneHeader(entry.Header),
147-
Content: entry.Content,
148-
}
149-
newEntry.Header.Name = trimmed
150-
result = append(result, newEntry)
112+
// Directory: strip prefix + "/" from the entry
113+
trimmed := strings.TrimPrefix(cleaned, prefix+"/")
114+
if trimmed == cleaned {
115+
return "" // outside the prefix
151116
}
152-
153-
if len(result) == 0 {
154-
return nil, fmt.Errorf("prompt path '%s' does not match any tar entries in the layer", promptPath)
117+
if trimmed == "" || strings.HasPrefix(trimmed, "..") {
118+
return ""
155119
}
156-
return result, nil
157-
}
158-
159-
func cloneHeader(h *tar.Header) *tar.Header {
160-
clone := *h
161-
return &clone
120+
return trimmed
162121
}
163122

164123
// InstallSkill writes the buffered tar entries as a skill to each agent's
165124
// skill directory. Does NOT fail-fast: attempts every agent, returns
166-
// per-agent results. Returns an error if the entries cannot be prepared
167-
// (e.g. prefix stripping fails), in which case no agents are attempted.
168-
func InstallSkill(entries []TarEntry, skillName string, prompt artifact.Prompt, opts *SkillInstallOptions) (InstallResult, error) {
169-
// Strip the prompt's path prefix so files land at the skill root
170-
stripped, err := stripPromptPrefix(entries, prompt.Path)
171-
if err != nil {
172-
return InstallResult{}, err
173-
}
174-
entries = stripped
175-
125+
// per-agent results.
126+
func InstallSkill(entries []TarEntry, skillName string, prompt artifact.Prompt, opts *SkillInstallOptions) InstallResult {
176127
result := InstallResult{
177128
SkillName: skillName,
178129
Prompt: prompt,
@@ -183,7 +134,7 @@ func InstallSkill(entries []TarEntry, skillName string, prompt artifact.Prompt,
183134
agent string
184135
path string
185136
}
186-
seen := map[string][]string{} // resolved path : list of agent names
137+
seen := map[string][]string{}
187138
var order []agentPath
188139

189140
for _, agent := range opts.Agents {
@@ -206,9 +157,8 @@ func InstallSkill(entries []TarEntry, skillName string, prompt artifact.Prompt,
206157
continue
207158
}
208159

209-
if agents, ok := seen[absDir]; ok {
210-
// Duplicate path — record as skipped
211-
seen[absDir] = append(agents, agent)
160+
if _, ok := seen[absDir]; ok {
161+
seen[absDir] = append(seen[absDir], agent)
212162
result.Agents = append(result.Agents, AgentInstallResult{
213163
Agent: agent,
214164
Path: absDir,
@@ -221,33 +171,43 @@ func InstallSkill(entries []TarEntry, skillName string, prompt artifact.Prompt,
221171
}
222172

223173
for _, ap := range order {
224-
agentResult := installForAgent(entries, skillName, ap.agent, ap.path, opts)
174+
agentResult := installForAgent(entries, skillName, prompt.Path, ap.agent, ap.path, opts)
225175
result.Agents = append(result.Agents, agentResult)
226176
}
227177

228-
return result, nil
178+
return result
229179
}
230180

231181
// installForAgent handles installation for a single agent at a resolved path.
232-
func installForAgent(entries []TarEntry, skillName, agent, skillDir string, opts *SkillInstallOptions) AgentInstallResult {
182+
// It strips the prompt path prefix from entry names at write time.
183+
func installForAgent(entries []TarEntry, skillName, promptPath, agent, skillDir string, opts *SkillInstallOptions) AgentInstallResult {
233184
errResult := func(err error) AgentInstallResult {
234185
return AgentInstallResult{Agent: agent, Path: skillDir, Err: err}
235186
}
236187

237-
// Validate skill name directly — it must be a safe relative path component
188+
// Validate skill name — it must be a safe relative path component
238189
if _, _, err := filesystem.VerifySubpath(".", skillName); err != nil {
239190
return errResult(fmt.Errorf("invalid skill name '%s': %w", skillName, err))
240191
}
241192

242193
// Validate all entries before writing anything
194+
var hasWritableEntries bool
243195
for _, entry := range entries {
244196
if entry.Header.Name == "" {
245197
return errResult(fmt.Errorf("tar entry with empty name"))
246198
}
247-
// Verify entry resolves inside skill directory
248-
if _, _, err := filesystem.VerifySubpath(skillDir, entry.Header.Name); err != nil {
199+
rel := relativeEntryName(entry.Header.Name, promptPath)
200+
if rel == "" {
201+
continue // directory prefix entry — will be skipped during write
202+
}
203+
if _, _, err := filesystem.VerifySubpath(skillDir, rel); err != nil {
249204
return errResult(fmt.Errorf("illegal file path in prompt layer: %s", entry.Header.Name))
250205
}
206+
hasWritableEntries = true
207+
}
208+
209+
if !hasWritableEntries {
210+
return errResult(fmt.Errorf("prompt path '%s' does not match any tar entries in the layer", promptPath))
251211
}
252212

253213
// Check if skill directory already exists
@@ -267,29 +227,27 @@ func installForAgent(entries []TarEntry, skillName, agent, skillDir string, opts
267227
return errResult(fmt.Errorf("creating skill directory: %w", err))
268228
}
269229

230+
// Write entries, stripping the prompt path prefix at write time
270231
for _, entry := range entries {
271-
outPath := filepath.Join(skillDir, entry.Header.Name)
272-
273-
// Verify the resolved write path is inside the skill directory.
274-
// This is a defense-in-depth check — entry names were validated above,
275-
// but filepath.Join can behave differently from VerifySubpath on edge cases.
276-
if _, _, err := filesystem.VerifySubpath(skillDir, entry.Header.Name); err != nil {
277-
return errResult(fmt.Errorf("illegal write path: %s", entry.Header.Name))
232+
rel := relativeEntryName(entry.Header.Name, promptPath)
233+
if rel == "" {
234+
continue
278235
}
236+
outPath := filepath.Join(skillDir, rel)
279237

280238
switch entry.Header.Typeflag {
281239
case tar.TypeDir:
282240
if err := os.MkdirAll(outPath, entry.Header.FileInfo().Mode()); err != nil {
283-
return errResult(fmt.Errorf("creating directory %s: %w", entry.Header.Name, err))
241+
return errResult(fmt.Errorf("creating directory %s: %w", rel, err))
284242
}
285243
default: // tar.TypeReg
286244
if dir := filepath.Dir(outPath); dir != skillDir {
287245
if err := os.MkdirAll(dir, 0755); err != nil {
288-
return errResult(fmt.Errorf("creating parent directory for %s: %w", entry.Header.Name, err))
246+
return errResult(fmt.Errorf("creating parent directory for %s: %w", rel, err))
289247
}
290248
}
291249
if err := os.WriteFile(outPath, entry.Content, entry.Header.FileInfo().Mode()); err != nil {
292-
return errResult(fmt.Errorf("writing file %s: %w", entry.Header.Name, err))
250+
return errResult(fmt.Errorf("writing file %s: %w", rel, err))
293251
}
294252
}
295253
}

‎pkg/lib/skill/install_test.go‎

Lines changed: 11 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,7 @@ func TestInstallSkill_SingleFile(t *testing.T) {
4545
ProjectDir: tmpDir,
4646
}
4747

48-
result, err := InstallSkill(entries, "my-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
49-
if err != nil {
50-
t.Fatalf("InstallSkill error: %v", err)
51-
}
48+
result := InstallSkill(entries, "my-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
5249
if result.HasErrors() {
5350
for _, e := range result.Errors() {
5451
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -87,10 +84,7 @@ func TestInstallSkill_MultipleAgents(t *testing.T) {
8784
ProjectDir: tmpDir,
8885
}
8986

90-
result, err := InstallSkill(entries, "test-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
91-
if err != nil {
92-
t.Fatalf("InstallSkill error: %v", err)
93-
}
87+
result := InstallSkill(entries, "test-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
9488
if result.HasErrors() {
9589
for _, e := range result.Errors() {
9690
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -128,10 +122,7 @@ func TestInstallSkill_PathDedup(t *testing.T) {
128122
ProjectDir: tmpDir,
129123
}
130124

131-
result, err := InstallSkill(entries, "test-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
132-
if err != nil {
133-
t.Fatalf("InstallSkill error: %v", err)
134-
}
125+
result := InstallSkill(entries, "test-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
135126
if result.HasErrors() {
136127
for _, e := range result.Errors() {
137128
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -186,10 +177,7 @@ func TestInstallSkill_IgnoreExisting(t *testing.T) {
186177
IgnoreExisting: true,
187178
}
188179

189-
result, err := InstallSkill(entries, "existing-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
190-
if err != nil {
191-
t.Fatalf("InstallSkill error: %v", err)
192-
}
180+
result := InstallSkill(entries, "existing-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
193181
if result.HasErrors() {
194182
t.Errorf("unexpected errors: %v", result.Errors())
195183
}
@@ -231,10 +219,7 @@ func TestInstallSkill_Overwrite(t *testing.T) {
231219
Overwrite: true,
232220
}
233221

234-
result, err := InstallSkill(entries, "overwrite-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
235-
if err != nil {
236-
t.Fatalf("InstallSkill error: %v", err)
237-
}
222+
result := InstallSkill(entries, "overwrite-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
238223
if result.HasErrors() {
239224
t.Errorf("unexpected errors: %v", result.Errors())
240225
}
@@ -269,10 +254,7 @@ func TestInstallSkill_ExistingWithoutFlags(t *testing.T) {
269254
ProjectDir: tmpDir,
270255
}
271256

272-
result, err := InstallSkill(entries, "existing-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
273-
if err != nil {
274-
t.Fatalf("InstallSkill error: %v", err)
275-
}
257+
result := InstallSkill(entries, "existing-skill", artifact.Prompt{Path: "SKILL.md"}, opts)
276258
if !result.HasErrors() {
277259
t.Error("expected error when directory exists without -o or -i")
278260
}
@@ -316,10 +298,7 @@ func TestInstallSkill_DirectoryPrefixStripping(t *testing.T) {
316298
ProjectDir: tmpDir,
317299
}
318300

319-
result, err := InstallSkill(entries, "docx", artifact.Prompt{Path: "skills/docx/"}, opts)
320-
if err != nil {
321-
t.Fatalf("InstallSkill error: %v", err)
322-
}
301+
result := InstallSkill(entries, "docx", artifact.Prompt{Path: "skills/docx/"}, opts)
323302
if result.HasErrors() {
324303
for _, e := range result.Errors() {
325304
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -367,10 +346,7 @@ func TestInstallSkill_WindowsPromptPathOnUnix(t *testing.T) {
367346
}
368347

369348
// Backslash path as stored in Kitfile from Windows pack
370-
result, err := InstallSkill(entries, "docx", artifact.Prompt{Path: "skills\\docx\\"}, opts)
371-
if err != nil {
372-
t.Fatalf("InstallSkill error: %v", err)
373-
}
349+
result := InstallSkill(entries, "docx", artifact.Prompt{Path: "skills\\docx\\"}, opts)
374350
if result.HasErrors() {
375351
for _, e := range result.Errors() {
376352
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -414,10 +390,7 @@ func TestInstallSkill_DotPath(t *testing.T) {
414390
ProjectDir: tmpDir,
415391
}
416392

417-
result, err := InstallSkill(entries, "copy-edit", artifact.Prompt{Path: "."}, opts)
418-
if err != nil {
419-
t.Fatalf("InstallSkill error: %v", err)
420-
}
393+
result := InstallSkill(entries, "copy-edit", artifact.Prompt{Path: "."}, opts)
421394
if result.HasErrors() {
422395
for _, e := range result.Errors() {
423396
t.Errorf("agent %s error: %v", e.Agent, e.Err)
@@ -456,8 +429,8 @@ func TestInstallSkill_PrefixMismatchErrors(t *testing.T) {
456429
ProjectDir: tmpDir,
457430
}
458431

459-
_, err := InstallSkill(entries, "test", artifact.Prompt{Path: "wrong-dir/"}, opts)
460-
if err == nil {
432+
result := InstallSkill(entries, "test", artifact.Prompt{Path: "wrong-dir/"}, opts)
433+
if !result.HasErrors() {
461434
t.Error("expected error when prompt path doesn't match tar entries")
462435
}
463436
}

0 commit comments

Comments
 (0)