From 3f1ee47f1e50427931643a63b8c3c92aa4226857 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Fri, 14 Aug 2026 11:03:57 +0000 Subject: [PATCH] fix(scripts): stop check-manifests passing on entries it cannot parse A marketplace entry missing its source key disabled both directions of the check at once. The helper required source to be a string, so a source-less entry was skipped and its plugin.json existence check never ran; the name axis selected on (.source | type) != "string", and null != "string" is true, so the same entry also marked its on-disk directory as listed. Delete source from an entry and delete its plugin.json and the script exited 0. Because sync-plugin-content.sh --all derives its work list from the same helper, that plugin silently dropped out of the content-mirror gate too. Also in this pass: - a wrongly typed skills value crashed the script mid-loop with a raw jq error and no "Manifest check failed:" line, leaving every later plugin unchecked. Note skills is legally string|string[] per both host schemas, so a string now resolves as a single path rather than erroring - array- and object-valued pointer fields were reported missing even when they resolved, because the whole JSON value was pretty-printed into a path test - an unparseable marketplace.json died inside a process substitution, so the run reported six "no entry in marketplace.json" errors that sent the reader to edit apm.yml when the real fault was a corrupt manifest - a missing marketplace.json exited 0 even with plugin directories present Tests: 14 -> 23 assertions. Every failure case asserts on message text, not exit code alone, since exit 1 here is reachable by several causes that call for opposite fixes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT --- scripts/check-manifests.sh | 157 +++++++++++++++----- scripts/lib/marketplace-plugins.sh | 38 +++++ tests/test-check-manifests.sh | 230 +++++++++++++++++++++++++++++ 3 files changed, 389 insertions(+), 36 deletions(-) diff --git a/scripts/check-manifests.sh b/scripts/check-manifests.sh index d3a499b..517c56f 100755 --- a/scripts/check-manifests.sh +++ b/scripts/check-manifests.sh @@ -29,6 +29,12 @@ set -euo pipefail # compiled-output drift of exactly the kind ADR-0017 wires pre-push gates for -- and it # is the same plugin set the validate-plugins pre-commit hook already globs as # plugins/*/. +# +# Every pass above reads its plugin set out of marketplace.json, so anything that makes +# that file yield nothing -- absent, unparseable, or an entry with no `source` -- used to +# read as "clean" rather than "unchecked". The guards below turn each of those into an +# explicit, attributable failure instead, because a vacuous pass is the one result a gate +# must never produce. REPO_ROOT="${1:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}" FAIL=0 @@ -41,15 +47,112 @@ if ! command -v jq &>/dev/null; then fi MARKETPLACE="$REPO_ROOT/.claude-plugin/marketplace.json" -if [[ ! -f "$MARKETPLACE" ]]; then - exit 0 -fi SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # Repo-root-relative, not script-dir-relative -- see tests/run-tests.sh for why. # shellcheck source=scripts/lib/marketplace-plugins.sh source "$SCRIPT_DIR/lib/marketplace-plugins.sh" +# Candidate plugin directories on disk. The trigger is any of the three markers that +# make a directory a plugin rather than scratch -- apm.yml (the ADR-0015 authoring +# source), .apm/ (its content tree), or a compiled .claude-plugin/plugin.json. Matching +# all three keeps this set aligned with the plugins/*/ glob the validate-plugins +# pre-commit hook uses, which is the disagreement the disk -> marketplace pass below +# exists to close; a directory with none of them is scratch and stays out of scope. +# +# It is collected before the marketplace is read because a missing marketplace.json is +# only "nothing to check" when there is also nothing on disk to check against it. +PLUGIN_DIRS=() +for candidate in "$REPO_ROOT"/plugins/*/; do + candidate="${candidate%/}" + [[ -d "$candidate" ]] || continue + if [[ ! -f "$candidate/apm.yml" && ! -d "$candidate/.apm" && ! -f "$candidate/.claude-plugin/plugin.json" ]]; then + continue + fi + PLUGIN_DIRS+=("$candidate") +done + +# An absent marketplace.json used to exit 0 unconditionally -- the same empty-set-reads- +# as-pass shape this script's other passes were fixed for. Per ADR-0015 the manifest is +# compiled output of root apm.yml's marketplace.packages[], so its absence alongside +# on-disk packages is drift, not an opt-out: it leaves every marketplace-derived gate +# (this one and sync-plugin-content.sh --all) walking an empty plugin set in silence. +if [[ ! -f "$MARKETPLACE" ]]; then + if [[ ${#PLUGIN_DIRS[@]} -eq 0 ]]; then + exit 0 + fi + listing="" + # Guarded expansion even though the check above makes an empty array unreachable + # here: bash 3.2 under `set -u` aborts on a bare expansion of an empty array, and + # tests/test-vale-wrap.sh's bash32_glob scan is line-based, so a guard two lines up + # cannot clear it. Same form as the disk -> marketplace loop below. + for candidate in ${PLUGIN_DIRS[@]+"${PLUGIN_DIRS[@]}"}; do + listing+="${listing:+, }${candidate#"$REPO_ROOT"/}" + done + err ".claude-plugin/marketplace.json does not exist, but plugins/ holds ${#PLUGIN_DIRS[@]} plugin directory/ies ($listing) — every marketplace-derived check (this one, and sync-plugin-content.sh --all) silently walks an empty plugin set without it. Recompile the manifests from root apm.yml with \`apm pack\`." + echo "Manifest check failed: $FAIL error(s)" >&2 + exit 1 +fi + +# Preconditions the marketplace walk below cannot report on itself: it runs inside a +# process substitution, so an abort in there is swallowed (see the helper's comment). +assert_marketplace_manifest_usable "$MARKETPLACE" + +# Validates one plugin.json pointer field against disk, for the non-apm fallback below. +# +# check_pointer_field +# +# test_flag is `test`'s: -d where only a directory is meaningful, -e otherwise. +# +# Per the vendored host docs (plugins/kyberforge/docs/research/docs/ +# claude-code-plugins/configuration.md and .../github-copilot-plugins/configuration.md) +# these fields are legally `string | string[] | object`. Reading them with +# `jq -r ".$field // empty"` collapsed the array and object shapes to their +# pretty-printed JSON text, which then matched no path on disk -- a manifest that +# resolves fine reported as broken. Reading `.skills | length` was worse than wrong: on +# a (legal) string value it returned the character count, and the `.skills[$i]` that +# followed aborted the whole script mid-loop under `set -e` with no summary line, so +# every plugin later in the marketplace went unchecked. +check_pointer_field() { + local name="$1" plugin_dir="$2" field="$3" test_flag="$4" + local manifest="$plugin_dir/.claude-plugin/plugin.json" + local field_type count i elem_type + + field_type="$(jq -r ".${field} | type" "$manifest")" + case "$field_type" in + null) ;; + # An inline definition (a hooks or mcpServers object written straight into the + # manifest) declares no path, so there is nothing on disk to resolve. + object) ;; + string) + check_pointer_path "$name" "$plugin_dir" "$field" "$(jq -r ".${field}" "$manifest")" "$test_flag" + ;; + array) + count="$(jq ".${field} | length" "$manifest")" + for ((i = 0; i < count; i++)); do + elem_type="$(jq -r ".${field}[$i] | type" "$manifest")" + if [[ "$elem_type" != "string" ]]; then + err "plugin '$name': ${field}[$i] must be a path string, got $elem_type" + continue + fi + check_pointer_path "$name" "$plugin_dir" "$field" "$(jq -r ".${field}[$i]" "$manifest")" "$test_flag" + done + ;; + *) + err "plugin '$name': $field must be a path string, an array of path strings, or an inline object, got $field_type" + ;; + esac +} + +check_pointer_path() { + local name="$1" plugin_dir="$2" field="$3" ref="$4" test_flag="$5" + local full_path="$plugin_dir/$ref" + full_path="${full_path%/}" + if ! test "$test_flag" "$full_path"; then + err "plugin '$name': $field path not found: $ref" + fi +} + # Every local plugin directory marketplace.json claimed, canonicalized, so the # disk -> marketplace pass below can tell "listed" from "unlisted" regardless of how # the `source:` string was spelled (./plugins/x, plugins/x, plugins/x/). @@ -78,33 +181,14 @@ while IFS=$'\t' read -r name plugin_dir; do # Fallback for a non-apm plugin: validate that any skills/hooks/mcpServers/agents # pointer fields in its hand-authored plugin.json still resolve to real paths. - skill_count=$(jq '.skills | if . then length else 0 end' "$manifest") - for ((s = 0; s < skill_count; s++)); do - skill_path=$(jq -r ".skills[$s]" "$manifest") - full_path="$plugin_dir/$skill_path" - full_path="${full_path%/}" - if [[ ! -d "$full_path" ]]; then - err "plugin '$name': skills path not found: $skill_path" - fi - done - - for field in hooks mcpServers agents; do - ref=$(jq -r ".${field} // empty" "$manifest") - [[ -z "$ref" ]] && continue - full_path="$plugin_dir/$ref" - full_path="${full_path%/}" - if [[ ! -e "$full_path" ]]; then - err "plugin '$name': $field path not found: $ref" - fi - done + # skills/agents point at directories; hooks/mcpServers may point at a file. + check_pointer_field "$name" "$plugin_dir" skills -d + check_pointer_field "$name" "$plugin_dir" agents -e + check_pointer_field "$name" "$plugin_dir" hooks -e + check_pointer_field "$name" "$plugin_dir" mcpServers -e done < <(list_marketplace_local_plugins "$REPO_ROOT" "$MARKETPLACE") -# Disk -> marketplace. The trigger is any of the three markers that make a directory -# a plugin rather than scratch -- apm.yml (the ADR-0015 authoring source), .apm/ (its -# content tree), or a compiled .claude-plugin/plugin.json. Matching all three keeps -# this set aligned with the plugins/*/ glob the validate-plugins pre-commit hook uses, -# which is the disagreement this check exists to close; a directory with none of them -# is scratch and stays out of scope. +# Disk -> marketplace, over the PLUGIN_DIRS candidate set collected above. # # A candidate counts as listed if it is either a directory some local entry pointed at # (path match, canonicalized above) or a directory whose name matches a REMOTE entry's @@ -117,18 +201,19 @@ done < <(list_marketplace_local_plugins "$REPO_ROOT" "$MARKETPLACE") # the basename of the directory it points at: an entry named "beta" pointing at # ./plugins/alpha would mark an unrelated, entirely unlisted plugins/beta/ as listed. # Local entries already have an exact path to match on, so they need no name fallback. +# +# `.source == null` has to be excluded explicitly: `(null | type) != "string"` is TRUE, +# so before this guard an entry with no `source` at all landed here and marked its +# same-named directory listed -- while the local walk above skipped it for lacking a +# string source. One malformed entry thus disabled BOTH directions of the check at once. +# assert_marketplace_manifest_usable now rejects such an entry outright; the guard stays +# because this select must not depend on that check running first. MARKETPLACE_NAMES=() while IFS= read -r entry_name; do [[ -n "$entry_name" ]] && MARKETPLACE_NAMES+=("$entry_name") -done < <(jq -r '.plugins[]? | select((.source | type) != "string") | .name // empty' "$MARKETPLACE") - -for candidate in "$REPO_ROOT"/plugins/*/; do - candidate="${candidate%/}" - [[ -d "$candidate" ]] || continue - if [[ ! -f "$candidate/apm.yml" && ! -d "$candidate/.apm" && ! -f "$candidate/.claude-plugin/plugin.json" ]]; then - continue - fi +done < <(jq -r '.plugins[]? | select(.source != null and (.source | type) != "string") | .name // empty' "$MARKETPLACE") +for candidate in ${PLUGIN_DIRS[@]+"${PLUGIN_DIRS[@]}"}; do candidate_abs="$(cd "$candidate" && pwd -P)" candidate_name="$(basename "$candidate")" listed=0 diff --git a/scripts/lib/marketplace-plugins.sh b/scripts/lib/marketplace-plugins.sh index a6c4128..f8c9b0d 100644 --- a/scripts/lib/marketplace-plugins.sh +++ b/scripts/lib/marketplace-plugins.sh @@ -7,6 +7,44 @@ # # Requires jq. Not meant to be executed directly -- source it. +# assert_marketplace_manifest_usable +# +# Checks the preconditions list_marketplace_local_plugins depends on but cannot +# report on. Both callers run the walk inside a process substitution +# (`done < <(list_marketplace_local_plugins ...)`), which is its own subshell: a +# jq abort in there kills only that subshell, so an unparseable manifest yields +# zero lines and reads exactly like "this marketplace declares no local plugins". +# The caller then blames whatever its empty-set branch blames -- for +# check-manifests.sh, every plugin directory on disk being unlisted. +# +# It also rejects an entry with no `source` at all. Such an entry is not a local +# plugin (the walk below requires a string `source`) and not a remote one either, +# so it silently drops out of every marketplace-derived work list -- this walk's +# and, through it, sync-plugin-content.sh --all's. +# +# Exits 1 with a specific message on any violation, so call it from the caller's +# own shell -- never inside `< <(...)`, which is the exact swallowing this guards. +assert_marketplace_manifest_usable() { + local marketplace="$1" plugins_type sourceless + + if ! jq empty "$marketplace" >/dev/null 2>&1; then + echo "Error: $marketplace is not valid JSON -- every marketplace-derived check reads as \"no plugins declared\" until it parses. Fix it, or recompile it with \`apm pack\`." >&2 + exit 1 + fi + + plugins_type="$(jq -r '.plugins | type' "$marketplace")" + if [[ "$plugins_type" != "array" && "$plugins_type" != "null" ]]; then + echo "Error: $marketplace has a \`plugins\` field of type $plugins_type; expected an array of plugin entries." >&2 + exit 1 + fi + + sourceless="$(jq -r '[.plugins[]? | select(.source == null) | .name // ""] | join(", ")' "$marketplace")" + if [[ -n "$sourceless" ]]; then + echo "Error: $marketplace has entries with no \`source\` field: $sourceless. An entry without a \`source\` is neither local nor remote, so it is skipped by every marketplace-derived check while still claiming its name. Give it a \`source\` in root apm.yml's marketplace.packages[] and recompile." >&2 + exit 1 + fi +} + # list_marketplace_local_plugins # # Prints one "\t" line per local (string `source:`) diff --git a/tests/test-check-manifests.sh b/tests/test-check-manifests.sh index 76a0226..309b7ee 100644 --- a/tests/test-check-manifests.sh +++ b/tests/test-check-manifests.sh @@ -9,6 +9,56 @@ FAIL=0 pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } +# Several distinct faults all end in exit 1, and the bugs fixed below were precisely +# about the WRONG one being reported (a corrupt manifest blamed on six unlisted plugin +# directories, a legal manifest blamed for unresolvable paths). Exit-code-only +# assertions cannot see that, so these cases assert on the message text. +RUN_OUT="" +RUN_RC=0 +run_script() { RUN_OUT="$(bash "$SCRIPT" "$1" 2>&1)" && RUN_RC=0 || RUN_RC=$?; } + +# assert_fails_with