diff --git a/docs/adr/0013-vale-harness-scope-and-rule-sources.md b/docs/adr/0013-vale-harness-scope-and-rule-sources.md index e9058b4..8ca796e 100644 --- a/docs/adr/0013-vale-harness-scope-and-rule-sources.md +++ b/docs/adr/0013-vale-harness-scope-and-rule-sources.md @@ -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` (`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 -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. **Governance docs are excluded as a rule source.** `docs/research/governance_principles/CONTROLS.md` 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 fe85904..3750ca9 100644 --- a/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md +++ b/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md @@ -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 `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"). -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` -still meets the `entry[0]`-only constraint. A return must first make `skill-size-check.sh` -self-contained again, by re-embedding the resolver or shipping the library beside the hook, and -restore a consumer test that proves it. + +**Amended (2026-09-20): that is not a consumer-facing defect.** The correction above went on to say +that a consumer's checkout has no such file, so restoring the manifest would ship a hook that fails +for every consumer. Reproduced and found false. pre-commit's `script` language clones the **whole** +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 through `.pre-commit-hooks.yaml`" and that it shares its gaps with `check-release-needed`, and diff --git a/docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md b/docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md index d420360..69f39a4 100644 --- a/docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md +++ b/docs/adr/0015-apm-replaces-plugin-marketplace-authoring.md @@ -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 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 (`.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 @@ -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`). **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 - 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, 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 diff --git a/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md b/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md index 09fc8fc..e8ce5df 100644 --- a/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md +++ b/docs/adr/0019-session-start-hook-keeps-the-apm-install-current.md @@ -156,6 +156,16 @@ via `url..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 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..insteadOf` adds no network call of its own. + **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. Until then the repo has the mechanism in source and not in effect. diff --git a/docs/adr/0020-skill-description-and-body-context-contract.md b/docs/adr/0020-skill-description-and-body-context-contract.md index edd30bf..c6bb937 100644 --- a/docs/adr/0020-skill-description-and-body-context-contract.md +++ b/docs/adr/0020-skill-description-and-body-context-contract.md @@ -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 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 -`agent-audit/scripts/validate.sh`" is one path now, `factory-audit/scripts/validate.sh`, which -auto-detects the artifact type; the skills/agents columns are unaffected, since the merged validator -applies the body tiers on the skill path only. The two copies must still stay byte-identical — a +`agent-audit/scripts/validate.sh`" now means `factory-audit/scripts/lib-checks-skill.sh:313-316` +(all four constants) and `lib-checks-agent.sh:164-165` (the two description ones). It does **not** +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 "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 `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 unchanged spec backstop. The SUGGESTION tier is what moves the average; the FAIL tier only stops 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 | |---|---|---|---| -| 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` | -| 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) | +| 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/lib-checks-skill.sh:315-316`, see the ADR-0025 amendment) | | 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 clause absent — `absent` (SUGGESTION) † | skills, agents | deterministic | same | diff --git a/docs/adr/0021-plugin-descriptions-state-a-domain-boundary.md b/docs/adr/0021-plugin-descriptions-state-a-domain-boundary.md index a54af77..2de9c9a 100644 --- a/docs/adr/0021-plugin-descriptions-state-a-domain-boundary.md +++ b/docs/adr/0021-plugin-descriptions-state-a-domain-boundary.md @@ -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 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 -`check-plugin-content-sync` hook with the flat mirror (ADR-0024). Of the two names left, -`apm audit --ci` was never a drift gate at all: against this repo it checks only that each `apm.yml` -parses and that a manifest declaring dependencies has a consistent `apm.lock.yaml`, and it reads no -`description`. So the sole surviving gate that compares compiled output against `apm.yml` is +`check-plugin-content-sync` hook with the flat mirror (ADR-0024). The other two survive. +`apm audit --ci` at the repo root (apm 0.28.0) runs ten checks — `lockfile-exists`, +`ref-consistency`, `deployment-ledger-owners`, `deployed-files-present`, `no-orphaned-packages`, +`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 — 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 -the finding: propagation is still not verification, and nothing anywhere reads the `description` -key for sense. +the finding: both gates compare bytes, neither reads the `description` key for sense, so propagation +is still not verification. **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 diff --git a/docs/adr/0022-skill-metadata-version-is-mandatory.md b/docs/adr/0022-skill-metadata-version-is-mandatory.md index c7f7703..cf69d46 100644 --- a/docs/adr/0022-skill-metadata-version-is-mandatory.md +++ b/docs/adr/0022-skill-metadata-version-is-mandatory.md @@ -54,6 +54,13 @@ per-plugin choice. `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. + > **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 **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 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 )` (`: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 27 SKILL.md files gain `metadata.version: "1.0.0"`, and a 28th — `bin/write-docs` — reaches the same