-
Notifications
You must be signed in to change notification settings - Fork 1
Chore/bump mcp schema #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Fixed directory structure diagrams in root README.md and AGENTS.md to show agent-skills-spec format - Moved marketplace.json location to .claude-plugin/ directory (was inconsistent) - Updated plugin structure to show skills/ directory with .md files instead of old .claude-plugin/commands/AGENTS.md format - Changed plugin discovery mechanism from "via AGENTS.md" to "via marketplace.json" in 4 plugin READMEs - Renamed "Skills" section to "Marketplace" in root README.md for consistency - Updated teams-anthropic package references to point to correct plugin structure This brings all documentation into alignment with the current agent-skills-spec implementation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @EdwardIrby's task in 1m 31s —— View job Code Review SummaryI've reviewed this PR which contains two commits:
✅ StrengthsSchema Update:
Documentation Updates:
🔍 ObservationsMissing PR Description:
Commit Message Scope: Schema Verification: 📋 RecommendationsBefore Merging:
✨ Overall AssessmentThe changes are technically sound and follow project conventions well. The documentation updates are thorough and well-explained. The main improvement would be adding context via a PR description to help reviewers and future maintainers understand the motivation and impact of the schema version change. Status: ✅ Approved with minor suggestions (non-blocking) |
| │ ├── AGENTS.md # Package dev guide | ||
| │ └── package.json # Package config | ||
| ├── plugins/ # Claude Code plugins | ||
| ├── plugins/ # Skills (agent-skills-spec format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does agent-skills-spec format mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://agentskills.io/home another anthropic spec that's being adopted widely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use them in cursor I love them.
- Rewrite install-plugin.sh to just copy skills/ folders - Remove all AGENTS.md file manipulation logic - Change flag from --agents.md to --agents - Install to .agents/skills/ for universal agent discovery - Update all plugin READMEs to reflect new behavior - Update MARKETPLACE.md with correct discovery mechanism - Add agent-skills-spec validation (checks skills/ directory exists) - Use git sparse checkout for efficient downloads Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @EdwardIrby's task in 3m 15s —— View job Code Review CompleteReviewing PR #22: Chore/bump mcp schema SummaryThis PR contains three commits that update the MCP schema version and modernize the plugin distribution system to follow the agent-skills-spec format. The changes significantly simplify the install script and improve documentation consistency. ✅ Strengths1. Schema Update (commit
2. Install Script Simplification (commit
3. Documentation Consistency (commit
🐛 Pre-existing Bug Found (Not Introduced by This PR)Location: The Claude Code installation example is missing the required # ❌ Current (line 20) - will fail with error
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration
# ✅ Should be
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration --claudeWhy this is a bug: The install script validates inputs at lines 77-78 and will error if no mode flag is provided: if [ -z "$PLUGIN_NAME" ] || [ -z "$INSTALL_MODE" ]; then
error "Usage: curl -fsSL ... | bash -s <plugin-name> --claude|--cursor|--agentsNote: This bug exists on main branch and was NOT introduced by this PR. However, since it affects user experience, it should be fixed in a follow-up PR.
|
- Save original working directory before downloading plugin - Return to original directory before installation - Create full installation path (not just parent directory) - Fixes issue where installation happened in temp dir instead of cwd - Fixes cp error "Not a directory" when destination doesn't exist Tested with --claude, --cursor, and --agents modes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @EdwardIrby's task in 2m 32s —— View job Code Review CompleteReviewing PR #22: Chore/bump mcp schema SummaryThis PR successfully updates the MCP schema version and modernizes the plugin distribution system to follow the agent-skills-spec format. The changes include four well-structured commits that improve the install script, update documentation, and fix a critical directory creation bug. Commit AnalysisCommit 1:
Commit 2:
Commit 3:
Commit 4:
Code Quality AssessmentShell Script Security (scripts/install-plugin.sh)✅ Security Strengths:
✅ Best Practices:
Adherence to Project StandardsCode Patterns (from
Git Workflow (from
Documentation (from
Pre-Existing Bug Found (Not Introduced by This PR)Location: The Claude Code installation example is missing the required # ❌ Current (line 20) - will fail with error
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration
# ✅ Should be
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration --claudeWhy this is a bug: The install script validates inputs at lines 77-78 and will error if no mode flag is provided: if [ -z "$PLUGIN_NAME" ] || [ -z "$INSTALL_MODE" ]; then
error "Usage: curl -fsSL ... | bash -s <plugin-name> --claude|--cursor|--agentsImpact: Users following the Quick Start guide will encounter an error instead of successful installation. Recommendation: Fix in separate PR → Note: This bug exists on the main branch and was NOT introduced by this PR. However, it affects user experience and should be fixed promptly. Missing ContextPR Description: The PR has no description explaining:
Recommendation: Add a PR description for future maintainers and to help reviewers understand the full context of the schema update. Performance ConsiderationsInstall Script Performance:
Architecture Notesagent-skills-spec Adoption:
Directory Structure Changes:
RecommendationsBefore Merging:
After Merging:
Overall AssessmentCode Quality: ⭐⭐⭐⭐⭐ Excellent
Documentation: ⭐⭐⭐⭐ Very Good
Architecture: ⭐⭐⭐⭐⭐ Excellent
Security: ⭐⭐⭐⭐⭐ Excellent
Status✅ APPROVED - All changes in this PR are production-ready. The pre-existing bug in MARKETPLACE.md should be fixed in a follow-up PR, but does not block this PR from merging. Strengths:
Minor Improvements (non-blocking):
|
|
Claude finished @EdwardIrby's task in 2m 30s —— View job Code Review Complete ✅Reviewing PR #22: Chore/bump mcp schema SummaryThis PR contains four commits that update the MCP schema version and modernize the plugin distribution system to follow the agent-skills-spec format. The changes significantly simplify the install script and improve documentation consistency across the repository. ✅ Strengths1. Schema Update (commit
2. Documentation Alignment (commit
3. Install Script Refactoring (commit
4. Install Script Fix (commit
🐛 Issues Found1. Pre-existing Bug in MARKETPLACE.md:20 (NOT introduced by this PR) Location: The Claude Code installation example is missing the required # ❌ Current (line 20) - will fail with error
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration
# ✅ Should be
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration --claudeWhy this is a bug: The install script validates inputs at lines 77-78 and will error if no mode flag is provided: if [ -z "$PLUGIN_NAME" ] || [ -z "$INSTALL_MODE" ]; then
error "Usage: curl -fsSL ... | bash -s <plugin-name> --claude|--cursor|--agentsImpact: Users copying this command will receive an error and won't be able to install Note: This bug exists on the main branch and was NOT introduced by this PR. However, since it affects user experience, it should be fixed in a follow-up PR. 2. Missing PR Description The PR has no description explaining:
Recommendation: Add a brief PR description for context 🔍 Security & Best Practices1. Sparse Checkout Security (scripts/install-plugin.sh:118-124) git init -q
git remote add origin "https://github.com/$REPO.git"
git config core.sparseCheckout true
echo "plugins/$PLUGIN_NAME/" >> .git/info/sparse-checkout
git pull -q --depth=1 origin "$BRANCH"✅ Good: Uses sparse checkout to download only needed files 2. jq Dependency Handling (scripts/install-plugin.sh:181-189) if command -v jq &> /dev/null; then
jq --arg name "$MARKETPLACE_NAME" --arg repo "$MARKETPLACE_REPO" '...'
else
warning "jq not found - please manually add marketplace to .claude/settings.json"✅ Good: Gracefully degrades when 3. Cleanup Trap (scripts/install-plugin.sh:39-47) TEMP_DIR=""
cleanup() {
if [ -n "$TEMP_DIR" ] && [ -d "$TEMP_DIR" ]; then
info "Cleaning up temporary files..."
rm -rf "$TEMP_DIR"
fi
}
trap cleanup EXIT✅ Good: Ensures temp files are cleaned up even on errors 4. Input Validation
📋 Code Quality Observations1. agent-skills-spec Compliance
2. User Experience
3. Documentation Consistency
4. Commit Message Quality
🎯 RecommendationsNon-blocking Improvements:
✨ Overall AssessmentCode Quality: Excellent - The install script refactoring is well-executed with proper error handling, validation, and user experience considerations. Documentation: Very good - Comprehensive updates across 7 files, though one pre-existing documentation bug was discovered. Architecture: Strong - The shift to agent-skills-spec format is the right direction, simplifying distribution and making plugins more portable. Security: Solid - Good input validation, proper cleanup, and safe git operations. Status: ✅ Approved - All changes in this PR are high quality. The pre-existing bug should be fixed in a separate PR. |
…oolkit into chore/bump-mcp-schema
|
Claude finished @EdwardIrby's task in 2m 6s —— View job Code Review CompleteReviewing PR #22: Chore/bump mcp schema SummaryThis PR successfully modernizes the plugin distribution system to follow agent-skills-spec format and bumps the MCP schema version. The changes span 4 well-structured commits with excellent code quality and security practices. Three previous Claude reviews have thoroughly covered most aspects - this review confirms their findings and identifies one additional concern. Commit Analysis✅ Commit 1:
✅ Commit 2:
✅ Commit 3:
✅ Commit 4:
Code Quality AssessmentShell Script Security Analysis (scripts/install-plugin.sh)✅ Strong Security Practices:
✅ Best Practices:
Adherence to Project StandardsCode Patterns (
Git Workflow (
Code Review Standards (
Issues Found🐛 Pre-Existing Bug (Not Introduced by This PR)Location: The Quick Start example is missing the required # ❌ Current (line 20) - will fail with error
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration
# ✅ Should be
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration --claudeWhy this fails: The install script validates inputs at lines 77-78: if [ -z "$PLUGIN_NAME" ] || [ -z "$INSTALL_MODE" ]; then
error "Usage: curl -fsSL ... | bash -s <plugin-name> --claude|--cursor|--agentsImpact: Users following the Quick Start guide will encounter an error instead of successful installation. Recommendation: Fix in follow-up PR → Note: This bug exists on the main branch and was NOT introduced by this PR, as confirmed by all three previous Claude reviews. Missing ContextPR Description Missing: The PR has no description explaining:
This context would help reviewers and future maintainers understand the schema update's motivation and impact. Architecture Assessment✅ Excellent Architecture Decisions:
Performance Considerations✅ Install Script Performance:
RecommendationsBefore Merging:
After Merging:
Overall AssessmentCode Quality: ⭐⭐⭐⭐⭐ Excellent
Documentation: ⭐⭐⭐⭐ Very Good
Architecture: ⭐⭐⭐⭐⭐ Excellent
Security: ⭐⭐⭐⭐⭐ Excellent
Status✅ APPROVED - All changes in this PR are production-ready. The pre-existing bug in MARKETPLACE.md should be fixed in a follow-up PR, but does not block this PR from merging. Strengths:
Minor Improvements (non-blocking):
|
- Remove eval-harness from ROADMAP.md plugins in development - Remove eval-harness from ROADMAP.md release timeline - Remove eval-harness from README.md roadmap section - Update create-package.md examples to use 'local-rag' instead Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @EdwardIrby's task in 2m 50s —— View job Code Review Complete ✅Reviewing PR #22: Chore/bump mcp schema SummaryThis PR successfully modernizes the plugin distribution system to follow agent-skills-spec format, bumps the MCP schema version, updates dependencies, and removes deprecated eval-harness references. The PR has evolved through 8 commits with excellent code quality overall. Four previous Claude reviews have thoroughly analyzed the core changes (schema bump, install script refactoring, documentation updates, directory creation fix). This review focuses on the additional commits and validates previous findings. Commit-by-Commit Analysis✅ Commit
✅ Commit
✅ Commit
✅ Commit
✅ Commit
✅ Commit
✅ Commit
Code Quality Assessment⭐ Security Analysis (scripts/install-plugin.sh)✅ Excellent Security Practices:
⭐ Adherence to Project StandardsGit Workflow (
Code Patterns (
Documentation Standards:
🐛 Issues Found1. Pre-Existing Bug in MARKETPLACE.md:20 (NOT introduced by this PR)Location: The Quick Start example is missing the required # ❌ Current (line 20) - will fail with error
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration
# ✅ Should be
curl -fsSL https://raw.githubusercontent.com/youdotcom-oss/dx-toolkit/main/scripts/install-plugin.sh | bash -s teams-anthropic-integration --claudeWhy this fails: The install script validates inputs at lines 77-78: if [ -z "$PLUGIN_NAME" ] || [ -z "$INSTALL_MODE" ]; then
error "Usage: curl -fsSL ... | bash -s <plugin-name> --claude|--cursor|--agentsImpact: Users following the Quick Start guide will encounter an error instead of successful installation. Note: This bug has been identified in all four previous Claude reviews but has not been fixed. It exists on the main branch and was NOT introduced by this PR. 2. Missing PR DescriptionThe PR has no description explaining:
Recommendation: Add a brief PR description summarizing the changes and their motivation. ✨ Strengths
📋 Performance & Architecture✅ Install Script Performance:
✅ Architecture Decisions:
🎯 RecommendationsNon-blocking Improvements:
Overall AssessmentCode Quality: ⭐⭐⭐⭐⭐ Excellent
Documentation: ⭐⭐⭐⭐ Very Good
Architecture: ⭐⭐⭐⭐⭐ Excellent
Security: ⭐⭐⭐⭐⭐ Excellent
Status✅ APPROVED - All changes in this PR are production-ready. The pre-existing MARKETPLACE.md bug should ideally be fixed before merge (or in immediate follow-up PR), but does not block this PR as it existed before these changes. Summary: This PR successfully achieves its goals of updating the MCP schema, modernizing the plugin system, updating dependencies, and cleaning up deprecated references. The code quality is excellent with strong security practices. Four previous reviews have validated the core changes, and this review confirms their findings while analyzing the additional commits. |
No description provided.