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 7ce9770..2c24640 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 @@ -9,7 +9,9 @@ could not. **Status: executed (2026-08-13, issue #90).** `scripts/sync-plugin-content.sh` has been run against all 6 plugins; flat `agents/`, `skills/`, `commands/` (etc., wherever `.apm/` populates -them), and a merged `hooks.json` now exist at each plugin root as tracked, generated files. +them), and a merged hooks file now exist at each plugin root as tracked, generated files. The +merged hooks file lands at `hooks/hooks.json`, not at the plugin root itself — see the second +amendment below, which corrects the path this ADR originally recorded. ## Context @@ -57,8 +59,9 @@ plugins from. Each plugin root gains a second, generated content category, produced by `scripts/sync-plugin-content.sh` (wraps `apm pack --format plugin`, copies the resulting bundle's -`agents/`, `skills/`, `commands/`, `instructions/`, `extensions/`, and merged `hooks.json` back to -the plugin root) — same governance status as `.claude-plugin/plugin.json`/`marketplace.json`: +`agents/`, `skills/`, `commands/`, `instructions/`, `extensions/`, and merged hooks file back to +the plugin root — the hooks file to `hooks/hooks.json`, per the second amendment below) — same +governance status as `.claude-plugin/plugin.json`/`marketplace.json`: **compiled output of `.apm/`, never hand-edited.** - `.apm/` remains the sole hand-edited authoring source, unchanged from ADR-0015. @@ -69,10 +72,21 @@ the plugin root) — same governance status as `.claude-plugin/plugin.json`/`mar `plugin.json` is already correctly generated by a separate, already-verified apm code path (`build_plugin_manifest`, run in the same `apm pack` invocation); `.mcp.json` is hand-authored at the plugin root per ADR-0015 and is not an `.apm/` primitive. -- Drift is enforced by a pre-push gate (`scripts/sync-plugin-content.sh --check`, wired into +- Dev-fixture `tests/` directories are excluded too — they are dev-time fixtures no plugin host + ever needs to discover, and several reference their own repo root through a hardcoded relative + walk-up sized for `.apm/`-nested depth, so a copy one directory level shallower breaks the + duplicate and double-runs the original under repo-wide bats discovery. The exclusion is + **depth-scoped to `//tests`**, deliberately: a skill may legitimately ship a + directory literally named `tests` as a template asset it scaffolds *from* + (`skills/skill-author/assets/templates/tests`, at depth 4). A depth-agnostic `-name tests` + matched that too and stripped it, making the mirrored `new-skill.sh` die mid-run on + `sed: can't read .../tests/README.md` — the scaffolder seds its way through the template tree + file by file. Scaffolding assets survive; fixtures do not. +- Drift is enforced by a pre-push gate (`scripts/sync-plugin-content.sh --check --all`, wired into `.pre-commit-config.yaml` as hook id `check-plugin-content-sync` by a parallel workstream on issue #90) — the same enforcement model `check-manifests.sh` already applies to the other - compiled-output category. + compiled-output category. `--check` alone is not the gate: the script requires either `--all` or + an explicit list of plugin directories, and run bare it prints usage and exits 1. - Verified two ways before landing: `claude plugin validate --strict` passes on all 6 real (non-scratch) plugin directories, and a live behavioral test (`claude --plugin-dir plugins/kyberforge -p "list your skills and agents"`) against the real @@ -125,24 +139,61 @@ explicit `mcpServers` declaration redundant, the way there is for skills/agents/ 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. +Given that, `scripts/sync-plugin-content.sh`'s `reinject_mcp_servers()`, called from `sync_one()`, +narrowly re-injects `mcpServers` into `.github/plugin/plugin.json` after `apm pack` runs, 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: {}`. **Both modes re-inject**, not just real syncs: real mode writes into the plugin +root directly, `--check` into its throwaway copy first, so the manifest diff compares against the +same content a real sync would actually produce (see the script's own header). A check-mode +re-injection is what keeps `--check` from reporting permanent phantom drift on every plugin that +ships an `.mcp.json`. 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. +Line numbers are deliberately omitted above. An earlier revision of this amendment cited +`reinject_mcp_servers()` at line 190 and its call site at line 269; both had already moved by the +next review round of the same PR, and moved again with the edits recorded in the amendment below. +A function name is stable enough to grep for; a line number in an ADR is stale by the next commit. + +## Amendment (2026-08-14): the merged hooks file lands at `hooks/hooks.json`, not the plugin root + +As originally executed, `sync-plugin-content.sh` wrote the merged hooks file to +`plugins//hooks.json`. That path is scanned by nothing. Claude Code convention-scans +`hooks/hooks.json`, and the "Plugin Directory Layout" table this ADR's own root-cause analysis +quotes above says so on the same line it says "All content directories must be at the plugin root, +not inside `.claude-plugin/`" +(`plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md:100`). The +implementation read "at the plugin root" and dropped the file there; the row it was reading names +`hooks/hooks.json`. So this ADR shipped with the contract quoted correctly in its diagnosis and +violated in its output — the flat mirror bridged skills and agents into discovery and left hooks +exactly as undiscoverable as before the fix. + +The merged file therefore moves to `plugins//hooks/hooks.json`. A root-level `hooks.json` +left over from a prior sync is stale output: a real sync deletes it, `--check` reports it as +drift. The real sync produced exactly these working-tree changes — `plugins/kyberforge/hooks.json` +and `plugins/lint/hooks.json` deleted, `plugins/kyberforge/hooks/hooks.json` and +`plugins/lint/hooks/hooks.json` created. Only those two plugins have an `.apm/hooks/` tree, so +only those two grow a mirrored hooks file at all. + +This does **not** reopen the "patch `plugin.json` pointer fields" option rejected above. The move +needs no `hooks` pointer in `plugin.json`: `hooks/hooks.json` *is* the convention path, so the +host finds it by auto-discovery, exactly as it finds `skills/` and `agents/`. The rejection stands +for the reason it was made — apm's `build_plugin_manifest` strips pointer fields unconditionally +and is right to, because convention directories need no pointer. Writing to the convention path is +what makes that premise true here rather than something to fight. + ## Consequences - Git now tracks real, visible duplication: `.apm/skills//SKILL.md` and `skills//SKILL.md` both exist and must match, likewise `.apm/agents/*.agent.md` vs. - `agents/*.agent.md`, and `.apm/hooks/*.json` vs. the merged `hooks.json`. This is an accepted + `agents/*.agent.md`, and `.apm/hooks/*.json` vs. the merged `hooks/hooks.json` (see the + 2026-08-14 amendment above for that path). This is an accepted tradeoff of bridging a gap apm itself doesn't close, not a bug — `.apm/` stays the single hand-edited source, and the drift gate (`check-plugin-content-sync`) is what keeps the mirror honest rather than trusting authors to remember to regenerate it by hand. diff --git a/plugins/kyberforge/hooks.json b/plugins/kyberforge/hooks/hooks.json similarity index 100% rename from plugins/kyberforge/hooks.json rename to plugins/kyberforge/hooks/hooks.json diff --git a/plugins/kyberforge/skills/skill-author/assets/templates/tests/README.md b/plugins/kyberforge/skills/skill-author/assets/templates/tests/README.md new file mode 100644 index 0000000..c5c18f6 --- /dev/null +++ b/plugins/kyberforge/skills/skill-author/assets/templates/tests/README.md @@ -0,0 +1,33 @@ +# tests/ + +Test files for scripts bundled with this skill. + +## When to add tests + +Add tests here when the skill has scripts in `scripts/` that are complex enough +to break silently — validators, parsers, generators, anything with branching +logic or edge cases. Test infrastructure (`.bats`, `*_test.*`, `test_*.sh`) +belongs here, not in `scripts/`. + +## Dependencies + +Tests require [bats-support](https://github.com/bats-core/bats-support) and +[bats-assert](https://github.com/bats-core/bats-assert). The test files load +helpers from the repo root's `tests/test_helper/`. + +From the repo root: + +```bash +git clone https://github.com/bats-core/bats-support tests/test_helper/bats-support +git clone https://github.com/bats-core/bats-assert tests/test_helper/bats-assert +``` + +Run all tests for this skill (from the repo root): + +```bash +bats /SKILL_NAME/tests/ +``` + +## If no tests are needed + +Delete this README and the `tests/` directory entirely. diff --git a/plugins/lint/hooks.json b/plugins/lint/hooks/hooks.json similarity index 100% rename from plugins/lint/hooks.json rename to plugins/lint/hooks/hooks.json diff --git a/scripts/sync-plugin-content.sh b/scripts/sync-plugin-content.sh index 60ed58e..0adb9fd 100755 --- a/scripts/sync-plugin-content.sh +++ b/scripts/sync-plugin-content.sh @@ -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 -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//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 //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 | [ ...])" >&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 //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 " " 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 diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index 024bd78..69fe1b0 100755 --- a/tests/test-sync-plugin-content.sh +++ b/tests/test-sync-plugin-content.sh @@ -14,14 +14,27 @@ if ! command -v apm &>/dev/null; then exit 77 fi -# Minimal fixture: one skill (with a tests/ fixture that must NOT be mirrored), one -# agent, one hooks.json -- enough to exercise every mirrored category -# (scripts/sync-plugin-content.sh's MIRROR_DIRS plus hooks.json) without needing -# network access (no apm.yml dependencies). +# Minimal fixture exercising every mirrored category -- all five of +# scripts/sync-plugin-content.sh's MIRROR_DIRS (agents, skills, commands, +# instructions, extensions) plus the merged hooks file -- without needing network +# access (no apm.yml dependencies). Two deliberately-shaped skill subdirectories: +# +# skills/hello/tests/ -- a dev-time fixture that must NOT be mirrored +# skills/hello/assets/templates/tests/ -- a template asset that MUST be mirrored +# +# Those two are the same basename at different depths. The exclusion is depth-scoped +# for exactly this reason: the real skill-author skill ships a template tree it +# scaffolds from, and a depth-agnostic strip amputated it. +# +# skills/hello/scripts/run.sh is executable so the mode/symlink drift checks have a +# real executable to tamper with. make_fixture() { local dir dir="$(mktemp -d)" - mkdir -p "$dir/.apm/skills/hello/tests" "$dir/.apm/agents" "$dir/.apm/hooks" + mkdir -p "$dir/.apm/skills/hello/tests" "$dir/.apm/skills/hello/scripts" \ + "$dir/.apm/skills/hello/assets/templates/tests" "$dir/.apm/agents" \ + "$dir/.apm/hooks" "$dir/.apm/commands" "$dir/.apm/instructions" \ + "$dir/.apm/extensions" cat > "$dir/apm.yml" <<'YAML' name: fixture version: 0.0.1 @@ -48,12 +61,35 @@ EOF cat > "$dir/.apm/skills/hello/tests/sample.bats" <<'EOF' @test "dummy" { true; } EOF + cat > "$dir/.apm/skills/hello/assets/templates/tests/README.md" <<'EOF' +Template asset: scaffolded into a new skill, not a dev fixture of this one. +EOF + cat > "$dir/.apm/skills/hello/scripts/run.sh" <<'EOF' +#!/usr/bin/env bash +echo hi +EOF + chmod +x "$dir/.apm/skills/hello/scripts/run.sh" cat > "$dir/.apm/agents/foo.agent.md" <<'EOF' --- name: foo description: foo --- Foo. +EOF + cat > "$dir/.apm/commands/mycmd.md" <<'EOF' +--- +description: mycmd +--- +Do a thing. +EOF + cat > "$dir/.apm/instructions/style.instructions.md" <<'EOF' +--- +applyTo: "**" +--- +Be consistent. +EOF + cat > "$dir/.apm/extensions/thing.md" <<'EOF' +Extension content. EOF cat > "$dir/.apm/hooks/hooks.json" <<'EOF' {"hooks": {"PreToolUse": []}} @@ -119,16 +155,45 @@ fi # --- 2. A real sync creates the flat mirror and exits 0 --- echo "" -echo "--- real sync creates skills/, agents/, hooks.json ---" +echo "--- real sync creates every MIRROR_DIRS category plus hooks/hooks.json ---" if bash "$SCRIPT" "$FIXTURE" > /dev/null 2>&1 \ && [[ -f "$FIXTURE/skills/hello/SKILL.md" ]] \ && [[ -f "$FIXTURE/agents/foo.agent.md" ]] \ - && [[ -f "$FIXTURE/hooks.json" ]]; then - pass "sync creates the expected flat mirror" + && [[ -f "$FIXTURE/commands/mycmd.md" ]] \ + && [[ -f "$FIXTURE/instructions/style.instructions.md" ]] \ + && [[ -f "$FIXTURE/extensions/thing.md" ]] \ + && [[ -f "$FIXTURE/hooks/hooks.json" ]]; then + pass "sync creates the expected flat mirror for all five MIRROR_DIRS plus hooks/hooks.json" else fail "sync did not create the expected flat mirror" fi +# --- 2b. The merged hooks file goes to hooks/hooks.json, never the plugin root --- +# Claude Code convention-scans `hooks/hooks.json` at the plugin root (see +# plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md's +# "Plugin Directory Layout" table, quoted in ADR-0017), and the compiled plugin.json +# carries no `hooks` pointer that could redirect it. A root-level hooks.json is read +# by nothing. +echo "" +echo "--- the merged hooks file is not left at the plugin root ---" +if [[ ! -e "$FIXTURE/hooks.json" ]]; then + pass "no root-level hooks.json after a sync" +else + fail "sync wrote hooks.json to the plugin root — Claude Code scans hooks/hooks.json" +fi + +# --- 2c. hooks/hooks.json is newline-terminated --- +# normalize_trailing_newline() exists so pre-commit's end-of-file-fixer does not +# re-dirty the tree on every sync: apm's bundle exporter emits hooks.json with no +# trailing newline, the committed file has one. +echo "" +echo "--- the synced hooks file ends in a newline ---" +if [[ -n "$(tail -c 1 "$FIXTURE/hooks/hooks.json")" ]]; then + fail "hooks/hooks.json has no trailing newline — end-of-file-fixer will re-dirty it every sync" +else + pass "hooks/hooks.json is newline-terminated" +fi + # --- 3. tests/ fixtures are excluded from the mirror --- echo "" echo "--- tests/ subdirectories are not mirrored ---" @@ -138,6 +203,20 @@ else fail "skills/hello/tests/ was copied into the mirror — should be excluded" fi +# --- 3b. ...but a deeper tests/ that is a template ASSET must survive --- +# The exclusion above is depth-scoped to //tests. Stripping every +# directory named tests at any depth also deletes template trees a skill ships for +# its own scaffolder to copy from — which is what broke the mirrored +# skills/skill-author/scripts/new-skill.sh (`sed: can't read .../tests/README.md`, +# half-written scaffold left behind) while the .apm/ original still worked. +echo "" +echo "--- a tests/ directory nested under assets/templates/ is preserved ---" +if [[ -f "$FIXTURE/skills/hello/assets/templates/tests/README.md" ]]; then + pass "skills/hello/assets/templates/tests/ survived the sync" +else + fail "skills/hello/assets/templates/tests/ was stripped — template assets are not dev fixtures" +fi + # --- 4. --check is clean immediately after a real sync --- echo "" echo "--- --check is clean right after syncing ---" @@ -277,29 +356,186 @@ else fi fi -# --- 13. --check detects an orphaned hooks.json after .apm/hooks/ is removed --- +# --- 13. --check detects an orphaned hooks file after .apm/hooks/ is removed --- echo "" -echo "--- --check detects an orphaned hooks.json when .apm/hooks/ is removed ---" +echo "--- --check detects an orphaned hooks/hooks.json when .apm/hooks/ is removed ---" FIXTURE13="$(make_fixture)"; track "$FIXTURE13" bash "$SCRIPT" "$FIXTURE13" > /dev/null 2>&1 -if [[ ! -f "$FIXTURE13/hooks.json" ]]; then - fail "initial sync did not create hooks.json -- can't test the orphan case" +if [[ ! -f "$FIXTURE13/hooks/hooks.json" ]]; then + fail "initial sync did not create hooks/hooks.json -- can't test the orphan case" fi rm -rf "$FIXTURE13/.apm/hooks" if bash "$SCRIPT" --check "$FIXTURE13" > /dev/null 2>&1; then - fail "no drift reported for an orphaned hooks.json after .apm/hooks/ removal" + fail "no drift reported for an orphaned hooks/hooks.json after .apm/hooks/ removal" else - pass "orphaned hooks.json is detected as drift" + pass "orphaned hooks/hooks.json is detected as drift" bash "$SCRIPT" "$FIXTURE13" > /dev/null 2>&1 - if [[ ! -e "$FIXTURE13/hooks.json" ]]; then - pass "re-sync removes the orphaned hooks.json" + if [[ ! -e "$FIXTURE13/hooks/hooks.json" ]]; then + pass "re-sync removes the orphaned hooks/hooks.json" else - fail "re-sync left the orphaned hooks.json in place" + fail "re-sync left the orphaned hooks/hooks.json in place" fi if bash "$SCRIPT" --check "$FIXTURE13" > /dev/null 2>&1; then - pass "re-sync clears the orphaned-hooks.json drift" + pass "re-sync clears the orphaned-hooks drift" else - fail "re-sync did not clear the orphaned-hooks.json drift" + fail "re-sync did not clear the orphaned-hooks drift" + fi +fi + +# --- 14. A legacy root-level hooks.json is stale output, not content --- +# Every plugin synced by an earlier revision of this script carries one. Nothing +# reads it (no `hooks` pointer in the compiled plugin.json, and Claude Code's +# convention scan looks at hooks/hooks.json), so --check must flag it and a real +# sync must delete it. +echo "" +echo "--- a legacy root-level hooks.json is reported as drift and removed by a sync ---" +FIXTURE14="$(make_fixture)"; track "$FIXTURE14" +bash "$SCRIPT" "$FIXTURE14" > /dev/null 2>&1 +printf '{"hooks": {"PreToolUse": []}}\n' > "$FIXTURE14/hooks.json" +if bash "$SCRIPT" --check "$FIXTURE14" > /dev/null 2>&1; then + fail "no drift reported for a leftover root-level hooks.json" +else + pass "a leftover root-level hooks.json is reported as drift" + bash "$SCRIPT" "$FIXTURE14" > /dev/null 2>&1 + if [[ ! -e "$FIXTURE14/hooks.json" ]] && [[ -f "$FIXTURE14/hooks/hooks.json" ]]; then + pass "re-sync deletes the root-level hooks.json and keeps hooks/hooks.json" + else + fail "re-sync did not clean up the root-level hooks.json" + fi +fi + +# --- 15. Deleting a skill from .apm/ leaves a stale mirror a re-sync must clear --- +# Without sync_dir()'s rm -rf of the destination before recopying, --check would +# report a drift that no amount of re-syncing could ever clear -- a permanently +# unfixable pre-push failure. This is the assertion that pins that wipe. +echo "" +echo "--- a skill deleted from .apm/ is removed from the mirror by a re-sync ---" +FIXTURE15="$(make_fixture)"; track "$FIXTURE15" +mkdir -p "$FIXTURE15/.apm/skills/doomed" +cat > "$FIXTURE15/.apm/skills/doomed/SKILL.md" <<'EOF' +--- +name: doomed +description: doomed +--- +Doomed. +EOF +bash "$SCRIPT" "$FIXTURE15" > /dev/null 2>&1 +if [[ ! -f "$FIXTURE15/skills/doomed/SKILL.md" ]]; then + fail "initial sync did not mirror skills/doomed -- can't test the stale-skill case" +else + rm -rf "$FIXTURE15/.apm/skills/doomed" + if bash "$SCRIPT" --check "$FIXTURE15" > /dev/null 2>&1; then + fail "no drift reported for a mirrored skill deleted from .apm/" + else + pass "a mirrored skill deleted from .apm/ is reported as drift" + bash "$SCRIPT" "$FIXTURE15" > /dev/null 2>&1 + if [[ ! -e "$FIXTURE15/skills/doomed" ]]; then + pass "re-sync removes the stale skills/doomed/ from the mirror" + else + fail "re-sync left the stale skills/doomed/ behind — this drift would be unfixable" + fi + if bash "$SCRIPT" --check "$FIXTURE15" > /dev/null 2>&1; then + pass "re-sync clears the stale-skill drift" + else + fail "re-sync did not clear the stale-skill drift" + fi + fi +fi + +# --- 16. Drift in each of the less-obvious MIRROR_DIRS is detected --- +# agents/ and skills/ are exercised everywhere above; commands/, instructions/, and +# extensions/ were previously unreachable by the fixture, so dropping them from +# MIRROR_DIRS entirely still passed the suite. +echo "" +echo "--- drift in commands/, instructions/, and extensions/ is detected ---" +for CATEGORY_PATH in commands/mycmd.md instructions/style.instructions.md extensions/thing.md; do + FIXTURE16="$(make_fixture)"; track "$FIXTURE16" + bash "$SCRIPT" "$FIXTURE16" > /dev/null 2>&1 + if [[ ! -f "$FIXTURE16/$CATEGORY_PATH" ]]; then + fail "sync did not mirror $CATEGORY_PATH at all — is its category still in MIRROR_DIRS?" + continue + fi + printf 'tampered\n' >> "$FIXTURE16/$CATEGORY_PATH" + if bash "$SCRIPT" --check "$FIXTURE16" > /dev/null 2>&1; then + fail "no drift reported after tampering with $CATEGORY_PATH" + else + pass "drift in $CATEGORY_PATH is detected" + fi +done + +# --- 17. --check reports every drift in one run, not just the first --- +# The DRIFT-detail `diff | sed` pipelines return non-zero under `set -o pipefail`; +# without an explicit `|| true` guard, `set -e` aborts the per-plugin subshell after +# the first reported drift, turning one push into N fix/re-push cycles. +echo "" +echo "--- --check reports all independent drifts in a single run ---" +FIXTURE17="$(make_fixture)"; track "$FIXTURE17" +bash "$SCRIPT" "$FIXTURE17" > /dev/null 2>&1 +printf 'tampered\n' >> "$FIXTURE17/agents/foo.agent.md" +printf 'tampered\n' >> "$FIXTURE17/skills/hello/SKILL.md" +printf 'tampered\n' >> "$FIXTURE17/commands/mycmd.md" +printf 'tampered\n' >> "$FIXTURE17/instructions/style.instructions.md" +mkdir -p "$FIXTURE17/hooks" +printf '{"hooks": {"PreToolUse": [], "tampered": true}}\n' > "$FIXTURE17/hooks/hooks.json" +CHECK17="$(bash "$SCRIPT" --check "$FIXTURE17" 2>&1 || true)" +MISSED="" +for CATEGORY_PATH in agents skills commands instructions hooks/hooks.json; do + case "$CHECK17" in + *"DRIFT $FIXTURE17/$CATEGORY_PATH"*) ;; + *) MISSED="$MISSED $CATEGORY_PATH" ;; + esac +done +if [[ -z "$MISSED" ]]; then + pass "all five independent drifts are reported in one --check run" +else + fail "--check stopped early — never reported drift for:$MISSED" +fi + +# --- 18. --check sees a mode change on a mirrored executable --- +# `diff -r` compares content only, so a chmod -x left --check at exit 0 while a real +# sync silently restored the bit — check and sync disagreeing. +echo "" +echo "--- --check detects a mode change on a mirrored executable ---" +FIXTURE18="$(make_fixture)"; track "$FIXTURE18" +bash "$SCRIPT" "$FIXTURE18" > /dev/null 2>&1 +if [[ ! -x "$FIXTURE18/skills/hello/scripts/run.sh" ]]; then + fail "sync did not preserve the executable bit on skills/hello/scripts/run.sh" +else + pass "sync preserves the executable bit on a mirrored script" + chmod -x "$FIXTURE18/skills/hello/scripts/run.sh" + if bash "$SCRIPT" --check "$FIXTURE18" > /dev/null 2>&1; then + fail "no drift reported after chmod -x on a mirrored executable" + else + pass "a mode change on a mirrored executable is detected as drift" + bash "$SCRIPT" "$FIXTURE18" > /dev/null 2>&1 + if [[ -x "$FIXTURE18/skills/hello/scripts/run.sh" ]]; then + pass "re-sync restores the executable bit" + else + fail "re-sync did not restore the executable bit" + fi + fi +fi + +# --- 19. --check sees a mirrored file replaced by a symlink --- +# `diff -r` dereferences symlinks, so a symlink to byte-identical content reads as +# no drift while a real sync replaces it with a regular file. +echo "" +echo "--- --check detects a mirrored file swapped for a symlink ---" +FIXTURE19="$(make_fixture)"; track "$FIXTURE19" +bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1 +SYMLINK_TARGET="$FIXTURE19/decoy-agent.md" +cp "$FIXTURE19/agents/foo.agent.md" "$SYMLINK_TARGET" +rm -f "$FIXTURE19/agents/foo.agent.md" +ln -s "$SYMLINK_TARGET" "$FIXTURE19/agents/foo.agent.md" +if bash "$SCRIPT" --check "$FIXTURE19" > /dev/null 2>&1; then + fail "no drift reported after replacing a mirrored file with a symlink to identical content" +else + pass "a mirrored file replaced by a symlink is detected as drift" + bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1 + if [[ -f "$FIXTURE19/agents/foo.agent.md" ]] && [[ ! -L "$FIXTURE19/agents/foo.agent.md" ]]; then + pass "re-sync restores it to a regular file" + else + fail "re-sync did not restore the symlinked mirror entry to a regular file" fi fi