Skip to content

Commit ca6ad23

Browse files
anandgupta42claude
anandgupta42
and
claude
committed
fix: address code review findings for skill CLI (#341)
- Extract `detectToolReferences`, `SHELL_BUILTINS`, `skillSource`, `isToolOnPath` into `skill-helpers.ts` so tests import production code instead of duplicating it - Anchor PATH tool dirs to `Instance.directory` (not `cwd`) to prevent external_directory workdirs from shadowing project tools - Change `skill test` to FAIL (not warn) on broken paired tools: timeouts, non-zero exits, and spawn failures now set `hasErrors` - Add `.opencode/skills/` to the Discovery Paths doc section for consistency with `skill create` and the Skill Paths section - Use `tmpdir()` fixture from `test/fixture/fixture.ts` in tests instead of manual `fs.mkdtemp` + `afterAll` cleanup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1c5d575 commit ca6ad23

6 files changed

Lines changed: 242 additions & 216 deletions

File tree

‎docs/docs/configure/skills.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ Focus on the query: $ARGUMENTS
3434

3535
Skills are loaded from these locations (in priority order):
3636

37-
1. **altimate-code directories** (project-scoped, highest priority):
37+
1. **Project directories** (project-scoped, highest priority):
38+
- `.opencode/skills/`
3839
- `.altimate-code/skill/`
3940
- `.altimate-code/skills/`
4041

‎packages/opencode/src/cli/cmd/skill-helpers.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
// altimate_change start — shared helpers for skill CLI commands
2+
import path from "path"
3+
import fs from "fs/promises"
4+
import { Global } from "@/global"
5+
import { Instance } from "../../project/instance"
6+
7+
/** Shell builtins, common utilities, and agent tool names to filter when detecting CLI tool references. */
8+
export const SHELL_BUILTINS = new Set([
9+
// Shell builtins
10+
"echo", "cd", "export", "set", "if", "then", "else", "fi", "for", "do", "done",
11+
"case", "esac", "printf", "source", "alias", "read", "local", "return", "exit",
12+
"break", "continue", "shift", "trap", "type", "command", "builtin", "eval", "exec",
13+
"test", "true", "false",
14+
// Common CLI utilities (not user tools)
15+
"cat", "grep", "awk", "sed", "rm", "cp", "mv", "mkdir", "ls", "chmod", "which",
16+
"curl", "wget", "pwd", "touch", "head", "tail", "sort", "uniq", "wc", "tee",
17+
"xargs", "find", "tar", "gzip", "unzip", "git", "npm", "yarn", "bun", "pip",
18+
"python", "python3", "node", "bash", "sh", "zsh", "docker", "make",
19+
// System utilities unlikely to be user tools
20+
"sudo", "kill", "ps", "env", "whoami", "id", "date", "sleep", "diff", "less", "more",
21+
// Agent tool names that appear in skill content but aren't CLI tools
22+
"glob", "write", "edit",
23+
])
24+
25+
/** Detect CLI tool references inside a skill's content (bash code blocks mentioning executables). */
26+
export function detectToolReferences(content: string): string[] {
27+
const tools = new Set<string>()
28+
29+
// Match "Tools used: bash (runs `altimate-dbt` commands), ..."
30+
const toolsUsedMatch = content.match(/Tools used:\s*(.+)/i)
31+
if (toolsUsedMatch) {
32+
const refs = toolsUsedMatch[1].matchAll(/`([a-z][\w-]*)`/gi)
33+
for (const m of refs) {
34+
if (!SHELL_BUILTINS.has(m[1])) tools.add(m[1])
35+
}
36+
}
37+
38+
// Match executable names in bash code blocks: lines starting with an executable name
39+
const bashBlocks = content.matchAll(/```(?:bash|sh)\r?\n([\s\S]*?)```/g)
40+
for (const block of bashBlocks) {
41+
const lines = block[1].split("\n")
42+
for (const line of lines) {
43+
const trimmed = line.trim()
44+
if (!trimmed || trimmed.startsWith("#")) continue
45+
// Extract the first word (the command)
46+
const cmdMatch = trimmed.match(/^(?:\$\s+)?([a-z][\w.-]*(?:-[\w]+)*)/i)
47+
if (cmdMatch) {
48+
const cmd = cmdMatch[1]
49+
if (!SHELL_BUILTINS.has(cmd)) {
50+
tools.add(cmd)
51+
}
52+
}
53+
}
54+
}
55+
56+
return Array.from(tools)
57+
}
58+
59+
/** Determine the source label for a skill based on its location. */
60+
export function skillSource(location: string): string {
61+
if (location.startsWith("builtin:")) return "builtin"
62+
const home = Global.Path.home
63+
// Builtin skills shipped with altimate-code
64+
if (location.startsWith(path.join(home, ".altimate", "builtin"))) return "builtin"
65+
// Global user skills (~/.claude/skills/, ~/.agents/skills/, ~/.config/altimate-code/skills/)
66+
const globalDirs = [
67+
path.join(home, ".claude", "skills"),
68+
path.join(home, ".agents", "skills"),
69+
path.join(home, ".altimate-code", "skills"),
70+
path.join(Global.Path.config, "skills"),
71+
]
72+
if (globalDirs.some((dir) => location.startsWith(dir))) return "global"
73+
// Everything else is project-level
74+
return "project"
75+
}
76+
77+
/** Check if a tool is available on the current PATH (including .opencode/tools/). */
78+
export async function isToolOnPath(toolName: string, cwd: string): Promise<boolean> {
79+
// Check .opencode/tools/ in both cwd and worktree (they may differ in monorepos)
80+
const dirsToCheck = new Set([
81+
path.join(cwd, ".opencode", "tools"),
82+
path.join(Instance.worktree !== "/" ? Instance.worktree : cwd, ".opencode", "tools"),
83+
path.join(Global.Path.config, "tools"),
84+
])
85+
86+
for (const dir of dirsToCheck) {
87+
try {
88+
await fs.access(path.join(dir, toolName), fs.constants.X_OK)
89+
return true
90+
} catch {}
91+
}
92+
93+
// Check system PATH
94+
const sep = process.platform === "win32" ? ";" : ":"
95+
const binDir = process.env.ALTIMATE_BIN_DIR
96+
const pathDirs = (process.env.PATH ?? "").split(sep).filter(Boolean)
97+
if (binDir) pathDirs.unshift(binDir)
98+
99+
for (const dir of pathDirs) {
100+
try {
101+
await fs.access(path.join(dir, toolName), fs.constants.X_OK)
102+
return true
103+
} catch {}
104+
}
105+
106+
return false
107+
}
108+
// altimate_change end

‎packages/opencode/src/cli/cmd/skill.ts‎

Lines changed: 9 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -7,113 +7,7 @@ import { bootstrap } from "../bootstrap"
77
import { cmd } from "./cmd"
88
import { Instance } from "../../project/instance"
99
import { Global } from "@/global"
10-
11-
// ---------------------------------------------------------------------------
12-
// Helpers
13-
// ---------------------------------------------------------------------------
14-
15-
/** Shell builtins, common utilities, and agent tool names to filter when detecting CLI tool references. */
16-
const SHELL_BUILTINS = new Set([
17-
// Shell builtins
18-
"echo", "cd", "export", "set", "if", "then", "else", "fi", "for", "do", "done",
19-
"case", "esac", "printf", "source", "alias", "read", "local", "return", "exit",
20-
"break", "continue", "shift", "trap", "type", "command", "builtin", "eval", "exec",
21-
"test", "true", "false",
22-
// Common CLI utilities (not user tools)
23-
"cat", "grep", "awk", "sed", "rm", "cp", "mv", "mkdir", "ls", "chmod", "which",
24-
"curl", "wget", "pwd", "touch", "head", "tail", "sort", "uniq", "wc", "tee",
25-
"xargs", "find", "tar", "gzip", "unzip", "git", "npm", "yarn", "bun", "pip",
26-
"python", "python3", "node", "bash", "sh", "zsh", "docker", "make",
27-
// System utilities unlikely to be user tools
28-
"sudo", "kill", "ps", "env", "whoami", "id", "date", "sleep", "diff", "less", "more",
29-
// Agent tool names that appear in skill content but aren't CLI tools
30-
"glob", "write", "edit",
31-
])
32-
33-
/** Detect CLI tool references inside a skill's content (bash code blocks mentioning executables). */
34-
function detectToolReferences(content: string): string[] {
35-
const tools = new Set<string>()
36-
37-
// Match "Tools used: bash (runs `altimate-dbt` commands), ..."
38-
const toolsUsedMatch = content.match(/Tools used:\s*(.+)/i)
39-
if (toolsUsedMatch) {
40-
const refs = toolsUsedMatch[1].matchAll(/`([a-z][\w-]*)`/gi)
41-
for (const m of refs) {
42-
if (!SHELL_BUILTINS.has(m[1])) tools.add(m[1])
43-
}
44-
}
45-
46-
// Match executable names in bash code blocks: lines starting with an executable name
47-
const bashBlocks = content.matchAll(/```(?:bash|sh)\r?\n([\s\S]*?)```/g)
48-
for (const block of bashBlocks) {
49-
const lines = block[1].split("\n")
50-
for (const line of lines) {
51-
const trimmed = line.trim()
52-
if (!trimmed || trimmed.startsWith("#")) continue
53-
// Extract the first word (the command)
54-
const cmdMatch = trimmed.match(/^(?:\$\s+)?([a-z][\w.-]*(?:-[\w]+)*)/i)
55-
if (cmdMatch) {
56-
const cmd = cmdMatch[1]
57-
// Filter out common shell builtins and generic commands
58-
if (!SHELL_BUILTINS.has(cmd)) {
59-
tools.add(cmd)
60-
}
61-
}
62-
}
63-
}
64-
65-
return Array.from(tools)
66-
}
67-
68-
/** Determine the source label for a skill based on its location. */
69-
function skillSource(location: string): string {
70-
if (location.startsWith("builtin:")) return "builtin"
71-
const home = Global.Path.home
72-
// Builtin skills shipped with altimate-code
73-
if (location.startsWith(path.join(home, ".altimate", "builtin"))) return "builtin"
74-
// Global user skills (~/.claude/skills/, ~/.agents/skills/, ~/.config/altimate-code/skills/)
75-
const globalDirs = [
76-
path.join(home, ".claude", "skills"),
77-
path.join(home, ".agents", "skills"),
78-
path.join(home, ".altimate-code", "skills"),
79-
path.join(Global.Path.config, "skills"),
80-
]
81-
if (globalDirs.some((dir) => location.startsWith(dir))) return "global"
82-
// Everything else is project-level
83-
return "project"
84-
}
85-
86-
/** Check if a tool is available on the current PATH (including .opencode/tools/). */
87-
async function isToolOnPath(toolName: string, cwd: string): Promise<boolean> {
88-
// Check .opencode/tools/ in both cwd and worktree (they may differ in monorepos)
89-
const dirsToCheck = new Set([
90-
path.join(cwd, ".opencode", "tools"),
91-
path.join(Instance.worktree !== "/" ? Instance.worktree : cwd, ".opencode", "tools"),
92-
path.join(Global.Path.config, "tools"),
93-
])
94-
95-
for (const dir of dirsToCheck) {
96-
try {
97-
await fs.access(path.join(dir, toolName), fs.constants.X_OK)
98-
return true
99-
} catch {}
100-
}
101-
102-
// Check system PATH
103-
const sep = process.platform === "win32" ? ";" : ":"
104-
const binDir = process.env.ALTIMATE_BIN_DIR
105-
const pathDirs = (process.env.PATH ?? "").split(sep).filter(Boolean)
106-
if (binDir) pathDirs.unshift(binDir)
107-
108-
for (const dir of pathDirs) {
109-
try {
110-
await fs.access(path.join(dir, toolName), fs.constants.X_OK)
111-
return true
112-
} catch {}
113-
}
114-
115-
return false
116-
}
10+
import { detectToolReferences, skillSource, isToolOnPath } from "./skill-helpers"
11711

11812
// ---------------------------------------------------------------------------
11913
// Templates
@@ -475,33 +369,34 @@ const SkillTestCommand = cmd({
475369
}
476370

477371
// 4. Detect and check paired tools
372+
const projectDir = Instance.directory
478373
const tools = detectToolReferences(skill.content)
479374
if (tools.length === 0) {
480375
warn(`No CLI tool references detected in skill content`)
481376
} else {
482377
process.stdout.write(EOL + ` Paired tools:` + EOL)
483378
for (const tool of tools) {
484-
const available = await isToolOnPath(tool, cwd)
379+
const available = await isToolOnPath(tool, projectDir)
485380
if (available) {
486381
pass(`"${tool}" found on PATH`)
487382

488383
// Try running --help (with 5s timeout to prevent hangs)
489384
try {
490-
const worktreeDir = Instance.worktree !== "/" ? Instance.worktree : cwd
385+
const worktreeDir = Instance.worktree !== "/" ? Instance.worktree : projectDir
491386
const toolEnv = {
492387
...process.env,
493388
PATH: [
494389
process.env.ALTIMATE_BIN_DIR,
495390
path.join(worktreeDir, ".opencode", "tools"),
496-
path.join(cwd, ".opencode", "tools"),
391+
path.join(projectDir, ".opencode", "tools"),
497392
path.join(Global.Path.config, "tools"),
498393
process.env.PATH,
499394
]
500395
.filter(Boolean)
501396
.join(process.platform === "win32" ? ";" : ":"),
502397
}
503398
const proc = Bun.spawn([tool, "--help"], {
504-
cwd,
399+
cwd: projectDir,
505400
stdout: "pipe",
506401
stderr: "pipe",
507402
env: toolEnv,
@@ -512,12 +407,12 @@ const SkillTestCommand = cmd({
512407
if (exitCode === 0) {
513408
pass(`"${tool} --help" exits cleanly`)
514409
} else if (exitCode === null || exitCode === 137 || exitCode === 143) {
515-
warn(`"${tool} --help" timed out after 5s`)
410+
fail(`"${tool} --help" timed out after 5s`)
516411
} else {
517-
warn(`"${tool} --help" exited with code ${exitCode}`)
412+
fail(`"${tool} --help" exited with code ${exitCode}`)
518413
}
519414
} catch {
520-
warn(`"${tool} --help" failed to execute`)
415+
fail(`"${tool} --help" failed to execute`)
521416
}
522417
} else {
523418
fail(`"${tool}" not found on PATH`)

‎packages/opencode/src/pty/index.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,11 +144,11 @@ export namespace Pty {
144144
const prependDirs: string[] = []
145145
const binDir = process.env.ALTIMATE_BIN_DIR
146146
if (binDir && !pathEntries.has(binDir)) prependDirs.push(binDir)
147-
const projectToolsDir = path.join(cwd, ".opencode", "tools")
147+
const projectToolsDir = path.join(Instance.directory, ".opencode", "tools")
148148
if (!pathEntries.has(projectToolsDir)) prependDirs.push(projectToolsDir)
149-
if (Instance.worktree !== "/") {
149+
if (Instance.worktree !== "/" && Instance.worktree !== Instance.directory) {
150150
const worktreeToolsDir = path.join(Instance.worktree, ".opencode", "tools")
151-
if (worktreeToolsDir !== projectToolsDir && !pathEntries.has(worktreeToolsDir)) prependDirs.push(worktreeToolsDir)
151+
if (!pathEntries.has(worktreeToolsDir)) prependDirs.push(worktreeToolsDir)
152152
}
153153
const globalToolsDir = path.join(Global.Path.config, "tools")
154154
if (!pathEntries.has(globalToolsDir)) prependDirs.push(globalToolsDir)

‎packages/opencode/src/tool/bash.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,14 +182,15 @@ export const BashTool = Tool.define("bash", async () => {
182182
}
183183

184184
// 2. Project-level user tools (.opencode/tools/) — user extensions
185-
// Check both the cwd and the worktree root (they may differ in monorepos or subdirs)
186-
const projectToolsDir = path.join(cwd, ".opencode", "tools")
185+
// Anchored to Instance.directory (not cwd) so external_directory workdirs
186+
// can't shadow project tools. Also check worktree root for monorepos.
187+
const projectToolsDir = path.join(Instance.directory, ".opencode", "tools")
187188
if (!pathEntries.has(projectToolsDir)) {
188189
prependDirs.push(projectToolsDir)
189190
}
190-
if (Instance.worktree !== "/") {
191+
if (Instance.worktree !== "/" && Instance.worktree !== Instance.directory) {
191192
const worktreeToolsDir = path.join(Instance.worktree, ".opencode", "tools")
192-
if (worktreeToolsDir !== projectToolsDir && !pathEntries.has(worktreeToolsDir)) {
193+
if (!pathEntries.has(worktreeToolsDir)) {
193194
prependDirs.push(worktreeToolsDir)
194195
}
195196
}

0 commit comments

Comments
 (0)