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