The gate's purpose is to make a non-conforming change *not merge*, so the
person merging must stop and decide whether to take it through the format
process. The escape hatches defeated exactly that: a developer who did not
want to write a FEP could name their key `x-whatever`, or append an entry to
feedpak-spec-exceptions.yml with any issue link, and merge. Both were
self-serve and in-repo. That is a speed bump with a signed excuse note, not a
gate.
The relief valve is the FEP process itself, not something in this repo. The
spec's governance already says so: "A change is not part of the format until
it lands here."
Removed the `x-` prefix bypass. It was invented here, not in the spec — the
spec reserves no experimental namespace. Its "unknown keys are reserved for
forward-compatibility" rule is about *tolerating* other implementations'
keys, not a licence for core to mint its own.
feedpak-spec-exceptions.yml is now a CLOSED grandfather list. A new check
(allowlist-closed) diffs it against the base branch and fails any PR that
ADDS an entry; removal stays allowed, so the list can only shrink. Deleting
an entry does not by itself pass the gate — key-coverage still fails while
core reads the key, so the entry goes when the code goes.
Every failure message now points at the FEP process and at bumping
.feedpak-spec-ref to the merged spec SHA, which is the one supported way a
new manifest key reaches core.
CI fetches the base branch to diff the allowlist; the bootstrap flag covers
the one case with no baseline — the PR introducing the gate.
Signed-off-by: topkoa <topkoa@gmail.com>
Six more findings from CodeRabbit and Copilot on #934. All valid; four were
my own docs lagging the write-checking change in d0626f5.
check_forward() now discovers example packs recursively, so a pack nested
under examples/<group>/ can't slip past the "every example pack" contract.
Taken WITHOUT the suggested is_file() filter, which would have broken it: a
feedpak is dual-form — a zip (foo.feedpak) or a directory (foo.feedpak/) —
and the spec's own examples ship as directories, so is_file() would have
matched zero packs. Suffix matching covers both forms.
load_exceptions() rejects duplicate keys instead of silently keeping the
last one, which would quietly retarget the tracking issue for a piece of
debt this file exists to track.
The sloppak import is wrapped so a missing dependency produces a CI-legible
::error:: rather than a bare traceback.
Docs caught up with the code: the exceptions file header, its stale-entry
rule, and the changelog all said "reads" when the gate checks reads AND
writes.
Signed-off-by: topkoa <topkoa@gmail.com>
Review feedback from CodeRabbit and Copilot on #934. All six findings were
valid; one is fixed the other way round from how it was suggested.
Key-coverage now checks manifest WRITES as well as reads. Copilot correctly
spotted that `ast.walk` ignored subscript context, so
`manifest["year"] = ...` (lib/songmeta.py) scored as a read — but the fix is
not to drop writes. A key core *writes* is spec surface pointed outward: it
lands in every pack we emit, so an undeclared one seeds the ecosystem with
non-spec data. Subscripts are now classified by ctx (Store = write, Load =
read) and both sets are checked, with distinct error messages. Today: 19
reads, 2 writes, all declared.
Workflow:
- persist-credentials: false on the repo checkout — the job runs repository
code and never pushes (CodeRabbit / zizmor artipacked).
- .feedpak-spec-ref must be a full 40-char SHA. actions/checkout resolves
branches and tags in `ref` too, so a non-SHA there would silently un-pin
the spec — precisely what the file exists to prevent.
- Pin jsonschema==4.26.0, for the same reason the spec SHA is pinned: an
upstream release must not redden this job on a PR that changed neither
this repo nor the spec.
Script:
- check_forward() guards a missing examples/ dir instead of raising an
unhandled FileNotFoundError.
- TemporaryDirectory() instead of mkdtemp(), so a local run doesn't leak.
Signed-off-by: topkoa <topkoa@gmail.com>
feedpak is published as an open format with its own repo, normative spec,
JSON Schemas, and reference validator. That makes the spec a contract with
everyone outside this repo: third-party packers, converters, and players
build against it, and it is meant to be the complete description of a pack.
Nothing enforced that. #583 added a manifest key (`original_audio`) that
core, lib/enrichment.py, and the stems plugin all now depend on, but which
was never added to the spec — so a spec-compliant pack stopped being a
fully-working pack, the reference validator could not warn authors about a
key it had never heard of, and third-party tooling began emitting an
`original/` directory reverse-engineered from an example in a code comment.
See #933.
We cannot mechanically prove core interprets a key the way the spec means.
We can prove three surface properties, and they cover the drift that
actually happens:
1. key-coverage — every manifest key core reads is declared in the spec's
manifest.schema.json (AST scan of lib/sloppak.py, lib/enrichment.py,
lib/songmeta.py).
2. forward — core's load_song() ingests every example pack the spec ships.
3. reverse — every pack committed here passes the spec's own
tools/validate.py (7/7 pass today).
The spec is pinned by SHA in .feedpak-spec-ref rather than tracked from its
default branch, so a change over there cannot redden an unrelated PR here;
bump it in its own PR, where a red result is precisely the signal that core
does not satisfy the new spec.
A gate with no legitimate way to say "yes, deliberately, not yet" gets
switched off the first time it blocks a release, so there are two escape
hatches: the reserved `x-` key prefix (always permitted, and it tells every
third-party packer the key is not stable surface), and
feedpak-spec-exceptions.yml, which requires a tracking issue per entry. An
exception that goes stale — the spec caught up, or core stopped reading the
key — fails the build, so the allowlist cannot become somewhere drift
quietly accumulates. `original_audio` is seeded there against #933 so the
gate lands green and starts blocking the next instance immediately, rather
than requiring #933 to be resolved first.
Dev/CI tooling only; never on the serve or Docker path (constitution
Principle I). jsonschema is installed in the CI job, not added to
requirements.txt.
Signed-off-by: topkoa <topkoa@gmail.com>
topkoa