refactor!: carry out the simplification audit across gates, tests, plugins and docs #135

Merged
Defame1297 merged 85 commits from docs/simplification-audit into main 2026-09-20 19:14:03 +00:00
7 changed files with 105 additions and 19 deletions
Showing only changes of commit 44bde9e9c9 - Show all commits

View File

@@ -26,7 +26,11 @@ stay out of this Vale-based harness because this repo already has dedicated tool
`skill-frontmatter` (required frontmatter fields), `validate-marketplace` `skill-frontmatter` (required frontmatter fields), `validate-marketplace`
(`claude plugin validate --strict`, schema), and `gitleaks`/`detect-private-key` (secrets). (`claude plugin validate --strict`, schema), and `gitleaks`/`detect-private-key` (secrets).
(ADR-0024 removed the companion `validate-plugins` gate along with the per-plugin manifests it (ADR-0024 removed the companion `validate-plugins` gate along with the per-plugin manifests it
checked; the argument here is unaffected.) checked, and the `skill-frontmatter` hook has since been removed as well — its required-field
checks were folded into `skill-size-check`, and the enforcer is now
`scripts/skill-size-check.sh:324-335` under the `skill-size-check` hook at
`.pre-commit-config.yaml:237`. The argument here is unaffected either way: a dedicated
non-Vale tool still owns required frontmatter fields.)
Duplicating those concerns as Vale rules would fight tools that already own them better. Duplicating those concerns as Vale rules would fight tools that already own them better.
**Governance docs are excluded as a rule source.** `docs/research/governance_principles/CONTROLS.md` **Governance docs are excluded as a rule source.** `docs/research/governance_principles/CONTROLS.md`

View File

@@ -277,11 +277,22 @@ release gate only once a server-side job can run it on merge.
`skill-size-check.sh`. `ef27c97` removed its embedded resolver copy: the hook now sources `skill-size-check.sh`. `ef27c97` removed its embedded resolver copy: the hook now sources
`plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh` by path and fails `plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh` by path and fails
closed without it (ADR-0020's 2026-09-16 amendment; `docs/spec/gates.md`, "Duplicated constants"). closed without it (ADR-0020's 2026-09-16 amendment; `docs/spec/gates.md`, "Duplicated constants").
An external consumer's checkout has no such file, so restoring the manifest as described would ship
a hook that fails for every consumer — the defect recorded at `LESSONS.md:101`. Only `vale-wrap.sh` **Amended (2026-09-20): that is not a consumer-facing defect.** The correction above went on to say
still meets the `entry[0]`-only constraint. A return must first make `skill-size-check.sh` that a consumer's checkout has no such file, so restoring the manifest would ship a hook that fails
self-contained again, by re-embedding the resolver or shipping the library beside the hook, and for every consumer. Reproduced and found false. pre-commit's `script` language clones the **whole**
restore a consumer test that proves it. hook repo into its store and prefixes `entry[0]` with the clone directory: `clientlib.py` maps
`script` to `unsupported_script`, whose `run_hook` does `cmd = (prefix.path(cmd[0]), *cmd[1:])` over
`Prefix(store.clone(...))`, and `store.clone` checks out the full tree — shallow in depth, not in
content. `skill-size-check.sh` locates the library from `${BASH_SOURCE[0]}`
(`scripts/skill-size-check.sh:481-483`), which points into that same clone, so
`../plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh` resolves beside
it. Verified end to end against pre-commit 4.6.1 with a probe hook of the same shape — bare script
entry, sibling file reached by climbing out of `scripts/` — and the file was found and sourced. Both
hook scripts therefore still meet the `entry[0]`-only constraint: `vale-wrap.sh` takes no `--config`,
and `skill-size-check.sh` passes no argv of its own. A return needs no re-embedding; restore
`test-vale-hooks-consumer.sh` with the manifest, extended to cover the sourced library, so the claim
stays checked rather than reasoned about.
**Superseded statements elsewhere.** ADR-0022's notes that the version-bump gate "is not exported **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 through `.pre-commit-hooks.yaml`" and that it shares its gaps with `check-release-needed`, and

View File

@@ -5,6 +5,12 @@ their authoring source; `.claude-plugin/marketplace.json` and every plugin's `pl
`apm pack`-compiled output. **Supersedes ADR-0001** ("Skills are distributed via plugins... each `apm pack`-compiled output. **Supersedes ADR-0001** ("Skills are distributed via plugins... each
plugin contains its own `skills/` directory") — in effect. plugin contains its own `skills/` directory") — in effect.
**Correction (2026-09-20): the present tense above has expired for `plugin.json`.** ADR-0024 made
apm the only supported install path and deleted per-plugin `plugin.json` with the native install
support that needed it. No plugin carries one at `HEAD` — `git ls-files | grep -c 'plugin\.json'`
returns 0 — so `.claude-plugin/marketplace.json` is the only `apm pack`-compiled output left. Read
the Status line as the state at execution, 2026-08-12.
This repo replaces its hand-maintained Claude Code plugin/marketplace authoring model This repo replaces its hand-maintained Claude Code plugin/marketplace authoring model
(`.claude-plugin/marketplace.json` + per-plugin `plugin.json`) with Microsoft APM (`apm.yml` + (`.claude-plugin/marketplace.json` + per-plugin `plugin.json`) with Microsoft APM (`apm.yml` +
`.apm/`) as the authoring source of truth — an outright replacement of the authoring layer, not an `.apm/`) as the authoring source of truth — an outright replacement of the authoring layer, not an
@@ -180,7 +186,11 @@ correction) sorted what they document into three buckets:
`git ls-remote`, which is why two pre-push hooks needed the network (see `AGENTS.md`). `git ls-remote`, which is why two pre-push hooks needed the network (see `AGENTS.md`).
**Superseded 2026-09-13:** the `mattpocock-skills` entry has been removed from root `apm.yml` **Superseded 2026-09-13:** the `mattpocock-skills` entry has been removed from root `apm.yml`
entirely, along with the `codex` marketplace output profile. No pre-push hook needs the network entirely, along with the `codex` marketplace output profile. No pre-push hook needs the network
any longer. any longer — **once `apm install` has populated `apm_modules/`**. The guarantee is a property of a
populated install, not of the hook set: on a fresh clone `apm-audit-ci`'s `deployed-files-present`
fails outright, and its `drift` and `config-consistency` install-replays have no cache to replay
from and clone from the holocron remote (`README.md:89`; `docs/spec/gates.md`, "Pushing without a
network").
- **Caveat on "Status: executed" above:** issue #90's own execution comment flagged, before merge, - **Caveat on "Status: executed" above:** issue #90's own execution comment flagged, before merge,
that Claude Code's ability to actually load content out of `.apm/` was unverified — that caveat that Claude Code's ability to actually load content out of `.apm/` was unverified — that caveat
turned out to be a real defect, not a formality: the native installer has zero awareness of turned out to be a real defect, not a formality: the native installer has zero awareness of

View File

@@ -156,6 +156,16 @@ via `url.<path>.insteadOf`, so the twelve-hooks-pass-under-`unshare -rn` propert
the real `apm outdated`, and replays its genuine output through the real hook. Reverting the grep the real `apm outdated`, and replays its genuine output through the real hook. Reverting the grep
to plural-only fails it. to plural-only fails it.
> **Correction (2026-09-20):** neither half of "the twelve-hooks-pass-under-`unshare -rn` property"
> is accurate. The count was never twelve: `main` declares 14 pre-push hooks and `HEAD` declares 8 —
> 10 counting the two `repo: meta` hooks, which set no `stages` and so run at every stage. And the
> property is conditional, not absolute: no pre-push hook needs the network **once `apm install` has
> populated `apm_modules/`**, but on a fresh clone `apm-audit-ci`'s `deployed-files-present` fails
> outright and its `drift` and `config-consistency` install-replays clone from the holocron remote
> (`README.md:89`; `docs/spec/gates.md`, "Pushing without a network"). What the probe itself
> establishes is unchanged and is the point of the sentence: staging the outdated dependency against
> a local git remote via `url.<path>.insteadOf` adds no network call of its own.
**The hook cannot install itself.** Dependencies resolve from the remote, so the hook does not **The hook cannot install itself.** Dependencies resolve from the remote, so the hook does not
deploy until this change is merged and `apm update` has run once against the new default branch. deploy until this change is merged and `apm update` has run once against the new default branch.
Until then the repo has the mechanism in source and not in effect. Until then the repo has the mechanism in source and not in effect.

View File

@@ -21,9 +21,12 @@ SHARED BOUNDARY RESOLVER` markers, and one plugin copy — extracted out of the
both. `validate-provenance.sh` is not a third reader: it sources `lib-contributing-files.sh` and one both. `validate-provenance.sh` is not a third reader: it sources `lib-contributing-files.sh` and one
of `lib-provenance-skill.sh`/`lib-provenance-agent.sh`, and never touches the resolver at all. The of `lib-provenance-skill.sh`/`lib-provenance-agent.sh`, and never touches the resolver at all. The
Enforcement table's "constants mirrored in `skill-audit/scripts/validate.sh` and Enforcement table's "constants mirrored in `skill-audit/scripts/validate.sh` and
`agent-audit/scripts/validate.sh`" is one path now, `factory-audit/scripts/validate.sh`, which `agent-audit/scripts/validate.sh`" now means `factory-audit/scripts/lib-checks-skill.sh:313-316`
auto-detects the artifact type; the skills/agents columns are unaffected, since the merged validator (all four constants) and `lib-checks-agent.sh:164-165` (the two description ones). It does **not**
applies the body tiers on the skill path only. The two copies must still stay byte-identical — a mean `factory-audit/scripts/validate.sh`, which holds none of them: `validate.sh` auto-detects the
artifact type and sources the matching check suite (`validate.sh:231-233`, `:244-246`). The
skills/agents columns are unaffected — only the skill suite carries the body tiers. The two copies
must still stay byte-identical — a
plugin script cannot source the root one, which is why a second copy exists at all. Read every plugin script cannot source the root one, which is why a second copy exists at all. Read every
"three" below as the count at the time of writing. "three" below as the count at the time of writing.
@@ -116,6 +119,12 @@ clause**, and a **boundary clause**. Capability enumeration, output-format detai
("composes X rather than duplicating Y"), and implementation detail move to the body or to ("composes X rather than duplicating Y"), and implementation detail move to the body or to
`README.md`. `README.md`.
**Correction (2026-09-20): not `README.md`.** The canonical destination for description overflow is
"the body or a `references/` file" (`plugins/kyberforge/.apm/skills/skill-author/references/contract.md:35`).
A skill-root `README.md` is no longer somewhere overflow can go: all 39 of them were deleted, and
`factory-audit/references/skill-file-structure.md:23` now FAILs a non-spec file at the skill root,
which a `README.md` is. Read every "or to `README.md`" below as "or to a `references/` file".
- **250 characters SUGGESTION, 400 FAIL.** The agentskills.io 1,024-character limit remains as an - **250 characters SUGGESTION, 400 FAIL.** The agentskills.io 1,024-character limit remains as an
unchanged spec backstop. The SUGGESTION tier is what moves the average; the FAIL tier only stops unchanged spec backstop. The SUGGESTION tier is what moves the average; the FAIL tier only stops
outliers. outliers.
@@ -284,8 +293,8 @@ which tier each rule is in, because the failure this ADR is most exposed to is a
| Check | Applies to | Tier | Home | | Check | Applies to | Tier | Home |
|---|---|---|---| |---|---|---|---|
| description characters (250 SUGGESTION † / 400 FAIL) | skills, agents | deterministic | `scripts/skill-size-check.sh`; constants mirrored in `skill-audit/scripts/validate.sh` and `agent-audit/scripts/validate.sh` | | description characters (250 SUGGESTION † / 400 FAIL) | skills, agents | deterministic | `scripts/skill-size-check.sh`; constants mirrored in `skill-audit/scripts/validate.sh` and `agent-audit/scripts/validate.sh` (now `factory-audit/scripts/lib-checks-skill.sh:313-314` and `lib-checks-agent.sh:164-165`, see the ADR-0025 amendment — **not** `factory-audit/scripts/validate.sh`, which holds no constants) |
| body-only words (600 SUGGESTION / 900 FAIL) | skills | deterministic | `skill-size-check.sh`, `skill-audit/scripts/validate.sh` (now `factory-audit/scripts/validate.sh`, see ADR-0025) | | body-only words (600 SUGGESTION / 900 FAIL) | skills | deterministic | `skill-size-check.sh`, `skill-audit/scripts/validate.sh` (now `factory-audit/scripts/lib-checks-skill.sh:315-316`, see the ADR-0025 amendment) |
| description present and non-empty (ERROR) | skills, agents | deterministic | same | | description present and non-empty (ERROR) | skills, agents | deterministic | same |
| boundary target resolves to a real skill or agent — **three** verdicts, not two (ERROR when written in route notation — `/name`, or any arrow form; or when a *terminal* bare name's own sentence names another target that resolves. SUGGESTION otherwise. INFO, "DID NOT RUN", exit 0, when no skill universe could be determined for the path at all — no authoring root above it, no apm package root, no declared apm dependencies, no deployed `.claude/` or `.agents/` tree: the targets are named and left unchecked) | skills, agents | deterministic | same | | boundary target resolves to a real skill or agent — **three** verdicts, not two (ERROR when written in route notation — `/name`, or any arrow form; or when a *terminal* bare name's own sentence names another target that resolves. SUGGESTION otherwise. INFO, "DID NOT RUN", exit 0, when no skill universe could be determined for the path at all — no authoring root above it, no apm package root, no declared apm dependencies, no deployed `.claude/` or `.agents/` tree: the targets are named and left unchecked) | skills, agents | deterministic | same |
| boundary clause absent — `absent` (SUGGESTION) † | skills, agents | deterministic | same | | boundary clause absent — `absent` (SUGGESTION) † | skills, agents | deterministic | same |

View File

@@ -103,17 +103,20 @@ output against `apm.yml`, so their entire job is to propagate whatever the descr
those four files byte-for-byte and confirm they match. The `wiki` claim passed every one of the fourteen pre-push hooks, every day it those four files byte-for-byte and confirm they match. The `wiki` claim passed every one of the fourteen pre-push hooks, every day it
was published. was published.
**Correction (2026-09-14): that gate list is down to one, and it was never two.** **Correction (2026-09-14): that gate list is down to two.**
`scripts/sync-plugin-content.sh --check --all` does not exist — `718c79a` deleted the script and its `scripts/sync-plugin-content.sh --check --all` does not exist — `718c79a` deleted the script and its
`check-plugin-content-sync` hook with the flat mirror (ADR-0024). Of the two names left, `check-plugin-content-sync` hook with the flat mirror (ADR-0024). The other two survive.
`apm audit --ci` was never a drift gate at all: against this repo it checks only that each `apm.yml` `apm audit --ci` at the repo root (apm 0.28.0) runs ten checks — `lockfile-exists`,
parses and that a manifest declaring dependencies has a consistent `apm.lock.yaml`, and it reads no `ref-consistency`, `deployment-ledger-owners`, `deployed-files-present`, `no-orphaned-packages`,
`description`. So the sole surviving gate that compares compiled output against `apm.yml` is `skill-subset-consistency`, `config-consistency`, `content-integrity`, `includes-consent` and
`drift` — and it *is* a drift gate: `drift` and `config-consistency` replay the install and diff the
result against the working tree, and `content-integrity` scans for hidden Unicode and hash drift.
(In a sub-package such as `plugins/lint` it runs one check, `lockfile-exists`.) The second is
`apm pack --check-versions --check-clean --dry-run`, run by the `apm-pack-check-clean` pre-push hook `apm pack --check-versions --check-clean --dry-run`, run by the `apm-pack-check-clean` pre-push hook
— and with the per-plugin manifests gone it propagates a description into exactly one file, — and with the per-plugin manifests gone it propagates a description into exactly one file,
`.claude-plugin/marketplace.json`, not four. This narrows the mechanism and changes nothing about `.claude-plugin/marketplace.json`, not four. This narrows the mechanism and changes nothing about
the finding: propagation is still not verification, and nothing anywhere reads the `description` the finding: both gates compare bytes, neither reads the `description` key for sense, so propagation
key for sense. is still not verification.
**And the obligation is unbounded.** Under enumeration, adding one skill to `bin`, `git` or `gitea` **And the obligation is unbounded.** Under enumeration, adding one skill to `bin`, `git` or `gitea`
means editing two copies of a prose string on top of the version bumps and regeneration any skill means editing two copies of a prose string on top of the version bumps and regeneration any skill

View File

@@ -54,6 +54,13 @@ per-plugin choice.
`name:` or `description:`; a missing `metadata.version` is now the same class of failure, not a `name:` or `description:`; a missing `metadata.version` is now the same class of failure, not a
style nit an audit might or might not catch. style nit an audit might or might not catch.
> **Correction (2026-09-20): that hook no longer exists.** `skill-frontmatter` was removed and its
> required-field checks folded into `skill-size-check`. The enforcer is now
> `scripts/skill-size-check.sh:324-335`, declared under the `skill-size-check` hook at
> `.pre-commit-config.yaml:237`. It checks presence and three-part-semver shape, on the same
> `SKILL.md` glob and at the same pre-commit stage, so the decision is unaffected — only the name
> of the hook that holds it. The same substitution applies to the Consequences section below.
## Considered options ## Considered options
**Leave it per-plugin, document the split.** This was the initial framing of #127 and is coherent — **Leave it per-plugin, document the split.** This was the initial framing of #127 and is coherent —
@@ -152,6 +159,38 @@ on the same skill. That is the case the rule exists for, and rebasing onto or me
shows the version to beat. The rule reads `origin/main` as last fetched, so a tip that moved since shows the version to beat. The rule reads `origin/main` as last fetched, so a tip that moved since
the last fetch is not seen until the next one. the last fetch is not seen until the next one.
## Amendment (2026-09-20): two exemptions and a third failure form the rules above never stated
The carve-outs enumerated above read as a closed list, and the baseline above reads as a single
merge-base. `scripts/check-skill-version-bump.sh` as shipped has two further exemptions and emits a
third failure form. **The gate is right and is not changing; this ADR was behind it.** Its own header
comments (`:9-93`) have described all three correctly since it shipped.
- **The baseline is `git merge-base --all`, not one merge-base.** A criss-cross history — `main`
merges a branch while that branch merges a commit of `main` — has two merge bases, and which one
`git merge-base` prints is an implementation detail. The script takes every base (`:154-157`) and
**intersects** the changed-skill sets across them (`:203-217`): a skill matching any one base is
already shipped by that base and is exempt, and a skill that does reach the comparison must exceed
the version at every base it exists at (`:367-376`). Picking one base made the verdict a coin
flip — an already-merged bump failed the push it should have passed.
- **A skill whose directory tree object equals the tip's skips the tip comparison.**
`same_subtree()` (`:288-297`, applied at `:334-337`) compares tree object ids rather than diffing:
the same tree is the same content, whatever route the history took to it. A branch cut before a
fix landed on `main` and then cherry-picking that fix has one merge-base, predating the fix, so the
skill counts as changed against it and reaches the tip comparison carrying exactly the tip's
version — same content, same version. The merge-base intersection catches that only when some base
carries the content, which the criss-cross shape gives and a linear one does not. Without the skip
the only escapes are a spurious bump, leaving `main` carrying two versions of identical content,
or a rebase the push does not otherwise need.
- **A third failure form.** The amendment above lists `(not above merge-base)` and
`(not above origin/main tip)`. When there is more than one base, the merge-base line is
sha-suffixed — `(not above merge-base <sha>)` (`:372`, against the unsuffixed `:374`) — because
"which merge-base" is the one question a reader cannot answer from the branch alone.
`8cfd54f` recorded that "ADR-0022 is not amended: the documented behaviour does not change". That
was wrong for the tree-identical case: the `same_subtree` skip makes a push **pass** that this ADR as
written requires to **fail**, which is documented behaviour changing, not an implementation detail.
## Consequences ## Consequences
27 SKILL.md files gain `metadata.version: "1.0.0"`, and a 28th — `bin/write-docs` — reaches the same 27 SKILL.md files gain `metadata.version: "1.0.0"`, and a 28th — `bin/write-docs` — reaches the same