From 9c140efa2e4e80b92c47c89bae218d7571893042 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Thu, 13 Aug 2026 19:47:36 +0000 Subject: [PATCH] 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 --- .pre-commit-config.yaml | 2 +- ...nt-mirror-bridges-apm-to-host-discovery.md | 31 ++++ plugins/bin/.github/plugin/plugin.json | 12 +- scripts/sync-plugin-content.sh | 151 ++++++++++++++++-- tests/test-sync-plugin-content.sh | 108 ++++++++++++- 5 files changed, 283 insertions(+), 21 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 7a11ad2..2d75850 100644 --- 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 diff --git 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 index 2c58197..c0c0187 100644 --- 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//` 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//SKILL.md` and diff --git a/plugins/bin/.github/plugin/plugin.json b/plugins/bin/.github/plugin/plugin.json index 0d37601..1bd8eab 100644 --- 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" + } + } } diff --git a/scripts/sync-plugin-content.sh b/scripts/sync-plugin-content.sh index 0db3c9b..f54e477 100755 --- 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] [ ...]" >&2 + echo "Usage: $0 [--check] (--all | [ ...])" >&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 diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index 53cde0f..650426c 100755 --- 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 ]]