Conversation
eb6459b to
21e7c3e
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive CI validation and project documentation to ensure plugin quality and consistency. It introduces automated validation through GitHub Actions and provides clear guidelines for contributors and AI agents working on the codebase.
Changes:
- Added GitHub Actions workflow to validate plugin structure on push/PR to main
- Created 5 validation scripts (JSON syntax, marketplace structure, SKILL.md frontmatter, hooks.json format, and a runner script)
- Added 4 documentation files (CONTRIBUTING.md, AGENTS.md, TESTING.md, CLAUDE.md)
- Updated README with CI badge and documentation links
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/validate.yml |
GitHub Actions workflow that runs validation scripts on push/PR |
scripts/validate.sh |
Bash script runner that executes all validation checks locally |
scripts/validate_json.py |
Validates JSON syntax for all .json files in repository |
scripts/validate_marketplace.py |
Validates marketplace.json structure and plugin directory structure |
scripts/validate_skills.py |
Validates SKILL.md files have proper YAML frontmatter |
scripts/validate_hooks.py |
Validates hooks.json files have correct format and event names |
CONTRIBUTING.md |
Contribution guidelines with plugin structure requirements |
TESTING.md |
Testing and validation instructions |
AGENTS.md |
Guidelines for LLMs working on the codebase |
CLAUDE.md |
Quick reference pointing to other documentation |
README.md |
Updated with CI badge and documentation links |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/validate_marketplace.py
Outdated
| with open(".claude-plugin/marketplace.json") as f: | ||
| data = json.load(f) |
There was a problem hiding this comment.
The script doesn't handle file reading errors (e.g., encoding issues, permission errors, missing file). If marketplace.json is missing or has encoding issues, the script will crash with an unhandled exception rather than providing a clear validation error message. Consider wrapping the file operations and JSON parsing in a try-except block to catch and report these errors gracefully.
| with open(".claude-plugin/marketplace.json") as f: | |
| data = json.load(f) | |
| try: | |
| with open(".claude-plugin/marketplace.json", encoding="utf-8") as f: | |
| data = json.load(f) | |
| except FileNotFoundError: | |
| print("Error: .claude-plugin/marketplace.json not found.") | |
| sys.exit(1) | |
| except PermissionError: | |
| print("Error: Permission denied when reading .claude-plugin/marketplace.json.") | |
| sys.exit(1) | |
| except (OSError, UnicodeDecodeError) as e: | |
| print(f"Error: Failed to read .claude-plugin/marketplace.json: {e}") | |
| sys.exit(1) | |
| except json.JSONDecodeError as e: | |
| print(f"Error: Invalid JSON in .claude-plugin/marketplace.json: {e}") | |
| sys.exit(1) |
scripts/validate_json.py
Outdated
| result = subprocess.run( | ||
| ["find", ".", "-name", "*.json", "-not", "-path", "./.git/*"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
There was a problem hiding this comment.
The subprocess.run call uses shell=False (implicitly), which is correct for security. However, the script relies on the find command being available, which may not be on all systems (particularly Windows without WSL). Consider adding a fallback using Python's pathlib or os.walk for better cross-platform compatibility, or at minimum, document the dependency on Unix-like systems.
scripts/validate_json.py
Outdated
| files = result.stdout.strip().split("\n") | ||
| errors = 0 | ||
|
|
||
| for f in files: | ||
| if not f: | ||
| continue |
There was a problem hiding this comment.
The validation script does not handle empty file list correctly. If find returns an empty result, files will be [''] (a list with one empty string), not an empty list. The check if not f: on line 19 handles this, but it's inefficient to iterate over a list with an empty string. Consider filtering the list: files = [f for f in result.stdout.strip().split("\n") if f] to avoid unnecessary iterations.
| files = result.stdout.strip().split("\n") | |
| errors = 0 | |
| for f in files: | |
| if not f: | |
| continue | |
| files = [f for f in result.stdout.strip().split("\n") if f] | |
| errors = 0 | |
| for f in files: |
| frontmatter = match.group(1) | ||
| if "name:" not in frontmatter: | ||
| errors.append(f"{path}: Missing 'name' in frontmatter") | ||
| if "description:" not in frontmatter: | ||
| errors.append(f"{path}: Missing 'description' in frontmatter") |
There was a problem hiding this comment.
The validation of frontmatter fields using string matching ("name:" not in frontmatter) is fragile and could produce false negatives. For example, if a SKILL.md has name: with no space after the colon, or if there's a YAML comment containing the word "name:", the validation would incorrectly pass or fail. Consider parsing the frontmatter as actual YAML using the yaml module (e.g., import yaml; parsed = yaml.safe_load(frontmatter)) and checking for the presence of keys in the parsed dictionary.
scripts/validate_skills.py
Outdated
| with open(path) as fp: | ||
| content = fp.read() |
There was a problem hiding this comment.
The script doesn't handle file reading errors (e.g., encoding issues, permission errors). If a SKILL.md file has encoding issues or is unreadable, the script will crash with an unhandled exception rather than reporting it as a validation error. Consider wrapping the file operations in a try-except block to catch and report these errors gracefully.
| with open(path) as fp: | |
| content = fp.read() | |
| try: | |
| with open(path, encoding="utf-8") as fp: | |
| content = fp.read() | |
| except (OSError, UnicodeError) as exc: | |
| errors.append(f"{path}: Unable to read file ({exc})") | |
| continue |
scripts/validate_marketplace.py
Outdated
| for skill_name in os.listdir(skills_dir): | ||
| skill_path = os.path.join(skills_dir, skill_name) | ||
| if os.path.isdir(skill_path): | ||
| skill_md = os.path.join(skill_path, "SKILL.md") | ||
| if not os.path.exists(skill_md): | ||
| errors.append( | ||
| f"Plugin '{name}': skill '{skill_name}' missing SKILL.md" | ||
| ) |
There was a problem hiding this comment.
The script doesn't handle potential OS errors when listing directories. If os.listdir(skills_dir) fails due to permission errors or other OS-level issues, the script will crash with an unhandled exception. Consider wrapping the directory operations in a try-except block to handle these cases gracefully and report them as validation errors.
| for skill_name in os.listdir(skills_dir): | |
| skill_path = os.path.join(skills_dir, skill_name) | |
| if os.path.isdir(skill_path): | |
| skill_md = os.path.join(skill_path, "SKILL.md") | |
| if not os.path.exists(skill_md): | |
| errors.append( | |
| f"Plugin '{name}': skill '{skill_name}' missing SKILL.md" | |
| ) | |
| try: | |
| skill_entries = os.listdir(skills_dir) | |
| except OSError as exc: | |
| errors.append( | |
| f"Plugin '{name}': cannot list skills directory '{skills_dir}': {exc}" | |
| ) | |
| else: | |
| for skill_name in skill_entries: | |
| skill_path = os.path.join(skills_dir, skill_name) | |
| if os.path.isdir(skill_path): | |
| skill_md = os.path.join(skill_path, "SKILL.md") | |
| if not os.path.exists(skill_md): | |
| errors.append( | |
| f"Plugin '{name}': skill '{skill_name}' missing SKILL.md" | |
| ) |
scripts/validate_skills.py
Outdated
| errors.append(f"{path}: Missing YAML frontmatter") | ||
| continue | ||
|
|
||
| match = re.match(r"^---\n(.*?)\n---", content, re.DOTALL) |
There was a problem hiding this comment.
The regex pattern assumes the frontmatter ends with exactly \n--- but doesn't account for possible trailing whitespace or different line endings. Files with \r\n (Windows line endings) or trailing spaces after --- would fail validation incorrectly. Consider using a more flexible pattern like r"^---\s*\n(.*?)\n---\s*" to handle these edge cases.
| match = re.match(r"^---\n(.*?)\n---", content, re.DOTALL) | |
| match = re.match(r"^---\s*\n(.*?)\n---\s*", content, re.DOTALL) |
scripts/validate_hooks.py
Outdated
| with open(path) as fp: | ||
| data = json.load(fp) |
There was a problem hiding this comment.
The script doesn't handle file reading errors (e.g., encoding issues, permission errors). If a hooks.json file has encoding issues or cannot be parsed, the script will crash with an unhandled exception rather than reporting it as a validation error. Consider wrapping the file operations and JSON parsing in a try-except block to catch and report these errors gracefully.
| with open(path) as fp: | |
| data = json.load(fp) | |
| try: | |
| with open(path, encoding="utf-8") as fp: | |
| data = json.load(fp) | |
| except (OSError, UnicodeError, json.JSONDecodeError) as exc: | |
| errors.append(f"{path}: Failed to load JSON: {exc}") | |
| continue |
20579b5 to
e527214
Compare
e527214 to
4b198cd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if "name:" not in frontmatter: | ||
| errors.append(f"{path}: Missing 'name' in frontmatter") | ||
| if "description:" not in frontmatter: | ||
| errors.append(f"{path}: Missing 'description' in frontmatter") |
There was a problem hiding this comment.
The frontmatter validation uses a simple string search ("name:" in frontmatter) which could produce false positives. For example, it would incorrectly match if "name:" appears in a multi-line description or comment. Consider using a proper YAML parser (like PyYAML) to parse and validate the frontmatter structure more reliably.
| if errors: | ||
| print("Errors found:") | ||
| for e in errors: | ||
| print(f" ✗ {e}") | ||
| sys.exit(1) | ||
| print("✓ All SKILL.md files have valid frontmatter") |
There was a problem hiding this comment.
When no SKILL.md files are found in the repository, the script silently exits with success (exit code 0). Consider adding a check to warn if no SKILL.md files were found, as this might indicate a problem with the repository structure or the script not running from the correct directory.
| if errors: | ||
| print("Errors found:") | ||
| for e in errors: | ||
| print(f" ✗ {e}") | ||
| sys.exit(1) | ||
| print("✓ All hooks.json files are valid") |
There was a problem hiding this comment.
When no hooks.json files are found in the repository, the script silently exits with success. Consider adding a check to report if no hooks.json files were found, to help distinguish between "no files found" and "all files valid".
| if errors: | ||
| print(f" ✗ {errors} JSON errors found") | ||
| sys.exit(1) | ||
| print("✓ All JSON files are valid") |
There was a problem hiding this comment.
The script doesn't check if any JSON files were found. When run in an empty repository or wrong directory, it will report success even though no validation actually occurred. Consider tracking the count of files processed and reporting it or warning if zero files are found.
| hooks = data.get("hooks", {}) | ||
| if isinstance(hooks, list): | ||
| errors.append(f"{path}: 'hooks' must be an object, not array") | ||
| continue | ||
|
|
There was a problem hiding this comment.
The validation doesn't verify that the "hooks" field is actually required or present in the data. If a hooks.json file doesn't contain a "hooks" key at all, it will pass validation with an empty dictionary from data.get("hooks", {}). Consider adding a check to ensure the "hooks" field exists in hooks.json files.
| hooks = data.get("hooks", {}) | |
| if isinstance(hooks, list): | |
| errors.append(f"{path}: 'hooks' must be an object, not array") | |
| continue | |
| if "hooks" not in data: | |
| errors.append(f"{path}: Missing required 'hooks' field") | |
| continue | |
| hooks = data["hooks"] | |
| if not isinstance(hooks, dict): | |
| if isinstance(hooks, list): | |
| errors.append(f"{path}: 'hooks' must be an object, not array") | |
| else: | |
| errors.append(f"{path}: 'hooks' must be an object") | |
| continue |
scripts/validate_skills.py
Outdated
| errors = [] | ||
|
|
||
| for root, dirs, files in os.walk("."): | ||
| if ".git" in root: |
There was a problem hiding this comment.
The check for ".git" in root will incorrectly skip any directory path that contains ".git" anywhere in its path (e.g., ".github" directory). This should use a more precise check to avoid skipping legitimate directories like ".github" which contains the workflows.
| if ".git" in root: | |
| path_parts = os.path.normpath(root).split(os.sep) | |
| if ".git" in path_parts: |
scripts/validate_hooks.py
Outdated
| if ".git" in root: | ||
| continue |
There was a problem hiding this comment.
The check for ".git" in root will incorrectly skip any directory path that contains ".git" anywhere in its path (e.g., ".github" directory). This should use a more precise check to avoid skipping legitimate directories like ".github" which may contain hooks.json files for workflow configurations.
| if ".git" in root: | |
| continue | |
| # Skip version control metadata directories named '.git' but still | |
| # traverse similarly named directories like '.github' that may contain | |
| # hooks.json files for workflow configurations. | |
| if ".git" in dirs: | |
| dirs.remove(".git") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/validate_hooks.py
Outdated
| errors = [] | ||
|
|
||
| for root, dirs, files in os.walk("."): | ||
| if ".git" in root: |
There was a problem hiding this comment.
The check if ".git" in root could incorrectly skip directories that contain ".git" as a substring (e.g., "my.github" or ".gitignore-backup"). Consider using a more precise check such as if root.split(os.sep).__contains__(".git") or if "/.git/" in root or root.startswith(".git") to ensure only the actual .git directory is skipped.
| if ".git" in root: | |
| if ".git" in root.split(os.sep): |
| errors.append(f"{path}: Missing YAML frontmatter") | ||
| continue | ||
|
|
||
| match = re.match(r"^---\s*\n(.*?)\n---\s*", content, re.DOTALL) |
There was a problem hiding this comment.
The regex pattern r"^---\s*\n(.*?)\n---\s*" may not correctly handle edge cases where the closing --- delimiter is at the end of the file without a trailing newline, or where there's content immediately after --- on the same line. Consider making the pattern more robust by using r"^---\s*\n(.*?)\n---\s*(?:\n|$)" to explicitly handle both newline and end-of-string after the closing delimiter.
| match = re.match(r"^---\s*\n(.*?)\n---\s*", content, re.DOTALL) | |
| match = re.match(r"^---\s*\n(.*?)\n---\s*(?:\n|$)", content, re.DOTALL) |
CI validation (GitHub Actions + local scripts): - JSON syntax for all .json files - marketplace.json required fields (name, owner, plugins) - Plugin structure (plugin.json exists, skills have SKILL.md) - SKILL.md frontmatter (name, description required) - hooks.json format (object not array, valid event names) - Cross-platform: uses os.walk instead of find - Robust error handling for file/JSON operations Documentation: - CONTRIBUTING.md: Contribution workflow, plugin structure guide - AGENTS.md: Guidelines for LLMs working on this codebase - TESTING.md: Validation and testing instructions - CLAUDE.md: Quick reference pointing to other docs - README.md: Add CI badge and documentation section Also fixes: - Default RESEARCHER_DIR to $HOME/research - .git check to not skip .github directories Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4b198cd to
89ecc5e
Compare
Summary
CI Validation
Validates on every push/PR to main:
.jsonfilesDocumentation
CONTRIBUTING.mdAGENTS.mdTESTING.mdCLAUDE.mdFiles Added
Test Plan
./scripts/validate.shlocally - all checks passGenerated with Claude Code