feat(skill-loader): include sibling .md files and script contents #10
Conversation
- Add CATEGORY_SKILLS mapping for auto-injecting skills per category - Add getCategorySkills() with user override support via config - Add category_skills schema for user customization - Fix Sisyphus→Musashi branding in skill-content tests - Fix CI workflow to use Musashi agent config - Add session files to .gitignore - Fix ToolContext mock in tests (metadata/ask props) - Update to @opencode-ai/plugin@1.1.30
- Remove depth=0 guard in collectMdFilesRecursive, add skipFiles param - Load file content inline for supporting files under 50KB - Format supporting files with syntax-highlighted code blocks - Add language detection for 20+ file extensions
Greptile SummaryThis PR enables skill-loader to include sibling .md files and script contents in agent context, solving the issue where agents saw references to supporting files but couldn't access them. Key changes:
Issues found:
Edge cases handled well:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as Claude Agent
participant Loader as loadSkillFromPath()
participant Utils as collectMdFilesRecursive()
participant Support as discoverSupportingFiles()
participant FS as File System
Agent->>Loader: Load skill directory
Loader->>FS: Read SKILL.md
FS-->>Loader: Main skill content
Loader->>Utils: collectMdFilesRecursive(dir, 0, 3, '', skipFiles={'SKILL.md'})
Utils->>FS: Scan directory
loop For each entry
alt Is directory
Utils->>Utils: Recurse (depth+1)
else Is .md file
alt currentDepth==0 && skipFiles.has(name)
Utils->>Utils: Skip SKILL.md at root
else
Utils->>FS: Read .md file
FS-->>Utils: File content
end
end
end
Utils-->>Loader: Sibling .md files (no SKILL.md)
Loader->>Support: discoverSupportingFiles(dir)
Support->>FS: Scan recursively for non-.md files
loop For each file (max 25, <1MB, <10MB total)
Support->>FS: Get file stats
alt size <= 50KB
Support->>FS: Read file as UTF-8
FS-->>Support: File content (or undefined if binary)
end
Support->>Support: Store metadata + content
end
Support-->>Loader: Supporting files with inline content
Loader->>Loader: Format with syntax highlighting
Note over Loader: For each file with content:<br/>```language<br/>content<br/>```
Loader-->>Agent: Complete skill template with supporting files
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (5)
-
src/features/opencode-skill-loader/supporting-files.ts, line 98 (link)syntax: comment says "max 20 files" but
MAX_FILESis now 25 -
src/features/opencode-skill-loader/supporting-files.ts, line 101 (link)syntax: return type comment says "no file contents" but files now include
contentfield -
src/features/opencode-skill-loader/supporting-files.test.ts, line 65 (link)syntax: test name says "20 files" but
MAX_FILESis now 25 -
src/features/opencode-skill-loader/supporting-files.test.ts, line 73 (link)syntax: assertion still checks for 20 but limit is now 25
-
src/features/opencode-skill-loader/supporting-files.test.ts, line 109 (link)style: test doesn't verify the new
contentfield exists for files under 50KB. Add assertion likeexpect(file.content).toBeDefined()orexpect(file.content).toBe('{"key":"value"}')
18 files reviewed, 7 comments
| } | ||
|
|
||
| if (stats.size <= INLINE_CONTENT_MAX_SIZE_BYTES) { | ||
| supportingFile.content = await fs.readFile(entryPath, 'utf-8').catch(() => undefined) |
There was a problem hiding this comment.
logic: reading binary files (images, PDFs, executables, etc.) as UTF-8 will produce garbled text or fail silently. The .catch(() => undefined) hides errors, so binary files get content: undefined but still appear in the list. Consider checking file extension against a text-file allowlist before attempting UTF-8 read.
| supportingFile.content = await fs.readFile(entryPath, 'utf-8').catch(() => undefined) | |
| const isTextFile = ['.ts', '.tsx', '.js', '.jsx', '.json', '.sh', '.bash', '.zsh', '.py', '.rb', '.rs', '.go', '.yaml', '.yml', '.toml', '.xml', '.html', '.css', '.sql', '.txt', '.md'].includes(extname(entry.name).toLowerCase()) | |
| if (isTextFile) { | |
| supportingFile.content = await fs.readFile(entryPath, 'utf-8').catch(() => undefined) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/opencode-skill-loader/supporting-files.ts
Line: 83:83
Comment:
**logic:** reading binary files (images, PDFs, executables, etc.) as UTF-8 will produce garbled text or fail silently. The `.catch(() => undefined)` hides errors, so binary files get `content: undefined` but still appear in the list. Consider checking file extension against a text-file allowlist before attempting UTF-8 read.
```suggestion
const isTextFile = ['.ts', '.tsx', '.js', '.jsx', '.json', '.sh', '.bash', '.zsh', '.py', '.rb', '.rs', '.go', '.yaml', '.yml', '.toml', '.xml', '.html', '.css', '.sql', '.txt', '.md'].includes(extname(entry.name).toLowerCase())
if (isTextFile) {
supportingFile.content = await fs.readFile(entryPath, 'utf-8').catch(() => undefined)
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (f.content) { | ||
| const lang = getLanguageFromExtension(f.extension) | ||
| return `${header}\n\`\`\`${lang}\n${f.content}\n\`\`\`` |
There was a problem hiding this comment.
style: if f.content contains triple backticks (```), the markdown rendering will break. Consider escaping or warning about this edge case in large codebases. Do skills typically contain files with triple backticks in their content?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/opencode-skill-loader/loader.ts
Line: 89:91
Comment:
**style:** if `f.content` contains triple backticks (\`\`\`), the markdown rendering will break. Consider escaping or warning about this edge case in large codebases. Do skills typically contain files with triple backticks in their content?
How can I resolve this? If you propose a fix, please make it concise.…Template - Remove console.warn logs from supporting-files.ts (file limit logs on startup) - Fix extractSkillTemplate to use skill.lazyContent.load() instead of raw file read - Update test expectations to match new MAX_FILES limit (25)
- Update docstring: max files 20→25, add inline content behavior - Update test: verify content field exists for small files
There was a problem hiding this comment.
Additional Comments (1)
-
src/features/opencode-skill-loader/loader.ts, line 98 (link)logic: async-loader.ts:98 still calls
collectMdFilesRecursive()without theskipFilesparam, meaning SKILL.md will be included twice in async loader (once as main content, once in subdirFiles). This inconsistency between loader.ts:78 and async-loader.ts:98 causes duplication in async loading path.
20 files reviewed, 2 comments
| // Plural takes priority (loaded first) as it's the newer convention | ||
| const skillsSingular = join(homedir(), ".config", "opencode", "skill") | ||
| const skillsPlural = join(homedir(), ".config", "opencode", "skills") | ||
| const [singular, plural] = await Promise.all([ | ||
| loadSkillsFromDir(skillsSingular, "opencode"), | ||
| loadSkillsFromDir(skillsPlural, "opencode"), | ||
| ]) | ||
| return [...singular, ...plural] | ||
| return [...plural, ...singular] |
There was a problem hiding this comment.
logic: comment says "loaded first" but in [...plural, ...singular], plural is actually first in the array. For LoadedSkill[] arrays, order matters differently than for Record conversion. In lines 228 and 240, [...singular, ...plural] gives plural priority via skillsToRecord() (last wins). Here, [...plural, ...singular] puts plural first, which may not give it priority depending on how the array is consumed downstream.
verify the intended priority behavior matches the comment and implementation. Does the order matter for LoadedSkill[] arrays in the same way as for Record conversion, or is first-in-array actually the correct priority here?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/opencode-skill-loader/loader.ts
Line: 306:313
Comment:
**logic:** comment says "loaded first" but in `[...plural, ...singular]`, plural is actually first in the array. For `LoadedSkill[]` arrays, order matters differently than for `Record` conversion. In lines 228 and 240, `[...singular, ...plural]` gives plural priority via `skillsToRecord()` (last wins). Here, `[...plural, ...singular]` puts plural first, which may not give it priority depending on how the array is consumed downstream.
verify the intended priority behavior matches the comment and implementation. Does the order matter for LoadedSkill[] arrays in the same way as for Record conversion, or is first-in-array actually the correct priority here?
How can I resolve this? If you propose a fix, please make it concise.
Summary
Skills like
superpowers:systematic-debuggingcontain multiple supporting files (.md docs, .sh scripts, .ts examples) but only SKILL.md content was loaded into agent context. Agents saw references like "See root-cause-tracing.md in this directory" but couldn't access that content.Changes
currentDepth > 0guard incollectMdFilesRecursive(), addskipFilesparam to exclude SKILL.md at root (avoids duplication)content?: stringfield toSupportingFileinterface, read file content inline for files under 50KBnew Set(['SKILL.md'])to skip duplication, format supporting files with syntax-highlighted code blocksGuardrails
Testing