fix(kyberforge): stop the content mirror amputating a shipped template asset
The mirror's `tests/` exclusion was depth-agnostic, so it deleted `skill-author/assets/templates/tests/` — a template the skill scaffolds FROM — alongside the depth-2 dev fixtures it was meant to drop. Since ADR-0017 makes the mirror the installed content, the shipped scaffolder was broken: the mirror copy of `new-skill.sh` exited 2 on `sed: can't read .../tests/README.md`, leaving a half-written skill, while the byte-identical `.apm/` copy exited 0. `--check` was green about it. Check mode was restructured rather than patched because `diff -x` matches a basename at any depth and cannot express the depth-2 scoping the fix needs — the two modes could not be made to agree by construction. Check mode now runs the real `sync_dir` into a throwaway root and diffs with no exclusions, leaving the exclusion rule and the hooks destination each in exactly one place. Also fixed here, all previously invisible to `--check`: - Merged hooks were written to `<plugin>/hooks.json`, which Claude Code does not convention-scan, while ADR-0017 itself quoted `hooks/hooks.json` as the contract. Moved, with the legacy path cleaned up as stale. No `hooks` pointer is added to `plugin.json`, so this does not reopen the option ADR-0017 rejected. - Only the first drift per plugin was reported: `diff | sed` returns 1 under `pipefail`, and `set -e` killed the subshell before the remaining checks and before `FAIL=1`. - File-mode and symlink drift were invisible, so `--check` and a real sync disagreed; a find-based type/mode manifest now covers both. The tests pinned almost none of this — the stale-skill wipe, the check-mode stale branch, three `MIRROR_DIRS` entries and the hooks newline normalization could each be deleted with the suite still green. All are now mutation-tested. Refs: #90 ADR: 0017 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -3,7 +3,7 @@ set -euo pipefail
|
||||
|
||||
# Mirrors each plugin's .apm/{agents,skills,prompts,commands,instructions,extensions,hooks}
|
||||
# into flat plugin-root directories (agents/, skills/, commands/, instructions/,
|
||||
# extensions/, hooks.json) -- Claude Code's and GitHub Copilot's plugin loaders
|
||||
# extensions/, hooks/hooks.json) -- Claude Code's and GitHub Copilot's plugin loaders
|
||||
# convention-scan those flat paths at the plugin root; neither has any awareness of
|
||||
# apm's .apm/ nesting (confirmed via `strings` on the installed claude binary and a
|
||||
# live `claude --plugin-dir <bundle> -p ...` discoverability test -- see issue #90).
|
||||
@@ -43,9 +43,18 @@ set -euo pipefail
|
||||
# drift in name/version/description/mcpServers -- only the copy's manifest files,
|
||||
# never the real ones, can get created as a first-write.
|
||||
#
|
||||
# hooks.json is mirrored like the other MIRROR_DIRS content: synced when .apm/hooks/
|
||||
# produces one, and removed (real mode) / flagged as drift (--check) when it no
|
||||
# longer does but a root-level hooks.json is still sitting there from a prior sync.
|
||||
# The merged hooks file is mirrored like the other MIRROR_DIRS content: synced when
|
||||
# .apm/hooks/ produces one, and removed (real mode) / flagged as drift (--check) when
|
||||
# it no longer does but a mirrored copy is still sitting there from a prior sync.
|
||||
# It lands at hooks/hooks.json, not at the plugin root: Claude Code convention-scans
|
||||
# `hooks/hooks.json` "at the plugin root, not inside .claude-plugin/"
|
||||
# (plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md's "Plugin
|
||||
# Directory Layout" table; quoted verbatim in ADR-0017's own root-cause analysis), and
|
||||
# the compiled plugin.json carries no `hooks` pointer to override that -- apm's
|
||||
# build_plugin_manifest strips pointer fields unconditionally, and re-injecting one is
|
||||
# the option ADR-0017 explicitly rejected. A root-level hooks.json (this script's own
|
||||
# pre-fix output shape) is therefore scanned by nothing at all, and is deleted as stale
|
||||
# by a real sync / reported as drift by --check.
|
||||
#
|
||||
# tests/ subdirectories (e.g. .apm/skills/<name>/tests/*.bats) are excluded from the
|
||||
# mirror -- they are dev-time fixtures a plugin host never needs to discover, and several
|
||||
@@ -53,6 +62,15 @@ set -euo pipefail
|
||||
# `../../../../../../`) sized for the .apm/-nested depth. Mirroring them verbatim would
|
||||
# duplicate each file one directory level shallower than that walk-up expects, breaking
|
||||
# the duplicate and double-running the original under any repo-wide bats/test discovery.
|
||||
# The exclusion is depth-scoped to <category>/<name>/tests, because a skill may legitimately
|
||||
# ship a directory literally named tests as a template asset it scaffolds FROM
|
||||
# (skills/skill-author/assets/templates/tests) -- stripping that breaks the shipped
|
||||
# scaffolder, which sed's its way through the template tree file by file.
|
||||
#
|
||||
# --check does NOT reimplement any of the above. It runs the real sync functions against
|
||||
# a throwaway copy of the plugin root and diffs the result against the live mirror with
|
||||
# no exclusions, so the tests/ scoping and the hooks path exist in exactly one place and
|
||||
# check mode is structurally incapable of disagreeing with what a real sync produces.
|
||||
|
||||
usage() {
|
||||
echo "Usage: $0 [--check] (--all | <plugin-dir> [<plugin-dir> ...])" >&2
|
||||
@@ -96,6 +114,11 @@ source "$SCRIPT_DIR/lib/batch-run.sh"
|
||||
# Convention subdirectories apm's plugin exporter can populate from .apm/.
|
||||
MIRROR_DIRS=(agents skills commands instructions extensions)
|
||||
|
||||
# Where the merged hooks file lands (Claude Code's convention-scanned path), and
|
||||
# the pre-fix root-level path a real sync now cleans up as stale.
|
||||
HOOKS_REL="hooks/hooks.json"
|
||||
LEGACY_HOOKS_REL="hooks.json"
|
||||
|
||||
FAIL=0
|
||||
SCRATCH_ROOT="$(mktemp -d)"
|
||||
trap 'rm -rf "$SCRATCH_ROOT"' EXIT
|
||||
@@ -142,69 +165,147 @@ normalize_trailing_newline() {
|
||||
printf '%s\n' "$(cat "$1")" >"$2"
|
||||
}
|
||||
|
||||
# Real-mode mirror write. There is no --check branch here on purpose: check mode
|
||||
# calls this same function against a throwaway copy of the plugin root and diffs
|
||||
# the result (see sync_one), so the tests/ exclusion below is the only copy of
|
||||
# that rule anywhere in this script.
|
||||
sync_dir() {
|
||||
local plugin_dir="$1" bundle_dir="$2" d="$3"
|
||||
local src="$bundle_dir/$d" dst="$plugin_dir/$d"
|
||||
|
||||
if [[ "$CHECK" -eq 1 ]]; then
|
||||
if [[ -d "$src" ]]; then
|
||||
if [[ ! -d "$dst" ]]; then
|
||||
echo "DRIFT $dst: missing (would be created from .apm/)" >&2
|
||||
FAIL=1
|
||||
elif ! diff -rq -x tests "$src" "$dst" >/dev/null 2>&1; then
|
||||
echo "DRIFT $dst: out of sync with .apm/" >&2
|
||||
diff -rq -x tests "$src" "$dst" 2>&1 | sed 's/^/ /' >&2
|
||||
FAIL=1
|
||||
fi
|
||||
elif [[ -d "$dst" ]]; then
|
||||
echo "DRIFT $dst: stale, no longer produced from .apm/" >&2
|
||||
FAIL=1
|
||||
fi
|
||||
return 0
|
||||
fi
|
||||
local target_dir="$1" bundle_dir="$2" d="$3"
|
||||
local src="$bundle_dir/$d" dst="$target_dir/$d"
|
||||
|
||||
if [[ -d "$src" ]]; then
|
||||
rm -rf "$dst"
|
||||
mkdir -p "$dst"
|
||||
cp -a "$src/." "$dst/"
|
||||
find "$dst" -type d -name tests -exec rm -rf {} +
|
||||
# Depth-scoped to <category>/<name>/tests (depth 2 relative to $dst), which is
|
||||
# where every dev-time fixture lives. A depth-agnostic `-name tests` also matched
|
||||
# template assets a skill ships for its own scaffolder to copy FROM -- e.g.
|
||||
# skills/skill-author/assets/templates/tests at depth 4, whose removal made the
|
||||
# mirrored new-skill.sh die on `sed: can't read .../tests/README.md` midway
|
||||
# through writing a scaffold.
|
||||
find "$dst" -mindepth 2 -maxdepth 2 -type d -name tests -exec rm -rf {} +
|
||||
elif [[ -d "$dst" ]]; then
|
||||
rm -rf "$dst"
|
||||
fi
|
||||
}
|
||||
|
||||
# Real-mode merged-hooks write; same single-implementation contract as sync_dir.
|
||||
sync_hooks_json() {
|
||||
local plugin_dir="$1" bundle_dir="$2"
|
||||
local src="$bundle_dir/hooks.json" dst="$plugin_dir/hooks.json"
|
||||
local target_dir="$1" bundle_dir="$2"
|
||||
local src="$bundle_dir/hooks.json"
|
||||
local dst="$target_dir/$HOOKS_REL" legacy="$target_dir/$LEGACY_HOOKS_REL"
|
||||
|
||||
if [[ "$CHECK" -eq 1 ]]; then
|
||||
if [[ -f "$src" ]]; then
|
||||
local normalized_src
|
||||
normalized_src="$(mktemp)"
|
||||
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
|
||||
fi
|
||||
rm -f "$normalized_src"
|
||||
elif [[ -f "$dst" ]]; then
|
||||
# Mirrors sync_dir()'s orphan handling: .apm/hooks/ no longer produces a
|
||||
# hooks.json, but one is still sitting at $dst from a prior sync -- that's
|
||||
# drift (stale mirrored output), not "no .apm/hooks/ content" (which would
|
||||
# mean $dst never existed in the first place).
|
||||
echo "DRIFT $dst: stale, no longer produced from .apm/hooks/" >&2
|
||||
FAIL=1
|
||||
fi
|
||||
return 0
|
||||
# Earlier revisions of this script wrote the merged hooks file to the plugin
|
||||
# root. Nothing scans it there (see the header comment), so it is stale output
|
||||
# regardless of whether .apm/hooks/ still produces one -- clean it up first.
|
||||
if [[ -f "$legacy" ]]; then
|
||||
rm -f "$legacy"
|
||||
fi
|
||||
|
||||
if [[ -f "$src" ]]; then
|
||||
mkdir -p "$target_dir/hooks"
|
||||
normalize_trailing_newline "$src" "$dst"
|
||||
elif [[ -f "$dst" ]]; then
|
||||
# .apm/hooks/ no longer produces a hooks.json, but one is still sitting at
|
||||
# $dst from a prior sync -- that's stale mirrored output, not "no .apm/hooks/
|
||||
# content" (which would mean $dst never existed in the first place).
|
||||
rm -f "$dst"
|
||||
rmdir "$target_dir/hooks" 2>/dev/null || true
|
||||
fi
|
||||
}
|
||||
|
||||
# --- check mode: compare the real plugin root against a synced throwaway copy ---
|
||||
#
|
||||
# Every function below is pure comparison. None of them knows what the mirror
|
||||
# rules are; $expected_dir is the output of the real sync functions above.
|
||||
|
||||
check_dir() {
|
||||
local plugin_dir="$1" expected_dir="$2" d="$3"
|
||||
local expected="$expected_dir/$d" actual="$plugin_dir/$d"
|
||||
|
||||
if [[ -d "$expected" ]]; then
|
||||
if [[ ! -d "$actual" ]]; then
|
||||
echo "DRIFT $actual: missing (would be created from .apm/)" >&2
|
||||
FAIL=1
|
||||
elif ! diff -rq "$expected" "$actual" >/dev/null 2>&1; then
|
||||
echo "DRIFT $actual: out of sync with .apm/" >&2
|
||||
# `|| true`: pipefail (set -o at the top) turns diff's exit 1 into a failed
|
||||
# pipeline, and set -e would abort sync_one right here -- reporting only the
|
||||
# first drifted directory per plugin and turning one push into N fix cycles.
|
||||
diff -rq "$expected" "$actual" 2>&1 | sed 's/^/ /' >&2 || true
|
||||
FAIL=1
|
||||
fi
|
||||
elif [[ -d "$actual" ]]; then
|
||||
echo "DRIFT $actual: stale, no longer produced from .apm/" >&2
|
||||
FAIL=1
|
||||
fi
|
||||
}
|
||||
|
||||
check_file() {
|
||||
local plugin_dir="$1" expected_dir="$2" rel="$3" desc="$4"
|
||||
local expected="$expected_dir/$rel" actual="$plugin_dir/$rel"
|
||||
|
||||
if [[ -f "$expected" ]]; then
|
||||
if [[ ! -f "$actual" ]]; then
|
||||
echo "DRIFT $actual: missing (would be created from $desc)" >&2
|
||||
FAIL=1
|
||||
elif ! diff -q "$expected" "$actual" >/dev/null 2>&1; then
|
||||
echo "DRIFT $actual: out of sync with $desc" >&2
|
||||
FAIL=1
|
||||
fi
|
||||
elif [[ -f "$actual" ]]; then
|
||||
echo "DRIFT $actual: stale, no longer produced from $desc" >&2
|
||||
FAIL=1
|
||||
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.
|
||||
path_manifest() {
|
||||
local root="$1"
|
||||
shift
|
||||
local rel f kind
|
||||
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
|
||||
kind="symlink"
|
||||
elif [[ -d "$f" ]]; then
|
||||
kind="dir"
|
||||
elif [[ -x "$f" ]]; then
|
||||
kind="exec"
|
||||
else
|
||||
kind="file"
|
||||
fi
|
||||
printf '%s %s\n' "$kind" "${f#"$root"/}"
|
||||
done || true
|
||||
done
|
||||
}
|
||||
|
||||
# `diff -r` compares content only: it dereferences symlinks and ignores file modes
|
||||
# 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.
|
||||
check_path_modes() {
|
||||
local plugin_dir="$1" expected_dir="$2"
|
||||
shift 2
|
||||
local expected actual
|
||||
expected="$(mktemp)"
|
||||
actual="$(mktemp)"
|
||||
path_manifest "$expected_dir" "$@" >"$expected"
|
||||
path_manifest "$plugin_dir" "$@" >"$actual"
|
||||
if ! diff -q "$expected" "$actual" >/dev/null 2>&1; then
|
||||
echo "DRIFT $plugin_dir: mirrored file types/modes differ from a fresh sync" >&2
|
||||
diff "$expected" "$actual" 2>&1 | sed 's/^/ /' >&2 || true
|
||||
FAIL=1
|
||||
fi
|
||||
rm -f "$expected" "$actual"
|
||||
}
|
||||
|
||||
reinject_mcp_servers() {
|
||||
local plugin_dir="$1" target_dir="$2"
|
||||
local mcp_src="$plugin_dir/.mcp.json" dst="$target_dir/.github/plugin/plugin.json"
|
||||
@@ -237,7 +338,8 @@ sync_plugin_manifest() {
|
||||
FAIL=1
|
||||
elif ! diff -q "$src" "$dst" >/dev/null 2>&1; then
|
||||
echo "DRIFT $dst: out of sync with apm.yml/.mcp.json" >&2
|
||||
diff "$src" "$dst" 2>&1 | sed 's/^/ /' >&2
|
||||
# `|| true` for the same pipefail/set -e reason as check_dir's diff above.
|
||||
diff "$src" "$dst" 2>&1 | sed 's/^/ /' >&2 || true
|
||||
FAIL=1
|
||||
fi
|
||||
elif [[ -f "$dst" ]]; then
|
||||
@@ -307,11 +409,15 @@ sync_one() {
|
||||
return 0
|
||||
fi
|
||||
|
||||
# Both modes run the identical real-mode mirror write; only the target differs.
|
||||
# In real mode that target IS the plugin root. In check mode it's the throwaway
|
||||
# pack_cwd copy, which then gets diffed against the plugin root below -- so
|
||||
# --check can only ever report what a real sync would actually change.
|
||||
local d
|
||||
for d in "${MIRROR_DIRS[@]}"; do
|
||||
sync_dir "$plugin_dir" "$bundle_dir" "$d"
|
||||
sync_dir "$pack_cwd" "$bundle_dir" "$d"
|
||||
done
|
||||
sync_hooks_json "$plugin_dir" "$bundle_dir"
|
||||
sync_hooks_json "$pack_cwd" "$bundle_dir"
|
||||
if [[ "$CHECK" -eq 0 ]]; then
|
||||
reinject_mcp_servers "$plugin_dir" "$plugin_dir"
|
||||
else
|
||||
@@ -319,6 +425,15 @@ sync_one() {
|
||||
# against what a real sync would actually produce (mcpServers included),
|
||||
# not apm's own Copilot-ecosystem output (which omits it).
|
||||
reinject_mcp_servers "$plugin_dir" "$pack_cwd"
|
||||
local -a checked_paths
|
||||
checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_REL" "$LEGACY_HOOKS_REL")
|
||||
for d in "${MIRROR_DIRS[@]}"; do
|
||||
check_dir "$plugin_dir" "$pack_cwd" "$d"
|
||||
done
|
||||
check_file "$plugin_dir" "$pack_cwd" "$HOOKS_REL" ".apm/hooks/"
|
||||
check_file "$plugin_dir" "$pack_cwd" "$LEGACY_HOOKS_REL" \
|
||||
".apm/hooks/ (Claude Code convention-scans $HOOKS_REL, not the plugin root)"
|
||||
check_path_modes "$plugin_dir" "$pack_cwd" "${checked_paths[@]}"
|
||||
sync_plugin_manifest "$plugin_dir" "$pack_cwd" ".claude-plugin/plugin.json"
|
||||
sync_plugin_manifest "$plugin_dir" "$pack_cwd" ".github/plugin/plugin.json"
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user