From 4de5b6b355a3c67717215b1f652da49a78918551 Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Wed, 16 Sep 2026 11:52:14 +0000 Subject: [PATCH] chore(gates): retire the external pre-commit hook contract Why: .pre-commit-hooks.yaml and its release-tag gate served external consumers that do not exist. No repo on the Gitea instance pins these hooks, and the README names apm as the only supported install path. The mechanism was also already failing: skill-size-check.sh changed after v2.0.1 with no tag cut, and the gate cannot fire through Gitea's merge button. (Simplification audit finding 36.) Implementation Notes: - Delete .pre-commit-hooks.yaml, scripts/check-release-needed.sh, tests/test-check-release-needed.sh and tests/test-vale-hooks-consumer.sh, and remove the check-release-needed pre-push hook. The repo: local skill-size-check and vale-audit-prefilter-* hooks are unchanged. - ADR-0014 is amended, not retired: its runtime decision to bundle Vale inside factory-audit stands. The amendment keeps the entry[0]-only constraint (LESSONS.md:101,105) in case the export returns. ADR-0025 gets a pointer. - test-vale-wrap.sh: drop case 33 (the cross-manifest drift check) and case 28's hook-scope half, which read the published manifest. Case 32 now also requires each hook to select every tracked file of its class, which keeps case 33's one-plugin-narrowing guard, with a mutation test. - test-skill-size-check.sh and test-adr0020-contract.sh now assert the hook contract and verbose: true on .pre-commit-config.yaml only. - gates.md: pre-push count goes from 9 to 8 authored hooks (11 to 10 reported), and the Release table, the External consumers section and the two-manifest scope table are removed. README and script/test comments no longer describe the export as live. The resolver comment is edited identically in both copies. - The v1.0.0/v2.0.0/v2.0.1 tags are left in place; they are inert. ADR: 0014 Co-Authored-By: Claude Opus 5 (1M context) --- .pre-commit-config.yaml | 11 +- .pre-commit-hooks.yaml | 31 -- README.md | 7 +- ...14-vale-prefilter-ships-from-the-plugin.md | 56 +++ ...nd-agent-audit-merge-into-factory-audit.md | 5 + docs/spec/gates.md | 155 ++---- .../scripts/lib-boundary-resolver.sh | 4 +- .../skills/factory-audit/scripts/vale-wrap.sh | 9 +- .../tests/validate-provenance-skill.bats | 2 +- .../factory-audit/tests/validate-skill.bats | 4 +- scripts/check-release-needed.sh | 242 ---------- scripts/check-skill-version-bump.sh | 6 +- scripts/skill-size-check.sh | 11 +- tests/test-adr0020-contract.sh | 41 +- tests/test-adr0020-differential.sh | 2 +- tests/test-check-release-needed.sh | 449 ------------------ tests/test-skill-size-check.sh | 16 +- tests/test-skill-version-bump.sh | 2 +- tests/test-vale-hooks-consumer.sh | 276 ----------- tests/test-vale-wrap.sh | 346 ++++---------- 20 files changed, 233 insertions(+), 1442 deletions(-) delete mode 100644 .pre-commit-hooks.yaml delete mode 100755 scripts/check-release-needed.sh delete mode 100755 tests/test-check-release-needed.sh delete mode 100755 tests/test-vale-hooks-consumer.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a985ec6..4a4b561 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -183,15 +183,6 @@ repos: pass_filenames: false always_run: true - - id: check-release-needed - name: Check a release tag covers .pre-commit-hooks.yaml's paths - description: On push to main only, fail if files exposed via .pre-commit-hooks.yaml changed since the last tag - entry: bash scripts/check-release-needed.sh - language: system - stages: [pre-push] - pass_filenames: false - always_run: true - - id: check-skill-version-bump name: Check changed skills bump metadata.version description: On every push, fail if a skill directory changed (tests/ excluded) since the merge-base with main without its SKILL.md metadata.version rising (ADR-0022) @@ -203,7 +194,7 @@ repos: # Baseline is the merge-base with origin/main (falling back to main), # not the remote branch tip: readers install from main. Fails closed # when no main ref resolves. Merges through Gitea's merge button run no - # local hook, so they bypass this, just as they bypass check-release-needed. + # local hook, so they bypass this. - id: validate-marketplace name: Validate marketplace manifest diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml deleted file mode 100644 index 298b187..0000000 --- a/.pre-commit-hooks.yaml +++ /dev/null @@ -1,31 +0,0 @@ -# PUBLISHED CONTRACT. External repos consume these IDs with `rev: `, so an -# ID or a `files:` regex here may not change without breaking them on upgrade. -# ADR-0025 merged skill-audit and agent-audit into factory-audit and re-pointed -# both `entry:` paths at its single vale-wrap.sh; both IDs and both regexes are -# unchanged, deliberately. Collapsing them into one was considered and rejected: -# it breaks every consumer pinning kyberforge-vale-audit-agent, and it re-creates -# ADR-0014's measured failure where one hook against one config silently scanned -# 0 files of the other type. Two IDs are what keep both file scopes addressable. -- id: kyberforge-vale-audit-skill - name: Kyberforge Vale prose audit (SKILL.md) - description: Deterministic prose-pattern prefilter for kyberforge's factory-audit skill flow, via its own bundled Vale config/styles - entry: plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh - language: script - files: '(^|/)SKILL\.md$' - -- id: kyberforge-vale-audit-agent - name: Kyberforge Vale prose audit (agent files) - description: Deterministic prose-pattern prefilter for kyberforge's factory-audit agent flow, via its own bundled Vale config/styles - entry: plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh - language: script - files: '(^|/)agents/[^/]+\.md$|\.agent\.md$' - -- id: kyberforge-skill-size-check - name: SKILL.md size and context-budget ceilings - description: Enforce agentskills.io's 500-line/2,770-whole-file-word spec ceilings plus ADR-0020's context budget (description 250 chars SUGGESTION / 400 FAIL, body-only 600 words SUGGESTION / 900 FAIL, resolvable boundary-clause routing targets) - entry: scripts/skill-size-check.sh - language: script - files: '(^|/)SKILL\.md$' - # verbose so the SUGGESTION tier reaches a human -- pre-commit prints - # nothing for a passing hook, and a SUGGESTION deliberately does not fail. - verbose: true diff --git a/README.md b/README.md index 13d0159..1dcfa75 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ Install all of these before setting up. Each one is a hard dependency of a git h | --- | --- | --- | | `apm` CLI | Two pre-push hooks shell out to it (`apm-audit-ci` and `apm-pack-check-clean`) | The `apm-install` skill, or `curl -sSL https://aka.ms/apm-unix \| sh`. Verify with `apm --version` | | `python3` + PyYAML | Required by `scripts/skill-size-check.sh` (the `skill-size-check` pre-commit hook) and `scripts/check-skill-version-bump.sh` (the `check-skill-version-bump` pre-push hook), which both parse 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 by the `test-vale-wrap.sh` / `test-vale-hooks-consumer.sh` suites that `run-tests --strict` runs at pre-push | `brew install vale` (macOS), `snap install vale` (Linux), `choco install vale` (Windows), or https://vale.sh/docs/vale-cli/installation/ | +| `vale` | Required by the `vale-audit-prefilter-skill` / `-agent` pre-commit hooks, and by the `test-vale-wrap.sh` suite that `run-tests --strict` runs at pre-push | `brew install vale` (macOS), `snap install vale` (Linux), `choco install vale` (Windows), or https://vale.sh/docs/vale-cli/installation/ | | `claude` CLI | Required by the `validate-marketplace` pre-push hook | Claude Code | Two notes worth reading before you skip one: @@ -84,11 +84,6 @@ Run the pre-push gate locally in one command: pre-commit run --hook-stage pre-push --all-files ``` -One caveat: `check-release-needed` is a silent no-op under this invocation. It exits 0 unless -`PRE_COMMIT_REMOTE_BRANCH` is `refs/heads/main`, and pre-commit exports that only from the real -pre-push git hook during an actual `git push` — so the hook reports `Passed` having checked nothing. -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-pack-check-clean` resolves everything from local sources. All pre-push hooks pass offline. diff --git a/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md b/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md index 1684e9e..1204a78 100644 --- a/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md +++ b/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md @@ -27,6 +27,14 @@ them by name — and the argument-free `entry:` contract is untouched. Read the sync-check paragraph, and the `tests/test-vale-wrap.sh` Consequences bullet below as the state this ADR established, not as current layout. +**Amended (2026-09-16): the `.pre-commit-hooks.yaml` export and its release tags are retired.** +The runtime half of this ADR — Vale config, styles and wrapper bundled inside the skill (now +`factory-audit`), self-located from `${BASH_SOURCE[0]}` — stands. The external git-hook/CI half does +not: the manifest, `check-release-needed` and the tag-cutting consequence are gone. See +[the amendment at the end of this file](#amendment-2026-09-16-the-external-hook-contract-is-retired) +before reading any paragraph below that names `.pre-commit-hooks.yaml`, a `rev:` tag, or +`check-release-needed` as current. + `skill-audit`/`agent-audit`'s Step 1 called `"$(git rev-parse --show-toplevel)/scripts/vale-wrap.sh" --config "$(git rev-parse --show-toplevel)/.vale.ini"` — which resolves to whichever repo the skill happens to be running in. Inside `ai-development` @@ -219,3 +227,51 @@ only when the original span is already two or more lines, so the pad count stays `tests/test-vale-wrap.sh` case 20 asserts an apostrophe-bearing token actually fires on a flattened description in all three apostrophe-carrying branches, and case 20b pins the pad arithmetic against a body line's true line number. + +## Amendment (2026-09-16): the external hook contract is retired + +Root `.pre-commit-hooks.yaml`, `scripts/check-release-needed.sh`, +`tests/test-check-release-needed.sh` and `tests/test-vale-hooks-consumer.sh` are deleted, and the +`check-release-needed` pre-push hook is removed from `.pre-commit-config.yaml`. The three exported +hook IDs — `kyberforge-vale-audit-skill`, `kyberforge-vale-audit-agent` and +`kyberforge-skill-size-check` — no longer exist, and no new `vX.Y.Z` tag is cut when hook files +change. (Simplification audit finding 36.) + +Three reasons, any one of which would have been enough to ask the question: + +- **No consumer was found.** The Gitea instance holds two repos. The other one pins seven hook + repos, and none of them is this one. None of the 13 commits that touched the mechanism came from a + consumer report; all were found by this repo's own tests. Clones outside the instance cannot be + counted, but ADR-0024 accepted the same standard when it deleted the mirror. +- **The mechanism was already failing at its one job.** `scripts/skill-size-check.sh` changed on + `main` after `v2.0.1`, and no tag was cut, so a consumer pinning `rev: v2.0.1` already ran a + stale hook. The gate could not have caught it. It acted only when pre-commit reported a push to + `refs/heads/main`, and PRs here merge through Gitea's server-side merge button, which runs no + local hook. The script's own header said that closing the gap needed a server-side CI job the + repo does not have. +- **The README already contradicted it.** Its "For external consumers" section says apm is the only + supported install path and never mentions `.pre-commit-hooks.yaml` or `rev:` pinning. + +**What is unaffected.** This repo's `repo: local` hooks — `skill-size-check`, +`vale-audit-prefilter-skill` and `vale-audit-prefilter-agent` — were always wired separately from +the export, so no internal lint coverage is lost. The two prefilter hook IDs stay separate for the +file-scope reason in "One hook per file-scope" above, not for an external contract. +`tests/test-vale-wrap.sh` case 33, the cross-manifest `files:` drift check that ADR-0025 ported, +went with the manifest it compared against. Its one guard that did not need a second manifest, a +local regex narrowed to a single plugin, is now a third property of case 32. The `v1.0.0`, `v2.0.0` +and `v2.0.1` tags are left in place. They are inert: nothing reads them, and apm's `per_package` +versioning never consults `tagPattern`. + +**What is preserved for a return.** The `entry[0]`-only constraint in the Consequences above, and +its incident records at `LESSONS.md:101` and `:105`, stay as written. That constraint says a +published entry is a bare script path, with every bundled file located from `${BASH_SOURCE[0]}`, +and it took three review rounds to find. Both hook scripts still meet it: `vale-wrap.sh` takes no +`--config`, and `skill-size-check.sh` keeps its embedded resolver copy. If a consumer appears, +restore the manifest under that constraint, and restore `test-vale-hooks-consumer.sh` with it: it +was the only test that exercised the entry-resolution path that once shipped broken. Restore a +release gate only once a server-side job can run it on merge. + +**Superseded statements elsewhere.** ADR-0022's notes that the version-bump gate "is not exported +through `.pre-commit-hooks.yaml`" and that it shares its gaps with `check-release-needed`, and +ADR-0025's point 5 ("Both exported Vale hook IDs survive unchanged") and its case-33 port, describe +the state before this amendment. diff --git a/docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md b/docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md index b60f317..6dcf571 100644 --- a/docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md +++ b/docs/adr/0025-skill-audit-and-agent-audit-merge-into-factory-audit.md @@ -5,6 +5,11 @@ exact pair, scoped itself to them, and then deferred the work as issue #101. The here. `skill-author` and `agent-author` stay separate — ADR-0020 excluded the author pair deliberately, and nothing in this change touches that exclusion. +**Amended by ADR-0014 (2026-09-16).** The published `.pre-commit-hooks.yaml` is retired. Point 5 +below (both exported hook IDs survive) and the case 33 port no longer describe the repo. Case 33 is +deleted, and its one-plugin narrowing guard is now a property of case 32. See +[ADR-0014's amendment](0014-vale-prefilter-ships-from-the-plugin.md#amendment-2026-09-16-the-external-hook-contract-is-retired). + ## Context Every figure below was measured against the worktree on 2026-09-15. Re-derive rather than quote; the diff --git a/docs/spec/gates.md b/docs/spec/gates.md index 040b2f6..4df806e 100644 --- a/docs/spec/gates.md +++ b/docs/spec/gates.md @@ -21,37 +21,28 @@ 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 **11** hooks, not 9. The extra two are pre-commit's own `meta` hooks, +The pre-push command reports **10** hooks, not 8. 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`. Nine -is the count of hooks this repo authors itself. - -**The caveat: one of those 9 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 8 hooks and a skip of the ninth. 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. +else — what separates them is `repo: meta` (pre-commit's own built-ins) from `repo: local`. Eight +is the count of hooks this repo authors itself, and `--hook-stage pre-push --all-files` is a full +rehearsal of all eight. A PR merged through Gitea's merge button runs none of them: no local push +happens at all. A real push has a gap of its own. When one `git push` carries several refs (`git push origin a b`, `git push --all`), pre-commit runs the pre-push stage once, for one ref. In pre-commit 4.6.1, `_pre_push_ns` in `hook_impl.py` skips delete lines and returns on the first remaining ref whose remote sha is non-zero and present locally; a ref whose remote sha is zero or unknown locally is returned only if it has commits that no remote-tracking ref of that remote has. -The two hooks that read the pushed ref — `check-release-needed` and -`check-skill-version-bump` — therefore check only that ref, and the others are pushed unchecked. +The one hook that reads the pushed ref, `check-skill-version-bump`, therefore checks only that +ref, and the others are pushed unchecked. When that ref's unpushed history reaches a root commit, pre-commit runs with all files and sets no `PRE_COMMIT_TO_REF`, so `check-skill-version-bump` checks `HEAD`, which is the pushed ref only if it -is checked out. -For `check-release-needed`, a multi-ref push whose first ref is not `main` never gates `main` at -all. Push one ref at a time when the gate matters. +is checked out. Push one ref at a time when the gate matters. ## The pre-push gate -Nine hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in. +Eight hooks, grouped below by what they guard rather than by the order `.pre-commit-config.yaml` declares them in. **Core checks** @@ -94,12 +85,6 @@ drift in generated text. |---|---| | `check-skill-version-bump` | fails if a skill directory changed since the pushed commit's merge-base with `main` without its `metadata.version` rising above both the merge-base's and `main`'s tip's (see [below](#check-skill-version-bump)) | -**Release** - -| Hook | Guards | -|---|---| -| `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)) | - Two of these shell out to `apm`: `apm-audit-ci` and `apm-pack-check-clean`. 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 @@ -111,8 +96,7 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu `skill-size-check` only checks the field's presence and shape, so this hook holds the bump itself. - **It runs on every push and under a manual `pre-commit run --hook-stage pre-push`.** It does not - read `PRE_COMMIT_REMOTE_BRANCH`, so unlike `check-release-needed` the manual rehearsal really - checks it. The pushed commit is `PRE_COMMIT_TO_REF`, or `HEAD` when that is unset. + read `PRE_COMMIT_REMOTE_BRANCH`, so the manual rehearsal really checks it. The pushed commit is `PRE_COMMIT_TO_REF`, or `HEAD` when that is unset. - **"Changed" is measured from the merge-base of the pushed commit with `origin/main`** (local `main` if `origin/main` does not resolve). Readers install from `main`, so "changed" means changed against the `main` the branch started from. The remote branch tip is not the baseline: @@ -147,7 +131,7 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu - **It needs `python3` and PyYAML** to read the frontmatter, and fails with a clear message if either is missing, for the reasons in [`python3` and PyYAML are hard requirements](#python3-and-pyyaml-are-hard-requirements). -- **Known gaps, both shared with `check-release-needed`:** a PR merged through Gitea's merge button +- **Known gaps:** a PR merged through Gitea's merge button runs no local hook; and a multi-ref push checks only the one ref pre-commit selects, and a push reaching a root commit gets no `PRE_COMMIT_TO_REF`, so `HEAD` is checked (see [Running the gates](#running-the-gates)). @@ -157,9 +141,7 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu ## Skill and agent context gates (ADR-0020) The `skill-size-check` pre-commit hook, scoped to `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$`, -runs `scripts/skill-size-check.sh`. It is also shipped to external repos as -`kyberforge-skill-size-check` (see -[External consumers](#external-consumers-the-root-pre-commit-hooksyaml)). Besides the ADR-0020 +runs `scripts/skill-size-check.sh`. Besides the ADR-0020 gates below, it also asserts required frontmatter is present: `name`, a non-empty `description`, and a `metadata.version` matching three-part semver (`1.0.0`) — folded in from a formerly standalone `skill-frontmatter` hook that parsed the same fields with a shell script. @@ -724,27 +706,20 @@ rule at a blocking bare `YES`/`error`. It is not redundant with the probes above (`DescriptionOpener`, `PaddingPhrase`, `SentenceOpenerThereIs`, `CompositionNote`) can each be overridden out of `error` underneath a passing probe. That gap is closed. -Two cases cover the hook manifests. +**Case 32** covers the prefilter hooks' own scope in `.pre-commit-config.yaml`, with three +properties. Each vale hook's `files:` regex must still match at least one tracked file; every path it +matches must be in that hook's own artifact class; and it must match **every** tracked file of that +class under `plugins/*/.apm/`. A hook narrowed to zero files never runs, and pre-commit reports no +error. A hook narrowed to one plugin still matches files of the right class, which is why the third +property exists: narrowing `vale-audit-prefilter-skill` from `^plugins/[^/]+/...` to +`^plugins/kyberforge/...` once left 6 of 38 skills prefiltered and the whole suite green. Part B +narrows both regexes to zero files and Part C narrows both to one plugin, each in a copy of the +config, and requires Part A to fail by hook name. -**Case 33** is the original's cross-manifest `files:` drift check, ported. It extracts each vale -hook's `files:` regex from `.pre-commit-hooks.yaml` and from `.pre-commit-config.yaml` -*independently*, compares them per hook and never as a union, and asserts that each shared probe path -is in scope of both or neither. The original selected each hook's record by matching `entry:` -against the owning skill's `vale-wrap.sh` path. After the merge both hook IDs share one `entry:`, so -the port pairs them by `id:` from an explicit table: `kyberforge-vale-audit-skill` ↔ -`vale-audit-prefilter-skill`, and `kyberforge-vale-audit-agent` ↔ `vale-audit-prefilter-agent`. A -missing hook id or a class with no shared probe fails by name. Part B requires three mutations to -fail: the skill hook narrowed to one plugin, the agent hook narrowed the same way, and a renamed -local hook id. - -This was briefly a real hole. Narrowing `vale-audit-prefilter-skill` from `^plugins/[^/]+/...` to -`^plugins/kyberforge/...` left 6 of 38 skills prefiltered, and the whole suite green, before case 33 -existed. - -**Case 32** covers the separate zero-match question on the local manifest alone. Each -`.pre-commit-config.yaml` vale hook's `files:` regex must still match at least one tracked file, and -every path it matches must be in that hook's own artifact class. A hook narrowed to zero files never -runs, and pre-commit reports no error. +Case 32 used to have a partner, case 33, which compared each hook's `files:` regex against the +published `.pre-commit-hooks.yaml`. It went with that manifest (ADR-0014's 2026-09-16 amendment), +and its one guard that did not need a second manifest, the one-plugin narrowing, is case 32's third +property. **Case 34** asks, statically and with no Vale binary, whether the shipped `.vale.ini` could load a style at all. Four assertions: every `[glob]` section declares a **non-empty** `BasedOnStyles`; every @@ -753,8 +728,8 @@ absolute**; and at least one `[glob]` section exists, so the check cannot pass v with nothing in it. A section whose `BasedOnStyles` is empty is the silent case — Vale lints every file that glob matches with no rule loaded, prints `0 errors` and exits 0. The absolute-path clause is the one that is not obvious: an absolute `StylesPath` passes on the machine that wrote it and -hard-fails for every external consumer of `.pre-commit-hooks.yaml`, which is the only reason those -styles ship at all. Part B is a mutation self-test against the same function Part A calls — it empties +hard-fails for every repo that installs `factory-audit`, which is the only reason those styles ship +with the skill at all. Part B is a mutation self-test against the same function Part A calls — it empties each section's `BasedOnStyles` in a copy of the assets, and absolutizes `StylesPath` in another pointed at that copy's own real `styles/` directory, and requires each to fail by name. @@ -805,27 +780,6 @@ from the hook definitions. Under this model they are no-ops; adding one is not a The `verbose: true` escape hatch that makes `skill-size-check`'s SUGGESTION tier audible has no analogue here — Vale has no tier to make audible. -### External consumers: the root `.pre-commit-hooks.yaml` - -The root `.pre-commit-hooks.yaml` exposes two Vale hook IDs (`kyberforge-vale-audit-skill`, -`kyberforge-vale-audit-agent`) plus `kyberforge-skill-size-check`, so any external repo can enforce -the same rules with `repo: , rev: ` in its own `.pre-commit-config.yaml`. -pre-commit clones the pinned rev into its own cache, independent of whether Claude Code or the -`kyberforge` plugin is installed at all; the same mechanism covers CI via `pre-commit run ---all-files`. `skill-size-check` has no external asset dependency, so it needed no relocation under -ADR-0014 — only exposure. - -**The two IDs survive the merge even though they now point at the same wrapper.** Both -`kyberforge-vale-audit-skill` and `kyberforge-vale-audit-agent` keep their IDs and their `files:` -regexes, because an external repo pins them by name in its own `.pre-commit-config.yaml` and -collapsing them to one would break every such consumer silently. What changed is only the `entry:` -target: both now name `factory-audit/scripts/vale-wrap.sh`. - -This repo's own `vale-audit-prefilter-skill` / `-agent` hooks consume the **identical** -plugin-bundled copy via `repo: local`. Deliberately not a second root copy, and deliberately **not a -pinned self-reference** — a pinned self-reference would lint working-tree edits against the last -tagged release rather than against the change being made. - ### Pre-commit Two prefilter hooks, with `.apm/`-scoped `files:` patterns: @@ -845,9 +799,10 @@ authors. Without the binary the hooks fail with a bare "command not found" and n reason was mechanical: with a config per skill, a single hook could point at only one copy and would silently 0-file-skip the other file shape (see [A 0-file Vale run is NOT RUN](#a-0-file-vale-run-is-not-run)). One `.vale.ini` carrying all three -sections removes that constraint. The split stays anyway because the two IDs are an exported -contract external consumers pin by name, and because the `files:` regexes still have to differ — -each hook hands Vale only the file shape it is scoped to. +sections removes that constraint. The split stays anyway because the `files:` regexes still have to +differ — each hook hands Vale only the file shape it is scoped to. Both hooks name the same +plugin-bundled `factory-audit/scripts/vale-wrap.sh` through `repo: local`; there is no second root +copy. ### The `.vale.ini` globs do no scoping @@ -856,15 +811,12 @@ The `.vale.ini`'s section globs are **path-agnostic** — `[**/SKILL.md]`, `[**/ location: Vale's `*` crosses `/`. A `SKILL.md` outside `plugins/` (a project-scope `.claude/skills/foo/SKILL.md`, say) still matches `[**/SKILL.md]` and gets linted normally. -All scoping therefore comes from the pre-commit hook's own `files:` regex and from `factory-audit` -passing one explicit file per invocation. The two manifests scope **differently on purpose**: +All scoping therefore comes from the pre-commit hooks' own `files:` regexes, which pin this repo's +layout (see [Pre-commit](#pre-commit)), and from `factory-audit` passing one explicit file per +invocation — in this repo or in any repo that installs it, whatever that repo's layout. -| Manifest | `-skill` | `-agent` | -|---|---|---| -| `.pre-commit-config.yaml` (pins this repo's layout) | `^plugins/[^/]+/\.apm/skills/[^/]+/SKILL\.md$` | `^plugins/[^/]+/\.apm/agents/[^/]+\.agent\.md$` | -| `.pre-commit-hooks.yaml` (layout-agnostic for consumers) | `(^\|/)SKILL\.md$` | `(^\|/)agents/[^/]+\.md$\|\.agent\.md$` | - -Narrowing a `.vale.ini` glob to a `plugins/`-shaped path to "tighten" it breaks the consumer case. +Narrowing a `.vale.ini` glob to a `plugins/`-shaped path to "tighten" it breaks the consumer case: +`factory-audit` run against a project-scope `.claude/skills/` tree would lint nothing. `check-vale-style-sync`'s probe set was built to catch exactly that; it moved to `tests/test-vale-wrap.sh` with the hook's deletion, and two of the six probes exist specifically to pin this location independence — see [One copy, one config](#one-copy-one-config). @@ -897,10 +849,6 @@ config declines to match, which is the [0-file NOT RUN](#a-0-file-vale-run-is-no green run that measured nothing. **Issue #117** records the style-scope half; the hook half has to land in the same change or the fix is cosmetic. -The consumer manifest is a third axis and does not rescue this either: `.pre-commit-hooks.yaml`'s -`(^|/)SKILL\.md$` is layout-agnostic but still filename-shaped, so an external repo running -`kyberforge-vale-audit-skill` has the same gap. - ### `vale-wrap.sh`, never bare `vale` `factory-audit`'s Step 1 and both pre-commit hooks call @@ -925,19 +873,19 @@ wrapper a bad description in any of the three broken forms sailed straight throu ### The `--config` argv defect Handed **no `--config` at all**, the wrapper falls back to its own sibling `assets/vale/.vale.ini`, -located from `${BASH_SOURCE[0]}` rather than from the cwd. That is why both manifests' `entry:` is -now the bare script path with **no argument after it**. +located from `${BASH_SOURCE[0]}` rather than from the cwd. That is why both hooks' `entry:` is +the bare script path with **no argument after it**. pre-commit prefixes only `entry[0]` with the hook-repo clone path (`cmd = (prefix.path(cmd[0]), *cmd[1:])`), so every later argument resolves against the **consuming** repo's root. A `--config` in -`.pre-commit-hooks.yaml` therefore pointed at a path no consumer has and hard-failed every external -run with `E100 [--config] Runtime error`. +the since-retired `.pre-commit-hooks.yaml` therefore pointed at a path no consumer has and +hard-failed every external run with `E100 [--config] Runtime error`. -`.pre-commit-config.yaml` drops the argument too, deliberately keeping the two entries identical. +`.pre-commit-config.yaml` drops the argument too, deliberately matching that entry. The local `repo: local` hook resolved its `--config` correctly only because the consuming repo *was* this repo — and that divergence is why three review rounds exercised a path no external consumer -takes and missed the defect. **Do not reintroduce a `--config` to either manifest to make the local -run "explicit".** +takes and missed the defect. **Do not reintroduce a `--config` to either hook to make the local run +"explicit"**, and keep a restored published manifest to `entry[0]` alone (ADR-0014). An explicit `--config` from any other caller still wins, in all three argv forms (`--config X`, `--config=/abs`, `--config=rel`), and a relative one resolves against the caller's cwd — matching @@ -973,12 +921,12 @@ clean. `CHECK_VALE_STYLE_SYNC_ALLOW_MISSING_VALE=1` opt-out downgraded them audibly rather than skipping the hook — is deleted with the second Vale copy (ADR-0025). The six glob probes survive it inside `test-vale-wrap.sh`, so `run-tests --strict` is now the gate that runs them. That is also what keeps -`vale` a pre-push requirement: `test-vale-hooks-consumer.sh` exits 77 without the binary, and so does -`test-vale-wrap.sh` once its static cases pass, and a skip fails the push. +`vale` a pre-push requirement: `test-vale-wrap.sh` exits 77 without the binary once its static cases +pass, and a skip fails the push. `test-vale-wrap.sh` without Vale skips only its Vale-dependent cases, not the whole suite. The cases -that are plain greps and awk over the config and the two hook manifests still run: case 0, 16, 26, -27, the static halves of 28, 31 Parts A and B, 32, 33 and 34. A static failure exits 1, because a +that are plain greps and awk over the Vale config and `.pre-commit-config.yaml` still run: case 0, 16, 26, +27, the static half of 28, 31 Parts A and B, 32 and 34. A static failure exits 1, because a real defect is not a setup error. Only an all-static-pass run exits 77. ### Mentioning banned phrasing without tripping the rule @@ -1005,10 +953,11 @@ run. The pre-push hook invokes the same script as `--strict` (`RUN_TESTS_STRICT= where a skip **does** fail the push: at pre-push a skip means one of the documented dependencies is absent on this machine, so the gate would otherwise report success having run fewer suites than it appears to. Without `--strict` the gate once went green having verified 15 of 17 suites on a -vale-less PATH, with the skip list swallowed. Without vale, two suites skip — -`test-vale-hooks-consumer.sh` and `test-vale-wrap.sh` — and the strict failure names each one and -what to install. (It was three until `test-check-vale-style-sync.sh` was deleted with its hook; see -[One copy, one config](#one-copy-one-config).) +vale-less PATH, with the skip list swallowed. Without vale, one suite skips — `test-vale-wrap.sh` — +and the strict failure names it and what to install. (It was three until +`test-check-vale-style-sync.sh` was deleted with its hook — see +[One copy, one config](#one-copy-one-config) — and `test-vale-hooks-consumer.sh` with the published +hook manifest.) **Output assertions use a here-string, never a pipe.** Write `grep -q PATTERN <<< "$OUT"`, not `echo "$OUT" | grep -q PATTERN`. Under `set -o pipefail` the pipe form fails depending on timing: diff --git a/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh b/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh index 24aaebf..02705d7 100755 --- a/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh +++ b/plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh @@ -45,8 +45,8 @@ IFS='' read -r -d '' KYBERFORGE_RESOLVER_PY <<'KYBERFORGE_ADR0020_RESOLVER_PY' | # plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh # The block between these markers must stay byte-identical in both. It is copied # rather than imported because a cache-installed plugin's scripts cannot read -# files outside their own plugin directory, and this repo-root hook resolves via -# .pre-commit-hooks.yaml, where entry[0] is the only token pre-commit rewrites -- +# files outside their own plugin directory, and this repo-root hook is kept fit for +# a published hook manifest (retired; ADR-0014), where only entry[0] is rewritten -- # so no single file is reachable by both (the same constraint that duplicates the # ADR-0020 constants). Edit one copy, then paste it over the other. # diff --git a/plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh b/plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh index dde287c..98c0fc7 100755 --- a/plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh +++ b/plugins/kyberforge/.apm/skills/factory-audit/scripts/vale-wrap.sh @@ -29,10 +29,11 @@ set -euo pipefail # # Divergence 1: with no `--config` at all, this script's own sibling # `assets/vale/.vale.ini` is used instead of vale's upward search. pre-commit -# prefixes only `entry[0]` with the hook-repo clone path, so a `--config` in -# `.pre-commit-hooks.yaml` would resolve against the *consuming* repo and -# hard-fail (E100) for every external consumer. The manifest therefore passes the -# script alone, and an explicit `--config` from any other caller still wins. +# prefixes only `entry[0]` with the hook-repo clone path, so a `--config` in a +# published `.pre-commit-hooks.yaml` would resolve against the *consuming* repo +# and hard-fail (E100) for every external consumer. That manifest is retired +# (ADR-0014, 2026-09-16 amendment), but the script still needs no `--config` so it +# can return; an explicit `--config` from any other caller still wins. # # Divergence 2: a path-shaped argument that does not exist is a hard error # (exit 2). Bare vale drops it, falls back to reading stdin, and prints diff --git a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats index 90b6a1e..03d8fe3 100644 --- a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats +++ b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-provenance-skill.bats @@ -1989,7 +1989,7 @@ EOF # NEW with the merge and additive: pre-merge, handing the SKILL.md itself to # skill-audit's validate-provenance.sh hit the "not a directory" precondition # and died. It matters because pre-commit `files:` hooks match FILES — the - # exported kyberforge-vale-audit-skill hook's regex is (^|/)SKILL\.md$ — so + # vale-audit-prefilter-skill hook's regex ends in /SKILL\.md$ — so # every hook-driven invocation hands over a SKILL.md path, never its # directory. The entry point rewrites the token to the directory in place. local skill="$TMPDIR/my-skill" diff --git a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-skill.bats b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-skill.bats index 925c4fd..f6983c7 100644 --- a/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-skill.bats +++ b/plugins/kyberforge/.apm/skills/factory-audit/tests/validate-skill.bats @@ -1016,8 +1016,8 @@ EOF # NEW with the merge and additive rather than ported: pre-merge, handing the # SKILL.md itself to skill-audit's validate.sh hit the directory precondition # and gave a useless exit 1. It matters because pre-commit `files:` hooks - # match FILES — the exported kyberforge-vale-audit-skill hook's regex is - # (^|/)SKILL\.md$ — so every hook-driven invocation hands over a SKILL.md + # match FILES — the vale-audit-prefilter-skill hook's regex ends in + # /SKILL\.md$ — so every hook-driven invocation hands over a SKILL.md # path, never the directory above it. The entry point resolves the file to # its directory before dispatching. local skill="$TMPDIR/my-skill" diff --git a/scripts/check-release-needed.sh b/scripts/check-release-needed.sh deleted file mode 100755 index ad2e094..0000000 --- a/scripts/check-release-needed.sh +++ /dev/null @@ -1,242 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -# Hard-fails only when pushing to main: if any file covered by .pre-commit-hooks.yaml -# (the external git-hook/CI contract, see ADR-0014) changed since the last tag, -# a release must be cut before landing on main, or external consumers pinning -# `rev: ` silently miss the change. Pre-commit sets PRE_COMMIT_REMOTE_BRANCH -# for pre-push hooks; on every other branch (feature work mid-review) this is a -# silent no-op — pushing WIP commits there must not be blocked on cutting a -# premature tag (see ADR-0014's repo: local vs pinned self-reference decision). -# -# Known gap: this only fires on a local `git push` through pre-commit's pre-push -# hook. A PR merged via Gitea's merge button (server-side, no local push) or a -# CI runner invoking `pre-commit run --hook-stage pre-push` directly does not set -# PRE_COMMIT_REMOTE_BRANCH and will not trigger this check — closing that -# requires a server-side CI job, which this repo does not have yet. - -TARGET_BRANCH="refs/heads/main" - -if [[ "${PRE_COMMIT_REMOTE_BRANCH:-}" != "$TARGET_BRANCH" ]]; then - exit 0 -fi - -# What is actually being pushed, which is only HEAD for the common -# `git push ` case. pre-commit's pre-push hook-impl -# exports the local sha of each pushed ref as PRE_COMMIT_TO_REF; a -# `git push topic:main` from a different checkout would otherwise be -# gated on the wrong tip — a false negative when HEAD is behind the pushed ref -# (unreleased changes sail through), a false positive when it is ahead. -# PRE_COMMIT_FROM_REF, the *remote's* current tip, is deliberately not used -# anywhere here: the baseline is the last release tag, not what the remote -# already has. Diffing from the remote tip would let an untagged -# release-relevant commit already on main excuse the next push from cutting a -# tag, which is precisely the drift this gate exists to catch. -PUSHED_REF="${PRE_COMMIT_TO_REF:-HEAD}" - -# pre-commit passes an all-zeros sha (40 hex zeros under sha1, 64 under sha256) -# as the "to" ref when the push deletes a branch. Nothing is being shipped, and -# every rev-taking command below would fail on an unresolvable sha, so bail out -# rather than turning a branch deletion into a confusing "could not diff". -if [[ "$PUSHED_REF" =~ ^0+$ ]]; then - exit 0 -fi - -REPO_ROOT="$(git rev-parse --show-toplevel)" -cd "$REPO_ROOT" - -HOOKS_MANIFEST=".pre-commit-hooks.yaml" - -if [[ ! -f "$HOOKS_MANIFEST" ]]; then - exit 0 -fi - -# Only vX.Y.Z release tags count as a baseline — an incidental checkpoint or -# experiment tag reachable from the pushed ref must not shift the diff baseline. -# The tag is resolved from $PUSHED_REF, not HEAD, for the same reason the diff -# is: a tag reachable only from HEAD is not part of the history being pushed. -# --match is a shell glob, not a regex: its trailing `*`s match any suffix, so -# without --exclude a pre-release/checkpoint tag like v1.2.3-checkpoint or -# v1.2.3-rc1 also satisfies 'v[0-9]*.[0-9]*.[0-9]*' and could be picked over the -# true last release tag. --exclude is glob syntax too, so '*-*' is what actually -# rules out any tag carrying a hyphenated suffix, leaving only bare vMAJOR.MINOR.PATCH. -LAST_TAG="$(git describe --tags --abbrev=0 --match 'v[0-9]*.[0-9]*.[0-9]*' --exclude '*-*' "$PUSHED_REF" 2>/dev/null || true)" - -if [[ -z "$LAST_TAG" ]]; then - echo "FAIL: no release tag exists yet, but .pre-commit-hooks.yaml already exposes hooks to external consumers." >&2 - echo " Fix: cut the first release tag (e.g. v1.0.0) before this lands on main." >&2 - exit 1 -fi - -# Derive release-relevant paths from .pre-commit-hooks.yaml's own entry: lines -# instead of hand-maintaining a parallel list — the manifest is the single -# source of truth for what external consumers actually pull at a pinned rev, -# so a hook added/removed/renamed there can't silently drift out of sync here. -# Everything is derived from tokens[0], the hook's script: pre-commit prefixes -# only entry[0] with the hook-repo clone path, so any later token that looks -# like a path resolves against the *consuming* repo and can never name a file -# this repo ships. A hook's bundled data therefore has to be self-located -# relative to the script — vale-wrap.sh reads its own -# /../assets/vale/.vale.ini plus the sibling styles/ tree — which -# makes /../assets release-relevant alongside the script itself. -# The ../ is normalised by stripping a path component rather than with -# `realpath -m`, which is a GNU-only extension. Two guards keep the derivation -# from inventing paths: a bundle root of "." is skipped, because a script in a -# top-level directory (scripts/skill-size-check.sh) would derive the repo's own -# shared assets/, which no hook owns and whose churn must not demand a release; -# and the assets/ directory is added only where it is known to exist, since a -# hook that bundles nothing must not contribute a pathspec matching nothing. -RELEASE_PATHS=("$HOOKS_MANIFEST") - -add_release_path() { - local candidate="$1" existing - for existing in "${RELEASE_PATHS[@]}"; do - [[ "$existing" == "$candidate" ]] && return 0 - done - RELEASE_PATHS+=("$candidate") -} - -# Emits one "" line per hook so a rejected entry can -# name the hook a human has to go fix. The id sits on its own line above its -# entry: in YAML, so it is carried forward and then cleared; a hook that somehow -# has no id still reports something printable rather than an empty name. Kept in -# bash rather than awk: matching `[[:space:]]` inside a bracket expression is -# reliable in bash's own globs but not in the BWK awk macOS ships. `read -r` with -# a single variable is the trimmer — it strips leading and trailing whitespace -# while preserving anything in between, so a multi-token entry survives intact -# for the error message to quote back. -manifest_entries() { - local line id="" value - while IFS= read -r line; do - # Drop the indentation and the optional list dash, so that `- id: x` and - # ` entry: y` both reduce to the same bare "key: value" shape. - line="${line#"${line%%[![:space:]]*}"}" - if [[ "$line" == -* ]]; then - line="${line#-}" - line="${line#"${line%%[![:space:]]*}"}" - fi - case "$line" in - id:*) - read -r id <<< "${line#id:}" - ;; - entry:*) - read -r value <<< "${line#entry:}" - printf '%s\t%s\n' "${id:-(unnamed hook)}" "$value" - id="" - ;; - esac - done -} - -# A hook's script is legitimate if it exists in the working tree *or* at -# $LAST_TAG — the same union the pathspec itself spans. Checking per-scope -# instead would reject exactly the case this gate exists to flag: a script -# deleted since the tag while its entry survives (see the no -e filtering note -# further down) is a real deletion to report, not a malformed manifest. -entry_path_exists() { - local candidate="$1" - [[ -e "$candidate" ]] && return 0 - git cat-file -e "$LAST_TAG:$candidate" 2>/dev/null && return 0 - return 1 -} - -# $1 selects where the "does this hook bundle an assets/ tree?" guard looks: -# "worktree" probes the filesystem, anything else is a rev whose tree is probed -# with git plumbing. Reading entry lines from stdin keeps one derivation for -# both the tagged manifest and the current one. -collect_release_paths() { - local scope="$1" line hook_id entry bundle_root where - local -a tokens - if [[ "$scope" == "worktree" ]]; then - where="the working tree's $HOOKS_MANIFEST" - else - where="$HOOKS_MANIFEST at $scope" - fi - while IFS= read -r line; do - hook_id="${line%%$'\t'*}" - entry="${line#*$'\t'}" - read -ra tokens <<< "$entry" - [[ ${#tokens[@]} -eq 0 ]] && continue - # ADR-0014 binds every entry to a bare script path and nothing else, because - # pre-commit rewrites only entry[0] into the hook-repo clone. That is a - # constraint nothing else enforces, and the sibling .pre-commit-config.yaml - # already ships the multi-token `bash