diff --git a/scripts/check-manifests.sh b/scripts/check-manifests.sh index 517c56f..957883a 100755 --- a/scripts/check-manifests.sh +++ b/scripts/check-manifests.sh @@ -31,12 +31,27 @@ set -euo pipefail # 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 +# that file yield nothing -- absent, unparseable, a non-object root, or an entry whose +# `source` is neither a path string nor a remote object -- used to read as "clean" +# rather than "unchecked". The same is true one level down, of a per-plugin +# .claude-plugin/plugin.json that does not parse: it aborted the walk mid-loop and left +# every later plugin silently 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)}" +# Hard error, not a `|| pwd` fallback, for the reason spelled out in +# scripts/sync-marketplace-mirror.sh: every path below hangs off REPO_ROOT, and the +# exit-0 path is "nothing on disk and no manifest", so a REPO_ROOT pointing somewhere +# that is not this repo reports "clean" over a tree it never looked at. Run this from +# an empty directory outside any worktree and the fallback made that the literal +# outcome -- rev-parse failed, REPO_ROOT became $PWD, no plugins/ and no +# marketplace.json were found, exit 0, silent. +if [[ -n "${1:-}" ]]; then + REPO_ROOT="$1" +elif ! REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || [[ -z "$REPO_ROOT" ]]; then + echo "Error: not inside a git worktree -- cannot locate the repository root, and guessing \$PWD would let this check report \"clean\" over a tree it never inspected. Run it from within the repository, or pass the repo root as an argument." >&2 + exit 1 +fi FAIL=0 err() { echo " FAIL: $1" >&2; FAIL=$((FAIL + 1)); } @@ -113,6 +128,11 @@ assert_marketplace_manifest_usable "$MARKETPLACE" # 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. +# +# The bare `$(jq ...)` assignments below are safe only because the caller has already +# established that $manifest parses AND that its root is an object (see the +# precondition in the marketplace walk). Do not call this without that check: `set -e` +# turns any jq failure in here into the same silent mid-loop abort described above. check_pointer_field() { local name="$1" plugin_dir="$2" field="$3" test_flag="$4" local manifest="$plugin_dir/.claude-plugin/plugin.json" @@ -179,6 +199,25 @@ while IFS=$'\t' read -r name plugin_dir; do # job (see header comment above). [[ -d "$plugin_dir/.apm" ]] && continue + # Precondition for check_pointer_field, which reads the manifest with bare + # `field_type="$(jq ... )"` assignments. Under `set -e` a jq failure in one of + # those aborts the whole script mid-loop: rc=5, a raw `jq: parse error` and no + # `Manifest check failed:` summary, with every later plugin left unchecked -- + # the same failure class the marketplace's own `jq empty` precondition closes, + # for a file that is equally generated output. Both shapes have to be caught + # here: `jq empty` passes on a valid non-object document like `[]` or `123`, and + # it is the `.skills` lookup on such a root ("Cannot index array with string") + # that aborts, not the parse. + if ! jq empty "$manifest" >/dev/null 2>&1; then + err "plugin '$name': .claude-plugin/plugin.json is not valid JSON — it is compiled output, so recompile it with \`apm pack\`." + continue + fi + manifest_type="$(jq -r 'type' "$manifest")" + if [[ "$manifest_type" != "object" ]]; then + err "plugin '$name': .claude-plugin/plugin.json is a JSON $manifest_type at its top level; expected an object." + continue + fi + # 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. # skills/agents point at directories; hooks/mcpServers may point at a file. @@ -202,16 +241,16 @@ done < <(list_marketplace_local_plugins "$REPO_ROOT" "$MARKETPLACE") # ./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. +# The select is an allowlist of the object shape, not a denylist of the string one -- +# see list_marketplace_remote_plugin_names in scripts/lib/marketplace-plugins.sh, which +# owns it, and tests/test-check-manifests.sh, which exercises it directly against +# malformed entries rather than through this caller (where +# assert_marketplace_manifest_usable rejects them first, and so would mask a regression +# in the select itself). MARKETPLACE_NAMES=() while IFS= read -r entry_name; do [[ -n "$entry_name" ]] && MARKETPLACE_NAMES+=("$entry_name") -done < <(jq -r '.plugins[]? | select(.source != null and (.source | type) != "string") | .name // empty' "$MARKETPLACE") +done < <(list_marketplace_remote_plugin_names "$MARKETPLACE") for candidate in ${PLUGIN_DIRS[@]+"${PLUGIN_DIRS[@]}"}; do candidate_abs="$(cd "$candidate" && pwd -P)" diff --git a/scripts/lib/marketplace-plugins.sh b/scripts/lib/marketplace-plugins.sh index f8c9b0d..26de32b 100644 --- a/scripts/lib/marketplace-plugins.sh +++ b/scripts/lib/marketplace-plugins.sh @@ -17,30 +17,51 @@ # 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. +# It also rejects an entry whose `source` is neither a local path string nor a +# remote source object. Only those two shapes are classifiable: the walk below +# takes the string ones, and the object ones are remote. Anything else -- absent +# (`null`), a number, an array, a boolean -- is neither, so it silently drops out +# of every marketplace-derived work list: this walk's and, through it, +# sync-plugin-content.sh --all's. +# +# The check is deliberately typed as "not string AND not object" rather than +# enumerating `.source == null`. Rejecting null specifically left every other +# malformed value (`"source": 42`, `"source": []`) passing the assert, skipped by +# the walk below, AND rescued by check-manifests.sh's disk -> marketplace name +# axis -- i.e. exactly the defect the null case was fixed for, reached with a +# different value. # # 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 + local marketplace="$1" root_type plugins_type unclassifiable 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 + # `jq empty` passes on any valid JSON document, including `[]`, `"x"` and `123`. + # The `.plugins` lookup on the next line then aborts with a raw + # `jq: error: Cannot index array with string "plugins"` and rc=5, attributed to + # nothing -- so assert the root shape here, where it can be named. + root_type="$(jq -r 'type' "$marketplace")" + if [[ "$root_type" != "object" ]]; then + echo "Error: $marketplace is a JSON $root_type at its top level; expected an object with a \`plugins\` array. 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 + unclassifiable="$(jq -r '[.plugins[]? + | select((.source | type) as $t | $t != "string" and $t != "object") + | "\(.name // "") (source: \(.source | type))"] | join(", ")' "$marketplace")" + if [[ -n "$unclassifiable" ]]; then + echo "Error: $marketplace has entries whose \`source\` is neither a local path string nor a remote source object: $unclassifiable. Such an entry is neither local nor remote, so it is skipped by every marketplace-derived check while still claiming its name. Fix it in root apm.yml's marketplace.packages[] and recompile." >&2 exit 1 fi } @@ -64,3 +85,26 @@ list_marketplace_local_plugins() { printf '%s\t%s\n' "$name" "$repo_root/$source" done } + +# list_marketplace_remote_plugin_names +# +# Prints the `name` of every REMOTE (object `source:`) marketplace entry, one per +# line -- the exact complement of list_marketplace_local_plugins. +# +# check-manifests.sh's disk -> marketplace pass uses it as its name axis: a plugin +# vendored on disk but declared with the remote-object shape has no local entry to +# path-match against, so without a name match it would be reported as unlisted when +# its entry is in fact right there. +# +# The select is `(.source | type) == "object"`, an allowlist of the one shape that +# axis is actually for -- NOT the denylist `(.source | type) != "string"` it used to +# be. That denylist was true for `null` (and for numbers, arrays, booleans), so a +# malformed entry marked its same-named directory "listed" while the local walk above +# skipped it for lacking a string source: one bad entry disabled BOTH directions of +# the check at once. assert_marketplace_manifest_usable rejects those shapes too, but +# this function must be correct on its own -- it is called from a different script, +# and a precondition that stops running is not a property of this select. +list_marketplace_remote_plugin_names() { + local marketplace="$1" + jq -r '.plugins[]? | select((.source | type) == "object") | .name // empty' "$marketplace" +} diff --git a/scripts/sync-plugin-content.sh b/scripts/sync-plugin-content.sh index 44aa705..2d0b245 100755 --- a/scripts/sync-plugin-content.sh +++ b/scripts/sync-plugin-content.sh @@ -165,7 +165,17 @@ if [[ "$ALL" -eq 1 ]]; then # the same one scripts/check-manifests.sh uses, instead of hand-maintaining a # duplicate walk at every call site (see .pre-commit-config.yaml's # check-plugin-content-sync). - REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" + # + # Hard error rather than a `|| pwd` fallback, on scripts/sync-marketplace-mirror.sh's + # reasoning: --all's entire work list hangs off REPO_ROOT, so a REPO_ROOT pointing at + # something that is not this repo checks a plugin set that is not this repo's. Run + # from outside a worktree the fallback happens to hit the `--all requires ...` error + # below instead -- but only by accident, because $PWD had no marketplace.json in it; + # $PWD holding an unrelated one is the case that would silently "pass". + if ! REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || [[ -z "$REPO_ROOT" ]]; then + echo "Error: not inside a git worktree -- cannot locate the repository root, and guessing \$PWD would let --all derive its plugin list from a marketplace.json that is not this repo's. Run this from within the repository." >&2 + exit 1 + fi MARKETPLACE="$REPO_ROOT/.claude-plugin/marketplace.json" if [[ ! -f "$MARKETPLACE" ]]; then echo "Error: --all requires $MARKETPLACE" >&2 @@ -341,34 +351,56 @@ check_file() { fi } +# Relative paths whose mode the manifest below deliberately does NOT record. See +# path_manifest's comment for the rule; this is the list of paths it applies to -- +# every file this pipeline WRITES rather than `cp -a`s. +NO_MODE_PATHS=("$HOOKS_REL" "$LEGACY_HOOKS_REL") + # Prints " " for every entry under the given # relative paths. `find` walks; $STAT_MODE_ARGS (probed once at startup) reads the # mode, because stat's flags for mode formatting are incompatible between GNU and # BSD/macOS. # -# Full permission bits on FILES, not just the exec bit: an earlier revision emitted -# a bare `exec`/`file` kind, so `chmod 444` on a mirrored SKILL.md left --check at -# exit 0 while a real sync restored 644 -- check and sync disagreeing again, in the -# same shape the exec-bit case already proved. Git tracks only the exec bit, so this -# cannot arrive via a clone, but the gate's contract is that it agrees with a real -# sync about everything a real sync writes. +# THE RULE: a mode is recorded for a path this pipeline COPIES, and not for one it +# WRITES. The two sides of the comparison are a git checkout (actual) and a fresh +# apm-pack-plus-mirror (expected), so a copied path's mode traces to the same +# checkout on both sides and comparing it is meaningful; a written path's mode is +# `0666 & ~umask` of whichever process wrote it -- the runtime umask on the expected +# side, the umask of the checkout that produced the committed file on the actual +# side. Those two are independent, git records neither, and no sync can make them +# converge, so comparing them reports the runner's umask instead of a property of +# the mirror. # -# DIRECTORIES record no mode, deliberately. Nothing in this script ever sets one: -# the expected side's directories come from `mkdir -p` and `cp -a` under the -# running process's umask, the real side's from git checkout under whatever umask -# cloned the repo, and git tracks no directory mode at any point in between. So the -# comparison would report the runner's umask rather than any property of the -# mirror -- a repo cloned at umask 002 and checked at 022 would fail this gate on -# every directory with nothing wrong. Verified concretely: extracting this repo -# with `git archive | tar -x` (which restores 0775/0664 when run as root) makes a -# --check against the extracted tree report drift on every mirrored directory, -# while the same check against the real 0755 tree is silent. Mirrored FILES do not -# have this problem: both sides trace to the same checkout, since the expected side -# is `cp -a`'d from a bundle apm built out of the same .apm/ files. +# Full permission bits on COPIED FILES, not just the exec bit: an earlier revision +# emitted a bare `exec`/`file` kind, so `chmod 444` on a mirrored SKILL.md left +# --check at exit 0 while a real sync restored 644 -- check and sync disagreeing +# again, in the same shape the exec-bit case already proved. Git tracks only the exec +# bit, so this cannot arrive via a clone, but the gate's contract is that it agrees +# with a real sync about everything a real sync writes. +# +# DIRECTORIES record no mode: nothing here sets one, they come from `mkdir -p` and +# `cp -a`, and git tracks no directory mode. Verified concretely -- extracting this +# repo with `git archive | tar -x` (which restores 0775/0664 when run as root) makes +# a --check against the extracted tree report drift on every mirrored directory, +# while the same check against the real 0755 tree is silent. +# +# $NO_MODE_PATHS record no mode for the identical reason, and this is where the +# "copied files are immune because both sides trace to the same checkout" premise +# stops holding. hooks/hooks.json is not copied: sync_hooks_json writes it with +# `printf '%s\n' >`, at the RUNTIME umask. Widening the file comparison from the exec +# bit to full permission bits therefore made the gate umask-dependent -- on a +# umask-002 machine, `--check --all` over a umask-022 checkout reported +# `< file 664 hooks/hooks.json` / `> file 644` for every plugin with hooks, and it +# was not fixable by committing: a real sync writes 664, `git status` stays empty +# because git tracks no non-exec mode, and the next --check from a umask-022 machine +# fails in the opposite direction. +# +# The two generated plugin.json manifests are not in this manifest AT ALL -- see +# sync_one's checked_paths for why listing them measured nothing. path_manifest() { local root="$1" shift - local rel f kind mode + local rel f kind mode no_mode for rel in "$@"; do if [[ ! -e "$root/$rel" ]] && [[ ! -L "$root/$rel" ]]; then continue @@ -386,6 +418,12 @@ path_manifest() { else kind="file" mode="$(stat "${STAT_MODE_ARGS[@]}" "$f")" + for no_mode in "${NO_MODE_PATHS[@]}"; do + if [[ "${f#"$root"/}" == "$no_mode" ]]; then + mode="-" + break + fi + done fi printf '%s %s %s\n' "$kind" "$mode" "${f#"$root"/}" done || true @@ -462,13 +500,17 @@ reinject_mcp_servers() { # manifest. Git records only the exec bit, so the demotion survived every # commit and review unnoticed -- plugins/bin/.github/plugin/plugin.json really # was 0600 on disk while its five siblings were 0644. Redirecting into $dst - # keeps its inode, owner and mode; the chmod then pins the mode of a file this - # script owns as generated output, so a fresh sync and a re-sync over a - # tampered tree converge on the same answer (and --check, which now carries - # this path in its mode manifest, can see when they would not). + # keeps its inode, owner and mode, which is the whole fix: whatever mode apm + # pack gave the manifest a moment ago is exactly the mode it still has. + # + # Deliberately NO `chmod 644` after it. A hardcoded mode here does not pin + # anything a re-sync could converge on -- apm pack created $dst at the runtime + # umask, and the committed file carries the umask of the checkout that produced + # it -- it only makes those two disagree. It did: on a umask-002 checkout, + # --check reported `< file 644 .github/plugin/plugin.json` / `> file 664` with + # nothing wrong. See path_manifest's $NO_MODE_PATHS comment for the rule. cat "$tmp" >"$dst" rm -f "$tmp" - chmod 644 "$dst" } # --check-only: diffs a freshly-regenerated manifest file (in the throwaway @@ -479,6 +521,18 @@ sync_plugin_manifest() { local plugin_dir="$1" pack_cwd="$2" rel="$3" local src="$pack_cwd/$rel" dst="$plugin_dir/$rel" + # A manifest that is a symlink is not a cosmetic difference: apm pack opens it + # for writing and reinject_mcp_servers redirects into it, and both follow the + # link -- so a real sync silently rewrites whatever it points at instead of the + # manifest. It has to be asserted against the real plugin root like this, not via + # check_path_modes: that compares against a `cp -a` of this same root, which + # reproduces the symlink on the expected side and reports the two as equal. + if [[ -L "$dst" ]]; then + echo "DRIFT $dst: is a symlink -- apm pack and the mcpServers re-injection both write THROUGH it, so a real sync would overwrite its target instead of the manifest. Replace it with a regular file." >&2 + FAIL=1 + return 0 + fi + if [[ -f "$src" ]]; then if [[ ! -f "$dst" ]]; then echo "DRIFT $dst: missing (would be created by apm pack from apm.yml/.mcp.json)" >&2 @@ -577,14 +631,21 @@ sync_one() { # generated directory (see check_path_modes), and listing it recursively already # covers hooks/hooks.json. # - # .github/plugin/plugin.json is in the list even though sync_plugin_manifest - # below already diffs its CONTENT: that diff is content-only, so the mode - # reinject_mcp_servers writes was outside --check's manifest entirely and the - # 0600 demotion above went undetected for as long as it existed. Its sibling - # .claude-plugin/plugin.json is listed for the same reason -- nothing here - # writes its mode today, which is exactly the state worth pinning. - checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_DIR_REL" "$LEGACY_HOOKS_REL" \ - ".claude-plugin/plugin.json" ".github/plugin/plugin.json") + # Neither generated plugin.json is listed here, and adding one back measures + # nothing on any of the three axes this manifest compares. Mode: in check mode + # the expected side is the seeded pack_cwd COPY of the real plugin root, where + # apm pack rewrites a file that is already there and open-for-write preserves + # the existing inode's mode -- so the expected mode is inherited from the actual + # mode by construction. Verified: `chmod 600 plugins/bin/.claude-plugin/ + # plugin.json` left --check at exit 0 the entire time that entry was listed. + # Type: a symlinked manifest survives the same `cp -a` as a symlink and apm pack + # writes straight through it, so both sides record `symlink` -- also verified at + # exit 0. Presence: sync_plugin_manifest already reports both directions, with a + # message naming apm.yml as the thing to fix. A symlinked manifest IS a real + # hazard (apm pack and reinject_mcp_servers both write through it, corrupting + # whatever it points at), so it is asserted where it can actually be seen -- + # against the real plugin root, in sync_plugin_manifest. + checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_DIR_REL" "$LEGACY_HOOKS_REL") for d in "${MIRROR_DIRS[@]}"; do check_dir "$plugin_dir" "$pack_cwd" "$d" done diff --git a/tests/test-check-manifests.sh b/tests/test-check-manifests.sh index 309b7ee..2c76648 100644 --- a/tests/test-check-manifests.sh +++ b/tests/test-check-manifests.sh @@ -467,7 +467,83 @@ 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' + '`source` is neither a local path string nor a remote source object' 'lint (source: null)' + +# --- 11b. Any other unclassifiable `source` is rejected the same way --- +# The guard used to test `.source == null` specifically, so every OTHER malformed value +# reached exactly the state the null case was fixed for: `"source": 42` passed the +# assert, was skipped by list_marketplace_local_plugins for not being a string, AND was +# rescued by the disk -> marketplace name axis (whose select was the denylist +# `(.source|type) != "string"`, true for a number). Verbatim the same defect, one value +# over. Only two shapes are classifiable -- a local path string and a remote source +# object -- so the guard is typed as "neither of those", not as a list of known-bad +# values. +echo "" +echo "--- a non-string, non-object source: is rejected by type, not by enumerating null ---" +for BAD_SOURCE in '42' '[]' 'true'; do + FIXTURE11B="$(mktemp -d)" + FIXTURES+=("$FIXTURE11B") + mkdir -p "$FIXTURE11B/.claude-plugin" "$FIXTURE11B/plugins/lint" + printf '{ "name": "test-marketplace", "plugins": [ { "name": "lint", "source": %s } ] }\n' \ + "$BAD_SOURCE" > "$FIXTURE11B/.claude-plugin/marketplace.json" + printf 'name: lint\nversion: 0.1.0\ntype: skill\n' > "$FIXTURE11B/plugins/lint/apm.yml" + assert_fails_with "$FIXTURE11B" \ + "a source: of $BAD_SOURCE is rejected instead of silently disabling both checks" \ + '`source` is neither a local path string nor a remote source object' 'lint (source:' +done + +# --- 11c. The disk -> marketplace name axis, exercised WITHOUT the precondition --- +# This is the one assertion that cannot go through bash "$SCRIPT": every malformed entry +# the select must reject is rejected first by assert_marketplace_manifest_usable, which +# exits before the select ever runs. So reverting the select alone left the whole suite +# green -- the code carried a comment claiming it "must not depend on that check running +# first", and nothing tested that independence. Call the function directly instead. +# +# The invariant: the name axis exists solely for a plugin vendored on disk under a +# REMOTE (object) `source:`, which has no local path to match on. Every other shape -- +# a local string (which matches by path and needs no name fallback, see case 9d) and +# every unclassifiable value -- must produce no name at all. +echo "" +echo "--- list_marketplace_remote_plugin_names emits object-source names only ---" +# shellcheck source=scripts/lib/marketplace-plugins.sh +source "$REPO_ROOT/scripts/lib/marketplace-plugins.sh" +FIXTURE11C="$(mktemp -d)" +FIXTURES+=("$FIXTURE11C") +cat > "$FIXTURE11C/marketplace.json" <<'JSON' +{ + "name": "test-marketplace", + "plugins": [ + { "name": "remote-obj", "source": { "repo": "someorg/somerepo", "source": "github" } }, + { "name": "local-str", "source": "./plugins/local-str" }, + { "name": "null-src" }, + { "name": "explicit-null", "source": null }, + { "name": "number-src", "source": 42 }, + { "name": "array-src", "source": [] }, + { "name": "bool-src", "source": true } + ] +} +JSON +NAMES11C="$(list_marketplace_remote_plugin_names "$FIXTURE11C/marketplace.json")" +if [[ "$NAMES11C" == "remote-obj" ]]; then + pass "only the remote object-source entry yields a name for the disk -> marketplace name axis" +else + fail "the name axis emitted $(printf '%s' "$NAMES11C" | tr '\n' ' ')— expected exactly 'remote-obj'; every other shape would rescue a same-named orphan directory" +fi + +# --- 11d. A valid-JSON, non-object marketplace root is named, not left to crash jq --- +# `jq empty` passes on `[]`, `"x"` and `123`; the `.plugins` lookup on the next line then +# died with a raw `jq: error: Cannot index array with string "plugins"` and rc=5, +# attributed to nothing at all. +echo "" +echo "--- a valid-JSON non-object marketplace root is reported as such ---" +FIXTURE11D="$(mktemp -d)" +FIXTURES+=("$FIXTURE11D") +mkdir -p "$FIXTURE11D/.claude-plugin" "$FIXTURE11D/plugins/one" +printf '[]\n' > "$FIXTURE11D/.claude-plugin/marketplace.json" +printf 'name: one\nversion: 0.1.0\ntype: skill\n' > "$FIXTURE11D/plugins/one/apm.yml" +assert_fails_with "$FIXTURE11D" \ + "a JSON array at the marketplace root is named as a root-shape error" \ + 'is a JSON array at its top level' # --- 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 @@ -623,6 +699,73 @@ 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" +# --- 19. An unparseable per-plugin plugin.json is attributed, and the walk continues --- +# check_pointer_field reads the manifest with bare `$(jq ...)` assignments, so under +# `set -e` a parse failure aborted the whole script mid-loop: rc=5, a raw +# `jq: parse error` on stderr, no `Manifest check failed:` summary, and every plugin +# later in the marketplace silently unchecked. That is the same failure class the +# marketplace's own `jq empty` precondition closes -- and .claude-plugin/plugin.json is +# equally generated output, so it is equally capable of being corrupt. +# +# The second entry is broken in an unrelated way; both messages plus the summary must +# appear, which is what proves the walk survived the first fault. +echo "" +echo "--- an unparseable plugin.json is reported and does not abort the marketplace walk ---" +FIXTURE19="$(mktemp -d)" +FIXTURES+=("$FIXTURE19") +mkdir -p "$FIXTURE19/plugins/corrupt/.claude-plugin" "$FIXTURE19/plugins/second" +write_marketplace "$FIXTURE19" "corrupt=./plugins/corrupt" "second=./plugins/second" +printf '{ "name": "corrupt",\n' > "$FIXTURE19/plugins/corrupt/.claude-plugin/plugin.json" +assert_fails_with "$FIXTURE19" \ + "an unparseable plugin.json is named and later plugins are still checked" \ + "plugin 'corrupt': .claude-plugin/plugin.json is not valid JSON" \ + "plugin 'second': .claude-plugin/plugin.json not found" \ + 'Manifest check failed: 2 error(s)' + +# --- 19b. A valid-JSON but non-object plugin.json is caught too --- +# `jq empty` passes on `[]`; it is the `.skills` lookup on such a root that aborts +# ("Cannot index array with string"), not the parse -- so the parse check alone would +# leave this exact crash reachable. +echo "" +echo "--- a valid-JSON non-object plugin.json is reported, not left to crash jq ---" +FIXTURE19B="$(mktemp -d)" +FIXTURES+=("$FIXTURE19B") +mkdir -p "$FIXTURE19B/plugins/arrayjson/.claude-plugin" "$FIXTURE19B/plugins/second" +write_marketplace "$FIXTURE19B" "arrayjson=./plugins/arrayjson" "second=./plugins/second" +printf '[]\n' > "$FIXTURE19B/plugins/arrayjson/.claude-plugin/plugin.json" +assert_fails_with "$FIXTURE19B" \ + "a non-object plugin.json is named by type and later plugins are still checked" \ + "plugin 'arrayjson': .claude-plugin/plugin.json is a JSON array at its top level" \ + "plugin 'second': .claude-plugin/plugin.json not found" \ + 'Manifest check failed: 2 error(s)' + +# --- 20. Run with no argument outside a worktree: refuse, do not guess $PWD --- +# Every path this script touches hangs off REPO_ROOT, and its exit-0 path is "no +# manifest and nothing on disk" -- so `|| pwd` made a run from an empty directory +# outside any worktree exit 0, silently, having inspected no repository at all. Same +# reasoning as scripts/sync-marketplace-mirror.sh, which dropped its fallback first. +# +# `env -u GIT_DIR -u GIT_WORK_TREE` because run-tests.sh runs as a pre-push hook and git +# hooks export both, which would re-target `git rev-parse --show-toplevel` at the LIVE +# repo from any cwd -- making this case pass for the wrong reason. +echo "" +echo "--- with no argument outside a git worktree, it refuses instead of guessing \$PWD ---" +FIXTURE20="$(mktemp -d)" +FIXTURES+=("$FIXTURE20") +if (cd "$FIXTURE20" && env -u GIT_DIR -u GIT_WORK_TREE git rev-parse --show-toplevel) >/dev/null 2>&1; then + fail "fixture precondition: $FIXTURE20 is inside a git worktree, so this case cannot test the no-worktree path" +else + RC20=0 + OUT20="$(cd "$FIXTURE20" && env -u GIT_DIR -u GIT_WORK_TREE bash "$SCRIPT" 2>&1)" || RC20=$? + if [[ $RC20 -eq 0 ]]; then + fail "exited 0 from outside a worktree with no argument -- it checked nothing and said so to no one" + elif [[ "$OUT20" != *"not inside a git worktree"* ]]; then + fail "exited $RC20 outside a worktree but not for the stated reason. Output: $OUT20" + else + pass "refuses to guess \$PWD when it cannot locate the repository root" + fi +fi + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]] diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index 51faeed..be880f5 100755 --- a/tests/test-sync-plugin-content.sh +++ b/tests/test-sync-plugin-content.sh @@ -324,21 +324,39 @@ else pass "mcpServers is not an inlined object" fi -# --- 10b. .github/plugin/plugin.json is world-readable 0644, not mktemp's 0600 --- +# --- 10b. The re-injection preserves the manifest's own mode, rather than importing one --- # reinject_mcp_servers used to build its replacement in a `mktemp` file (mode 0600) # and `mv` it over the manifest, carrying 0600 onto a tracked, published file. Git # records only the exec bit, so the demotion survived every commit unnoticed — this # repo's own plugins/bin/.github/plugin/plugin.json really was 0600 on disk while # its five siblings were 0644. +# +# The fix is `cat "$tmp" >"$dst"`, which keeps the destination inode: the manifest +# ends up at whatever mode apm pack gave it a moment earlier, i.e. 0666 & ~umask like +# any other freshly created file. So this is asserted across two umasks rather than +# against a hardcoded 644 — that is what distinguishes "preserved" from "assigned". +# A `mv` of the mktemp yields 600 under both; a `chmod 644` yields 644 under both, +# which is the umask dependence that made --check fail on a umask-002 checkout. +mode_of() { + stat -c '%a' "$1" 2>/dev/null || stat -f '%Lp' "$1" 2>/dev/null +} echo "" -echo "--- a real sync leaves .github/plugin/plugin.json at mode 644 ---" -MODE10="$(stat -c '%a' "$FIXTURE10/.github/plugin/plugin.json" 2>/dev/null \ - || stat -f '%Lp' "$FIXTURE10/.github/plugin/plugin.json" 2>/dev/null)" -if [[ "$MODE10" == "644" ]]; then - pass "mcpServers re-injection leaves the manifest at 644" -else - fail "mcpServers re-injection left .github/plugin/plugin.json at mode $MODE10, expected 644" -fi +echo "--- the mcpServers re-injection leaves the manifest at the umask's own file mode ---" +for UMASK10B in 022 002; do + case "$UMASK10B" in + 022) EXPECT10B=644 ;; + 002) EXPECT10B=664 ;; + esac + FIXTURE10B="$(umask "$UMASK10B"; make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')" + track "$FIXTURE10B" + (umask "$UMASK10B"; bash "$SCRIPT" "$FIXTURE10B" > /dev/null 2>&1) + MODE10B="$(mode_of "$FIXTURE10B/.github/plugin/plugin.json")" + if [[ "$MODE10B" == "$EXPECT10B" ]]; then + pass "under umask $UMASK10B the re-injected manifest is $EXPECT10B (its own mode, not mktemp's 600 and not a hardcoded one)" + else + fail "under umask $UMASK10B the re-injected manifest is $MODE10B, expected $EXPECT10B" + fi +done # --- 11. An empty .mcp.json does not add a redundant mcpServers: {} --- echo "" @@ -782,35 +800,62 @@ else fi fi -# --- 25. --check sees a mode change on the generated Copilot manifest --- -# sync_plugin_manifest diffs that file's CONTENT only, and check_path_modes did not -# cover .github/plugin/ at all — so the mode reinject_mcp_servers writes was outside -# --check's manifest entirely, and check and sync could disagree about it forever. +# --- 25. The generated manifests: --check and a real sync agree about their mode --- +# The gate's contract is agreement between --check and a real sync, not "every +# property is repaired". Neither touches a generated manifest's permission bits, and +# neither can: in check mode the expected side is a `cp -a` of the real plugin root, +# so apm pack rewrites a file whose mode is already the actual side's. A revision that +# listed these paths in the mode manifest was measuring that inheritance, not the +# mirror — `chmod 600` left --check at exit 0 for as long as it was listed. This case +# pins the agreement instead, so a future "fix" that makes --check report a mode it +# cannot repair fails here. echo "" -echo "--- --check detects a mode change on .github/plugin/plugin.json ---" +echo "--- --check and a real sync agree that a manifest's mode is not theirs to change ---" FIXTURE25="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE25" bash "$SCRIPT" "$FIXTURE25" > /dev/null 2>&1 if ! bash "$SCRIPT" --check "$FIXTURE25" > /dev/null 2>&1; then fail "check reported drift right after the initial sync — cannot test the manifest-mode case" else chmod 600 "$FIXTURE25/.github/plugin/plugin.json" - CHECK25="$(bash "$SCRIPT" --check "$FIXTURE25" 2>&1 || true)" - case "$CHECK25" in - *"mirrored paths/types/modes differ"*) - pass "a mode change on .github/plugin/plugin.json is reported as drift" ;; - *) - fail "no mode drift reported after chmod 600 on .github/plugin/plugin.json" ;; - esac - bash "$SCRIPT" "$FIXTURE25" > /dev/null 2>&1 - MODE25="$(stat -c '%a' "$FIXTURE25/.github/plugin/plugin.json" 2>/dev/null \ - || stat -f '%Lp' "$FIXTURE25/.github/plugin/plugin.json" 2>/dev/null)" - if [[ "$MODE25" == "644" ]] && bash "$SCRIPT" --check "$FIXTURE25" > /dev/null 2>&1; then - pass "re-sync restores mode 644 and clears the drift" + if bash "$SCRIPT" --check "$FIXTURE25" > /dev/null 2>&1; then + bash "$SCRIPT" "$FIXTURE25" > /dev/null 2>&1 + MODE25="$(mode_of "$FIXTURE25/.github/plugin/plugin.json")" + if [[ "$MODE25" == "600" ]]; then + pass "--check reports no mode drift on a manifest, and a real sync indeed leaves the mode alone" + else + fail "--check reported no mode drift but a real sync changed the mode to $MODE25 — check and sync disagree" + fi else - fail "re-sync left .github/plugin/plugin.json at mode $MODE25 / did not clear the drift" + fail "--check reported drift after chmod 600 on .github/plugin/plugin.json, but a real sync cannot repair it — an unfixable pre-push failure" fi fi +# --- 25b. A manifest replaced by a SYMLINK is real drift and is reported --- +# This is the one property of the generated manifests worth asserting, and the reason +# it cannot live in check_path_modes: that compares against a `cp -a` of the same +# plugin root, which reproduces the symlink on the expected side and calls the two +# equal (verified — it sat at exit 0). The hazard is concrete: apm pack opens the +# manifest for writing and reinject_mcp_servers redirects into it, and both follow the +# link, so a real sync rewrites the link's TARGET instead of the manifest. +echo "" +echo "--- a plugin.json replaced by a symlink is reported as drift ---" +FIXTURE25B="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE25B" +bash "$SCRIPT" "$FIXTURE25B" > /dev/null 2>&1 +if [[ ! -f "$FIXTURE25B/.claude-plugin/plugin.json" ]]; then + fail "sync produced no .claude-plugin/plugin.json — cannot test the symlinked-manifest case" +else + cp "$FIXTURE25B/.claude-plugin/plugin.json" "$FIXTURE25B/decoy.json" + rm -f "$FIXTURE25B/.claude-plugin/plugin.json" + ln -s ../decoy.json "$FIXTURE25B/.claude-plugin/plugin.json" + CHECK25B="$(bash "$SCRIPT" --check "$FIXTURE25B" 2>&1 || true)" + case "$CHECK25B" in + *"DRIFT $FIXTURE25B/.claude-plugin/plugin.json: is a symlink"*) + pass "a symlinked .claude-plugin/plugin.json is reported as drift" ;; + *) + fail "no drift reported for a symlinked .claude-plugin/plugin.json — a real sync would write through it. Output: $CHECK25B" ;; + esac +fi + # --- 26. --check compares full permission bits, not just the exec bit --- # The manifest used to record a bare exec/file kind, so `chmod 444` on a mirrored # SKILL.md left --check at exit 0 while a real sync restored 644 — the same @@ -838,6 +883,47 @@ else fi fi +# --- 26b. The mode comparison must not depend on the runtime umask --- +# Case 26's widening from the exec bit to full permission bits is correct for the +# files this script COPIES — both sides of the comparison trace to the same checkout. +# It is wrong for the files it WRITES: sync_hooks_json creates hooks/hooks.json with +# `printf '%s\n' >`, at the RUNTIME umask, while the real side carries the umask of +# the checkout that produced the committed file. Those are independent, so on a +# umask-002 machine `--check --all` over this repo's own umask-022 checkout reported +# `< file 664 hooks/hooks.json` / `> file 644` for every plugin with hooks — a pre-push +# failure with nothing wrong, and unfixable by committing, since git records no +# non-exec mode and the next --check from a umask-022 machine fails the other way. +# +# Two directions, because a one-sided assertion passes on the wrong fix: (a) the same +# tree checked under several runtime umasks, and (b) a tree whose GENERATED files carry +# a foreign umask — which is exactly what a umask-002 clone of a umask-022 commit looks +# like, git having recorded nothing to distinguish them. +echo "" +echo "--- --check is umask-independent over the files this script generates ---" +FIXTURE26B="$(umask 022; make_fixture)"; track "$FIXTURE26B" +(umask 022; bash "$SCRIPT" "$FIXTURE26B" > /dev/null 2>&1) +if [[ ! -f "$FIXTURE26B/hooks/hooks.json" ]]; then + fail "sync did not create hooks/hooks.json — cannot test the umask-independence case" +else + for UMASK26B in 022 002 077; do + if (umask "$UMASK26B"; bash "$SCRIPT" --check "$FIXTURE26B" > /dev/null 2>&1); then + pass "--check at umask $UMASK26B is clean on a tree synced at umask 022" + else + fail "--check at umask $UMASK26B reported drift on a tree synced at umask 022 — the gate is reporting the runner's umask, not the mirror" + fi + done + # What a umask-002 clone of the same commit looks like on disk. + chmod 664 "$FIXTURE26B/hooks/hooks.json" + [[ -f "$FIXTURE26B/.claude-plugin/plugin.json" ]] && chmod 664 "$FIXTURE26B/.claude-plugin/plugin.json" + for UMASK26B in 022 002; do + if (umask "$UMASK26B"; bash "$SCRIPT" --check "$FIXTURE26B" > /dev/null 2>&1); then + pass "--check at umask $UMASK26B is clean when the generated files carry a umask-002 checkout's mode" + else + fail "--check at umask $UMASK26B reported drift on generated files carrying a umask-002 checkout's mode — no commit can fix that" + fi + done +fi + # --- 27. A plugin-dir argument whose basename is . or .. is rejected --- # Every scratch path is "$SCRATCH_ROOT/$(basename "$plugin_dir")", so `..` resolves # to the scratch root's PARENT: `apm pack -o` then writes outside the tree the EXIT @@ -963,9 +1049,46 @@ check_all_fails_with "unparseable JSON" \ check_all_fails_with "a plugins field that is not an array" \ '{"plugins":{"a":1}}' \ "expected an array of plugin entries" +check_all_fails_with "a JSON array at the marketplace root" \ + '[]' \ + "is a JSON array at its top level" check_all_fails_with "an entry with no source field" \ '{"plugins":[{"name":"orphan"}]}' \ - "entries with no \`source\` field" + "\`source\` is neither a local path string nor a remote source object" +# The guard used to name `.source == null` specifically, so every other malformed +# value walked straight through it into the same silence. +check_all_fails_with "an entry whose source is a number" \ + '{"plugins":[{"name":"orphan","source":42}]}' \ + "\`source\` is neither a local path string nor a remote source object" +check_all_fails_with "an entry whose source is an array" \ + '{"plugins":[{"name":"orphan","source":[]}]}' \ + "\`source\` is neither a local path string nor a remote source object" + +# --- 28b. --all outside a git worktree refuses instead of guessing $PWD --- +# --all's entire work list hangs off REPO_ROOT, so a `|| pwd` fallback lets it derive +# that list from a marketplace.json belonging to some other tree. Same reasoning +# scripts/sync-marketplace-mirror.sh dropped its own fallback on. Run from a directory +# with no marketplace.json the old form happened to hit the "--all requires ..." error, +# but only by accident — the dangerous case is a $PWD that HAS one. +echo "" +echo "--- --all outside a git worktree refuses to guess the repository root ---" +NOGIT="$(mktemp -d)"; track "$NOGIT" +mkdir -p "$NOGIT/.claude-plugin" +printf '%s' '{"plugins":[{"name":"decoy","source":"./plugins/decoy"}]}' > "$NOGIT/.claude-plugin/marketplace.json" +if (cd "$NOGIT" && env -u GIT_DIR -u GIT_WORK_TREE git rev-parse --show-toplevel) >/dev/null 2>&1; then + fail "fixture precondition: $NOGIT is inside a git worktree, so this case cannot test the no-worktree path" +else + RC28B=0 + OUT28B="$(cd "$NOGIT" && env -u GIT_DIR -u GIT_WORK_TREE bash "$SCRIPT" --check --all 2>&1)" || RC28B=$? + case "$RC28B:$OUT28B" in + 0:*) + fail "--check --all exited 0 outside a worktree, having derived its plugin list from \$PWD" ;; + *"not inside a git worktree"*) + pass "--all refuses to guess \$PWD when it cannot locate the repository root" ;; + *) + fail "--check --all exited $RC28B outside a worktree but not for the stated reason: $OUT28B" ;; + esac +fi echo "" echo "Results: $PASS passed, $FAIL failed"