fix(kyberforge): harden plugin-content sync, reinject Copilot mcpServers

PR #95's review of the issue #90 apm-conversion work found several defects in scripts/sync-plugin-content.sh and the gate wired to it: - --check claimed never to mutate the plugin root, but apm pack still wrote .claude-plugin/plugin.json and .github/plugin/plugin.json into the real plugin_dir on first-time creation. --check now packs a throwaway copy instead. - check-plugin-content-sync hardcoded the six plugin directories instead of deriving them the way check-manifests.sh already does. Added an --all flag that parses .claude-plugin/marketplace.json, and simplified the pre-commit hook to use it. - A missing plugin_dir and one that legitimately has no .apm/ yet both reported SKIP/success; a missing directory now FAILs. - The dispatch loop backgrounded every plugin with no concurrency cap, unlike the JOBS-bounded pattern this same PR added to tests/run-bats.sh and tests/run-tests.sh. Added the same bash-3.2-safe getconf + batched-wait cap here for consistency. - Per-plugin scratch/log/status files were keyed only by basename, with no collision guard across arguments; added a fail-fast check. - sync_hooks_json()'s trailing-newline normalization was duplicated between its --check and write branches; factored into one helper. - tests/test-sync-plugin-content.sh set two competing `trap ... EXIT` statements, so the first (cleaning up $FIXTURE) was silently replaced by the second and its tmp dir leaked every run. Adopted the track()/CLEANUP_DIRS pattern already used in tests/test-check-release-needed.sh. Separately: apm's Copilot-ecosystem plugin.json builder unconditionally strips mcpServers, citing (in its own docstring) that the field is out of schema for Copilot -- a claim this repo's own researched Copilot plugin schema docs contradict. reinject_mcp_servers() narrowly restores it from the plugin's .mcp.json on real syncs only, regenerating plugins/bin/.github/plugin/plugin.json (the only plugin that currently declares any MCP servers). Documented as an amendment to ADR-0017, since it's a deliberate, narrow exception to that ADR's rejection of patching apm's compiled output -- apm's premise for stripping skills/agents/commands/hooks pointers is still accurate; its premise for stripping mcpServers is not. All 12 assertions in tests/test-sync-plugin-content.sh pass individually, plus 5 new regression tests added for this round; the full bats and shell-script suites are green; shellcheck is clean. Refs: #95 ADR: 0017
2026-08-13 19:47:36 +00:00
parent bff9662c52
commit 9c140efa2e
5 changed files with 283 additions and 21 deletions
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -65,7 +65,7 @@ repos:
      - id: check-plugin-content-sync
        name: Check plugin content sync
        description: Verify each plugin's flat skills/agents/commands/hooks.json mirror is in sync with .apm/ -- Claude Code has no .apm/ awareness so this compiled mirror must stay current (see issue #90)
-        entry: bash scripts/sync-plugin-content.sh --check plugins/bin plugins/core plugins/git plugins/gitea plugins/kyberforge plugins/lint
+        entry: bash scripts/sync-plugin-content.sh --check --all
        language: system
        stages: [pre-push]
        pass_filenames: false
--- a/docs/adr/0017-plugin-content-mirror-bridges-apm-to-host-discovery.md
+++ b/docs/adr/0017-plugin-content-mirror-bridges-apm-to-host-discovery.md
@@ -107,6 +107,37 @@ source and scans directories; it does not execute a package manager's build comm
 Copying the relevant subset back to the stable `plugins/<name>/` path — where `marketplace.json`
 already points — needed no change to the marketplace source model at all.

+## Amendment (2026-08-13): `mcpServers` is narrowly reinjected into Copilot's `plugin.json`
+
+PR #95's review (a follow-on to this same issue #90 workstream) found a second field apm's
+compiler strips for the Copilot ecosystem: `build_plugin_manifest` unconditionally removes
+`mcpServers` from every Copilot-ecosystem `plugin.json`, its docstring stating the field is "not
+part of the Copilot plugin manifest schema." That claim is contradicted by this repo's own
+researched documentation — `plugins/kyberforge/docs/research/docs/github-copilot-plugins/
+configuration.md:49` documents `mcpServers` as a valid, optional `plugin.json` field for Copilot.
+
+This is not the same situation "Considered options" above rejected. That rejection concerned
+fields apm strips *correctly*, on a stable and accurate premise: convention directories
+(`skills/`, `agents/`, `commands/`) are host-auto-discovered, so an explicit pointer is redundant
+by design. Here, apm's own stated justification for stripping `mcpServers` is factually wrong
+against documented Copilot behavior — there is no host-auto-discovery mechanism that makes an
+explicit `mcpServers` declaration redundant, the way there is for skills/agents/commands. Applying
+the same "don't fight a stable, intentional apm code path" reasoning here would mean shipping a
+plugin manifest known to be missing a field Copilot actually reads.
+
+Given that, `scripts/sync-plugin-content.sh`'s `reinject_mcp_servers()` (line 190, called from
+`sync_one()` at line 269, real syncs only) narrowly re-injects `mcpServers` into
+`.github/plugin/plugin.json` after a real sync, sourced from the plugin's own `.mcp.json`, and
+only when it declares at least one server — matching apm's own Claude-ecosystem builder, which
+omits the field entirely rather than emitting `mcpServers: {}`. This is scoped to one field found
+to be incorrectly stripped, not a reversal of the broader position above: the rejection of
+patching `skills`/`agents`/`commands`/`hooks` pointers still holds, since apm's premise for
+stripping those remains accurate.
+
+Consequence: if a future apm release corrects the Copilot `mcpServers` omission, `reinject_mcp_servers()`
+and its call site become dead code and should be deleted — nothing else in this ADR depends on the
+reinjection existing beyond working around this specific upstream gap.
+
 ## Consequences

 - Git now tracks real, visible duplication: `.apm/skills/<name>/SKILL.md` and
--- a/plugins/bin/.github/plugin/plugin.json
+++ b/plugins/bin/.github/plugin/plugin.json
@@ -16,5 +16,15 @@
    "prototyping",
    "tdd",
    "research"
-  ]
+  ],
+  "mcpServers": {
+    "obsidian": {
+      "args": [
+        "@bitbonsai/mcpvault@latest",
+        "docs/"
+      ],
+      "command": "npx",
+      "type": "stdio"
+    }
+  }
 }
--- a/scripts/sync-plugin-content.sh
+++ b/scripts/sync-plugin-content.sh
@@ -22,8 +22,20 @@ set -euo pipefail
 # refreshes both files from current apm.yml/.apm/ content -- apm pack silently skips
 # regenerating an existing plugin.json otherwise ("already exists; skipping plugin.json
 # generation"), which would let them go stale after a name/version/description edit.
-# --check does NOT pass --force (it must not mutate the plugin root), so plugin.json
-# staleness is not currently detected by --check -- only fixed by the next real sync.
+#
+# apm's Copilot-ecosystem plugin.json builder omits mcpServers entirely -- its own
+# docstring calls it out-of-schema for Copilot, but this repo's researched Copilot
+# plugin schema docs (plugins/kyberforge/docs/research/docs/github-copilot-plugins/
+# configuration.md) document mcpServers as valid there. Real-mode syncs re-inject it
+# into .github/plugin/plugin.json from the plugin's own .mcp.json after apm pack runs
+# (see reinject_mcp_servers below); staleness there, like the rest of plugin.json, is
+# only fixed by the next real sync, not detected by --check.
+#
+# apm pack also writes .claude-plugin/plugin.json and .github/plugin/plugin.json into
+# cwd whenever those files don't already exist yet -- regardless of --force -- so
+# --check (which must never mutate the real plugin root) never cds into plugin_dir
+# directly. It packs a throwaway copy instead (see sync_one's pack_cwd); only that
+# copy's manifest files, never the real ones, can get created as a first-write.
 #
 # hooks.json is only synced when .apm/hooks/ actually produces one -- a plugin with
 # no .apm/hooks/ content is left alone even if a root-level hooks.json already exists
@@ -37,7 +49,7 @@ set -euo pipefail
 # the duplicate and double-running the original under any repo-wide bats/test discovery.

 usage() {
-  echo "Usage: $0 [--check] <plugin-dir> [<plugin-dir> ...]" >&2
+  echo "Usage: $0 [--check] (--all | <plugin-dir> [<plugin-dir> ...])" >&2
  exit 1
 }

@@ -46,13 +58,29 @@ if [[ "${1:-}" == "--check" ]]; then
  CHECK=1
  shift
 fi
-[[ $# -ge 1 ]] || usage
+
+ALL=0
+if [[ "${1:-}" == "--all" ]]; then
+  ALL=1
+  shift
+fi
+
+if [[ "$ALL" -eq 1 ]]; then
+  [[ $# -eq 0 ]] || usage
+else
+  [[ $# -ge 1 ]] || usage
+fi

 if ! command -v apm &>/dev/null; then
  echo "Error: apm is required but not installed (see kyberforge:apm-install)" >&2
  exit 1
 fi

+if ! command -v jq &>/dev/null; then
+  echo "Error: jq is required but not installed" >&2
+  exit 1
+fi
+
 # Convention subdirectories apm's plugin exporter can populate from .apm/.
 MIRROR_DIRS=(agents skills commands instructions extensions)

@@ -60,6 +88,52 @@ FAIL=0
 SCRATCH_ROOT="$(mktemp -d)"
 trap 'rm -rf "$SCRATCH_ROOT"' EXIT

+if [[ "$ALL" -eq 1 ]]; then
+  # Derives the plugin list from marketplace.json the same way
+  # scripts/check-manifests.sh does, instead of hand-maintaining a duplicate list
+  # 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)"
+  MARKETPLACE="$REPO_ROOT/.claude-plugin/marketplace.json"
+  if [[ ! -f "$MARKETPLACE" ]]; then
+    echo "Error: --all requires $MARKETPLACE" >&2
+    exit 1
+  fi
+  declare -a plugin_dirs=()
+  plugin_count="$(jq '.plugins | length' "$MARKETPLACE")"
+  for ((i = 0; i < plugin_count; i++)); do
+    source_type="$(jq -r ".plugins[$i].source | type" "$MARKETPLACE")"
+    # Remote sources (github, git, npm objects) have no local directory to sync.
+    [[ "$source_type" == "string" ]] || continue
+    source="$(jq -r ".plugins[$i].source" "$MARKETPLACE")"
+    source="${source#./}"
+    plugin_dirs+=("$REPO_ROOT/$source")
+  done
+else
+  declare -a plugin_dirs=("$@")
+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.
+declare -a seen_names=()
+for plugin_dir in "${plugin_dirs[@]}"; do
+  name="$(basename "${plugin_dir%/}")"
+  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
+      exit 1
+    fi
+  done
+  seen_names+=("$name")
+done
+
+normalize_trailing_newline() {
+  # apm's bundle exporter writes hooks.json without a trailing newline, which
+  # end-of-file-fixer (pre-commit) would flag on every regeneration -- normalize
+  # instead of fighting that hook on every sync.
+  printf '%s\n' "$(cat "$1")" >"$2"
+}
+
 sync_dir() {
  local plugin_dir="$1" bundle_dir="$2" d="$3"
  local src="$bundle_dir/$d" dst="$plugin_dir/$d"
@@ -98,14 +172,10 @@ sync_hooks_json() {
  # No .apm/hooks/ content -- hooks.json (if any) is out of scope for this script.
  [[ -f "$src" ]] || return 0

-  # apm's bundle exporter writes hooks.json without a trailing newline, which
-  # end-of-file-fixer (pre-commit) would flag on every regeneration -- normalize both
-  # sides of the comparison (and the real write) to exactly one trailing newline
-  # instead of fighting that hook on every sync.
  if [[ "$CHECK" -eq 1 ]]; then
    local normalized_src
    normalized_src="$(mktemp)"
-    printf '%s\n' "$(cat "$src")" >"$normalized_src"
+    normalize_trailing_newline "$src" "$normalized_src"
    if [[ ! -f "$dst" ]] || ! diff -q "$normalized_src" "$dst" >/dev/null 2>&1; then
      echo "DRIFT $dst: out of sync with .apm/hooks/" >&2
      FAIL=1
@@ -114,7 +184,25 @@ sync_hooks_json() {
    return 0
  fi

-  printf '%s\n' "$(cat "$src")" >"$dst"
+  normalize_trailing_newline "$src" "$dst"
+}
+
+reinject_mcp_servers() {
+  local plugin_dir="$1"
+  local mcp_src="$plugin_dir/.mcp.json" dst="$plugin_dir/.github/plugin/plugin.json"
+  [[ -f "$mcp_src" ]] || return 0
+  [[ -f "$dst" ]] || return 0
+
+  # 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.
+  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"
 }

 # Runs entirely inside a backgrounded subshell (see the dispatch loop below), so
@@ -125,22 +213,36 @@ sync_one() {
  local apm_dir="$plugin_dir/.apm"
  FAIL=0

+  if [[ ! -d "$plugin_dir" ]]; then
+    echo "FAIL $plugin_dir: plugin directory does not exist" >&2
+    FAIL=1
+    echo "$FAIL" >"$status_file"
+    return 0
+  fi
+
  if [[ ! -d "$apm_dir" ]]; then
    echo "SKIP $plugin_dir: no .apm/ directory" >&2
    echo "$FAIL" >"$status_file"
    return 0
  fi

-  local name scratch bundle_dir pack_log
+  local name scratch bundle_dir pack_log pack_cwd
  name="$(basename "$plugin_dir")"
  scratch="$SCRATCH_ROOT/$name"
  mkdir -p "$scratch"
  pack_log="$(mktemp)"

  local force_flag=()
-  [[ "$CHECK" -eq 0 ]] && force_flag=(--force)
+  if [[ "$CHECK" -eq 0 ]]; then
+    force_flag=(--force)
+    pack_cwd="$plugin_dir"
+  else
+    pack_cwd="$SCRATCH_ROOT/$name.checkcopy"
+    mkdir -p "$pack_cwd"
+    cp -a "$plugin_dir/." "$pack_cwd/"
+  fi

-  if ! (cd "$plugin_dir" && apm pack --format plugin "${force_flag[@]}" -o "$scratch") >"$pack_log" 2>&1; then
+  if ! (cd "$pack_cwd" && apm pack --format plugin "${force_flag[@]}" -o "$scratch") >"$pack_log" 2>&1; then
    echo "FAIL $plugin_dir: apm pack failed:" >&2
    sed 's/^/    /' "$pack_log" >&2
    rm -f "$pack_log"
@@ -163,6 +265,9 @@ sync_one() {
    sync_dir "$plugin_dir" "$bundle_dir" "$d"
  done
  sync_hooks_json "$plugin_dir" "$bundle_dir"
+  if [[ "$CHECK" -eq 0 ]]; then
+    reinject_mcp_servers "$plugin_dir"
+  fi
  echo "$FAIL" >"$status_file"
 }

@@ -171,10 +276,22 @@ sync_one() {
 # than paying that startup cost N times serially. Output is buffered per plugin
 # (not streamed) so concurrent DRIFT/FAIL messages from different plugins never
 # interleave; it's flushed in stable $@ order once every job has finished.
-declare -a plugin_dirs=("$@")
+#
+# Batched (not a rolling pool) because a bounded rolling pool needs `wait -n`,
+# which is bash 4.3+ -- tests/run-tests.sh and tests/run-bats.sh in this same repo
+# are explicitly bash-3.2-safe, so this script matches their pattern for
+# consistency. `getconf` over `nproc` for the same reason: `nproc` doesn't exist
+# on macOS.
+JOBS_LIMIT="$(getconf _NPROCESSORS_ONLN 2>/dev/null || echo 4)"
+running=0
 for plugin_dir in "${plugin_dirs[@]}"; do
  name="$(basename "${plugin_dir%/}")"
  (sync_one "$plugin_dir" "$SCRATCH_ROOT/$name.status") >"$SCRATCH_ROOT/$name.log" 2>&1 &
+  running=$((running + 1))
+  if [[ $running -ge $JOBS_LIMIT ]]; then
+    wait
+    running=0
+  fi
 done
 wait

@@ -187,7 +304,11 @@ done

 if [[ "$FAIL" -ne 0 ]]; then
  if [[ "$CHECK" -eq 1 ]]; then
-    echo "Plugin content mirror is out of sync with .apm/. Fix: bash scripts/sync-plugin-content.sh $*" >&2
+    if [[ "$ALL" -eq 1 ]]; then
+      echo "Plugin content mirror is out of sync with .apm/. Fix: bash scripts/sync-plugin-content.sh --all" >&2
+    else
+      echo "Plugin content mirror is out of sync with .apm/. Fix: bash scripts/sync-plugin-content.sh $*" >&2
+    fi
  fi
  exit 1
 fi
--- a/tests/test-sync-plugin-content.sh
+++ b/tests/test-sync-plugin-content.sh
@@ -61,11 +61,56 @@ EOF
  echo "$dir"
 }

+# Same base fixture, but with a copilot target (so apm pack produces
+# .github/plugin/plugin.json) and a caller-supplied .mcp.json -- for exercising
+# reinject_mcp_servers().
+make_fixture_with_mcp() {
+  local mcp_json="$1" dir
+  dir="$(mktemp -d)"
+  mkdir -p "$dir/.apm/skills/hello" "$dir/.apm/agents"
+  cat > "$dir/apm.yml" <<'YAML'
+name: fixture
+version: 0.0.1
+description: fixture
+license: MIT
+type: hybrid
+targets:
+  - claude
+  - copilot
+dependencies:
+  apm: []
+  mcp: []
+includes: auto
+devDependencies:
+  apm: []
+scripts: {}
+YAML
+  cat > "$dir/.apm/skills/hello/SKILL.md" <<'EOF'
+---
+name: hello
+description: hello
+---
+Hello.
+EOF
+  cat > "$dir/.apm/agents/foo.agent.md" <<'EOF'
+---
+name: foo
+description: foo
+---
+Foo.
+EOF
+  printf '%s' "$mcp_json" > "$dir/.mcp.json"
+  echo "$dir"
+}
+
+CLEANUP_DIRS=()
+trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT
+track() { CLEANUP_DIRS+=("$1"); }
+
 # --- 1. --check reports drift before any sync has run ---
 echo ""
 echo "--- --check reports drift on an unsynced fixture ---"
-FIXTURE="$(make_fixture)"
-trap 'rm -rf "$FIXTURE"' EXIT
+FIXTURE="$(make_fixture)"; track "$FIXTURE"
 if bash "$SCRIPT" --check "$FIXTURE" > /dev/null 2>&1; then
  fail "exited 0 on an unsynced fixture — expected drift (exit 1)"
 else
@@ -128,14 +173,69 @@ fi
 # --- 6. A plugin dir with no .apm/ is skipped cleanly, not treated as an error ---
 echo ""
 echo "--- a plugin dir with no .apm/ is skipped, not failed ---"
-NO_APM="$(mktemp -d)"
-trap 'rm -rf "$NO_APM"' EXIT
+NO_APM="$(mktemp -d)"; track "$NO_APM"
 if bash "$SCRIPT" "$NO_APM" > /dev/null 2>&1 && bash "$SCRIPT" --check "$NO_APM" > /dev/null 2>&1; then
  pass "a plugin dir with no .apm/ exits 0 in both real and --check mode"
 else
  fail "a plugin dir with no .apm/ should exit 0 (skip), not fail"
 fi

+# --- 7. A plugin dir that doesn't exist at all is a hard failure, not a skip ---
+echo ""
+echo "--- a plugin dir that does not exist fails, distinct from an existing-but-empty one ---"
+MISSING_ROOT="$(mktemp -d)"; track "$MISSING_ROOT"
+MISSING="$MISSING_ROOT/does-not-exist"
+if bash "$SCRIPT" "$MISSING" > /dev/null 2>&1; then
+  fail "exited 0 for a plugin dir that does not exist — expected a hard failure"
+else
+  pass "a nonexistent plugin dir fails instead of silently skipping"
+fi
+
+# --- 8. --check never mutates the real plugin root, even on first-time manifest creation ---
+echo ""
+echo "--- --check does not create .claude-plugin/plugin.json or .github/plugin/plugin.json ---"
+FIXTURE8="$(make_fixture)"; track "$FIXTURE8"
+bash "$SCRIPT" --check "$FIXTURE8" > /dev/null 2>&1 || true
+if [[ ! -e "$FIXTURE8/.claude-plugin/plugin.json" ]] && [[ ! -e "$FIXTURE8/.github/plugin/plugin.json" ]]; then
+  pass "--check leaves the real plugin root without a first-write plugin.json"
+else
+  fail "--check created plugin.json in the real plugin root — it must never mutate it"
+fi
+
+# --- 9. A duplicate plugin-dir basename among arguments fails fast, not silently ---
+echo ""
+echo "--- duplicate plugin dir basenames among arguments are rejected ---"
+DUP_PARENT_A="$(mktemp -d)"; track "$DUP_PARENT_A"
+DUP_PARENT_B="$(mktemp -d)"; track "$DUP_PARENT_B"
+mkdir -p "$DUP_PARENT_A/dup" "$DUP_PARENT_B/dup"
+if bash "$SCRIPT" "$DUP_PARENT_A/dup" "$DUP_PARENT_B/dup" > /dev/null 2>&1; then
+  fail "exited 0 with two plugin-dir arguments sharing a basename — expected a collision error"
+else
+  pass "rejects two plugin-dir arguments that share a basename"
+fi
+
+# --- 10. Real sync re-injects mcpServers that apm's Copilot builder strips ---
+echo ""
+echo "--- real sync re-injects mcpServers into .github/plugin/plugin.json ---"
+FIXTURE10="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE10"
+bash "$SCRIPT" "$FIXTURE10" > /dev/null 2>&1
+if jq -e '.mcpServers.demo.command == "demo-server"' "$FIXTURE10/.github/plugin/plugin.json" > /dev/null 2>&1; then
+  pass "mcpServers from .mcp.json is present in .github/plugin/plugin.json after a real sync"
+else
+  fail "mcpServers was not re-injected into .github/plugin/plugin.json"
+fi
+
+# --- 11. An empty .mcp.json does not add a redundant mcpServers: {} ---
+echo ""
+echo "--- an empty .mcp.json does not add mcpServers: {} ---"
+FIXTURE11="$(make_fixture_with_mcp '{"mcpServers":{}}')"; track "$FIXTURE11"
+bash "$SCRIPT" "$FIXTURE11" > /dev/null 2>&1
+if jq -e 'has("mcpServers") | not' "$FIXTURE11/.github/plugin/plugin.json" > /dev/null 2>&1; then
+  pass "an empty .mcp.json does not add mcpServers to .github/plugin/plugin.json"
+else
+  fail "an empty .mcp.json still added mcpServers -- should match apm's own omit-when-empty convention"
+fi
+
 echo ""
 echo "Results: $PASS passed, $FAIL failed"
 [[ $FAIL -eq 0 ]]