The allowlist-closed diff hardcoded `origin main`, but ship-ci.yml also runs
this workflow for PRs into release/** and for pushes to release/**, where a
main baseline diffs against the wrong branch and can fail changes that have
nothing to do with the allowlist. It now resolves the base:
PR -> github.event.pull_request.base.ref (the branch it merges into)
push -> github.ref_name (the branch itself; its tip already contains the
change, so the diff is a no-op — enforcement happens at PR time)
Also from review, all documentation drift introduced by my own earlier
commits:
- The layer count said "three" in the module docstring, the workflow comment,
the docs, and the changelog. There are four (allowlist-closed was added).
- The changelog listed three scanned modules; there are five.
- The docs and changelog stated the rule for keys core *reads*, omitting
writes — which are equally gated, and land in every pack we emit.
- The CI summary line labelled grandfathered keys "pending spec", implying
adoption is the only resolution. For original_audio it is not: the fix is
removal. Relabelled "grandfathered (tracked debt)".
- feedpak-spec-exceptions.yml said an entry clears when core "stops reading"
the key; the rule is "no longer reads or writes".
- Replaced a bitwise `&` over two bools with two named results and an
explicit `and` — both checks must run (a stale READERS list and an
undeclared key are separate failures; short-circuiting would hide one), and
`&` reads like a typo.
Signed-off-by: topkoa <topkoa@gmail.com>
8.0 KiB
The feedpak spec-conformance gate
tools/check_spec_conformance.py, run in CI as the feedpak-spec job.
Why
feedpak is published as an open format: its own repo (got-feedback/feedpak-spec), a normative spec, JSON Schemas, and a reference validator. That is a promise to everyone outside this codebase — third-party packers, converters, and players build against the spec, and the spec is meant to be the complete and authoritative description of a pack.
The moment core reads a manifest key the spec doesn't define, that promise breaks silently:
- A spec-compliant pack is no longer guaranteed to be a fully-working pack.
- The reference validator can't warn authors about a key it has never heard of — it will happily green-light the key, and every misspelling of it.
- The format's real definition drifts into our source tree. In the case that motivated this gate
(#933), third-party tooling started emitting an
original/directory that no code anywhere requires — the convention was reverse-engineered from an example in a code comment.
The rule this gate enforces: any manifest key core reads or writes must be in the spec before core ships code that depends on it. Spec first, implementation second. Writes are not exempt — a key core writes lands in every pack we emit, so an undeclared one seeds the ecosystem with non-spec data.
Note that "get it into the spec" is not automatically the right fix for an existing violation — for
original_audio it isn't. The spec already carries the pre-separation mixdown as a stem
({id: full, file: stems/full.ogg}), so that key added a second, redundant location for audio to a format
that already had one, and the resolution is to remove it rather than bless it. The gate takes no position on
which way a violation resolves; it only insists that one of the two happens deliberately, in the open,
before the code merges.
What it checks
We can't mechanically prove core interprets a key the way the spec means. We can prove four surface properties, and they cover the drift that actually occurs.
| Layer | Check | Catches |
|---|---|---|
| 1. key-coverage | Every manifest key core reads or writes is declared in the spec's manifest.schema.json. |
Core growing a key the spec never defined — the #933 class. |
| 2. allowlist-closed | feedpak-spec-exceptions.yml has not grown relative to the base branch. |
Someone routing around the FEP process by allowlisting their own new key. |
| 3. forward | Core's load_song() ingests every example pack the spec ships. |
The spec adding or tightening something core ignores or breaks on. |
| 4. reverse | Every pack committed to this repo passes the spec's tools/validate.py. |
Core (or a contributor) committing a pack the spec would reject. |
Layer 1 works by walking the AST of the modules listed in READERS and collecting every literal key touched
on a manifest dict (manifest.get("x"), manifest["x"], and the wrapped
(load_manifest(p) or {}).get("x") form used in lib/enrichment.py).
Reads and writes are both checked, and reported differently. A key core writes
(manifest["x"] = v, as lib/songmeta.py does) is spec surface pointed outward: it puts a key into every
pack we emit, so an undeclared one seeds the ecosystem with non-spec data. Subscripts are classified by AST
context — Store is a write, Load is a read — so manifest["year"] = ... is not miscounted as a read.
When it fails
You added a manifest key the spec doesn't define. There is exactly one way forward, and it is not in this repo.
Land the key in the spec through the feedpak Enhancement Proposal (FEP) process (feedpak-spec/CONTRIBUTING.md):
- Open a FEP issue on
got-feedback/feedpak-spec— the problem, the proposed on-disk shape (manifest key and/or side-file), backward compatibility, and the version bump it implies. - Discuss, until it has a clear shape and rough consensus.
- Land one PR there that updates the normative spec (
spec/feedpak-v1.md), the relevant JSON Schema(s), an example inexamples/that exercises it, and the changelog — together. A PR touching only one of those is incomplete. - Back here, bump
.feedpak-spec-refto that merged SHA, in the same PR as your code. The gate goes green, because the key is now genuinely part of the format.
That is deliberately the only route. There is no in-repo escape hatch — no experimental prefix, no self-serve allowlist. If your PR is blocked, the answer is a FEP, not a workaround. The person merging has to stop and decide whether the change is worth taking through the format process, which is the whole point.
The spec's own governance says the same thing:
This repository defines the format only. Applications that read or write feedpak ... track this spec as a dependency; they do not drive it. A change is not part of the format until it lands here. — feedpak-spec/GOVERNANCE.md
feedpak-spec-exceptions.yml is a closed grandfather list, not a hatch
It exists solely because original_audio predates the gate. CI fails any PR that adds an entry (layer 2
diffs it against the base branch), so the list can only ever shrink. Entries are debt, each carries a
tracking issue, and each disappears when the underlying key is removed from core. The gate also fails on a
stale entry — the spec caught up, or core stopped touching the key — so the file cannot quietly become
somewhere drift accumulates.
Deleting an entry does not, by itself, get you past the gate: layer 1 still fails while core reads the key. The entry goes when the code goes.
Pinning
.feedpak-spec-ref holds the SHA of the feedpak-spec commit this repo is verified against. Pinned rather
than tracking the spec's default branch on purpose — a change over there must never turn CI red on an
unrelated PR here.
When the spec moves, bump the SHA in its own PR. If that PR is red, the spec changed in a way core doesn't satisfy — exactly the signal we want, delivered as a reviewable PR rather than a surprise on someone else's branch.
Limitations
Known, and worth fixing in follow-ups rather than blocking on:
- Layer 1 is name-heuristic. It recognises manifest dicts bound to locals named in
MANIFEST_VARS(manifest,mf) plus theload_manifest(...)call form. This works because the loaders use a uniform idiom, but it is fragile against a refactor that renames the local. Thereaders-completeguard limits the blast radius — a module that touches manifest keys can't go unscanned — but a renamed local inside a scanned module would still slip. The hardening step is to route all manifest access through a single declaredKNOWN_MANIFEST_KEYSregistry inlib/sloppak.py; the gate then compares registry against schema exactly instead of inferring. - Layer 1 covers top-level keys only. Nested structure (
arrangements[].file,.id,.notation) isn't checked. Extending to it means walking the schema's$refsubschemas. - Layer 1 recognises
get,setdefault, and subscripts as key access.update()andpop()aren't used against a feedpak manifest anywhere in the tree, so they're deliberately not special-cased rather than speculatively handled — but noteKEY_OPS(which drivesreaders-complete) doesn't look for them either. - Layer 4 can't catch unknown keys, because
manifest.schema.jsonsetsadditionalProperties: trueand the reference validator deliberately "treats unknown keys/files as forward-compatible". Fixing this properly belongs in the spec (tighten the schema, or give the validator a--strictmode). Until then, layer 1 is the only thing standing between us and the nextoriginal_audio.