feat: introduce skills management feature in supercode-cli:#226
Conversation
- Added commands for managing agent skills: add, remove, and list. - Implemented skill loading functionality to enhance AI interactions. - Updated context and documentation to include new skill management capabilities. - Bumped supercode-cli version to 0.1.84 to reflect these changes.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds a reusable ChangesAgent Skills
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant handleSlashCommand
participant skillCommand
participant chatLoop
participant streamAIResponse
User->>handleSlashCommand: enter a skill command
handleSlashCommand->>skillCommand: load the selected skill
skillCommand-->>handleSlashCommand: return skill content
handleSlashCommand-->>chatLoop: return skill message result
chatLoop->>streamAIResponse: send skill content with user message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)apps/supercode-cli/server/package.jsonTraceback (most recent call last): packages/skills/package.jsonTraceback (most recent call last): packages/skills/tsconfig.jsonTraceback (most recent call last): 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/supercode-cli/server/package.json`:
- Line 75: Move the `@super/skills` entry from devDependencies to dependencies in
the package manifest, keeping the existing workspace version unchanged so clean
published CLI installs include it for the skill add/remove/list commands.
In `@apps/supercode-cli/server/src/cli/ai/chat/chat.ts`:
- Around line 2113-2125: Clear loadedSkillContent after persisting the combined
skill message in the result.skillContent branch, matching the parallel path
before streamAIResponse runs. Keep loadedSkillName and the persisted combined
user message unchanged.
In `@apps/supercode-cli/server/src/cli/workspace/context.ts`:
- Line 182: Guard the pushSkillsSection call with hasTools so skills are
advertised only when the tool is available; leave the existing behavior
unchanged when hasTools is true.
- Line 249: Update the unavailable-skill description in the workspace context
flow to remove the `supercode skill sync` instruction and instead direct users
to reinstall using the displayed source. Preserve the existing
unavailable-on-disk context and update only the recovery guidance.
In `@apps/supercode-cli/server/src/tools/definitions/skill.ts`:
- Around line 2-3: Replace the skill context read in skill.ts with
Bun.file(...).text(). In apps/supercode-cli/server/src/cli/workspace/context.ts
lines 1-3, make pushSkillsSection asynchronous and use Bun.file(...).text(),
make buildSystemPrompt asynchronous, and await buildSystemPrompt at every
streamAIResponse caller.
In `@apps/supercode-cli/server/src/tools/registry.ts`:
- Line 97: Update the `skill` entry in the tool registry to require permission,
ensuring `install` and `remove` cannot mutate the user home directory without
approval; alternatively, split read-only skill actions from the mutating actions
and mark only the latter as permissioned.
In `@packages/skills/package.json`:
- Around line 10-14: Replace the placeholder scripts in the package.json scripts
block for build, lint, and typecheck with the repository’s actual TypeScript
type-checking and build/lint commands, ensuring Turbo validation performs real
checks for `@super/skills`; if the package is not ready for those commands, remove
the scaffolding scripts instead.
In `@packages/skills/src/lock-file.ts`:
- Around line 25-52: Update readSkillsLock to default to an empty lock only for
ENOENT, while propagating other read errors and rejecting malformed JSON or
invalid lock data. Make addSkillToLock and removeSkillFromLock concurrency-safe
by serializing mutations and rereading the latest lock within the serialized
operation, then update writeSkillsLock to atomically replace the lock file via a
temporary file and rename rather than writing directly.
- Around line 13-18: Reject path traversal through a shared strict skill-name
validator in packages/skills/src/lock-file.ts, and invoke it before getSkillDir
constructs any filesystem path so installer, removal, sync, and list flows are
protected. In apps/supercode-cli/server/src/tools/definitions/skill.ts lines
70-81, use the validated shared path helper before loading skill content. In
apps/supercode-cli/server/src/cli/workspace/context.ts lines 241-245, validate
lock-file entry names before reading their SKILL.md paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77e150e3-9212-4f0e-b11a-bb8578422b57
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
apps/supercode-cli/server/package.jsonapps/supercode-cli/server/src/cli/ai/chat/chat.tsapps/supercode-cli/server/src/cli/commands/skill/add.tsapps/supercode-cli/server/src/cli/commands/skill/index.tsapps/supercode-cli/server/src/cli/commands/skill/list.tsapps/supercode-cli/server/src/cli/commands/skill/remove.tsapps/supercode-cli/server/src/cli/commands/slashCommands/index.tsapps/supercode-cli/server/src/cli/commands/slashCommands/skill.tsapps/supercode-cli/server/src/cli/main.tsapps/supercode-cli/server/src/cli/workspace/context.tsapps/supercode-cli/server/src/tools/definitions/skill.tsapps/supercode-cli/server/src/tools/registry.tspackages/skills/package.jsonpackages/skills/src/index.tspackages/skills/src/installer.tspackages/skills/src/lock-file.tspackages/skills/src/types.tspackages/skills/tsconfig.json
| "@openrouter/ai-sdk-provider": "^2.9.0", | ||
| "@openrouter/sdk": "^0.12.79", | ||
| "@super/db-terminal": "workspace:*", | ||
| "@super/skills": "workspace:*", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check where `@super/skills` is declared and how the CLI is packaged/bundled.
fd -t f 'package.json' apps/supercode-cli/server --max-depth 2 --exec cat -n {}
echo "----- build/publish scripts -----"
rg -nC2 '"(build|prepublishOnly|prepack|bin)"|bun build|--compile' apps/supercode-cli/server/package.jsonRepository: yashdev9274/supercli
Length of output: 6664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- repo files of interest -----"
git ls-files | rg '(^|/)(apps/supercode-cli/server/package.json|apps/supercode-cli/server/src/.*skills.*|apps/supercode-cli/server/src/cli/main.ts)' || true
echo
echo "----- imports/usages of `@super/skills` -----"
rg -n "(`@super/skills`|installSkill|uninstallSkill|listInstalledSkills)" apps/supercode-cli/server/src apps/supercode-cli/server/package.json || true
echo
echo "----- bun availability/version -----"
if command -v bun >/dev/null 2>&1; then
bun --version
else
echo "bun not found"
fi
echo
echo "----- package workspace/config files -----"
rg -n "packages|\$workspace|`@super/skills`|`@super/db-terminal`|bun" package.json bun.lockb bun.lock apps/supercode-cli apps || true
echo
echo "----- deterministic package.json role check -----"
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('apps/supercode-cli/server/package.json')
data=json.loads(p.read_text())
bin=data.get('bin')
prepublish=data.get('scripts',{}).get('prepublishOnly')
build=data.get('scripts',{}).get('build')
files=data.get('files')
deps=data.get('dependencies',{})
dev=data.get('devDependencies',{})
print('bin_entrypoint_is_dist_main_js=',bin.get('supercode')==data.get('main')=='dist/main.js')
print('prepublish_builds=',prepublish is not None and 'build' in ' '.join(prepublish))
print('publish_includes_dist=', 'dist/main.js' in files)
print('external_dependencies',list(deps.keys()))
print('/@super/skills in dependencies:', '/@super/skills' in '/'.join(deps.keys()))
print('/@super/skills in devDependencies:', '/@super/skills' in '/'.join(dev.keys()))
PYRepository: yashdev9274/supercli
Length of output: 38539
Move @super/skills into dependencies.
The published CLI binary points to dist/main.js, and prepublishOnly runs bun run build:cli instead of bun run build:cli --prod, so the registry bundle is produced with devDependencies available. A clean published install running the CLI would not install devDependencies, leaving @super/skills imports unresolved for the skill add/remove/list commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/package.json` at line 75, Move the `@super/skills`
entry from devDependencies to dependencies in the package manifest, keeping the
existing workspace version unchanged so clean published CLI installs include it
for the skill add/remove/list commands.
| if (result.skillContent) { | ||
| loadedSkillName = result.skillName || "" | ||
| loadedSkillContent = result.skillContent | ||
| const combinedMsg = `${result.skillContent}\n\n${result.message}` | ||
| userMessage(`[${result.skillName}]\n\n${result.skillContent}\n\n${result.message}`) | ||
| messageCount++ | ||
| await addMessage(conversation.id, "user", combinedMsg) | ||
| } else { | ||
| userMessage(trimmed) | ||
| messageCount++ | ||
| await addMessage(conversation.id, "user", result.message) | ||
| await trySetAutoTitle(conversation.id, result.message, messageCount) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skill content is injected twice in the picker→type flow.
Here combinedMsg already embeds the full skill content in the persisted user message, but loadedSkillContent is left set, so the subsequent streamAIResponse (Line 2127) re-injects the same content into the system prompt via the ## Loaded Skill block at Lines 323-325. The whole SKILL.md ends up duplicated in one request (wasted context tokens, and inconsistent with the AI seeing it as both user turn and system instruction).
The parallel path at Lines 2154-2155 already clears the state before streaming; do the same here.
🐛 Clear skill state after persisting the combined message
await addMessage(conversation.id, "user", combinedMsg)
+ // Content is now persisted in the user message; clear module state so
+ // streamAIResponse doesn't also inject it into the system prompt.
+ loadedSkillName = undefined
+ loadedSkillContent = undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (result.skillContent) { | |
| loadedSkillName = result.skillName || "" | |
| loadedSkillContent = result.skillContent | |
| const combinedMsg = `${result.skillContent}\n\n${result.message}` | |
| userMessage(`[${result.skillName}]\n\n${result.skillContent}\n\n${result.message}`) | |
| messageCount++ | |
| await addMessage(conversation.id, "user", combinedMsg) | |
| } else { | |
| userMessage(trimmed) | |
| messageCount++ | |
| await addMessage(conversation.id, "user", result.message) | |
| await trySetAutoTitle(conversation.id, result.message, messageCount) | |
| } | |
| if (result.skillContent) { | |
| loadedSkillName = result.skillName || "" | |
| loadedSkillContent = result.skillContent | |
| const combinedMsg = `${result.skillContent}\n\n${result.message}` | |
| userMessage(`[${result.skillName}]\n\n${result.skillContent}\n\n${result.message}`) | |
| messageCount++ | |
| await addMessage(conversation.id, "user", combinedMsg) | |
| // Content is now persisted in the user message; clear module state so | |
| // streamAIResponse doesn't also inject it into the system prompt. | |
| loadedSkillName = undefined | |
| loadedSkillContent = undefined | |
| } else { | |
| userMessage(trimmed) | |
| messageCount++ | |
| await addMessage(conversation.id, "user", result.message) | |
| await trySetAutoTitle(conversation.id, result.message, messageCount) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/cli/ai/chat/chat.ts` around lines 2113 - 2125,
Clear loadedSkillContent after persisting the combined skill message in the
result.skillContent branch, matching the parallel path before streamAIResponse
runs. Keep loadedSkillName and the persisted combined user message unchanged.
| lines.push("- When the user wraps a URL in single or double quotes, treat it as a") | ||
| lines.push(" string literal — strip the quotes before calling `url_fetch`.") | ||
| lines.push("") | ||
| pushSkillsSection(lines) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only advertise skills when the tool is available.
pushSkillsSection runs even when hasTools is false, so those prompts can instruct the model to call an unavailable skill tool. Guard this call with hasTools.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/cli/workspace/context.ts` at line 182, Guard
the pushSkillsSection call with hasTools so skills are advertised only when the
tool is available; leave the existing behavior unchanged when hasTools is true.
| const match = content.match(/description:\s*(.+)/) | ||
| description = match ? match[1]!.trim() : "" | ||
| } catch { | ||
| description = "(not on disk — run `supercode skill sync`)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the unavailable skill sync recovery instruction.
The supplied CLI command set contains add/remove/list, and the tool schema has no sync action. This recovery path tells users to run a command they cannot access; instruct them to reinstall using the displayed source instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/cli/workspace/context.ts` at line 249, Update
the unavailable-skill description in the workspace context flow to remove the
`supercode skill sync` instruction and instead direct users to reinstall using
the displayed source. Preserve the existing unavailable-on-disk context and
update only the recovery guidance.
| import fs from "fs/promises" | ||
| import path from "path" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'apps/supercode-cli/server/src/tools/definitions/skill.ts' 'apps/supercode-cli/server/src/cli/workspace/context.ts'
echo "== skill.ts outline =="
ast-grep outline apps/supercode-cli/server/src/tools/definitions/skill.ts || true
echo "== skill.ts relevant contents =="
cat -n apps/supercode-cli/server/src/tools/definitions/skill.ts | sed -n '1,220p'
echo "== context.ts outline =="
ast-grep outline apps/supercode-cli/server/src/cli/workspace/context.ts || true
echo "== context.ts relevant contents =="
cat -n apps/supercode-cli/server/src/cli/workspace/context.ts | sed -n '1,280p'Repository: yashdev9274/supercli
Length of output: 22290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== streamAIResponse usages =="
rg -n "buildSystemPrompt|streamAIResponse" apps/supercode-cli/server/src -g '*.ts' -g '*.tsx'
echo "== all fs node imports in server src =="
rg -n 'import .*from ["'\'']fs(/promises)?["'\'']|readFileSync|readFile\(' apps/supercode-cli/server/src -g '*.ts' -g '*.tsx' | sed -n '1,220p'
echo "== context.ts surrounding exports/users =="
cat -n apps/supercode-cli/server/src/cli/workspace/context.ts | sed -n '188,280p'Repository: yashdev9274/supercli
Length of output: 10908
Use Bun file APIs for skill context reads.
In apps/supercode-cli/server/src/tools/definitions/skill.ts, replace the fs.readFile call with Bun.file(...).text(). In apps/supercode-cli/server/src/cli/workspace/context.ts, switch pushSkillsSection to async/Bun.file(...).text() and make buildSystemPrompt async, then update the streamAIResponse callers to await the prompt builder.
📍 Affects 2 files
apps/supercode-cli/server/src/tools/definitions/skill.ts#L2-L3(this comment)apps/supercode-cli/server/src/cli/workspace/context.ts#L1-L3
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/definitions/skill.ts` around lines 2 - 3,
Replace the skill context read in skill.ts with Bun.file(...).text(). In
apps/supercode-cli/server/src/cli/workspace/context.ts lines 1-3, make
pushSkillsSection asynchronous and use Bun.file(...).text(), make
buildSystemPrompt asynchronous, and await buildSystemPrompt at every
streamAIResponse caller.
Source: Coding guidelines
| task: { category: "agent", requiresPermission: false, description: taskTool.description }, | ||
| question: { category: "agent", requiresPermission: false, description: questionTool.description }, | ||
| todowrite: { category: "agent", requiresPermission: false, description: todowriteTool.description }, | ||
| skill: { category: "agent", requiresPermission: false, description: skillTool.description }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require approval for mutating skill actions.
skill is marked requiresPermission: false, but install downloads remote content and writes to the user home directory while remove recursively deletes from it. Mark the tool as permissioned, or split read-only actions from install/remove so autonomous chat flows cannot mutate local state without approval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/supercode-cli/server/src/tools/registry.ts` at line 97, Update the
`skill` entry in the tool registry to require permission, ensuring `install` and
`remove` cannot mutate the user home directory without approval; alternatively,
split read-only skill actions from the mutating actions and mark only the latter
as permissioned.
| "scripts": { | ||
| "build": "echo 'skills build pending'", | ||
| "lint": "echo 'skills lint pending'", | ||
| "typecheck": "echo 'skills typecheck pending'" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status short =="
git status --short || true
echo "== root package.json =="
cat -n packages/skills/package.json
echo "== workspace manifests =="
for f in package.json packages/*/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
jq -c '{name, scripts, dependencies, devDependencies}' "$f"
fi
done
echo "== TypeScript/lint config candidates =="
git ls-files | rg '(^|/)(package.json|tsconfig.*\.json|eslint\.|biome\.|prettier|vitest|jest|pnpm-lock\.yaml|package-lock\.json|yarn\.lock)$' || true
echo "== script keywords refs =="
rg -n '"(build|lint|typecheck|check|dev|test|ci|check-ts|tsc|typescript|eslint|biome)"|tsc|typescript|eslint|biome|vitest|jest' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' .
echo "== workflow files =="
git ls-files | rg '(^|/)\.github/workflows/.*\.ya?ml$|(^|/)CI|(^|/)Makefile|(^|/)justfile' || true
for f in $(git ls-files | rg '(^|/)\.github/workflows/.*\.ya?ml$|(^|/)CI|(^|/)Makefile|(^|/)justfile' || true); do
echo "--- $f ---"
sed -n '1,220p' "$f"
doneRepository: yashdev9274/supercli
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status short =="
git status --short || true
echo "== root package.json =="
cat -n package.json
echo "== skills package =="
cat -n packages/skills/package.json
echo "== package entry files =="
git ls-files packages/skills | sed -n '1,160p'
echo "== package declarations across workspace =="
python3 - <<'PY'
import json, pathlib
for path in sorted(p for p in pathlib.Path('.').rglob('package.json') if 'node_modules' not in p.parts):
try:
data=json.loads(path.read_text())
except Exception as e:
print(f"--- {path}: parse error {e} ---")
continue
fields={}
for k in ("name","type","version","main","types","exports","scripts"):
if k in data: fields[k]=data[k]
if fields:
print("---", path, "---")
for k,v in fields.items():
print(f"{k}: {v!r}")
PY
echo "== script command invocations =="
python3 - <<'PY'
import json, pathlib, re
terms = ["build","lint","typecheck","check","check-ts","test","ci","tsc","typescript","eslint","biome","prettier","vitest","jest"]
for path in sorted(p for p in pathlib.Path('.').rglob('*') if p.is_file() and 'node_modules' not in p.parts and '.git' not in p.parts and 'dist' not in p.parts and 'build' not in p.parts):
try:
text=path.read_text(errors='ignore')
except Exception:
continue
if not any(b in text for b in terms):
continue
for i,line in enumerate(text.splitlines(),1):
if any(b in line for b in terms):
print(f"{path}:{i}:{line}")
PYRepository: yashdev9274/supercli
Length of output: 50377
Wire the package scripts to real validation commands.
root routes build, lint, and typecheck through Turbo, so this package should define non-placeholder scripts; otherwise @super/skills passes those gates without building, linting, or type-checking. Use the repo’s TypeScript tooling for typecheck and a build pipeline for build/lint, or remove these scripts if they are still scaffolding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/package.json` around lines 10 - 14, Replace the placeholder
scripts in the package.json scripts block for build, lint, and typecheck with
the repository’s actual TypeScript type-checking and build/lint commands,
ensuring Turbo validation performs real checks for `@super/skills`; if the package
is not ready for those commands, remove the scaffolding scripts instead.
| export function getSkillDir(name: string): string { | ||
| return path.join(getSkillsDir(), name) | ||
| } | ||
|
|
||
| export function getSkillFilePath(name: string): string { | ||
| return path.join(getSkillDir(name), "SKILL.md") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Reject path traversal in skill names. A caller-controlled name reaches filesystem path construction; values such as ../... can escape the skills directory. This enables arbitrary local reads through load, writes through install, and recursive deletion through uninstall.
packages/skills/src/lock-file.ts#L13-L18: enforce one strict skill-name validator before constructing skill paths; this protects installer, removal, sync, and list flows.apps/supercode-cli/server/src/tools/definitions/skill.ts#L70-L81: use the validated shared path helper before loading content.apps/supercode-cli/server/src/cli/workspace/context.ts#L241-L245: validate lock-file entry names before reading theirSKILL.mdpaths.
Suggested validation
+export function assertSkillName(name: string): void {
+ if (!/^[a-z0-9][a-z0-9-]*$/i.test(name)) {
+ throw new Error("Invalid skill name")
+ }
+}
+
export function getSkillDir(name: string): string {
+ assertSkillName(name)
return path.join(getSkillsDir(), name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getSkillDir(name: string): string { | |
| return path.join(getSkillsDir(), name) | |
| } | |
| export function getSkillFilePath(name: string): string { | |
| return path.join(getSkillDir(name), "SKILL.md") | |
| export function assertSkillName(name: string): void { | |
| if (!/^[a-z0-9][a-z0-9-]*$/i.test(name)) { | |
| throw new Error("Invalid skill name") | |
| } | |
| } | |
| export function getSkillDir(name: string): string { | |
| assertSkillName(name) | |
| return path.join(getSkillsDir(), name) | |
| } | |
| export function getSkillFilePath(name: string): string { | |
| return path.join(getSkillDir(name), "SKILL.md") | |
| } |
📍 Affects 3 files
packages/skills/src/lock-file.ts#L13-L18(this comment)apps/supercode-cli/server/src/tools/definitions/skill.ts#L70-L81apps/supercode-cli/server/src/cli/workspace/context.ts#L241-L245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/src/lock-file.ts` around lines 13 - 18, Reject path traversal
through a shared strict skill-name validator in
packages/skills/src/lock-file.ts, and invoke it before getSkillDir constructs
any filesystem path so installer, removal, sync, and list flows are protected.
In apps/supercode-cli/server/src/tools/definitions/skill.ts lines 70-81, use the
validated shared path helper before loading skill content. In
apps/supercode-cli/server/src/cli/workspace/context.ts lines 241-245, validate
lock-file entry names before reading their SKILL.md paths.
Source: Linters/SAST tools
| export async function readSkillsLock(): Promise<SkillsLock> { | ||
| try { | ||
| const data = await fs.readFile(LOCK_FILE, "utf-8") | ||
| return JSON.parse(data) as SkillsLock | ||
| } catch { | ||
| return { version: 1, skills: {} } | ||
| } | ||
| } | ||
|
|
||
| export async function writeSkillsLock(lock: SkillsLock): Promise<void> { | ||
| await fs.mkdir(SUPERCODE_DIR, { recursive: true }) | ||
| await fs.writeFile(LOCK_FILE, JSON.stringify(lock, null, 2), "utf-8") | ||
| } | ||
|
|
||
| export async function addSkillToLock( | ||
| name: string, | ||
| definition: SkillDefinition, | ||
| ): Promise<SkillsLock> { | ||
| const lock = await readSkillsLock() | ||
| lock.skills[name] = definition | ||
| await writeSkillsLock(lock) | ||
| return lock | ||
| } | ||
|
|
||
| export async function removeSkillFromLock(name: string): Promise<SkillsLock> { | ||
| const lock = await readSkillsLock() | ||
| delete lock.skills[name] | ||
| await writeSkillsLock(lock) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make lock mutations failure- and concurrency-safe.
Invalid JSON or any read error becomes an empty lock, so a following add/remove can overwrite existing metadata. Concurrent clients also overwrite each other’s snapshots, and direct writes can leave a truncated lock file. Default only on ENOENT, reject malformed locks, serialize mutations, and atomically replace the file.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 26-26: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(LOCK_FILE, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 35-35: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(LOCK_FILE, JSON.stringify(lock, null, 2), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/src/lock-file.ts` around lines 25 - 52, Update readSkillsLock
to default to an empty lock only for ENOENT, while propagating other read errors
and rejecting malformed JSON or invalid lock data. Make addSkillToLock and
removeSkillFromLock concurrency-safe by serializing mutations and rereading the
latest lock within the serialized operation, then update writeSkillsLock to
atomically replace the lock file via a temporary file and rename rather than
writing directly.
1 participant
Description
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
bun testpassesbun run typecheckpassesbun run lintpasses (if applicable)Checklist:
Summary by CodeRabbit
New Features
/skillsand/skcommands with an interactive skill picker and filtering.Chores