Feat/drop claude plugins

[EdwardIrby]

No description provided.

[claude]

Pull Request Review: Feat/drop claude plugins

Overview

This PR represents a significant architectural shift from Claude Code plugins to a broader agent-skills-spec installer supporting multiple AI coding agents. The changes are well-structured with comprehensive test coverage.

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive test suite with 484 additions covering all conversion functions
• Tests validate JSON parsing, directory mappings, and format conversions
• Good use of integration tests for the conversion functions

2. Clean Architecture

• Clear separation of concerns with dedicated conversion functions
• Modular approach makes it easy to add new agent support
• Consistent naming conventions (projects vs plugins)

3. Good Documentation

• README.md properly updated with new agent support
• Help text is clear and comprehensive
• Installation instructions cover multiple use cases

---

🔍 Issues & Concerns

Security Issues

1. Command Injection Vulnerability (HIGH SEVERITY)

• install.sh:334-346 - The convert_md_to_toml function uses unescaped user input in shell substitution:

  body=$(echo "$body" | sed 's/\$ARGUMENTS/{{args}}/g')

  If markdown content contains shell metacharacters, this could lead to command injection.

  Recommendation: Use printf '%s\n' instead of echo to safely handle arbitrary input.

2. Path Traversal Risk (MEDIUM SEVERITY)

• install.sh:591-596 - The clone_project function doesn't validate the sparse_path:

  git sparse-checkout set "$sparse_path/skills" "$sparse_path/commands" 2>/dev/null

  A malicious projects.json could potentially use path traversal (../) in sparse paths.

  Recommendation: Validate that sparse_path doesn't contain .. or other dangerous patterns.

3. Temp File Handling (LOW SEVERITY)

• install.sh:1017-1020 - If PROJECTS_JSON doesn't exist, it creates a temp file but the cleanup trap may not catch all exit scenarios.

Code Quality Issues

4. Inconsistent Error Handling

• Many operations silently redirect errors to /dev/null (e.g., line 596, 599)
• Missing error checks after critical operations like git clone
• install_project continues even if skills directory doesn't exist

5. AWK Script Maintainability

• Large inline AWK scripts (lines 317-346, 356-404, 420-509) are hard to read and test
• No validation that AWK parsing succeeded
• Multiple similar AWK patterns could be DRYed up

6. Magic Numbers and Hardcoded Values

• Line 481: 11500 character limit for Windsurf without explanation
• Line 481: Why 11500 instead of 12000? Document the 500-char buffer reasoning

Potential Bugs

7. Race Condition in Conversion Functions

• All three conversion functions use similar AWK patterns but with subtle differences
• convert_md_to_codex_prompt:358 has different quote handling than convert_md_to_toml:318
• Inconsistent frontmatter parsing could cause bugs

8. Gemini Command Directory Missing

• get_commands_dir doesn't return a directory for Gemini (line 49)
• But supports_commands returns true for Gemini (line 210)
• The install function handles this with conversion, but the inconsistency is confusing

9. Codex Prompts Directory

• Uses $HOME/.codex/prompts (global) instead of project-local .codex/prompts
• This means all projects share prompts, which could cause conflicts
• Should document this or make it configurable

Performance Concerns

10. Sequential Cloning

• Projects are cloned sequentially in the loop (line 721-746)
• Could parallelize git clones for faster installation
• Each clone fetches history even with --depth 1

11. Inefficient File Operations

• Line 650-660: Converting commands reads entire file multiple times via different AWK calls
• Could be optimized with a single pass

---

🎯 Recommendations

Critical (Must Fix)

1. Fix command injection in convert_md_to_toml by using printf instead of echo
2. Add path traversal validation in parse_source and clone_project
3. Add error handling for git operations

Important (Should Fix)

4. Extract AWK scripts to separate functions for better testability
5. Add tests for edge cases: empty files, malformed frontmatter, very long content
6. Document the Codex global prompts directory behavior
7. Fix the Gemini commands directory inconsistency

Nice to Have

8. Add shellcheck compliance checks to CI
9. Consider parallelizing git clone operations
10. Add validation for projects.json schema
11. Add a dry-run mode for testing installations

---

📊 Test Coverage Analysis

Well-Covered:

• Directory mapping functions
• JSON parsing functions
• Basic conversion scenarios

Needs More Tests:

• Error conditions (malformed JSON, network failures)
• Edge cases in conversion (empty descriptions, missing frontmatter)
• Security scenarios (malicious input, path traversal)
• Large file handling (>12k chars)

---

🔧 Specific Code Suggestions

install.sh:334 - Replace unsafe echo with printf:

# Before
body=$(echo "$body" | sed 's/\$ARGUMENTS/{{args}}/g')

# After  
body=$(printf '%s\n' "$body" | sed 's/\$ARGUMENTS/{{args}}/g')

install.sh:148 - Add path validation:

parse_source() {
  local source="$1"
  local repo="${source}"
  
  # Validate no path traversal
  if [[ "$repo" =~ \.\. ]]; then
    print_error "Invalid repository path: $repo"
    return 1
  fi
  
  echo "https://github.com/$repo.git" ".claude"
}

install.sh:595 - Add error handling:

if ! git clone --depth 1 --filter=blob:none --sparse "$repo_url" "$project_temp" --branch "$BRANCH" 2>/dev/null; then
  print_error "Failed to clone $project_name from $repo_url"
  return 1
fi

---

✨ Summary

This is a solid refactoring that successfully pivots from Claude Code plugins to a multi-agent installer. The test coverage is excellent and the architecture is clean. However, there are some security concerns that should be addressed before merging, particularly around command injection and path traversal. The code quality is generally good but could benefit from better error handling and some refactoring of the AWK scripts.

Overall Assessment: ⚠️ Approve with Changes Required

• Security: 3 issues (1 high, 1 medium, 1 low)
• Code Quality: Good overall, needs minor improvements
• Test Coverage: Excellent (484 new tests)
• Documentation: Well updated

Blocking Issues:

• Command injection vulnerability must be fixed
• Path traversal risk should be mitigated
• Error handling needs improvement

Non-Blocking Improvements:

• Consider the recommendations for code quality
• Add more edge case tests
• Document the Codex global directory behavior

[claude]

Pull Request Review: Drop Claude Plugins Support

Summary

This PR transitions from a Claude Code plugin marketplace to a general-purpose skills installer for multiple AI coding agents. The changes are extensive (+1017/-373 lines) and include:

• Renaming from "plaited-marketplace" to "skills-installer"
• Migrating from .claude-plugin/marketplace.json to projects.json
• Adding support for Codex and Windsurf agents
• Implementing format conversion for different agent types (TOML, prompts, workflows)
• Comprehensive test coverage for new functionality

Code Quality ✅

Strengths:

• Well-structured bash script with clear separation of concerns
• Comprehensive test coverage using Bun's test framework
• Good use of functions for maintainability
• Consistent error handling and user feedback
• README and documentation updated to match changes

Areas for Improvement:

1. install.sh:196 - The body variable assignment uses echo which can be unsafe. The comment on line 196 says "Use printf instead" but the code still uses echo in the test script at line 504. Consider using printf consistently.

2. install.sh:378-400 - The detect_agent function uses a long if/elif chain. Consider using a case statement for consistency with other functions.

Security Analysis 🔒

Good Security Practices:

• install.sh:152-162 - Excellent path traversal validation with regex check for ".."
• install.sh:159-162 - Repo format validation prevents injection attacks
• install.sh:479 - Uses printf to safely write sparse_path (no newline interpretation)

Security Concerns:

1. install.sh:247, 327, 342 - Uses cat on user-controlled files without size limits. While the Windsurf workflow has a 12k char limit (line 340-346), the Codex conversion doesn't. Consider adding size limits to prevent resource exhaustion.

2. install.sh:850-858 - Downloads projects.json from GitHub without signature verification. If the GitHub account is compromised, malicious projects could be installed. Consider adding checksum verification or signing.

3. install.sh:260, 262 - Uses grep -oE and processes placeholders. While the regex is constrained, ensure downstream processing doesn't introduce injection risks.

Performance Considerations ⚡

Efficient:

• Sparse git checkouts minimize clone size (line 475)
• AWK-based JSON parsing avoids jq dependency (lines 123-145)
• Proper cleanup with trap handlers (line 116)

Potential Issues:

1. Sequential project installation - Projects are installed one at a time in the loop (lines 592-611). For large numbers of projects, parallel cloning could improve performance.

2. install.sh:340-346 - Windsurf content truncation uses head -c which loads the entire file before truncating. For very large files, this could be memory-intensive.

Potential Bugs 🐛

1. install.sh:504 - Uses echo instead of printf despite the comment saying to use printf. This is inconsistent with line 197 which correctly uses printf.

2. install.sh:246 - The sed expression for title-casing may not work in all bash versions (backslash-u is GNU sed specific). Consider portability.

3. install.spec.ts:504 - Test uses echo which doesn't match the actual implementation that uses printf (install.sh:197). This test may pass even if the actual code has issues.

Test Coverage ✅

Comprehensive Coverage:

• JSON validation and parsing tests
• Directory mapping tests for all agents
• Format conversion tests (TOML, Codex, Windsurf)
• CLI argument parsing tests
• README consistency validation

Missing Test Coverage:

1. Security validation tests (path traversal, malformed repo names)
2. Error handling for failed git clones
3. Cleanup behavior when installation fails mid-way
4. Edge cases for the format converters (empty files, very large files, malformed YAML)

Breaking Changes 🔴

This PR introduces several breaking changes:

1. Removes Claude Code plugin support entirely
2. Changes the project metadata format (marketplace.json to projects.json)
3. Changes terminology throughout ("plugin" to "project")

Recommendation: Ensure existing Claude Code users are notified of this change, as their workflows will break.

Recommendations 📋

1. High Priority:

   • Add checksum or signature verification for downloaded projects.json
   • Fix the echo/printf inconsistency between install.sh and tests
   • Add size limits to all file read operations

2. Medium Priority:

   • Add error handling tests
   • Consider parallel project installation for performance
   • Add migration guide for existing Claude Code users

3. Low Priority:

   • Refactor detect_agent to use case statement
   • Document the security model in README

Overall Assessment

This is a well-executed refactoring with good test coverage and clear documentation. The main concerns are around security (unsigned downloads) and a few consistency issues between code and tests. The functionality is solid and the new multi-agent support significantly expands the tool's utility.

Recommendation: Approve with minor changes - Address the security concern around unsigned downloads and fix the echo/printf inconsistency before merging.

---

Review generated by Claude Code

[claude]

Pull Request Review

Summary

This PR successfully transitions from a Claude Code plugin marketplace to a universal skills installer supporting multiple AI coding agents (Gemini, Copilot, Cursor, OpenCode, Amp, Goose, Factory, Codex, Windsurf). The changes are substantial but well-tested and thoughtfully implemented.

Code Quality ✅

Strengths:

• Excellent test coverage with comprehensive unit tests for all new functions
• Clean separation of concerns with dedicated conversion functions for each agent format
• Good error handling with proper validation and informative error messages
• Consistent coding style throughout bash script
• Well-documented functions with inline comments explaining behavior

Minor suggestions:

1. install.sh:262 - Consider adding input sanitization for the argument_hint variable before writing to files, though current usage appears safe
2. install.sh:326 - The awk command for title case conversion is clever but might fail on some edge cases. Consider adding a comment explaining the transformation

Security ✅

Good practices implemented:

1. Path traversal protection (install.sh:153-156): Validates repo paths against .. patterns
2. Format validation (install.sh:159-162): Regex validation for GitHub repo format
3. Command injection mitigation (install.sh:197): Uses printf instead of echo to safely handle arbitrary input
4. Error handling (install.sh:631-635): Proper git clone error handling with fallback

Recommendations:

1. install.sh:269-275 - When writing frontmatter, the $description variable is not quoted in the heredoc. While current usage is safe, consider sanitizing or validating descriptions to prevent potential YAML injection:

   # Sanitize description (remove or escape special YAML chars)
   description=$(echo "$description" | sed 's/[:|>]//g')

2. install.sh:662 - The sparse_path is read from a temp file. Consider validating it matches expected patterns before use:

   if [[ ! "$sparse_path" =~ ^\.[a-z]+$ ]]; then
     print_error "Invalid sparse path: $sparse_path"
     return 1
   fi

Test Coverage ✅

Excellent coverage:

• All new agent mappings tested (install.spec.ts:209-245)
• Conversion functions thoroughly tested with edge cases (install.spec.ts:1583-1915)
• JSON parsing functions validated (install.spec.ts:336-366)
• CLI arguments and flags tested (install.spec.ts:371-420)
• README consistency checks ensure documentation stays in sync

Test quality:

• Uses temporary directories for conversion tests (good isolation)
• Tests both happy paths and error cases
• Validates output formats match expected schemas

Performance Considerations ⚠️

Current approach is reasonable, but watch for:

1. install.sh:123-131 - AWK parsing of JSON is functional but will scan entire file for each project. For the current 3 projects this is fine, but consider caching results if the project list grows significantly.

2. Conversion functions - Each markdown file is processed individually with multiple AWK/sed passes. This is acceptable for typical use cases (< 100 commands), but could be optimized with single-pass processing if needed.

3. Git operations - Sparse checkout is used appropriately to minimize bandwidth. Good optimization.

Additional Observations

Positive:

• Migration from .claude-plugin/marketplace.json to projects.json simplifies the data model
• Added support for 2 new agents (Codex, Windsurf) with format-specific conversions
• README and documentation kept in sync with code changes
• License added (ISC) - appropriate for open source
• Badge additions improve project visibility

Potential improvements:

1. Consider adding a --dry-run flag to preview what would be installed without making changes
2. The Codex global prompts directory behavior (install.sh:59-61) is well-documented but might surprise users - consider adding a warning message during installation
3. Error messages could include suggestions for resolution (e.g., "Invalid agent: foo. Did you mean: factory?")

Conclusion

This is a well-executed refactoring that significantly expands the project's utility. The code quality is high, security considerations have been addressed, and test coverage is comprehensive. The few suggestions above are minor improvements rather than blocking issues.

Recommendation: ✅ Approve with minor suggestions

The PR is ready to merge. The suggested security hardening and performance optimizations can be addressed in follow-up PRs if needed.

[EdwardIrby]

Applied Recommendations from Review

Thanks for the thorough review! I've implemented the high and medium priority recommendations:

Changes Made

High Priority (Security):

1. ✅ Added size limits to file read operations - New safe_read_file() helper with 100KB max limit to prevent resource exhaustion
2. ✅ Added checksum verification for projects.json - Downloads and verifies SHA256 checksum when fetching remotely

Medium Priority (Code Quality):
3. ✅ Fixed echo/printf inconsistency - Test helper now uses printf to match actual implementation
4. ✅ Refactored detect_agent() - Replaced if/elif chain with maintainable loop-based approach

Test Results

All 72 tests pass ✅

Commit: f0803c8

[claude]

Pull Request Review: Feat/drop claude plugins

Summary

This PR represents a significant architectural shift from Claude Code plugin marketplace to a broader agent-skills-spec installer. The changes are substantial (1099 additions, 389 deletions) and well-tested. Overall, this is high-quality work with good security practices and comprehensive test coverage.

Code Quality ✅

Strengths:

• Excellent test coverage with 505 new test lines covering all major functionality
• Clear separation of concerns with dedicated functions for each agent
• Good use of helper functions (safe_read_file, conversion functions)
• Comprehensive error handling throughout

Areas for Improvement:

1. Function Length (install.sh:395-421, 437-509, 511-598)
   The conversion functions are quite long. Consider breaking them into smaller, testable units.

2. Code Duplication (install.sh:448-465, 522-546)
   The frontmatter extraction logic is repeated across conversion functions. A shared extract_frontmatter() helper would reduce duplication.

Security 🔒

Strong Security Practices:

• ✅ File size limits (MAX_FILE_SIZE=102400) to prevent resource exhaustion
• ✅ SHA-256 checksum verification for projects.json
• ✅ Path traversal protection with validation
• ✅ Repository format validation using regex
• ✅ Use of printf instead of echo to prevent command injection

Concerns:

1. Checksum Verification Silent Failure (install.sh:1140-1153) - MEDIUM
   The checksum verification only runs if checksum_file is non-empty. Make this mandatory, not optional.

2. Command Injection Risk (install.sh:408-409) - MEDIUM
   Using awk with file content could be risky. While files are from trusted git repos, consider additional validation.

3. eval Usage in Tests (install.spec.ts:1830) - LOW
   Using eval with extracted shell functions could be risky. Consider sourcing the script directly.

Performance ⚡

Good:

• Sparse checkout to minimize clone sizes
• Parallel-safe temp directory creation
• Early validation prevents unnecessary work

Considerations:

• Multiple AWK invocations per file - consider single-pass parsing
• File size check uses wc -c which reads entire file - consider stat instead

Potential Bugs 🐛

1. Checksum Verification - CRITICAL
   Make checksum verification mandatory. Currently optional if download fails.

2. AWK Regex Escaping (install.sh:306-313) - LOW
   Project names used directly in regex without escaping.

3. UTF-8 Truncation (install.sh:569-575) - LOW
   Content truncated at byte boundary could split UTF-8 characters.

Test Coverage 📊

Excellent Coverage:

• ✅ All new functions have tests
• ✅ Edge cases covered
• ✅ Integration tests for CLI
• ✅ Consistency tests between README and code

Missing Tests:

• Security validation (path traversal, oversized files)
• Error conditions (network failures, checksum mismatches)
• Conversion edge cases

Final Recommendations

Critical:

1. Make checksum verification mandatory
2. Add error handling for git clone failures

High Priority:

1. Add security tests for path traversal
2. Extract duplicate frontmatter parsing logic
3. Use stat instead of wc for file size checks

Medium Priority:

1. Document conversion algorithms
2. Add migration guide for breaking changes
3. Improve AWK error handling

Overall Assessment

Grade: A-

Well-executed refactoring with strong security practices and excellent test coverage. Main concerns are making checksum verification mandatory and improving error handling. Code quality is high and architectural changes are sound.

Great work on the comprehensive test suite! 🎉

[EdwardIrby]

Implemented All Critical, High, and Medium Priority Recommendations

Critical

1. Checksum verification mandatory - Now fails if checksum file cannot be downloaded or verification fails
2. Git clone error handling - Added comprehensive error messages with git output on failure

High Priority

3. Security tests for path traversal - Added 9 new tests covering:
   • Path traversal with ..
   • Invalid repo formats (no slash, multiple slashes)
   • Invalid characters in repo names
   • Valid format acceptance
4. Extracted duplicate frontmatter parsing - Created shared helpers:
   • has_frontmatter() - checks for YAML frontmatter
   • extract_frontmatter_field() - extracts any field from frontmatter
   • extract_body() - extracts content after frontmatter
5. Use stat for file size - Added get_file_size() helper using stat (falls back to wc on unsupported systems)

Medium Priority

6. Documented conversion algorithms - Added comprehensive documentation block explaining source/target formats and security considerations
7. Improved AWK error handling - Added file existence checks and proper error propagation

Test Results

All 81 tests pass (9 new security validation tests added)

Commit: 1e3e19f

[claude]

Pull Request Review: Drop Claude Plugins & Add Multi-Agent Support

This is an impressive PR that successfully transitions from Claude-specific plugins to a multi-agent skills installer supporting 9 different AI coding agents.

Overall Assessment

Strengths:

• Excellent security hardening with multiple layers of protection
• Comprehensive test coverage (659 lines of tests)
• Well-documented code with clear comments
• Good error handling throughout

Impact: Major feature expansion (1324 additions, 390 deletions)

Code Quality

Strong Points:

1. Security-First Approach:

   • Path traversal validation (install.sh:190-199)
   • SHA256 checksum verification (install.sh:945-971)
   • File size limits to prevent resource exhaustion
   • Proper use of printf to prevent command injection
   • Input validation for repo format

2. Code Organization:

   • Well-structured with clear sections
   • Good use of helper functions
   • Shared frontmatter parsing reduces duplication

3. Testing:

   • Comprehensive security tests
   • All conversion functions tested
   • Edge cases covered

Areas for Improvement:

1. Inconsistent string handling (minor)
2. Magic numbers could be constants
3. Conversion functions could validate helper returns

Potential Bugs

1. get_file_size() Issue (Medium Priority)

Location: install.sh:32-33

The explicit return 0 statements don't output the size. Works because stat outputs to stdout first, but logic is unclear.

Suggested fix: Capture stat output and printf it explicitly.

2. TOML Escaping (Low Priority)

Location: install.sh:299

Only escapes quotes. May need backslash/newline escaping too.

Security Assessment

Excellent security measures:

• Path traversal protection
• Command injection protection
• Resource exhaustion protection
• Supply chain security (checksums)
• Input validation

Minor notes:

• Temp files in main() could leak if killed
• Git error output could expose info (edge case)

Test Coverage

Excellent coverage:

• 9 security validation tests
• All conversion functions tested
• Agent mappings tested
• README consistency tests

Documentation

Strong inline documentation with algorithm overviews and security notes.

Suggestions:

• Migration guide for Claude plugin users
• Document conversion format expectations

Architecture

Well-considered:

• Centralized format conversion reduces maintenance
• Shared helpers reduce duplication
• Loop-based agent detection is maintainable

Recommendations

High Priority:

1. Fix get_file_size() return logic
2. Consider temp file cleanup

Medium Priority:

1. Add constants for magic numbers
2. Enhance error handling
3. Document conversions in README

Low Priority:

1. Printf consistency
2. Migration guide
3. Enhanced TOML escaping

Conclusion

Excellent work! Successfully:

• Removes Claude-specific deps
• Adds 9 agent support
• Implements secure conversions
• Comprehensive tests
• Strong security

Recommendation: Approve with minor fixes (get_file_size bug).

Great refactor!

[EdwardIrby]

Addressed Review Feedback

High Priority

1. Fixed get_file_size() return logic - Now explicitly captures stat output and uses printf for clarity
2. Added temp file cleanup - Tracked TEMP_PROJECTS_JSON variable cleaned up via trap on exit

Medium Priority

3. Added constant for magic number - WINDSURF_CHAR_LIMIT=11500 replaces hardcoded value

All 81 tests pass.

Commit: 80fec31

[claude]

Pull Request Review: feat/drop-claude-plugins

This PR transitions from Claude plugins to agent-skills-spec, adding support for 9 AI coding agents.

Code Quality ✅

Strengths:

• Excellent security: input validation, path traversal prevention, checksum verification
• Comprehensive test coverage including security tests
• Well-structured with clear separation of concerns
• Good documentation and inline comments

Issues Found

Medium Priority:

1. Git error messages could leak credentials (install.sh:547)
2. File size validation wc fallback reads entire file
3. AWK YAML parsing lacks output validation
4. Quote escaping in TOML conversion could fail on pre-escaped quotes

Low Priority:

1. Agent detection returns first match only (install.sh:461-476)
2. Hard-coded .claude directory (install.sh:212)
3. Sequential project installation could be parallelized

Security 🔒

Strong security posture with validation, size limits, and checksums. Consider adding:

• Timeout for git operations
• Signature verification
• set -u flag

Tests 🧪

Excellent coverage. Add: integration tests, cleanup handler tests, missing dependency tests

Performance

Good use of stat and sparse checkout. Could optimize with parallel installation.

Breaking Changes ⚠️

Removes Claude plugin marketplace entirely. Major version bump needed.

Recommendation

✅ Approve - Production ready with minor suggestions for improvement.

Review by Claude Code