feat(skill-loader): Claude Code skill loader parity + fix slashcommand builtin discovery

[codewithkenzo]

Summary

Implements full Claude Code skill loader parity + fixes slashcommand builtin command discovery bug.

Changes

Skill Loader Parity (7 tasks)

• Extract collectMdFilesRecursive to shared utils
• Extend SkillMetadata with Claude Code frontmatter fields
• Implement {{OPENCODE_SESSION_ID}} substitution
• Implement shell preprocessing with security sandbox
• Implement supporting files discovery
• Implement context: fork via session API
• Implement user-invocable and disable-model-invocation filtering

Bug Fix: Slashcommand Builtin Discovery

• Problem: /start-work, /ralph-loop, etc. were not discoverable via slashcommand tool
• Fix: Added getBuiltinCommandsAsInfoArray() and updated discoverCommandsSync() to include builtins

Test Coverage

• 103+ tests passing (skill-loader)
• Typecheck passes
• Build passes

Commits (8)

1. refactor(skill-loader): extract collectMdFilesRecursive to shared utils
2. feat(skill-loader): add Claude Code frontmatter fields to SkillMetadata
3. feat(skill-loader): implement session ID substitution
4. feat(skill-loader): implement shell preprocessing with security sandbox
5. feat(skill-loader): implement supporting files discovery
6. feat(skill-loader): implement context fork via session API
7. feat(skill-loader): implement user-invocable and model-invocation filtering
8. fix(slashcommand): include builtin commands in slash command discovery

[codewithkenzo]

@greptileai review

[greptile-apps]

Greptile Summary

• Implements Claude Code skill loader parity with 7 new features: shell preprocessing with security sandbox, supporting files discovery, session ID substitution, context forking via session API, and user-invocable/model-invocation filtering
• Fixes slashcommand builtin command discovery bug where /start-work, /ralph-loop commands were not discoverable via the slashcommand tool by adding getBuiltinCommandsAsInfoArray() function
• Adds comprehensive test coverage with 103+ tests for all new features following project's BDD conventions with proper test isolation and cleanup

Important Files Changed

Filename	Overview
src/features/opencode-skill-loader/loader.ts	Missing deduplication logic for dual skill/skills path loading, causing potential duplicate skill registration
src/features/opencode-skill-loader/async-loader.ts	Partial parity implementation missing shell preprocessing and supporting files features that exist in sync loader
src/hooks/skill-invocation-filter/index.ts	Race condition: 5-second auto-cleanup timer conflicts with skills taking longer than 5 seconds to execute
src/features/opencode-skill-loader/shell-preprocessing.ts	Security-critical shell command sandbox implementation with allowlist validation and output limits

Confidence score: 3/5

• This PR has significant functionality gaps and race conditions that could cause production issues
• Score lowered due to: missing deduplication logic in loader.ts causing duplicate skills, async/sync loader parity gaps creating inconsistent behavior, and race condition in skill-invocation-filter where cleanup timer interferes with long-running skills
• Pay close attention to src/features/opencode-skill-loader/loader.ts for the duplicate skill loading issue and src/hooks/skill-invocation-filter/index.ts for the timing race condition

Sequence Diagram

sequenceDiagram
    participant User
    participant "Auto Slash Command Hook" as Hook
    participant "Slash Command Executor" as Executor
    participant "Builtin Commands" as BuiltinCmds
    participant "Skill Loader" as SkillLoader
    participant "Session API" as Session

    User->>Hook: "User types /start-work project-setup"
    Hook->>Hook: "detectSlashCommand()"
    Hook->>Executor: "executeSlashCommand(parsed)"
    Executor->>Executor: "discoverAllCommands()"
    Executor->>BuiltinCmds: "getBuiltinCommandsAsInfoArray()"
    BuiltinCmds-->>Executor: "builtin commands list"
    Executor->>SkillLoader: "discoverAllSkills()"
    SkillLoader-->>Executor: "skills list"
    Executor->>Executor: "findCommand('start-work')"
    Executor->>Executor: "formatCommandTemplate()"
    Executor-->>Hook: "success with replacement text"
    Hook->>Hook: "markSessionAsSlashCommand()"
    Hook->>Hook: "replace message with template"

Loading

[greptile-apps]

Additional Comments (1)

1. src/tools/skill/tools.ts, line 1433-1445 (link)

   logic: Polling checks currentMsgCount then session.status(), but messages could arrive between these calls. If new message arrives after count check but before status check, could incorrectly mark as complete.

_{22 files reviewed, 6 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: async-loader.ts missing shell preprocessing and supporting files features that exist in loader.ts:70-86. Skills loaded via async path won't have !command`` substitution or <supporting-files> section.

Suggested change

	const processedBody = await preprocessShellCommands(body, resolvedPath)
	const frontmatterMcp = parseSkillMcpConfigFromFrontmatter(content)
	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' +
	subdirFiles.map(f => f.content).join('\n\n')
	: ''
	const supportingFiles = await discoverSupportingFiles(resolvedPath)
	const supportingFilesSection = supportingFiles.length > 0
	? '\n<supporting-files>\n' +
	supportingFiles.map(f => `${f.relativePath} (${formatSize(f.sizeBytes)})`).join('\n') +
	'\n</supporting-files>\n\n'
	: ''
	const skillName = data.name || defaultName
	const originalDescription = data.description || ""
	const isOpencodeSource = scope === "opencode" || scope === "opencode-project"
	const formattedDescription = `(${scope} - Skill) ${originalDescription}`
	const wrappedTemplate = `<skill-instruction>
	Base directory for this skill: ${resolvedPath}/
	File references (@path) in this skill are relative to this directory.
	${supportingFilesSection}${processedBody.trim()}${mergedContent}
	</skill-instruction>

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/features/opencode-skill-loader/async-loader.ts
Line: 89:104

Comment:
**logic:** `async-loader.ts` missing shell preprocessing and supporting files features that exist in `loader.ts:70-86`. Skills loaded via async path won't have `!`command`` substitution or `<supporting-files>` section.

```suggestion
    const processedBody = await preprocessShellCommands(body, resolvedPath)
    const frontmatterMcp = parseSkillMcpConfigFromFrontmatter(content)
    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' +
        subdirFiles.map(f => f.content).join('\n\n')
      : ''

    const supportingFiles = await discoverSupportingFiles(resolvedPath)
    const supportingFilesSection = supportingFiles.length > 0
      ? '\n<supporting-files>\n' +
        supportingFiles.map(f => `${f.relativePath} (${formatSize(f.sizeBytes)})`).join('\n') +
        '\n</supporting-files>\n\n'
      : ''

    const skillName = data.name || defaultName
    const originalDescription = data.description || ""
    const isOpencodeSource = scope === "opencode" || scope === "opencode-project"
    const formattedDescription = `(${scope} - Skill) ${originalDescription}`

    const wrappedTemplate = `<skill-instruction>
Base directory for this skill: ${resolvedPath}/
File references (@path) in this skill are relative to this directory.
${supportingFilesSection}${processedBody.trim()}${mergedContent}
</skill-instruction>
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: 5s auto-cleanup creates race if skill invocation takes >5s. Session marked as slash-command at line 1299 (auto-slash-command/index.ts), but cleared here before tool.execute.before check at line 39-45. Skill will be blocked incorrectly.

Suggest increasing timeout to 60s or clearing on tool completion instead of timer. What's the typical max duration for skill tool execution in production?

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/hooks/skill-invocation-filter/index.ts
Line: 7:9

Comment:
**logic:** 5s auto-cleanup creates race if skill invocation takes >5s. Session marked as slash-command at line 1299 (`auto-slash-command/index.ts`), but cleared here before `tool.execute.before` check at line 39-45. Skill will be blocked incorrectly.

Suggest increasing timeout to 60s or clearing on tool completion instead of timer. What's the typical max duration for skill tool execution in production?

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Sequential replacement with replace() can corrupt output if command output contains the next match pattern. E.g., if !echo 'Next: !cat file``'`` and !`cat file`` both exist, first replacement breaks second match.

Suggested change

	for (let i = 0; i < processLimit; i++) {
	const match = matches[i]
	const fullMatch = match[0]
	const command = match[1]
	if (command.length > SHELL_SECURITY.MAX_COMMAND_LENGTH) {
	result = result.replace(fullMatch, '[COMMAND_BLOCKED: exceeds max length]')
	continue
	}
	const { allowed, binary } = isCommandAllowed(command)
	if (!allowed) {
	result = result.replace(fullMatch, `[COMMAND_BLOCKED: ${binary} not permitted]`)
	continue
	}
	const output = await executeCommand(command, skillDir)
	result = result.replace(fullMatch, output)
	}
	for (let i = 0; i < processLimit; i++) {
	const match = matches[i]
	const fullMatch = match[0]
	const command = match[1]
	if (command.length > SHELL_SECURITY.MAX_COMMAND_LENGTH) {
	replacements.push({ original: fullMatch, replacement: '[COMMAND_BLOCKED: exceeds max length]' })
	continue
	}
	const { allowed, binary } = isCommandAllowed(command)
	if (!allowed) {
	replacements.push({ original: fullMatch, replacement: `[COMMAND_BLOCKED: ${binary} not permitted]` })
	continue
	}
	const output = await executeCommand(command, skillDir)
	replacements.push({ original: fullMatch, replacement: output })
	}
	// Apply all replacements at once to avoid conflicts
	for (const { original, replacement } of replacements) {
	result = result.replace(original, replacement)
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/features/opencode-skill-loader/shell-preprocessing.ts
Line: 103:121

Comment:
**logic:** Sequential replacement with `replace()` can corrupt output if command output contains the next match pattern. E.g., if `!`echo 'Next: !`cat file``'`` and `!`cat file`` both exist, first replacement breaks second match.

```suggestion
  for (let i = 0; i < processLimit; i++) {
    const match = matches[i]
    const fullMatch = match[0]
    const command = match[1]
    
    if (command.length > SHELL_SECURITY.MAX_COMMAND_LENGTH) {
      replacements.push({ original: fullMatch, replacement: '[COMMAND_BLOCKED: exceeds max length]' })
      continue
    }
    
    const { allowed, binary } = isCommandAllowed(command)
    if (!allowed) {
      replacements.push({ original: fullMatch, replacement: `[COMMAND_BLOCKED: ${binary} not permitted]` })
      continue
    }
    
    const output = await executeCommand(command, skillDir)
    replacements.push({ original: fullMatch, replacement: output })
  }
  
  // Apply all replacements at once to avoid conflicts
  for (const { original, replacement } of replacements) {
    result = result.replace(original, replacement)
  }
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

style: Redundant timeout - spawn already has timeout option at line 35. This creates double SIGKILL on timeout.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/features/opencode-skill-loader/shell-preprocessing.ts
Line: 42:45

Comment:
**style:** Redundant timeout - `spawn` already has `timeout` option at line 35. This creates double SIGKILL on timeout.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Vercel compatibility: both paths loaded but no deduplication if same skill exists in both. Could return duplicates with same name.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/features/opencode-skill-loader/loader.ts
Line: 207:217

Comment:
**logic:** Vercel compatibility: both paths loaded but no deduplication if same skill exists in both. Could return duplicates with same name.

How can I resolve this? If you propose a fix, please make it concise.

[codewithkenzo]

Commits already merged to dev via squash. Closing stale PR.