fix(scripts): make the mirror's mode check umask-independent

The previous round widened path_manifest from the exec bit to full permission
bits, and that made check-plugin-content-sync fail at pre-push on a pristine
tree. hooks/hooks.json is not copied from the bundle -- sync_hooks_json writes
it with printf, i.e. at the runtime umask -- while the real side comes from the
checkout. On a umask-002 clone the two disagree, 664 vs 644, and no commit can
reconcile them because git tracks no non-exec mode.

The rule adopted: record a mode for a path this pipeline copies, never for one
it writes. A copied path's mode traces to the same checkout on both sides, so
comparing it means something; a written path's mode is the writer's umask on
one side and the checkout's on the other, which are independent. That is the
same rationale the directory exclusion already carried -- what broke was the
premise that files are immune. Normalising instead was rejected: pinning the
generated side cannot fix a checked-out side that is already 664.

The unconditional chmod 644 in reinject_mcp_servers goes for the same reason;
writing through the destination inode already closed the original 0600 bug.

The mode coverage added for the two plugin.json manifests is removed rather
than documented, because it measured nothing on any axis. In check mode the
expected side is a cp -a of the real plugin root, so apm rewrites an existing
inode and inherits its mode; and a symlinked manifest is copied as a symlink
and written straight through, so both sides agreed no matter what. That
symlink case is a real hazard -- the re-injection corrupts the link's target --
so it is now asserted directly instead.

Also: an unparseable or non-object per-plugin plugin.json killed the manifest
walk mid-loop; the source-less-entry guard closed only source: null and let
every other malformed value through; the select it backstops was extracted so
a test can exercise it independently, which nothing could before; and two more
`|| pwd` fallbacks now hard-error -- with a decoy marketplace.json in $PWD,
--all derived its plugin list from it.

Tests: 63 -> 77 and 23 -> 31 assertions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT

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)"

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 // "<unnamed>"] | 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 // "<unnamed>") (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 <marketplace_json_path>
+#
+# 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"
+}

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 "<kind> <octal-mode> <relative-path>" 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