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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -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 <fixture> <label> <expected substring>...
|
||||
assert_fails_with() {
|
||||
local fixture="$1" label="$2"
|
||||
shift 2
|
||||
run_script "$fixture"
|
||||
if [[ $RUN_RC -eq 0 ]]; then
|
||||
fail "$label -- expected exit 1, got 0. Output: $RUN_OUT"
|
||||
return
|
||||
fi
|
||||
local needle
|
||||
for needle in "$@"; do
|
||||
if [[ "$RUN_OUT" != *"$needle"* ]]; then
|
||||
fail "$label -- exited $RUN_RC but message lacked '$needle'. Output: $RUN_OUT"
|
||||
return
|
||||
fi
|
||||
done
|
||||
pass "$label"
|
||||
}
|
||||
|
||||
# assert_passes <fixture> <label>
|
||||
assert_passes() {
|
||||
run_script "$1"
|
||||
if [[ $RUN_RC -eq 0 ]]; then
|
||||
pass "$2"
|
||||
else
|
||||
fail "$2 -- expected exit 0, got $RUN_RC. Output: $RUN_OUT"
|
||||
fi
|
||||
}
|
||||
|
||||
# Writes a marketplace.json listing every "<name>=<source>" pair given.
|
||||
write_marketplace() {
|
||||
local dir="$1" entries="" pair name src
|
||||
shift
|
||||
for pair in "$@"; do
|
||||
name="${pair%%=*}"
|
||||
src="${pair#*=}"
|
||||
entries+="${entries:+,}"$'\n'" { \"name\": \"$name\", \"source\": \"$src\" }"
|
||||
done
|
||||
mkdir -p "$dir/.claude-plugin"
|
||||
printf '{\n "name": "test-marketplace",\n "plugins": [%s\n ]\n}\n' "$entries" > "$dir/.claude-plugin/marketplace.json"
|
||||
}
|
||||
|
||||
# One trap over a registry rather than a fresh `trap 'rm -rf "$FIXTUREn"' EXIT`
|
||||
# per fixture: each such trap REPLACES the previous one, so only the last
|
||||
# fixture was ever cleaned and the rest leaked into TMPDIR every run. Same
|
||||
@@ -393,6 +443,186 @@ else
|
||||
fail "flagged a listed plugin because its source: string was spelled differently"
|
||||
fi
|
||||
|
||||
# --- 11. A marketplace entry with no `source` at all is rejected outright ---
|
||||
# It used to disable BOTH directions of the check for that plugin at once:
|
||||
# list_marketplace_local_plugins requires a string `source`, so the entry was skipped and
|
||||
# its .claude-plugin/plugin.json never checked; and the disk -> marketplace name axis
|
||||
# selected on `(.source | type) != "string"`, which is TRUE for null, so the same entry
|
||||
# also marked its on-disk directory "listed". Net effect: a plugin with a broken manifest
|
||||
# and a malformed entry passed clean, and silently dropped out of
|
||||
# sync-plugin-content.sh --all's work list too, since that derives from the same helper.
|
||||
echo ""
|
||||
echo "--- a marketplace entry with no source: field is a hard error ---"
|
||||
FIXTURE11="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE11")
|
||||
mkdir -p "$FIXTURE11/.claude-plugin" "$FIXTURE11/plugins/lint"
|
||||
cat > "$FIXTURE11/.claude-plugin/marketplace.json" <<'JSON'
|
||||
{
|
||||
"name": "test-marketplace",
|
||||
"plugins": [
|
||||
{ "name": "lint" }
|
||||
]
|
||||
}
|
||||
JSON
|
||||
printf 'name: lint\nversion: 0.1.0\ntype: skill\n' > "$FIXTURE11/plugins/lint/apm.yml"
|
||||
assert_fails_with "$FIXTURE11" \
|
||||
"an entry with no source: is reported by name instead of silently disabling both checks" \
|
||||
'no `source` field' 'lint'
|
||||
|
||||
# --- 12. A `skills` string (a legal shape per the host docs) is resolved, not counted ---
|
||||
# `jq '.skills | if . then length else 0 end'` is null-safe but not type-safe: on the
|
||||
# string "./skills/x" it returned the CHARACTER count, and the `.skills[0]` that followed
|
||||
# errored ("Cannot index string with number"), killing the whole script under `set -e`
|
||||
# with no "Manifest check failed:" line -- and every plugin later in the marketplace
|
||||
# unchecked. Both configuration.md references document `skills` as string | string[].
|
||||
echo ""
|
||||
echo "--- a string-valued skills field resolves instead of crashing the script ---"
|
||||
FIXTURE12="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE12")
|
||||
mkdir -p "$FIXTURE12/plugins/strskills/.claude-plugin" "$FIXTURE12/plugins/strskills/custom/skills"
|
||||
write_marketplace "$FIXTURE12" "strskills=./plugins/strskills"
|
||||
cat > "$FIXTURE12/plugins/strskills/.claude-plugin/plugin.json" <<'JSON'
|
||||
{
|
||||
"name": "strskills",
|
||||
"skills": "./custom/skills/"
|
||||
}
|
||||
JSON
|
||||
assert_passes "$FIXTURE12" "a resolving string-valued skills field passes"
|
||||
|
||||
# --- 13. A broken string `skills` is reported, and later plugins are still checked ---
|
||||
# The mid-loop `set -e` abort meant a fault in the FIRST plugin hid every fault after it.
|
||||
# The second entry here is broken in an unrelated way; both messages must appear.
|
||||
echo ""
|
||||
echo "--- a broken string skills field is reported without aborting the marketplace walk ---"
|
||||
FIXTURE13="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE13")
|
||||
mkdir -p "$FIXTURE13/plugins/first/.claude-plugin" "$FIXTURE13/plugins/second"
|
||||
write_marketplace "$FIXTURE13" "first=./plugins/first" "second=./plugins/second"
|
||||
cat > "$FIXTURE13/plugins/first/.claude-plugin/plugin.json" <<'JSON'
|
||||
{
|
||||
"name": "first",
|
||||
"skills": "./skills/does-not-exist"
|
||||
}
|
||||
JSON
|
||||
assert_fails_with "$FIXTURE13" \
|
||||
"a broken string skills field is reported and the walk continues to later plugins" \
|
||||
'skills path not found: ./skills/does-not-exist' \
|
||||
"plugin 'second': .claude-plugin/plugin.json not found" \
|
||||
'Manifest check failed: 2 error(s)'
|
||||
|
||||
# --- 14. A genuinely wrong-typed `skills` is named as such, walk still continues ---
|
||||
echo ""
|
||||
echo "--- a wrong-typed skills field is reported as a type error, not a missing path ---"
|
||||
FIXTURE14="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE14")
|
||||
mkdir -p "$FIXTURE14/plugins/first/.claude-plugin" "$FIXTURE14/plugins/second"
|
||||
write_marketplace "$FIXTURE14" "first=./plugins/first" "second=./plugins/second"
|
||||
cat > "$FIXTURE14/plugins/first/.claude-plugin/plugin.json" <<'JSON'
|
||||
{
|
||||
"name": "first",
|
||||
"skills": 42
|
||||
}
|
||||
JSON
|
||||
assert_fails_with "$FIXTURE14" \
|
||||
"a wrong-typed skills field names the type and does not abort the walk" \
|
||||
'skills must be a path string, an array of path strings, or an inline object, got number' \
|
||||
"plugin 'second': .claude-plugin/plugin.json not found" \
|
||||
'Manifest check failed: 2 error(s)'
|
||||
|
||||
# --- 15. Array- and object-valued pointer fields that resolve are not reported missing ---
|
||||
# `ref=$(jq -r ".$field // empty")` returned the PRETTY-PRINTED JSON for an array or an
|
||||
# object, which `[[ ! -e ]]` then rejected: a manifest whose paths all resolve was
|
||||
# reported broken. Both host docs give `agents` as string | string[] and `hooks` /
|
||||
# `mcpServers` as string | object (an inline definition, with no path to resolve).
|
||||
echo ""
|
||||
echo "--- array- and inline-object pointer fields that resolve are accepted ---"
|
||||
FIXTURE15="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE15")
|
||||
mkdir -p "$FIXTURE15/plugins/shapes/.claude-plugin" "$FIXTURE15/plugins/shapes/agents" "$FIXTURE15/plugins/shapes/skills/one"
|
||||
touch "$FIXTURE15/plugins/shapes/agents/real.md"
|
||||
write_marketplace "$FIXTURE15" "shapes=./plugins/shapes"
|
||||
cat > "$FIXTURE15/plugins/shapes/.claude-plugin/plugin.json" <<'JSON'
|
||||
{
|
||||
"name": "shapes",
|
||||
"skills": ["./skills/one"],
|
||||
"agents": ["./agents/real.md"],
|
||||
"hooks": { "PreToolUse": [{ "hooks": [{ "type": "command", "command": "true" }] }] },
|
||||
"mcpServers": { "demo": { "command": "true" } }
|
||||
}
|
||||
JSON
|
||||
assert_passes "$FIXTURE15" \
|
||||
"an array-valued agents and an inline-object hooks/mcpServers are not reported missing"
|
||||
|
||||
# --- 16. A broken element inside an array-valued pointer field is still caught ---
|
||||
# Guards the fix in #15 against over-correcting into "arrays are always fine".
|
||||
echo ""
|
||||
echo "--- a broken path inside an array-valued pointer field is still caught ---"
|
||||
FIXTURE16="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE16")
|
||||
mkdir -p "$FIXTURE16/plugins/shapes/.claude-plugin" "$FIXTURE16/plugins/shapes/agents"
|
||||
touch "$FIXTURE16/plugins/shapes/agents/real.md"
|
||||
write_marketplace "$FIXTURE16" "shapes=./plugins/shapes"
|
||||
cat > "$FIXTURE16/plugins/shapes/.claude-plugin/plugin.json" <<'JSON'
|
||||
{
|
||||
"name": "shapes",
|
||||
"agents": ["./agents/real.md", "./agents/ghost.md"]
|
||||
}
|
||||
JSON
|
||||
assert_fails_with "$FIXTURE16" \
|
||||
"a missing path in an array-valued agents field is reported with its own path" \
|
||||
'agents path not found: ./agents/ghost.md'
|
||||
|
||||
# --- 17. An unparseable marketplace.json is reported as such, not as unlisted plugins ---
|
||||
# The walk runs in a process substitution, so the helper's `set -e` abort on invalid JSON
|
||||
# never reached the caller. The run still exited 1 -- backstopped by the disk -> marketplace
|
||||
# pass -- but printed one "has no entry in .claude-plugin/marketplace.json ... add it to
|
||||
# root apm.yml" per plugin directory, sending the reader to edit apm.yml when the actual
|
||||
# fault was a corrupt manifest.
|
||||
echo ""
|
||||
echo "--- an unparseable marketplace.json is attributed to the manifest, not to the plugins ---"
|
||||
FIXTURE17="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE17")
|
||||
mkdir -p "$FIXTURE17/.claude-plugin" "$FIXTURE17/plugins/one/.claude-plugin" "$FIXTURE17/plugins/two/.claude-plugin"
|
||||
printf '{ "name": "test-marketplace", "plugins": [ { "name": "one",\n' > "$FIXTURE17/.claude-plugin/marketplace.json"
|
||||
echo '{ "name": "one" }' > "$FIXTURE17/plugins/one/.claude-plugin/plugin.json"
|
||||
echo '{ "name": "two" }' > "$FIXTURE17/plugins/two/.claude-plugin/plugin.json"
|
||||
run_script "$FIXTURE17"
|
||||
if [[ $RUN_RC -eq 0 ]]; then
|
||||
fail "exited 0 on an unparseable marketplace.json -- expected exit 1"
|
||||
elif [[ "$RUN_OUT" != *"is not valid JSON"* ]]; then
|
||||
fail "an unparseable marketplace.json was not named as such. Output: $RUN_OUT"
|
||||
elif [[ "$RUN_OUT" == *"has no entry in .claude-plugin/marketplace.json"* ]]; then
|
||||
fail "an unparseable marketplace.json was misreported as unlisted plugin directories. Output: $RUN_OUT"
|
||||
else
|
||||
pass "an unparseable marketplace.json is reported as invalid JSON, not as unlisted plugin directories"
|
||||
fi
|
||||
|
||||
# --- 18. A missing marketplace.json with plugins on disk is drift, not an opt-out ---
|
||||
# `[[ ! -f "$MARKETPLACE" ]] && exit 0` was the same empty-set-reads-as-pass shape as the
|
||||
# rest: per ADR-0015 the manifest is compiled from root apm.yml, so its absence next to
|
||||
# on-disk packages means the compiled output is missing, and every marketplace-derived
|
||||
# gate walks an empty plugin set in silence.
|
||||
echo ""
|
||||
echo "--- a missing marketplace.json alongside on-disk plugin directories fails ---"
|
||||
FIXTURE18="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE18")
|
||||
mkdir -p "$FIXTURE18/plugins/orphan/.apm/skills"
|
||||
assert_fails_with "$FIXTURE18" \
|
||||
"a missing marketplace.json with plugin directories present is reported as drift" \
|
||||
'.claude-plugin/marketplace.json does not exist' \
|
||||
'plugins/orphan'
|
||||
|
||||
# --- 18b. A missing marketplace.json with nothing to check still exits 0 ---
|
||||
# Guards the fix above against over-correcting into "always fail without a manifest":
|
||||
# a repo with no plugin directories genuinely has nothing for this gate to check.
|
||||
echo ""
|
||||
echo "--- a missing marketplace.json with no plugin directories still exits 0 ---"
|
||||
FIXTURE18B="$(mktemp -d)"
|
||||
FIXTURES+=("$FIXTURE18B")
|
||||
mkdir -p "$FIXTURE18B/plugins/scratch/notes" "$FIXTURE18B/docs"
|
||||
assert_passes "$FIXTURE18B" \
|
||||
"no marketplace.json and no plugin-marked directories is a genuine no-op, not a failure"
|
||||
|
||||
echo ""
|
||||
echo "Results: $PASS passed, $FAIL failed"
|
||||
[[ $FAIL -eq 0 ]]
|
||||
|
||||
Reference in New Issue
Block a user