Skip to content

Conversation

@GrosQuildu
Copy link
Contributor

@GrosQuildu GrosQuildu commented Feb 12, 2026

bats fails because plugins/gh-cli/hooks is now linted - need to fix the hooks in another PR

@GrosQuildu GrosQuildu requested a review from dguido as a code owner February 12, 2026 08:45
GrosQuildu and others added 7 commits February 12, 2026 10:01
- Remove dead CHANGED_FILES code path (unreachable in CI)
- Add version and description consistency checks between plugin.json
  and marketplace.json
- Eliminate duplicate plugin.json reads via parse_plugin_json()
- Fix bats test discovery for filenames with spaces (print0/xargs -0)
- Sync marketplace.json with plugin.json for 5 pre-existing mismatches
  (second-opinion version, ask-questions/burpsuite/fix-review/insecure-defaults descriptions)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dguido
Copy link
Member

dguido commented Feb 12, 2026

Code Review Summary

Reviewed with: security-sentinel, architecture-strategist, pattern-recognition-specialist, performance-oracle, code-simplicity-reviewer

Findings by Severity

Severity Count Fixed Dismissed
P2 (Important) 3 3 0
P3 (Nice-to-have) 5 1 4
Total 8 4 4

Fixed (P2)

  1. Dead CHANGED_FILES code pathextract_plugins_from_changed_files(), PLUGIN_PATH_PATTERN, os import, and the branching in main() were unreachable since no workflow sets CHANGED_FILES. Removed ~25 lines.
  2. Duplicate plugin.json reads — Both validate_plugin_json() and validate_marketplace_entry() independently opened and parsed plugin.json. Added parse_plugin_json() to read once, pass the result to both validators.
  3. Missing version/description consistency checksvalidate_marketplace_entry() checked name and source but not version or description. Added both checks. Also synced 5 pre-existing mismatches in marketplace.json (second-opinion version 1.0.0→1.2.0, plus description drifts in ask-questions-if-underspecified, burpsuite-project-parser, fix-review, insecure-defaults).

Fixed (P3)

  1. Bats find | xargs not space-safe — Changed to find -print0 | xargs -0 --no-run-if-empty bats.

Dismissed (P3)

  1. Shellcheck severity downgrade — Intentional change by PR author (warning→info is reasonable for CI).
  2. ValidationError as dataclass vs namedtuple — Dataclass is fine here; namedtuple would save negligible memory with no readability benefit.
  3. Missing unit tests for validation script — Valid observation but out of scope for this PR. The script is exercised by CI on every push.
  4. Silent failures on missing config filesparse_marketplace() etc. returning empty on missing files is correct behavior (graceful degradation for repos without all config files).

Quality Pipeline

  • ruff check: passed
  • ruff format: passed
  • pre-commit hooks: all passed
  • validate_plugin_metadata.py: all registered plugins pass (local-only dirs humanizer/skill-extractor correctly flagged)

dguido and others added 2 commits February 12, 2026 10:09
Resolve conflict in marketplace.json: accept removal of fix-review
entry (moved to skills-internal in PR #86).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The _no_gh test helpers used PATH=/usr/bin:/bin to exclude gh, but
on Ubuntu CI runners gh is installed at /usr/bin/gh. Fix by creating
a temp directory with symlinks to only jq and bash.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dguido dguido requested a review from Ninja3047 as a code owner February 12, 2026 15:20
- ask-questions-if-underspecified: restore trigger context ("asking
  questions") while keeping invocation constraint
- burpsuite-project-parser: drop filler "directly from the command
  line", use outcome-oriented "for security analysis"
- insecure-defaults: restore specific scenarios (hardcoded credentials,
  fallback secrets, weak auth defaults) for better matching

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dguido dguido merged commit f4b2a72 into main Feb 12, 2026
8 checks passed
@dguido dguido deleted the plugin-validation branch February 12, 2026 15:28
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.

2 participants