fix(scripts): re-inject .mcp.json as a pointer, not resolved content

reinject_mcp_servers copied .mcp.json's mcpServers into the compiled Copilot manifest verbatim via jq. apm's own path does not: collect_mcp_servers runs _sanitize_mcp_servers(), which drops env/headers/authorization and redacts secret-shaped keys, because copying them into a committed manifest exfiltrates them into the distributed artefact. The re-injection was the only route around that sanitizer, and it wrote to a tracked, marketplace-distributed file. Both host schemas document mcpServers as "string or object -- config path or inline definitions", so the pointer form is valid and carries no resolved content. It also preserves the ${VAR} indirection the sanitizer strips. Also in this pass: - mktemp+mv left the manifest at 0600 while --check compared content only, so a real sync silently demoted a mode the gate could not see - --check --all exited 0 when the marketplace yielded zero plugins, including on unparseable JSON: the one gate whose work list comes from a generated file could be silenced by regenerating its own input - sync_dir took an unguarded $target_dir despite a comment claiming otherwise - basename '.'/'..' escaped $SCRATCH_ROOT and made bundle selection arbitrary - path_manifest compared only the exec bit, so check and sync disagreed - sync-marketplace-mirror.sh fell back to pwd outside a worktree and reported no drift on a tree it never identified Mode comparison is deliberately files-only: directory modes come from umask on one side and checkout on the other and git tracks neither, so comparing them reports the runner's umask rather than a property of the mirror. Tests: 44 -> 67 and 15 -> 19 assertions, each verified to fail under the mutation it exists to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
2026-08-14 11:03:45 +00:00
parent 0f0ac5821f
commit 9e612fd183
4 changed files with 551 additions and 49 deletions
--- a/scripts/sync-marketplace-mirror.sh
+++ b/scripts/sync-marketplace-mirror.sh
@@ -12,7 +12,18 @@ set -euo pipefail
 # script keeps that legacy mirror byte-identical to .claude-plugin/marketplace.json
 # instead of letting it silently drift (see issue #90 comment thread).

-REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
+# Hard error, not a `|| pwd` fallback. Every path this script touches hangs off
+# REPO_ROOT, and both of its exits-0 paths are "the files agree" or "neither file
+# exists" -- so a REPO_ROOT pointing somewhere that is not this repo reports "no
+# drift" over a tree it never looked at. Run `--check` from an empty directory
+# outside any worktree and the fallback made that the literal outcome: rev-parse
+# failed, REPO_ROOT became $PWD, neither file was there, exit 0. Refusing to guess
+# is the only answer that cannot be silently wrong; the `-f "$DST"` branch below
+# covers a genuinely stale mirror, which is a different condition.
+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 --check report \"no drift\" over a tree it never inspected. Run this from within the repository." >&2
+  exit 1
+fi
 SRC="$REPO_ROOT/.claude-plugin/marketplace.json"
 DST="$REPO_ROOT/.github/plugin/marketplace.json"

@@ -32,9 +43,9 @@ if [[ ! -f "$SRC" ]]; then
  # A missing source with a surviving mirror is drift, not absence: the mirror
  # can only be stale (nothing is left for it to be byte-identical to), which is
  # precisely the silent divergence this script exists to prevent. Exiting 0
-  # here would report "no drift" over a mirror of a file that no longer exists,
-  # and would also swallow the case where REPO_ROOT resolved to the wrong tree —
-  # `git rev-parse --show-toplevel` falls back to `pwd` outside a worktree.
+  # here would report "no drift" over a mirror of a file that no longer exists.
+  # (An unresolvable REPO_ROOT is handled above and is a hard error; this branch
+  # is only about a source file that is genuinely gone from a real worktree.)
  # scripts/sync-plugin-content.sh --check --all already errors on the same
  # condition ("requires .../marketplace.json"); this matches it.
  # Neither file present stays a genuine no-op: nothing to mirror, nothing stale.
--- a/scripts/sync-plugin-content.sh
+++ b/scripts/sync-plugin-content.sh
@@ -142,6 +142,23 @@ FAIL=0
 SCRATCH_ROOT="$(mktemp -d)"
 trap 'rm -rf "$SCRATCH_ROOT"' EXIT

+# The manifest comparison in check_path_modes needs octal permission bits, and
+# GNU coreutils and BSD/macOS stat disagree on both the flag and the format
+# specifier. Probe once at startup against a path known to exist rather than
+# branching on `uname` (which says nothing about which coreutils is installed --
+# GNU stat is perfectly common on macOS via Homebrew).
+declare -a STAT_MODE_ARGS=()
+if [[ "$(stat -c '%a' "$SCRIPT_DIR" 2>/dev/null)" =~ ^[0-7]+$ ]]; then
+  STAT_MODE_ARGS=(-c '%a')
+elif [[ "$(stat -f '%Lp' "$SCRIPT_DIR" 2>/dev/null)" =~ ^[0-7]+$ ]]; then
+  STAT_MODE_ARGS=(-f '%Lp')
+else
+  # Hard error rather than degrading to a no-mode manifest: silently checking
+  # less than advertised is the exact failure mode this gate exists to prevent.
+  echo "Error: cannot read octal file modes -- neither \`stat -c '%a'\` (GNU coreutils) nor \`stat -f '%Lp'\` (BSD/macOS) works here" >&2
+  exit 1
+fi
+
 if [[ "$ALL" -eq 1 ]]; then
  # Derives the plugin list from marketplace.json via the shared
  # list_marketplace_local_plugins helper (scripts/lib/marketplace-plugins.sh),
@@ -154,10 +171,25 @@ if [[ "$ALL" -eq 1 ]]; then
    echo "Error: --all requires $MARKETPLACE" >&2
    exit 1
  fi
+  # Called from this shell, NOT from inside the `< <(...)` below -- that process
+  # substitution is its own subshell, so a `set -e` abort or a jq parse failure in
+  # there kills only the subshell and the `while read` loop simply gets no input.
+  # An unparseable marketplace.json then reads exactly like "declares no plugins",
+  # plugin_dirs comes back empty, batch_run dispatches nothing, and --check --all
+  # exits 0 having verified nothing at all (see the floor below).
+  assert_marketplace_manifest_usable "$MARKETPLACE"
  declare -a plugin_dirs=()
  while IFS=$'\t' read -r _name plugin_dir; do
    plugin_dirs+=("$plugin_dir")
  done < <(list_marketplace_local_plugins "$REPO_ROOT" "$MARKETPLACE")
+  # Floor: --all is a gate whose work list comes from a GENERATED file, so an
+  # empty derived set is drift, not a pass -- regenerating marketplace.json badly
+  # would otherwise silence the very hook that guards it. assert_... above rejects
+  # the malformed shapes; this rejects the well-formed-but-empty one.
+  if [[ ${#plugin_dirs[@]} -eq 0 ]]; then
+    echo "Error: $MARKETPLACE declares no local (string-source) plugin entries -- --all would check nothing and report success. Expected at least one; recompile it with \`apm pack\` if it is stale." >&2
+    exit 1
+  fi
 else
  declare -a plugin_dirs=("$@")
 fi
@@ -165,9 +197,24 @@ fi
 # Fail fast on a basename collision rather than letting two plugin_dir arguments
 # silently share (and corrupt) the same $name.log/$name.status/$name.checkcopy
 # scratch paths below.
+#
+# The same loop rejects `.` and `..`, because every scratch path here is built by
+# pasting this basename onto $SCRATCH_ROOT. `basename ..` is `..`, so
+# "$SCRATCH_ROOT/$name" resolves to $SCRATCH_ROOT's PARENT -- apm pack then writes
+# its bundle into a directory this script neither owns nor cleans up (the EXIT
+# trap only removes $SCRATCH_ROOT itself), and sync_one's
+# `find "$scratch" -mindepth 1 -maxdepth 1 -type d | head -1` picks whatever
+# unrelated directory readdir happens to hand back first as the "bundle" -- whose
+# agents/ and skills/ a real sync then cp -a's into the plugin root, after an
+# rm -rf. The collision check below cannot catch this: a single `..` argument
+# collides with nothing.
 declare -a seen_names=()
 for plugin_dir in ${plugin_dirs[@]+"${plugin_dirs[@]}"}; do
  name="$(basename "${plugin_dir%/}")"
+  if [[ "$name" == "." || "$name" == ".." ]]; then
+    echo "Error: plugin dir '$plugin_dir' has basename '$name' -- scratch paths built from it would escape the scratch root. Pass the plugin directory by name, not by a relative traversal." >&2
+    exit 1
+  fi
  for seen in ${seen_names[@]+"${seen_names[@]}"}; do
    if [[ "$seen" == "$name" ]]; then
      echo "Error: duplicate plugin basename '$name' among arguments -- scratch paths would collide" >&2
@@ -190,7 +237,13 @@ normalize_trailing_newline() {
 # that rule anywhere in this script.
 sync_dir() {
  local target_dir="$1" bundle_dir="$2" d="$3"
-  local src="$bundle_dir/$d" dst="$target_dir/$d"
+  # ${target_dir:?} for the same reason sync_hooks_json spells it out: `set -u`
+  # aborts on an UNSET variable but not an empty one, and an empty $target_dir
+  # would make the rm -rf calls below `rm -rf /agents`, `/skills`, `/commands`,
+  # `/instructions`, `/extensions`. Unreachable from today's two call sites
+  # (both pass either a `[[ -d ]]`-validated plugin_dir or a scratch path), but
+  # the guard costs nothing and the next caller added here gets it for free.
+  local src="$bundle_dir/$d" dst="${target_dir:?}/$d"

  if [[ -d "$src" ]]; then
    rm -rf "$dst"
@@ -232,8 +285,9 @@ sync_hooks_json() {
  #
  # ${target_dir:?} rather than a bare expansion: `set -u` aborts on an UNSET
  # variable but not an empty one, and an empty $target_dir would make this
-  # `rm -rf /hooks`. The other rm -rf calls here take a $dst built by their
-  # caller; this one is the only place a bare parameter is the whole prefix.
+  # `rm -rf /hooks`. sync_dir above builds its own $dst from the same
+  # caller-supplied parameter and carries the identical guard for the identical
+  # reason -- neither function is special here.
  rm -rf "${target_dir:?}/$HOOKS_DIR_REL"

  if [[ -f "$src" ]]; then
@@ -287,28 +341,53 @@ check_file() {
  fi
 }

-# Prints "<kind> <relative-path>" for every entry under the given relative paths.
-# `find` is used rather than a stat(1) call because stat's flags for mode
-# formatting are incompatible between GNU and BSD/macOS.
+# 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.
+#
+# 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.
 path_manifest() {
  local root="$1"
  shift
-  local rel f kind
+  local rel f kind mode
  for rel in "$@"; do
    if [[ ! -e "$root/$rel" ]] && [[ ! -L "$root/$rel" ]]; then
      continue
    fi
    find "$root/$rel" -print 2>/dev/null | LC_ALL=C sort | while IFS= read -r f; do
      if [[ -L "$f" ]]; then
+        # A symlink's own lstat mode is 0777 on Linux and 0755 on macOS and is
+        # not something either side controls -- the type difference is the whole
+        # signal here, so record no mode for it.
        kind="symlink"
+        mode="-"
      elif [[ -d "$f" ]]; then
        kind="dir"
-      elif [[ -x "$f" ]]; then
-        kind="exec"
+        mode="-"
      else
        kind="file"
+        mode="$(stat "${STAT_MODE_ARGS[@]}" "$f")"
      fi
-      printf '%s %s\n' "$kind" "${f#"$root"/}"
+      printf '%s %s %s\n' "$kind" "$mode" "${f#"$root"/}"
    done || true
  done
 }
@@ -317,7 +396,7 @@ path_manifest() {
 # entirely. So `chmod -x` on a mirrored script, or swapping a mirrored file for a
 # symlink to identical content, both leave --check at exit 0 while a real sync
 # silently repairs them -- check and sync disagreeing, which is the one thing this
-# gate exists to prevent. Compare an explicit type+exec-bit manifest as well.
+# gate exists to prevent. Compare an explicit type+permission manifest as well.
 #
 # The manifest is a full recursive listing, so it is also what makes an entry that
 # exists on only one side visible. That is the sole coverage the generated hooks/
@@ -342,6 +421,25 @@ check_path_modes() {
  rm -f "$expected" "$actual"
 }

+# Sets `mcpServers` on the generated Copilot manifest to the STRING ".mcp.json" --
+# the path form of the field, not the resolved server objects. Copilot's schema
+# types the field "string or object -- MCP server config path or inline
+# definitions" (plugins/kyberforge/docs/research/docs/github-copilot-plugins/
+# configuration.md), so both are valid there; only one of them is safe.
+#
+# An earlier revision inlined the objects with
+# `jq --slurpfile mcp '.mcpServers = $mcp[0].mcpServers'`. That copies .mcp.json
+# verbatim into a committed, marketplace-distributed file, bypassing apm's own
+# _sanitize_mcp_servers() (apm_cli/core/plugin_manifest.py), which drops
+# env/environment/headers/authorization and any key matching
+# token/secret/password/credential/apikey/key at any depth before writing the
+# Claude manifest. Proven with a fixture: an `env` block holding a token-shaped
+# value produced a sanitized .claude-plugin/plugin.json and a
+# .github/plugin/plugin.json carrying the live value. A path reference cannot
+# carry a secret at all -- the manifest names a file and the host resolves it at
+# load time -- and it preserves the ${VAR} indirection apm documents as the
+# posture for MCP secrets, rather than stripping it. See ADR-0017's 2026-08-14
+# amendment.
 reinject_mcp_servers() {
  local plugin_dir="$1" target_dir="$2"
  local mcp_src="$plugin_dir/.mcp.json" dst="$target_dir/.github/plugin/plugin.json"
@@ -350,14 +448,27 @@ reinject_mcp_servers() {

  # Match apm's own Claude-ecosystem plugin.json builder: mcpServers is omitted
  # entirely when the plugin declares none, not written out as an empty object.
+  # A plugin whose .mcp.json is `{"mcpServers": {}}` gets no key at all -- not a
+  # ".mcp.json" pointer at an empty file.
  local count
  count="$(jq '(.mcpServers // {}) | length' "$mcp_src")"
  [[ "$count" -gt 0 ]] || return 0

  local tmp
  tmp="$(mktemp)"
-  jq --slurpfile mcp "$mcp_src" '.mcpServers = $mcp[0].mcpServers' "$dst" >"$tmp"
-  mv "$tmp" "$dst"
+  jq '.mcpServers = ".mcp.json"' "$dst" >"$tmp"
+  # Write THROUGH the existing file rather than `mv`-ing the mktemp over it:
+  # mktemp creates 0600, and mv carries that mode onto a tracked, published
+  # 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).
+  cat "$tmp" >"$dst"
+  rm -f "$tmp"
+  chmod 644 "$dst"
 }

 # --check-only: diffs a freshly-regenerated manifest file (in the throwaway
@@ -465,7 +576,15 @@ sync_one() {
    # $HOOKS_DIR_REL, not $HOOKS_REL: the manifest comparison has to see the whole
    # generated directory (see check_path_modes), and listing it recursively already
    # covers hooks/hooks.json.
-    checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_DIR_REL" "$LEGACY_HOOKS_REL")
+    #
+    # .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")
    for d in "${MIRROR_DIRS[@]}"; do
      check_dir "$plugin_dir" "$pack_cwd" "$d"
    done