Skip to content

Conversation

@numman-ali
Copy link
Owner

Summary

This PR addresses multiple open issues and incorporates security fixes to improve CLI usability for CI/CD pipelines, local development workflows, and overall security.

Issues Covered

#3 - Don't skip symlinked skill directories

What: Added support for symlinked skill directories in findAllSkills().

Why: Users who clone the anthropics/skills repo and symlink skills into ~/.claude/skills/ were not having those skills discovered. This enables git-based skill updates and local development workflows.

Implementation: Added isDirectoryOrSymlinkToDirectory() helper in src/utils/skills.ts that uses statSync() to follow symlinks and verify targets are directories. Broken symlinks are gracefully skipped.


#5 - Sync & append to .ruler/AGENT.md

What: Added --output / -o flag to the sync command for configurable output path.

Why: Users of ruler want to sync skills to .ruler/AGENTS.md instead of the root AGENTS.md. This also enables any custom output path.

Implementation:

  • Added --output <path> option to sync command
  • Auto-creates the file with a heading if it doesn't exist
  • Auto-creates nested directories if needed
  • Validates that output file ends in .md

Usage:

openskills sync --output .ruler/AGENTS.md
openskills sync -o custom-rules.md

#6 - --yes should overwrite

What: Made --yes / -y flag skip ALL interactive prompts, including overwrite confirmations.

Why: Users automating openskills in CI/CD pipelines or scripts couldn't run fully non-interactively because overwrite prompts still appeared even with -y.

Implementation: Added skipPrompt parameter to warnIfConflict() function and passed options.yes through all install code paths. When skipping, shows Overwriting: <skill-name> message.

Usage:

openskills install anthropics/skills -y  # Fully non-interactive, overwrites existing

#10 - Support local skills and private git repo

What: Added support for installing skills from local paths and private git repositories.

Why: Users wanted to:

  1. Install skills from local directories during development
  2. Install from private/enterprise git repos using SSH authentication

Implementation:

  • Added isLocalPath() to detect /absolute, ./relative, ../parent, ~/home paths
  • Added isGitUrl() to detect git@, git://, https://, and .git URLs
  • Added expandPath() for tilde expansion
  • Added installFromLocal() and installSingleLocalSkill() functions
  • Improved error messages for clone failures with SSH tip

Usage:

# Local paths
openskills install /path/to/skill
openskills install ./local-skill
openskills install ~/my-skills/custom-skill

# Private git repos (uses your SSH keys)
openskills install git@github.com:org/private-skills.git
openskills install https://gitlab.com/group/skills.git

✅ Security Fixes (from PR #8)

Incorporated security fixes from PR #8:

  1. Path traversal protection: Added validation before cpSync to ensure target paths stay within the target directory
  2. Symlink dereference: Added dereference: true to cpSync calls to copy symlink targets safely
  3. Non-greedy YAML regex: Changed .+ to .+? in extractYamlField() to prevent potential issues

Issues NOT Covered (and why)

⏸️ #13 - Support skill switch

Why deferred: This would require persistent state management (config file or marker files) to track enabled/disabled skills. The current architecture is stateless - skills are just directories, and sync regenerates the XML from scratch. Adding toggle state would be a significant architectural change.

⏸️ #9 - Document workflow for updating skills from upstream

Why deferred: This requires storing metadata about where skills were originally installed from. Would need a manifest file or similar. Can revisit after core features stabilize.

⚠️ #7 - Allow Local Development of Skills

Partially addressed: The symlink support (#3) enables local development - users can symlink a skill being developed into their skills directory. Additional convenience commands (openskills create, openskills promote) could be added later.


PRs That Can Be Closed

This PR supersedes:

  • PR #4 - Fix discovery of symlinked skills (same goal, different implementation)
  • PR #8 - Security fixes (incorporated)
  • PR #11 - Install from file:/// URLs (superseded by simpler local path approach)

Test Coverage

Added 77 new tests across 5 test files (88 total):

Test File Tests Coverage
tests/utils/skills.test.ts 13 Symlink detection, deduplication, broken symlinks
tests/commands/install.test.ts 27 Local paths, git URLs, path traversal security
tests/commands/sync.test.ts 17 XML generation, --output flag, auto-create
tests/integration/e2e.test.ts 16 Full CLI flows, symlinks, --yes overwrites
tests/utils/yaml.test.ts 10 YAML parsing, regex security, ReDoS protection

Files Changed

  • src/utils/skills.ts - Symlink support
  • src/commands/sync.ts - --output flag, auto-create
  • src/commands/install.ts - Local paths, git URLs, security checks, --yes overwrites
  • src/utils/yaml.ts - Non-greedy regex fix
  • src/cli.ts - Added --output option

🤖 Generated with Claude Code

numman-ali and others added 4 commits December 11, 2025 21:29
Features:
- #3: Support symlinked skill directories in findAllSkills()
- #5: Add --output flag to sync for configurable output path
- #6: Make --yes flag skip all prompts including overwrites
- #10: Support local paths and private git repos for install

Security (from PR #8):
- Add path traversal protection before cpSync
- Use dereference: true to copy symlink targets safely
- Fix YAML regex to be non-greedy

Changes:
- src/utils/skills.ts: Add isDirectoryOrSymlinkToDirectory() helper
- src/commands/sync.ts: Add --output option, auto-create file if missing
- src/commands/install.ts: Add local path detection, git URL handling,
  security checks, and pass --yes through to overwrite prompts
- src/utils/yaml.ts: Non-greedy regex fix

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests added:
- tests/utils/skills.test.ts: Symlink support, skill discovery, deduplication
- tests/commands/install.test.ts: Local paths, git URLs, path traversal security
- tests/commands/sync.test.ts: --output flag, auto-create, utilities
- tests/integration/e2e.test.ts: End-to-end CLI tests
- tests/utils/yaml.test.ts: Security tests for regex (non-greedy, ReDoS)

Coverage:
- 88 tests across 6 test files
- Unit tests for helper functions
- Integration tests for CLI commands
- Security tests for path traversal and regex

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changed complex ReturnType<typeof readdirSync<...>> type to simple
Dirent import for better TypeScript version compatibility in CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
E2E tests require dist/cli.js to exist. Move build step before
test step in CI workflow.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@numman-ali numman-ali merged commit 2efb508 into main Dec 14, 2025
2 checks passed
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.

1 participant