-
Notifications
You must be signed in to change notification settings - Fork 1
✅ Fix marketplace validation with schema checks #19
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
Conversation
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Code Review - PR #19: Add Automated Marketplace ValidationSummaryThis PR adds automated validation to prevent keyword inconsistencies between ✅ Strengths1. Solid Shell Scripting Practices
2. Comprehensive Validation Logic
3. Well-Designed CI Integration
4. Excellent Developer Experience
🔍 Observations & Suggestions1. Minor: Arithmetic Expression Style (Line 50)The current code uses: for i in $(seq 0 $((plugin_count - 1))); doThis works perfectly, but for consistency with the modern bash style seen in for ((i=0; i<plugin_count; i++)); doHowever, the current approach is more portable and equally readable. No change needed unless you prefer the C-style loop. 2. Minor: GitHub Actions YAML Lint Warnings
These don't affect functionality but could be addressed for strict YAML compliance: ---
name: Validate Marketplace
"on":
pull_request:Not critical - many GitHub Actions workflows omit these. Your choice based on project style preferences. 3. Enhancement Opportunity: Handle Missing Marketplace Tags GracefullyCurrent code: marketplace_tags=$(jq -c ".plugins[$i].tags // []" "$MARKETPLACE_FILE")This correctly defaults to empty array if if [[ "$marketplace_tags" == "[]" ]]; then
echo -e "${YELLOW} $plugin_name: warning - no tags in marketplace.json${NC}"
fiNot a blocker - just a suggestion for extra validation rigor. 4. Documentation: README UpdateConsider adding a brief mention in the project README about this validation:
This helps contributors understand the quality checks in place. 🔒 Security Review✅ No security concerns:
🧪 Testing✅ Validation passed successfully: Tested locally and confirmed:
📊 Performance✅ Negligible performance impact:
🎯 Final VerdictRecommendation: Approve with minor optional enhancements This PR solves a real problem (preventing keyword inconsistencies) with a clean, maintainable solution. The validation script is well-written, the CI integration is appropriate, and it will prevent issues like those manually caught in PR #16. Optional Follow-ups (Non-Blocking):
What Works Great As-Is:✅ Core validation logic is sound Great work on this preventative quality measure! 🎉 Review conducted following project conventions from CLAUDE.md and git-interaction.mdc |
Code ReviewThis is a solid PR that adds important validation infrastructure. The validation script and CI integration will prevent configuration issues before they reach production. ✅ StrengthsCritical bug fix:
Comprehensive validation script:
CI integration:
Code quality:
🔍 ObservationsVersion inconsistency (minor):
This creates a three-way version mismatch. While not breaking, it could cause confusion. Consider whether these should be synchronized or if they track independently by design. Script robustness:
Potential edge case:
🎯 Suggestions (non-blocking)
🔒 Security & Performance
✅ RecommendationApprove and merge. This PR successfully:
The minor observations above are opportunities for future enhancement, not blockers. Great work on catching this issue and building preventive tooling! 🎉 |
2887035 to
a739f3e
Compare
|
✅ Fixed the validator exit bug (commit 3fd9329) Good catch, Cursor Bugbot! The script was using Changed all instances to |
Validates keyword consistency between marketplace.json tags and plugin.json keywords on every PR. This prevents the discoverability issues that were manually caught in PR #16. - scripts/validate-marketplace.sh: Local validation with clear error output - .github/workflows/validate-marketplace.yml: CI on PRs touching plugin files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The validator now catches Claude Code schema requirements like source paths needing "./" prefix. This would have caught the issue before it reached production. Changes: - Add schema validation for required fields and source format - Fix all plugin source paths to start with "./" - Bump marketplace version to 5.1.1 - Improve validator output with clear sections and checkmarks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixed bash arithmetic that was incompatible with set -e. Changed `((errors++))` to `errors=$((errors + 1))` throughout. The former returns exit status 1 when errors is 0, causing the script to exit immediately instead of collecting all validation errors. Now the script correctly accumulates all errors and reports them at the end. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3fd9329 to
c19998e
Compare
Code Review - PR #19: Fix marketplace validation with schema checksOverall AssessmentThis PR adds valuable validation infrastructure to prevent broken plugin configurations. The validation script is well-structured and the GitHub Actions integration is appropriate. However, I've identified one critical issue and several improvement opportunities. 🐛 Critical Issue: Version MismatchIssue: The core plugin has a version mismatch between marketplace.json and plugin.json:
Impact: This creates inconsistency between what the marketplace advertises and what the plugin actually declares. Recommendation: Either:
Enhancement: Consider adding version consistency validation to the script to catch this automatically. ✅ Strengths
🔍 Code Quality Observationsvalidate-marketplace.sh:
GitHub Actions workflow:
💡 Enhancement Suggestions
# After line 150, add version validation
marketplace_version=$(jq -r ".plugins[$i].version" "$MARKETPLACE_FILE")
plugin_version=$(jq -r '.version' "$plugin_json_path")
if [[ "$marketplace_version" != "$plugin_version" ]]; then
echo -e " ${RED}✗ Version mismatch${NC}"
echo -e " marketplace: $marketplace_version"
echo -e " plugin.json: $plugin_version"
errors=$((errors + 1))
fi
🔒 Security Considerations
📊 Test Coverage
🎯 RecommendationsBefore merging:
Future enhancements: SummaryThis is a solid PR that addresses a real production issue. The validation script is well-written and the CI integration is appropriate. Fix the version mismatch and this is ready to merge. The suggested enhancements can be addressed in follow-up PRs. Recommendation: Approve pending version mismatch fix |
|
|
||
| # Check plugins array exists and is not empty | ||
| plugin_count=$(jq '.plugins | length' "$MARKETPLACE_FILE") | ||
| if [[ "$plugin_count" -eq 0 ]]; then |
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.
Missing null handling for plugins array causes bash error
If the marketplace.json file doesn't have a plugins field at all (or it's null), jq '.plugins | length' returns the string "null" rather than a number. The subsequent bash comparison [[ "$plugin_count" -eq 0 ]] then fails with a cryptic arithmetic syntax error due to set -e. While validation correctly fails, the error message is confusing instead of clearly stating the plugins array is missing. Adding a fallback like jq '.plugins // [] | length' would handle this edge case gracefully.
| echo -e " ${GREEN}✓ plugin.json exists${NC}" | ||
|
|
||
| # Get keywords from plugin.json | ||
| plugin_keywords=$(jq -c '.keywords // []' "$plugin_json_path") |
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.
Plugin JSON syntax errors produce cryptic jq error messages
The script validates marketplace.json syntax with a clear error message (line 41-43), but individual plugin.json files are not validated for JSON syntax before parsing. If a plugin.json contains invalid JSON, the jq command on line 149 fails with a raw parse error, and set -e causes immediate exit. This produces a cryptic jq error instead of a clear message like "Invalid JSON in plugin.json at path: ...". The inconsistent error handling could confuse developers debugging validation failures.
Summary
What the validator now checks
Schema validation:
name,source,description,versionConsistency validation:
Testing
All 8 plugins pass validation.
🤖 Generated with Claude Code