From 0dffff3c21582f643024600fe53ab68d5b57c6b0 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Mon, 14 Sep 2026 11:35:44 +0000 Subject: [PATCH] chore: drop redundant apm-marketplace-check and Copilot marketplace mirror (findings 1, 2) apm-marketplace-check checked network reachability of remote marketplace refs, but finding 35 already removed the only remote package entry, so every marketplace.packages[] source is local and the hook is pure overlap with apm-pack-check-clean. apm-audit-ci was re-examined and kept as-is -- its pre-commit-config.yaml comment already carries a dated, verified justification the audit had missed. check-marketplace-mirror-sync guarded .github/plugin/marketplace.json against drift from .claude-plugin/marketplace.json. Verified against current GitHub Copilot CLI docs: Copilot's marketplace discovery already falls back through .github/plugin/marketplace.json to .claude-plugin/marketplace.json, which this repo generates anyway -- the dedicated mirror bought a discovery-order preference, not a capability. Deleted the mirror file, its sync script, its test, and the hook. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD --- .github/plugin/marketplace.json | 54 ----- .pre-commit-config.yaml | 22 +- README.md | 4 +- SIMPLIFICATION-AUDIT.md | 5 +- docs/spec/architecture.md | 2 +- docs/spec/gates.md | 23 +- scripts/sync-marketplace-mirror.sh | 81 ------- scripts/sync-plugin-content.sh | 6 +- tests/test-sync-marketplace-mirror.sh | 304 -------------------------- tests/test-sync-plugin-content.sh | 8 +- 10 files changed, 25 insertions(+), 484 deletions(-) delete mode 100644 .github/plugin/marketplace.json delete mode 100755 scripts/sync-marketplace-mirror.sh delete mode 100755 tests/test-sync-marketplace-mirror.sh diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json deleted file mode 100644 index c8211b6..0000000 --- a/.github/plugin/marketplace.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "name": "holocron", - "description": "AI development skills for Claude Code and GitHub Copilot CLI — factory, design, implement, review, and cross-cutting workflows.", - "version": "0.4.6", - "owner": { - "name": "Defame1297", - "email": "defame1297@rkdr.net", - "url": "https://git.dev.rkdr.net/Defame1297/" - }, - "plugins": [ - { - "name": "kyberforge", - "description": "Skills and agents for creating, maintaining, and managing a Claude Code / Copilot CLI plugin marketplace.", - "version": "1.6.2", - "category": "Developer Tools", - "source": "./plugins/kyberforge" - }, - { - "name": "bin", - "description": "Skills for everyday AI-assisted development work that is not tied to a single tool, forge or language, and has not yet been split into a focused plugin.", - "version": "1.1.7", - "category": "Utilities", - "source": "./plugins/bin" - }, - { - "name": "git", - "description": "Skills and agents for working with a local Git clone over the git wire protocol, and for authoring and running the pre-commit hooks that guard it.", - "version": "1.3.7", - "category": "Version Control", - "source": "./plugins/git" - }, - { - "name": "gitea", - "description": "Skills and agents for working with a Gitea forge through its HTTP API — the forge's own objects, as distinct from the local git clone.", - "version": "1.3.8", - "category": "Version Control", - "source": "./plugins/gitea" - }, - { - "name": "core", - "description": "Skills for authoring and auditing a repo's AGENTS.md and the provider adapter files that defer to it.", - "version": "1.1.2", - "category": "Productivity", - "source": "./plugins/core" - }, - { - "name": "lint", - "description": "Skills and agents for configuring and running linters.", - "version": "1.1.7", - "category": "Developer Tools", - "source": "./plugins/lint" - } - ] -} diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1fc6145..c6af7e8 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -84,15 +84,6 @@ repos: pass_filenames: false always_run: true - - id: check-marketplace-mirror-sync - name: Check marketplace mirror sync - description: Verify .github/plugin/marketplace.json (Copilot CLI's legacy manifest path) is byte-identical to .claude-plugin/marketplace.json -- apm has no output profile for this path, so it must be kept in sync explicitly (see issue #90) - entry: bash scripts/sync-marketplace-mirror.sh --check - language: system - stages: [pre-push] - pass_filenames: false - always_run: true - - id: check-executables-allow-sync name: Check executables allow key sync description: Verify root apm.yml's executables.allow key names kyberforge's actual version -- apm matches that key by exact "#" lookup, so a version bump on one side alone silently stops deploying kyberforge's hooks/ and bin/ and lets the apm install go stale (see ADR-0019) @@ -102,15 +93,6 @@ repos: pass_filenames: false always_run: true - - id: apm-marketplace-check - name: apm marketplace check - description: Validate every marketplace.packages[] entry resolves, including network reachability of remote refs -- the only hook that checks remote package references rather than local-source paths - entry: apm marketplace check - language: system - stages: [pre-push] - pass_filenames: false - always_run: true - - id: apm-audit-ci name: apm audit --ci description: Run apm's producer-side CI gate over the root manifest AND each of the six plugin packages. Verifies exactly two things per manifest -- apm.yml parses as a valid APM manifest (manifest-parse), and, if it declares dependencies, apm.lock.yaml exists and is consistent (lockfile-exists). It does NOT enforce an org policy and does NOT scan for hidden Unicode; see the comment below for why. Reference:plugins/kyberforge/.apm/skills/apm-workflow/references/audit.md @@ -153,8 +135,8 @@ repos: # six plugin packages is what makes either reachable for them at all -- # the root-only invocation audits the marketplace manifest and nothing # else. Costs ~0.5s per package, needs no network (checked under - # `unshare -rn`), so this does NOT join apm-marketplace-check and - # apm-pack-check-clean on the offline SKIP= list. + # `unshare -rn`) -- consistent with every other pre-push hook: none of + # them need the network (see README.md's "Offline?" section). - id: check-apm-agents-valid name: Validate real APM agent files diff --git a/README.md b/README.md index 58dc5db..3a1c9a5 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Install all of these before setting up. Each one is a hard dependency of a git h | Tool | Why | Install | | --- | --- | --- | -| `apm` CLI | Four pre-push hooks shell out to it (`apm-marketplace-check`, `apm-audit-ci`, `apm-pack-check-clean`, and `check-plugin-content-sync` via `scripts/sync-plugin-content.sh`) | The `apm-install` skill, or `curl -sSL https://aka.ms/apm-unix \| sh`. Verify with `apm --version` | +| `apm` CLI | Three pre-push hooks shell out to it (`apm-audit-ci`, `apm-pack-check-clean`, and `check-plugin-content-sync` via `scripts/sync-plugin-content.sh`) | The `apm-install` skill, or `curl -sSL https://aka.ms/apm-unix \| sh`. Verify with `apm --version` | | `jq` | Required by `scripts/sync-plugin-content.sh`, pre-push | Your package manager | | `python3` + PyYAML | Required by `scripts/skill-size-check.sh` (the `skill-size-check` pre-commit hook), which reads folded YAML frontmatter | `python3` is usually present — pre-commit is itself a Python application. `pip install pyyaml` if the hook reports PyYAML missing | | `vale` | Required by the `vale-audit-prefilter-skill` / `-agent` pre-commit hooks and the `check-vale-style-sync` pre-push hook | `brew install vale` (macOS), `snap install vale` (Linux), `choco install vale` (Windows), or https://vale.sh/docs/vale-cli/installation/ | @@ -92,7 +92,7 @@ Every other pre-push hook does run. See [`docs/spec/gates.md`](docs/spec/gates.md) for what each hook enforces and why. -**Offline?** No pre-push hook needs the network: root `apm.yml`'s marketplace has no remote package entries (the last one, `mattpocock-skills`, was removed), so `apm-marketplace-check` and `apm-pack-check-clean` resolve everything from local sources. All pre-push hooks pass offline. +**Offline?** No pre-push hook needs the network: root `apm.yml`'s marketplace has no remote package entries (the last one, `mattpocock-skills`, was removed), so `apm-pack-check-clean` resolves everything from local sources. All pre-push hooks pass offline. ## Editing plugin content diff --git a/SIMPLIFICATION-AUDIT.md b/SIMPLIFICATION-AUDIT.md index a518039..0c7541a 100644 --- a/SIMPLIFICATION-AUDIT.md +++ b/SIMPLIFICATION-AUDIT.md @@ -61,11 +61,14 @@ This is the area you named as hardest to understand and slowest. Root cause: mos 1. **Six hooks validate overlapping sets of the same manifests.** `check-manifests`, `validate-plugins`, `validate-marketplace`, `apm-pack-check-clean`, `apm-marketplace-check`, `apm-audit-ci`. Keep the two `claude plugin validate` hooks plus `apm-pack-check-clean`. ~~Delete `check-manifests` (282 lines + 771 test lines; its `lib/marketplace-plugins.sh` stays because `sync-plugin-content.sh` sources it).~~ `apm-audit-ci` spends 12 s confirming that manifests `apm pack` already parses do parse; drop or keep on that basis. Move the network-dependent `apm-marketplace-check` to a release checklist. Effort S. > **Done (2026-09-12):** see commit `e647f14` on `docs/simplification-audit`. Deleted the `check-manifests` pre-commit hook entry, `scripts/check-manifests.sh` (282 lines), and `tests/test-check-manifests.sh` (771 lines); kept `scripts/lib/marketplace-plugins.sh`, still sourced by `sync-plugin-content.sh`. Updated the now-stale `check-manifests.sh` mentions in `README.md` and `docs/spec/gates.md` (hook table row and hook counts). The `apm-audit-ci` and `apm-marketplace-check` decisions in this finding remain open — out of scope for this change. + > **Grilled and closed (2026-09-14):** `apm-audit-ci` — already resolved before this audit was written: `.pre-commit-config.yaml`'s own comment block (added in commit `a155af6`, months before this audit) already rebuts the "overclaimed description" complaint and gives a dated, verified justification for what the hook still checks. Keep, no action. `apm-marketplace-check` — its stated purpose ("the only hook that checks remote package references rather than local-source paths") is void: finding 35 (commit `568ca74`) already removed the only remote package entry, so every `marketplace.packages[]` entry is now a local `./plugins/` path and the hook is pure overlap with `apm-pack-check-clean`. Removed the hook entry, and corrected the now-stale "does NOT join apm-marketplace-check ... on the offline SKIP= list" comment on `apm-audit-ci` (there is no offline skip list any more — every pre-push hook already passes offline per `README.md`). Updated `README.md` (tool table, "Offline?" section) and `docs/spec/gates.md` (hook table, hook counts 13→11 self-authored / 15→13 total, the "Three of these shell out to apm" paragraph, and the "Pushing without a network" section) accordingly. Verified: `apm audit --ci` still passes per-plugin, and the pre-push hook count now matches `.pre-commit-config.yaml`. 2. **Four "keep two copies in sync" gates: 1,100 script lines + 1,600 test lines.** Each one is a symptom of duplication that could be removed instead of guarded: - `check-vale-style-sync`: 413 lines + 798 test lines guarding a byte-identical 526-line `vale-wrap.sh` and style directory copied between skill-audit and agent-audit. About 350 of its lines run Vale glob probes against the hook file patterns. Disappears if the two audit skills merge (finding 14); the probes belong in `test-vale-wrap.sh`. - `check-scope-walkup-sync`: 365 lines cross-checking four independent ports of the same package-root walk-up. Disappears if the ports share one script or the skills merge. - - `check-marketplace-mirror-sync`: guards `.github/plugin/marketplace.json`. The script header calls it Copilot's legacy convention path and says Copilot also accepts the Claude path; the vendored Copilot docs list it as primary. Verify against current Copilot CLI before deleting hook, script, test, and mirror file. + > **Grilled, held (2026-09-14):** both of the above are gated on findings 14/15 (merging skill-audit+agent-audit and skill-author+agent-author), deliberately held for a separate session rather than decided here. Correction for that session: the audit's §8 grouping is wrong — these merges don't need ADR-0012 revisited (that ADR governs the unrelated `core` plugin's three `agentsmd-*` skills). The actual constraint is ADR-0014 (no-cross-skill file sharing on plugin cache-install), and merging sidesteps it rather than requiring it be reversed. The open question for that session is a design one — a shared skill's `description` carrying both skill- and agent-audit trigger phrases — not an ADR supersession. ADR-0012 revisit is needed only for finding 24. + - [x] ~~`check-marketplace-mirror-sync`: guards `.github/plugin/marketplace.json`. The script header calls it Copilot's legacy convention path and says Copilot also accepts the Claude path; the vendored Copilot docs list it as primary. Verify against current Copilot CLI before deleting hook, script, test, and mirror file.~~ + > **Grilled and done (2026-09-14):** verified against GitHub's current Copilot CLI plugin docs (not the vendored copy, which risked drift). Copilot CLI's marketplace discovery checks paths in order — `marketplace.json`, `.plugin/marketplace.json`, `.github/plugin/marketplace.json`, `.claude-plugin/marketplace.json` — falling through to whichever exists first. `.claude-plugin/marketplace.json` (apm's own `claude` output) already satisfies that chain's last step, so the dedicated `.github/plugin/marketplace.json` mirror bought Copilot users its *preferred* discovery path rather than a required one. Decided against reopening ADR-0018 (native install for both Claude Code and Copilot CLI stays supported) to justify this — the deletion holds either way, since Copilot's own fallback covers it. Deleted `.github/plugin/marketplace.json`, `scripts/sync-marketplace-mirror.sh` (81 lines), `tests/test-sync-marketplace-mirror.sh` (304 lines), and the `check-marketplace-mirror-sync` pre-push hook; removed the dangling references to the deleted script in `scripts/sync-plugin-content.sh` and `tests/test-sync-plugin-content.sh` (both had comments citing its reasoning by name), and updated `docs/spec/architecture.md`'s description of the marketplace-manifest compile step. `tests/test-sync-plugin-content.sh` (92 cases) still passes in full. - [x] ~~`check-executables-allow-sync`: 474 lines to assert one string equals kyberforge's version. A six-line grep, or drop it (the failure mode is visible and recoverable).~~ > **Corrected then partially done (2026-09-13):** see commit `1b01e25` on `docs/simplification-audit`. Independent re-verification found "drop it" unsafe — ADR-0019's own Consequences section calls this failure mode *silent* and says a silent-staleness failure here is worse than the duplication the other gates catch, directly contradicting the finding's "visible and recoverable" claim. The hook stays. Shrunk `scripts/check-executables-allow-sync.sh` 231 → 222 lines by deduplicating two comment blocks that re-derived ADR-0019's own reasoning inline, replacing them with a pointer at the ADR. The dual-reader design (PyYAML plus a hand-rolled fallback, so a missing PyYAML can't silently skip the check) was found to be load-bearing, not redundant, and left intact; test file unchanged (behavior unaffected). All 23 test cases and the live pre-push hook run still pass. Effort S each, M for the walk-up. diff --git a/docs/spec/architecture.md b/docs/spec/architecture.md index 61a2c34..bc473d2 100644 --- a/docs/spec/architecture.md +++ b/docs/spec/architecture.md @@ -44,7 +44,7 @@ These are routing boundaries, not inventories — they answer "where does a new Two compilers produce the plugin roots you see in the tree: -- **`apm pack` compiles the manifests** (ADR-0015). Per plugin: `.claude-plugin/plugin.json` and `.github/plugin/plugin.json`, both generated from `plugins//apm.yml`. Repo-wide, from the root `apm.yml`'s `marketplace:` block: `.claude-plugin/marketplace.json` (apm's `claude` output profile) — the only marketplace output this repo declares. A second root manifest, `.github/plugin/marketplace.json` (Copilot CLI's legacy path), is a byte-identical mirror of the Claude one maintained by `scripts/sync-marketplace-mirror.sh` and gated by the `check-marketplace-mirror-sync` pre-push hook. +- **`apm pack` compiles the manifests** (ADR-0015). Per plugin: `.claude-plugin/plugin.json` and `.github/plugin/plugin.json`, both generated from `plugins//apm.yml`. Repo-wide, from the root `apm.yml`'s `marketplace:` block: `.claude-plugin/marketplace.json` (apm's `claude` output profile) — the only marketplace manifest this repo generates or ships. Copilot CLI checks for a marketplace manifest at several conventional paths, falling back through `.github/plugin/marketplace.json` to `.claude-plugin/marketplace.json` — since this repo already generates the latter, no dedicated Copilot-path mirror is maintained. - **`scripts/sync-plugin-content.sh` compiles the content mirror** (ADR-0017). It wraps `apm pack --format plugin` and copies the resulting bundle's flat `agents/`, `skills/`, `commands/`, `instructions/`, `extensions/`, and merged `hooks/hooks.json` back to the plugin root. Claude Code's installer convention-scans those flat paths and has no `.apm/` awareness whatsoever, so the mirror exists solely to satisfy the host's discovery contract. `.apm/` is the sole hand-edited authoring source for plugin content. An edit made in the flat mirror is discarded by the next sync and is reported as drift by the `check-plugin-content-sync` pre-push hook. Hand-authored material that is not an `.apm/` primitive — `README.md`, `docs/`, `bin/`, `sources.md`, `.mcp.json`, and per-plugin extras such as `plugins/gitea/references/` and `plugins/bin/evals/` — lives at the plugin **root** and is untouched by either compiler. diff --git a/docs/spec/gates.md b/docs/spec/gates.md index 436566d..a9d1640 100644 --- a/docs/spec/gates.md +++ b/docs/spec/gates.md @@ -21,24 +21,24 @@ Install hooks via `pc-run`, wiring **all three stages**. This repo's `.pre-commi `default_install_hook_types`, so a plain install silently skips `commit-msg` (Conventional Commits) and `pre-push` (everything below). -The pre-push command reports **15** hooks, not 13. The extra two are pre-commit's own `meta` hooks, +The pre-push command reports **13** hooks, not 11. The extra two are pre-commit's own `meta` hooks, `check-hooks-apply` and `check-useless-excludes`: they declare no `stages:`, so they run at every stage including this one. Both are declared in this repo's `.pre-commit-config.yaml` like everything -else — what separates them is `repo: meta` (pre-commit's own built-ins) from `repo: local`. Thirteen +else — what separates them is `repo: meta` (pre-commit's own built-ins) from `repo: local`. Eleven is the count of hooks this repo authors itself. -**The caveat: one of those 13 is a silent no-op under that invocation.** +**The caveat: one of those 11 is a silent no-op under that invocation.** `check-release-needed` exits 0 immediately unless `PRE_COMMIT_REMOTE_BRANCH` equals `refs/heads/main`, and pre-commit exports that variable only from the real pre-push git hook during an actual `git push`. Running the stage by hand — or from a CI runner — therefore reports it `Passed` having checked nothing. That is by design for feature branches — pushing WIP must not be blocked on cutting a premature tag — but it means `--hook-stage pre-push --all-files` is a full -rehearsal of 12 hooks and a skip of the thirteenth. The script's own header records the same gap for +rehearsal of 10 hooks and a skip of the eleventh. The script's own header records the same gap for a PR merged through Gitea's merge button, where no local push happens at all. ## The pre-push gate -Thirteen hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in. +Eleven hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in. **Core checks** @@ -51,7 +51,6 @@ Thirteen hooks, grouped below by what they guard rather than by the order `.pre- | Hook | Guards | |---|---| | `check-plugin-content-sync` | each plugin's flat `skills/agents/commands/hooks` mirror matches `.apm/` (issue #90) | -| `check-marketplace-mirror-sync` | `.github/plugin/marketplace.json` is byte-identical to `.claude-plugin/marketplace.json` — no apm output profile targets that path | | `check-vale-style-sync` | skill-audit's Vale copy matches agent-audit's canonical copy, plus six glob-coverage probes (see [Vale](#vale)) | | `check-scope-walkup-sync` | `validate.sh`, `validate-provenance.sh`, `new-agent.sh` and `new-skill.sh`'s four independent `$HOME`/`.git`/`apm.yml` walk-up ports still agree behaviorally | | `check-executables-allow-sync` | root `apm.yml`'s `executables.allow` key names kyberforge's actual version (see [apm gates](#apm-gates)) | @@ -69,7 +68,6 @@ drift in generated text. | Hook | Guards | |---|---| -| `apm-marketplace-check` | every `marketplace.packages[]` entry resolves, including network reachability of remote refs | | `apm-audit-ci` | `apm audit --ci` once per manifest — root plus each of the six plugin packages | | `apm-pack-check-clean` | `apm pack --check-versions --check-clean --dry-run` — the compiled marketplace still matches what `apm.yml` + `.apm/` would generate, and per-package versions agree with the `per_package` strategy | @@ -86,10 +84,10 @@ drift in generated text. |---|---| | `check-release-needed` | on a real `git push` to `main` only — fails if files exposed via `.pre-commit-hooks.yaml` changed since the last tag. A no-op everywhere else, including under `pre-commit run --hook-stage pre-push` (see [the caveat above](#running-the-gates)) | -Four of these shell out to `apm`: `apm-marketplace-check`, `apm-audit-ci`, `apm-pack-check-clean`, -and `check-plugin-content-sync` (via `scripts/sync-plugin-content.sh`, which wraps `apm pack`). The -first and third are bare `apm …` entries and the second is a `bash -c` loop calling `apm` once per -package, so without the CLI the push dies with an unhelpful "command not found". Install with +Three of these shell out to `apm`: `apm-audit-ci`, `apm-pack-check-clean`, and +`check-plugin-content-sync` (via `scripts/sync-plugin-content.sh`, which wraps `apm pack`). The +second is a bare `apm …` entry and the first is a `bash -c` loop calling `apm` once per package, so +without the CLI the push dies with an unhelpful "command not found". Install with `apm-install`, or `curl -sSL https://aka.ms/apm-unix | sh`; verify with `apm --version`. `jq` is needed by `scripts/sync-plugin-content.sh` — it at least fails loudly (`Error: jq is required but not installed`). @@ -889,8 +887,7 @@ fix. ## Pushing without a network No pre-push hook needs the network. Every entry in root `apm.yml`'s `marketplace.packages[]` -resolves from a local `./plugins/` path, so `apm-marketplace-check` and `apm-pack-check-clean` -never call `git ls-remote`. +resolves from a local `./plugins/` path, so `apm-pack-check-clean` never calls `git ls-remote`. `apm-audit-ci` calls `apm` too but was always local: its org-policy discovery resolves nothing on this remote before any network call. diff --git a/scripts/sync-marketplace-mirror.sh b/scripts/sync-marketplace-mirror.sh deleted file mode 100755 index 5587117..0000000 --- a/scripts/sync-marketplace-mirror.sh +++ /dev/null @@ -1,81 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -# .claude-plugin/marketplace.json is apm's compiled Claude marketplace output (see -# apm.yml's marketplace.outputs.claude). GitHub Copilot CLI's manifest lookup accepts -# that same file at .claude-plugin/marketplace.json directly, but also has a legacy -# convention path at .github/plugin/marketplace.json (see -# plugins/kyberforge/docs/research/docs/github-copilot-plugins/marketplace.md) -- and -# that path is a mirror of the Claude output, not a separate apm output profile (this -# repo declares only the "claude" output). This script keeps that legacy mirror -# byte-identical to .claude-plugin/marketplace.json instead of letting it silently -# drift (see issue #90 comment thread). - -# Hard error, not a `|| pwd` fallback. Every path this script touches hangs off -# REPO_ROOT, and both of its exits-0 paths are "the files agree" or "neither file -# exists" -- so a REPO_ROOT pointing somewhere that is not this repo reports "no -# drift" over a tree it never looked at. Run `--check` from an empty directory -# outside any worktree and the fallback made that the literal outcome: rev-parse -# failed, REPO_ROOT became $PWD, neither file was there, exit 0. Refusing to guess -# is the only answer that cannot be silently wrong; the `-f "$DST"` branch below -# covers a genuinely stale mirror, which is a different condition. -if ! REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || [[ -z "$REPO_ROOT" ]]; then - echo "Error: not inside a git worktree -- cannot locate the repository root, and guessing \$PWD would let --check report \"no drift\" over a tree it never inspected. Run this from within the repository." >&2 - exit 1 -fi -SRC="$REPO_ROOT/.claude-plugin/marketplace.json" -DST="$REPO_ROOT/.github/plugin/marketplace.json" - -usage() { - echo "Usage: $0 [--check]" >&2 - exit 1 -} - -CHECK=0 -if [[ "${1:-}" == "--check" ]]; then - CHECK=1 - shift -fi -[[ $# -eq 0 ]] || usage - -if [[ ! -f "$SRC" ]]; then - # A missing source with a surviving mirror is drift, not absence: the mirror - # can only be stale (nothing is left for it to be byte-identical to), which is - # precisely the silent divergence this script exists to prevent. Exiting 0 - # here would report "no drift" over a mirror of a file that no longer exists. - # (An unresolvable REPO_ROOT is handled above and is a hard error; this branch - # is only about a source file that is genuinely gone from a real worktree.) - # scripts/sync-plugin-content.sh --check --all already errors on the same - # condition ("requires .../marketplace.json"); this matches it. - # Neither file present stays a genuine no-op: nothing to mirror, nothing stale. - if [[ "$CHECK" -eq 1 && -f "$DST" ]]; then - echo "DRIFT $DST: mirror exists but .claude-plugin/marketplace.json does not" >&2 - echo "Fix: restore .claude-plugin/marketplace.json (apm's compiled Claude marketplace output), or delete $DST" >&2 - exit 1 - fi - exit 0 -fi - -if [[ "$CHECK" -eq 1 ]]; then - if [[ ! -f "$DST" ]] || ! diff -q "$SRC" "$DST" >/dev/null 2>&1; then - echo "DRIFT $DST: out of sync with .claude-plugin/marketplace.json" >&2 - # The runnable command gets a line to ITSELF, and the rationale gets its own - # echo. It was one line -- `Fix: bash scripts/sync-marketplace-mirror.sh -- - # apm ships no output profile...` -- which put the prose after `--`, the - # POSIX end-of-options marker, so copy-pasting the Fix line ran this script - # with ~24 stray argv entries: `${1:-}` was `--` (so CHECK stayed 0 and no - # shift happened), `[[ $# -eq 0 ]]` failed, and the tool meant to fix the - # drift answered with its own usage error and exit 1. The backticks around - # `apm pack` made it worse: the paste also command-substituted a real - # `apm pack` run before the script was even reached. Hence plain quotes - # below too. Keep the command alone on its line. - echo "Fix: run, from the repository root:" >&2 - echo " bash scripts/sync-marketplace-mirror.sh" >&2 - echo "Note: apm ships no output profile targeting this path, so 'apm pack' does not refresh it. Expecting it to is exactly the drift this script and its pre-push hook exist to prevent." >&2 - exit 1 - fi - exit 0 -fi - -mkdir -p "$(dirname "$DST")" -cp "$SRC" "$DST" diff --git a/scripts/sync-plugin-content.sh b/scripts/sync-plugin-content.sh index 7d7f4b6..bc1584c 100755 --- a/scripts/sync-plugin-content.sh +++ b/scripts/sync-plugin-content.sh @@ -196,9 +196,9 @@ if [[ "$ALL" -eq 1 ]]; then # duplicate at every call site (see .pre-commit-config.yaml's # check-plugin-content-sync). # - # Hard error rather than a `|| pwd` fallback, on scripts/sync-marketplace-mirror.sh's - # reasoning: --all's entire work list hangs off REPO_ROOT, so a REPO_ROOT pointing at - # something that is not this repo checks a plugin set that is not this repo's. Run + # Hard error rather than a `|| pwd` fallback: --all's entire work list hangs off + # REPO_ROOT, so a REPO_ROOT pointing at something that is not this repo checks a + # plugin set that is not this repo's. Run # from outside a worktree the fallback happens to hit the `--all requires ...` error # below instead -- but only by accident, because $PWD had no marketplace.json in it; # $PWD holding an unrelated one is the case that would silently "pass". diff --git a/tests/test-sync-marketplace-mirror.sh b/tests/test-sync-marketplace-mirror.sh deleted file mode 100755 index 416ba13..0000000 --- a/tests/test-sync-marketplace-mirror.sh +++ /dev/null @@ -1,304 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -SCRIPT="$REPO_ROOT/scripts/sync-marketplace-mirror.sh" -PASS=0 -FAIL=0 - -pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } -fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } - -# Fixture: a temp directory that is its own git worktree. Since -# scripts/sync-marketplace-mirror.sh resolves REPO_ROOT via -# `git rev-parse --show-toplevel 2>/dev/null || pwd`, isolation has to come -# from that call answering "the fixture" -- so the fixture owns a real .git. -# -# Relying instead on `git rev-parse` FAILING inside a bare `mktemp -d` (its -# previous form) is not isolation: two ordinary conditions make it succeed and -# resolve to the LIVE repo, at which point every test writes to this repo's own -# tracked .github/plugin/marketplace.json. Both are reproduced and fixed here: -# - TMPDIR pointing inside a git worktree, which puts the fixture in one. -# - An inherited GIT_DIR/GIT_WORK_TREE, which re-targets `git -C` and -# `git rev-parse` regardless of cwd. tests/run-tests.sh is itself a pre-push -# hook, and git hooks export exactly those variables -- the same leak -# tests/test-git-hooks-install.sh:6-10 already defends against. -# run_script() strips GIT_DIR/GIT_WORK_TREE for the second; `git init` here -# covers the first and makes the resolution positive rather than accidental. -# run-tests.sh:52-53 asserts no test writes back into the live repo tree, and -# that claim now carries the concurrent runner. -make_fixture() { - local dir - dir="$(mktemp -d)" - # Physical path: `git rev-parse --show-toplevel` reports the resolved path, - # and on macOS `mktemp -d` hands back one under the /tmp -> /private/tmp - # symlink. Without -P the script's REPO_ROOT and the assertions' $FIXTURE - # would name the same directory differently and every diff would compare - # against a path the script never wrote. - dir="$(cd "$dir" && pwd -P)" - # Checked, not best-effort: this init IS the isolation invariant. `echo "$dir"` - # is the last statement, so a silently-failed init would return a plain temp - # dir, `git rev-parse --show-toplevel` would walk up to whatever repo encloses - # it, and the suite would go back to writing into the live tree. - if ! env -u GIT_DIR -u GIT_WORK_TREE git -C "$dir" init -q >/dev/null 2>&1; then - echo "make_fixture: 'git init' failed in $dir — every test would then resolve REPO_ROOT to an enclosing repo and write outside the fixture" >&2 - exit 1 - fi - echo "$dir" -} - -SRC_REL=".claude-plugin/marketplace.json" -DST_REL=".github/plugin/marketplace.json" -CONTENT_A='{"plugins":[{"name":"demo","version":"1.0.0"}]}' -CONTENT_B='{"plugins":[{"name":"demo","version":"2.0.0"}]}' - -write_src() { - local dir="$1" content="$2" - mkdir -p "$dir/.claude-plugin" - printf '%s' "$content" > "$dir/$SRC_REL" -} - -write_dst() { - local dir="$1" content="$2" - mkdir -p "$dir/.github/plugin" - printf '%s' "$content" > "$dir/$DST_REL" -} - -# `env -u GIT_DIR -u GIT_WORK_TREE` mirrors tests/test-git-hooks-install.sh:10: -# under a git hook (run-tests.sh runs as pre-push) those are exported, and the -# script's `git rev-parse --show-toplevel` would then answer with the LIVE repo -# no matter which directory it was invoked from. -run_script() { - local dir="$1" - shift - (cd "$dir" && env -u GIT_DIR -u GIT_WORK_TREE bash "$SCRIPT" "$@") -} - -CLEANUP_DIRS=() -trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT -track() { CLEANUP_DIRS+=("$1"); } - -# --- 1. No .claude-plugin/marketplace.json at all: real-sync mode is a no-op, exit 0 --- -echo "" -echo "--- missing source: real sync exits 0 and creates nothing ---" -FIXTURE1="$(make_fixture)"; track "$FIXTURE1" -if run_script "$FIXTURE1" > /dev/null 2>&1 && [[ ! -e "$FIXTURE1/$DST_REL" ]]; then - pass "missing source exits 0 in real-sync mode without creating a mirror" -else - fail "missing source should exit 0 and create nothing in real-sync mode" -fi - -# --- 2. No source: --check mode is also a no-op, exit 0 (nothing to drift-check) --- -echo "" -echo "--- missing source: --check exits 0 (nothing to compare) ---" -FIXTURE2="$(make_fixture)"; track "$FIXTURE2" -if run_script "$FIXTURE2" --check > /dev/null 2>&1; then - pass "missing source exits 0 in --check mode" -else - fail "missing source should exit 0 in --check mode, not report drift" -fi - -# --- 2b. Source missing but a mirror still present: --check must FAIL --- -# --check used to exit 0 on any missing source, so deleting -# .claude-plugin/marketplace.json left a stale .github/plugin/marketplace.json -# reported as "no drift" -- a mirror of a file that no longer exists. That is -# the silent divergence this script's header says it prevents ("keeps that -# legacy mirror byte-identical ... instead of letting it silently drift"), and -# scripts/sync-plugin-content.sh --check --all already errors on the same -# condition. Case 2 above still holds: neither file present stays a no-op. -echo "" -echo "--- missing source with a surviving mirror: --check reports drift ---" -FIXTURE2B="$(make_fixture)"; track "$FIXTURE2B" -write_dst "$FIXTURE2B" "$CONTENT_A" -if run_script "$FIXTURE2B" --check > /dev/null 2>&1; then - fail "exited 0 with a stale mirror and no source -- expected drift (exit 1)" -else - pass "a mirror with no source left to mirror is reported as drift" -fi -# Real-sync mode keeps its no-op: it has nothing to copy, and deleting a -# tracked file is not this script's call to make. -if run_script "$FIXTURE2B" > /dev/null 2>&1 && [[ -f "$FIXTURE2B/$DST_REL" ]]; then - pass "real-sync mode still no-ops on a missing source, leaving the mirror alone" -else - fail "real-sync mode should no-op on a missing source, not fail or delete the mirror" -fi - -# --- 3. Source exists, mirror missing entirely: --check reports drift (exit 1) --- -echo "" -echo "--- --check reports drift when the mirror file does not exist yet ---" -FIXTURE3="$(make_fixture)"; track "$FIXTURE3" -write_src "$FIXTURE3" "$CONTENT_A" -if run_script "$FIXTURE3" --check > /dev/null 2>&1; then - fail "exited 0 with no mirror file present -- expected drift (exit 1)" -else - pass "missing mirror file is reported as drift" -fi - -# --- 4. Real sync creates the mirror (and its parent dir) byte-identical to the source --- -echo "" -echo "--- real sync creates .github/plugin/marketplace.json byte-identical to source ---" -FIXTURE4="$(make_fixture)"; track "$FIXTURE4" -write_src "$FIXTURE4" "$CONTENT_A" -if run_script "$FIXTURE4" > /dev/null 2>&1 \ - && [[ -f "$FIXTURE4/$DST_REL" ]] \ - && diff -q "$FIXTURE4/$SRC_REL" "$FIXTURE4/$DST_REL" > /dev/null 2>&1; then - pass "real sync creates the mirror file identical to the source" -else - fail "real sync did not create a byte-identical mirror file" -fi - -# --- 5. --check is clean immediately after a real sync --- -echo "" -echo "--- --check is clean right after syncing ---" -if run_script "$FIXTURE4" --check > /dev/null 2>&1; then - pass "no drift reported immediately after syncing" -else - fail "drift reported right after syncing -- sync and check disagree" -fi - -# --- 6. Editing the source after a sync introduces drift; re-sync clears it --- -echo "" -echo "--- source edited after sync is detected as drift, cleared by re-sync ---" -write_src "$FIXTURE4" "$CONTENT_B" -if run_script "$FIXTURE4" --check > /dev/null 2>&1; then - fail "no drift reported after editing the source -- expected drift" -else - pass "editing the source after a sync is detected as drift" - run_script "$FIXTURE4" > /dev/null 2>&1 - if run_script "$FIXTURE4" --check > /dev/null 2>&1; then - pass "re-sync clears the drift" - else - fail "re-sync did not clear the drift" - fi -fi - -# --- 7. A mirror that was hand-seeded already identical to the source: --check is clean --- -echo "" -echo "--- --check is clean when the mirror already matches, without ever syncing ---" -FIXTURE7="$(make_fixture)"; track "$FIXTURE7" -write_src "$FIXTURE7" "$CONTENT_A" -write_dst "$FIXTURE7" "$CONTENT_A" -if run_script "$FIXTURE7" --check > /dev/null 2>&1; then - pass "an already-in-sync mirror reports no drift" -else - fail "an already-in-sync mirror should report no drift" -fi - -# --- 8. A stale, pre-existing mirror that differs from source: --check flags it, sync fixes it --- -echo "" -echo "--- a stale pre-existing mirror is flagged by --check and overwritten by sync ---" -FIXTURE8="$(make_fixture)"; track "$FIXTURE8" -write_src "$FIXTURE8" "$CONTENT_A" -write_dst "$FIXTURE8" "$CONTENT_B" -if run_script "$FIXTURE8" --check > /dev/null 2>&1; then - fail "a stale mirror was not flagged as drift" -else - pass "a stale pre-existing mirror is flagged as drift" -fi -run_script "$FIXTURE8" > /dev/null 2>&1 -if diff -q "$FIXTURE8/$SRC_REL" "$FIXTURE8/$DST_REL" > /dev/null 2>&1; then - pass "real sync overwrites the stale mirror to match the source" -else - fail "real sync did not overwrite the stale mirror" -fi - -# --- 9. An unknown/extra positional argument is rejected with a usage error --- -echo "" -echo "--- an unrecognized argument is rejected (usage, exit 1) ---" -FIXTURE9="$(make_fixture)"; track "$FIXTURE9" -write_src "$FIXTURE9" "$CONTENT_A" -if run_script "$FIXTURE9" --bogus > /dev/null 2>&1; then - fail "exited 0 with an unrecognized argument -- expected a usage error" -else - pass "an unrecognized argument is rejected" -fi - -# --- 10. --check followed by a stray extra argument is also rejected --- -echo "" -echo "--- --check plus a trailing extra argument is rejected (usage, exit 1) ---" -if run_script "$FIXTURE9" --check extra > /dev/null 2>&1; then - fail "exited 0 with --check plus a trailing argument -- expected a usage error" -else - pass "--check plus a trailing extra argument is rejected" -fi - -# --- 11. Real sync is idempotent: running it twice in a row leaves the mirror unchanged --- -echo "" -echo "--- running real sync twice in a row is idempotent ---" -FIXTURE11="$(make_fixture)"; track "$FIXTURE11" -write_src "$FIXTURE11" "$CONTENT_A" -run_script "$FIXTURE11" > /dev/null 2>&1 -run_script "$FIXTURE11" > /dev/null 2>&1 -if run_script "$FIXTURE11" --check > /dev/null 2>&1; then - pass "running sync twice in a row is idempotent" -else - fail "a second sync run introduced unexpected drift" -fi - -# --- 12. Fixture isolation survives an inherited GIT_DIR/GIT_WORK_TREE --- -# The whole suite's isolation is REPO_ROOT resolving to the fixture. With -# GIT_DIR/GIT_WORK_TREE exported -- which is every git-hook context, and -# run-tests.sh runs as pre-push -- `git rev-parse --show-toplevel` answers with -# THAT repo from any cwd, so the script wrote to the live tree and 5 of these -# cases failed. Point both variables at a decoy repo (never the live one, so -# this assertion cannot itself write where it must not) and assert the fixture -# still wins: the mirror lands in the fixture and the decoy stays untouched. -echo "" -echo "--- fixture isolation holds with GIT_DIR/GIT_WORK_TREE inherited from elsewhere ---" -FIXTURE12="$(make_fixture)"; track "$FIXTURE12" -DECOY="$(make_fixture)"; track "$DECOY" -write_src "$FIXTURE12" "$CONTENT_A" -if (export GIT_DIR="$DECOY/.git" GIT_WORK_TREE="$DECOY"; run_script "$FIXTURE12" > /dev/null 2>&1) \ - && [[ -f "$FIXTURE12/$DST_REL" ]] && [[ ! -e "$DECOY/$DST_REL" ]]; then - pass "an inherited GIT_DIR/GIT_WORK_TREE does not redirect writes out of the fixture" -else - fail "an inherited GIT_DIR/GIT_WORK_TREE redirected the sync outside the fixture" -fi - -# --- 13. Outside any git worktree, REPO_ROOT cannot be guessed: hard error --- -# `git rev-parse --show-toplevel 2>/dev/null || pwd` used to fall back to $PWD. -# Both of this script's exit-0 paths are "the two files agree" or "neither file -# exists", so a REPO_ROOT that is not this repo reports "no drift" over a tree it -# never inspected — run --check from an empty non-worktree directory and that was -# the literal outcome. Case 2 above (a real worktree with no source file) still -# exits 0; the difference is whether the tree was identified at all. -# -# GIT_CEILING_DIRECTORIES rather than trusting `mktemp -d` to land outside a -# worktree: TMPDIR may itself sit inside one (the same hazard make_fixture's -# header documents), in which case rev-parse would succeed and this case would -# quietly test nothing. The ceiling stops git's upward walk at the fixture's -# parent, and the precondition below asserts it actually did. -echo "" -echo "--- outside a git worktree, --check errors instead of guessing \$PWD ---" -NOREPO_PARENT="$(mktemp -d)"; track "$NOREPO_PARENT" -NOREPO_PARENT="$(cd "$NOREPO_PARENT" && pwd -P)" -NOREPO="$NOREPO_PARENT/not-a-worktree" -mkdir -p "$NOREPO" -if (cd "$NOREPO" && env -u GIT_DIR -u GIT_WORK_TREE GIT_CEILING_DIRECTORIES="$NOREPO_PARENT" \ - git rev-parse --show-toplevel > /dev/null 2>&1); then - fail "precondition: git rev-parse still resolves a worktree under the ceiling — this case would test nothing" -else - for MODE in "--check" ""; do - RC13=0 - OUT13="$( (cd "$NOREPO" && env -u GIT_DIR -u GIT_WORK_TREE \ - GIT_CEILING_DIRECTORIES="$NOREPO_PARENT" bash "$SCRIPT" ${MODE:+"$MODE"}) 2>&1 )" || RC13=$? - case "$RC13:$OUT13" in - 0:*) - fail "'${MODE:-real sync}' exited 0 outside a git worktree — it reported on a tree it never identified" ;; - *"not inside a git worktree"*) - pass "'${MODE:-real sync}' errors with a not-a-worktree message instead of falling back to \$PWD" ;; - *) - fail "'${MODE:-real sync}' failed for an unexpected reason (rc=$RC13): $OUT13" ;; - esac - done - # And it must not have written anything into the directory it refused to trust. - if [[ ! -e "$NOREPO/.github" ]]; then - pass "nothing is written into the unidentified directory" - else - fail "the script created files under a directory it could not identify as the repo root" - fi -fi - -echo "" -echo "Results: $PASS passed, $FAIL failed" -[[ $FAIL -eq 0 ]] diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index b83e756..59a869a 100755 --- a/tests/test-sync-plugin-content.sh +++ b/tests/test-sync-plugin-content.sh @@ -1012,9 +1012,8 @@ make_repo_fixture() { printf '%s' "$marketplace_json" > "$dir/.claude-plugin/marketplace.json" echo "$dir" } -# `env -u GIT_DIR -u GIT_WORK_TREE` for the same reason -# tests/test-sync-marketplace-mirror.sh does it: run-tests.sh runs as a pre-push -# hook, and git hooks export both variables, which would re-target the script's +# `env -u GIT_DIR -u GIT_WORK_TREE`: run-tests.sh runs as a pre-push hook, and git +# hooks export both variables, which would re-target the script's # `git rev-parse --show-toplevel` at the LIVE repo from any cwd. run_all() { local dir="$1" @@ -1066,8 +1065,7 @@ check_all_fails_with "an entry whose source is an array" \ # --- 28b. --all outside a git worktree refuses instead of guessing $PWD --- # --all's entire work list hangs off REPO_ROOT, so a `|| pwd` fallback lets it derive -# that list from a marketplace.json belonging to some other tree. Same reasoning -# scripts/sync-marketplace-mirror.sh dropped its own fallback on. Run from a directory +# that list from a marketplace.json belonging to some other tree. Run from a directory # with no marketplace.json the old form happened to hit the "--all requires ..." error, # but only by accident — the dangerous case is a $PWD that HAS one. echo ""