Defame1297/holocron
0
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases 3 Wiki Activity

fix(gates): hold skill versions above main's tip as well as the merge-base

Browse Source 
Why: two branches that both bump a skill 1.0.0 -> 1.0.1 with different
content merge without a conflict, and each passed the gate against its own
merge-base, so main could ship two changes under one version.

Implementation Notes:
- check-skill-version-bump requires the pushed version to exceed both the
  merge-base and the main tip; failures name the baseline they missed.
- Presence is read from the tree, so a blob missing from a partial clone is
  a read failure instead of a silently exempt "new" skill.
- A leading UTF-8 BOM no longer reads as a missing version.
- Version parts reject leading zeros in all three validators
  (check-skill-version-bump, skill-size-check, factory-audit).
- New tests cover equal bumps, moved files, major/minor ordering, bad refs,
  unreadable blobs, mode-only changes, symlinks and tag peeling.

Impact: ADR-0022 amended (reverses "not main's current tip"); gates.md
updated to match, including pre-commit 4.6.1's exact ref selection.

ADR: 0022
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Defame1297
2026-09-16 11:24:08 +00:00
parent b426460f75
commit 1d40544075
8 changed files with 446 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
docs/adr/0022-skill-metadata-version-is-mandatory.md
View File

@@ -67,23 +67,14 @@ it's asked, not one that varies by plugin domain.
create/improve pass owes a bump, so the 12 skills carrying it are not tracking dead weight — removing
it discards real revision signal for no gain.
## Consequences
27 SKILL.md files gain `metadata.version: "1.0.0"`, and a 28th — `bin/write-docs` — reaches the same
value by relocating its top-level `version: "1.0"` into `metadata:`. `skill-author`'s `create.md`
moves the field from "Optional frontmatter" to the required list, citing this ADR. `skill-author`'s
own SKILL.md drops the "with `metadata.version` present" conditional in its bump-rule line, since
presence is no longer in question. `.pre-commit-config.yaml`'s `skill-frontmatter` hook is extended
to require the field, closing the gap #113 and #118 both named in the same audit pass: a stated rule
with nothing enforcing it drifts the same way an unstated one does.
## Amendment (2026-09-16): the bump is enforced at push, not only required to exist
Making the field mandatory did not make it move. The only thing that bumped it was `skill-author`
Step 4, so every hand edit and every trim pass skipped the bump: on `docs/simplification-audit`, 17
of the 40 skill directories that changed against `main` carried the same `metadata.version` as
`main`, and `gitea` alone sat at six different values. Both validators checked presence and semver
shape, never movement, so the field could not answer the question this ADR gives it — "did this
`main`, and `gitea` alone sat at six different values. Both validators —
`scripts/skill-size-check.sh` (the pre-commit hook) and `factory-audit`'s
`scripts/lib-checks-skill.sh` — checked presence and semver shape, never movement, so the field could not answer the question this ADR gives it — "did this
change since I last read it". (Simplification audit finding 33.)
`scripts/check-skill-version-bump.sh` now runs as a pre-push hook on every push, whatever the
@@ -91,7 +82,8 @@ target branch. It takes its baseline from the merge-base of the pushed commit wi
(local `main` if `origin/main` does not resolve). For each skill directory under
`plugins/*/.apm/skills/` that differs between the pushed commit and that merge-base, ignoring
`tests/`, the pushed `metadata.version` must be strictly greater than the version the skill had at
the merge-base — not the version on `main`'s current tip.
the merge-base — not the version on `main`'s current tip. (The amendment below reverses that last
choice: the pushed version must now also exceed `main`'s tip.)
The baseline is the merge-base, not the previous commit. Readers only ever see `main` — installs
resolve against the default branch (ADR-0018) — so one bump per branch is what the field owes
@@ -106,8 +98,10 @@ through `.pre-commit-hooks.yaml`.
The gate fails closed rather than passing when it has no trustworthy baseline: when neither
`origin/main` nor `main` resolves, when the pushed commit shares no merge-base with it, and when
only local `main` resolves and the pushed commit is that merge-base, since a local `main` the
pushed commit already contains is no independent record of what shipped. It reads versions with
`python3` and PyYAML and fails with a clear message if either is missing.
pushed commit already contains is no independent record of what shipped. It also fails closed when
the pushed ref does not resolve to a commit, and when a `SKILL.md` the tree names cannot be read by
`git show` or parsed by `python3` — a read failure is reported as such, never as a missing version.
It reads versions with `python3` and PyYAML and fails with a clear message if either is missing.
Three alternatives were rejected. Declaring the field advisory is the cheapest, but concedes the
field cannot do its job. Dropping the field was rejected by this ADR already, and costs more now.
@@ -120,6 +114,46 @@ The gate differs from `check-release-needed` in when it runs: that hook acts onl
reports a push to `main`, so a manual `pre-commit run --hook-stage pre-push` skips it, while this
gate runs there too and checks `HEAD`. The two hooks share both known gaps. A merge made with
Gitea's merge button runs no local hooks, so it is not checked. And pre-commit's pre-push
integration checks only the first ref with new commits in a multi-ref push (`git push origin a b`,
`git push --all`): `_pre_push_ns` in pre-commit's `hook_impl.py` returns after that ref, so the
other refs are pushed unchecked.
integration checks only one ref of a multi-ref push (`git push origin a b`, `git push --all`).
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.
Every later ref is pushed unchecked. When the returned ref's unpushed history reaches a root
commit, pre-commit sets no `PRE_COMMIT_TO_REF` at all, so the gate checks `HEAD`, which is the
pushed ref only if it is checked out.
## Amendment (2026-09-16): the pushed version must also exceed `main`'s tip
This reverses the choice above that the baseline is the merge-base "not the version on `main`'s
current tip". A changed skill's pushed `metadata.version` must now be strictly greater than **both**
its version at the merge-base and its version at the tip of `origin/main` (local `main` under the
same fallback, with the same fail-closed rules).
The merge-base alone lets two branches ship two different changes under one version. Branches A and
B both start from a skill at `1.0.0`, change it differently, and bump it to `1.0.1`. A merges. B's
merge-base is still the `1.0.0` commit, so B passes, and the two `1.0.1` bumps are the same line
change, so git merges B without a conflict. `main` then carries two different `1.0.1` contents, and
the field again fails to answer "did this change since I last read it". Checking against the tip as
well makes B fail until it bumps past `1.0.1`.
- **A skill absent at the tip** (deleted on `main` since the branch started) is held to the
merge-base rule alone. **A skill absent at both** is new and stays exempt.
- **When `main` has not moved since the merge-base**, the two baselines are the same commit and the
skill is checked once.
- **The failure names the baseline it missed**: `(not above merge-base)` or
`(not above origin/main tip)`, one line per baseline missed.
The cost is that a branch behind `main` may have to bump again after another branch lands a bump
on the same skill. That is the case the rule exists for, and rebasing onto or merging `main` first
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.
## Consequences
27 SKILL.md files gain `metadata.version: "1.0.0"`, and a 28th — `bin/write-docs` — reaches the same
value by relocating its top-level `version: "1.0"` into `metadata:`. `skill-author`'s `create.md`
moves the field from "Optional frontmatter" to the required list, citing this ADR. `skill-author`'s
own SKILL.md drops the "with `metadata.version` present" conditional in its bump-rule line, since
presence is no longer in question. `.pre-commit-config.yaml`'s `skill-frontmatter` hook is extended
to require the field, closing the gap #113 and #118 both named in the same audit pass: a stated rule
with nothing enforcing it drifts the same way an unstated one does.

51
docs/spec/gates.md
View File

@@ -37,10 +37,15 @@ rehearsal of 8 hooks and a skip of the ninth. The script's own header records th
a PR merged through Gitea's merge button, where 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 the first
ref that has new commits: `_pre_push_ns` in pre-commit's `hook_impl.py` returns as soon as it finds
that ref. The two hooks that read the pushed ref — `check-release-needed` and
(`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.
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.
@@ -87,7 +92,7 @@ drift in generated text.
| Hook | Guards |
|---|---|
| `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 the merge-base's (see [below](#check-skill-version-bump)) |
| `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**
@@ -108,32 +113,46 @@ ADR-0022 makes `metadata.version` mandatory and says a skill change carries a bu
- **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.
- **Baseline is the merge-base of the pushed commit with `origin/main`** (local `main` if
`origin/main` does not resolve). The pushed version is compared with the skill's version *at
that merge-base*, not with `main`'s current tip. Readers install from `main`, so "changed" means
- **"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:
a second push would excuse an unbumped change the first push already carried.
- **A changed skill's version must beat two baselines**: its version at that merge-base *and* its
version at the tip of the same `main` ref (ADR-0022's second 2026-09-16 amendment). The tip
check stops two branches that make the same bump (`1.0.0` → `1.0.1`) with different content from
both landing, since the identical version lines merge without a conflict. A skill absent at the
tip is held to the merge-base alone; when `main` has not moved, the two are the same commit. Each
failure line names the baseline it missed: `(not above merge-base)` or
`(not above origin/main tip)`. The tip is `origin/main` as last fetched.
- **It fails closed when it has no trustworthy baseline:** neither `origin/main` nor `main`
resolves; there is no merge-base (shallow clone, unrelated history); or only local `main`
resolves and the pushed commit *is* the merge-base, so local `main` already contains the pushed
commit and says nothing independent about what shipped.
- **A changed skill must end strictly above its baseline version**, compared numerically
- **A changed skill must end strictly above each baseline version**, compared numerically
(`1.0.10` > `1.0.9`). Any bump size passes. A missing or non-`MAJOR.MINOR.PATCH` version at the
pushed ref fails, and so does a skill directory left without its `SKILL.md`. Versions are
ASCII-only with at most nine digits per part — stricter than `skill-size-check`'s shape check,
so a Unicode digit or an overflowing part cannot pass as a bump. Skills absent at the baseline
(new, renamed, merged) or at the pushed ref (deleted) are exempt.
- **It also fails closed on read errors:** a pushed ref that does not resolve to a commit, or a
`SKILL.md` that `git show` or `python3` cannot read, stops the push with a read-failure message
rather than being reported as a missing version. Frontmatter that reads but does not parse
pushed ref fails, and so does a skill directory left without its `SKILL.md`. Each part is ASCII
digits, at most nine of them, with no leading zero (`1.0.08` is malformed) — the same shape
`skill-size-check` enforces, so a Unicode digit, an overflowing part or an octal-looking part
cannot pass as a bump. A leading UTF-8 BOM is ignored. A baseline with no valid version accepts
any valid version. Skills absent at both baselines (new, renamed, merged) or at the pushed ref
(deleted, or replaced by a symlink) are exempt. A file moved between skills counts as a change
to both: renames are diffed as delete plus add. A mode-only change counts too.
- **It also fails closed on read errors:** a pushed ref that does not resolve to a commit
(including a tag on a tree), or a `SKILL.md` that the tree names but `git show` or `python3`
cannot read, stops the push with a read-failure message rather than being reported as a missing
version or treated as an absent skill. Presence is read from the tree, so a blob missing from a
corrupt or partial clone cannot make a skill look new. Frontmatter that reads but does not parse
counts as an invalid version.
- **`<skill>/tests/` is excluded**: no agent loads it, so a test-only change ships nothing.
- **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
runs no local hook; and a multi-ref push checks only its first ref with new commits (see
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)).
- **An all-zeros `PRE_COMMIT_TO_REF` (a branch delete) exits 0.** The branch is defensive:
pre-commit 4.6.1 skips delete lines before it sets the variable.
## Skill and agent context gates (ADR-0020)