mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-11 02:44:30 +00:00
Merge pull request #934 from got-feedBack/chore/feedpak-spec-gate
ci: gate core against the feedpak spec
This commit is contained in:
@@ -0,0 +1,17 @@
|
|||||||
|
## What
|
||||||
|
|
||||||
|
<!-- What does this PR do, and why? Link the issue it addresses. -->
|
||||||
|
|
||||||
|
## feedpak surface
|
||||||
|
|
||||||
|
<!-- The feedpak spec is sacrosanct: the spec defines the format, this app implements it.
|
||||||
|
Delete this section ONLY if your change doesn't touch how the app reads or writes packs. -->
|
||||||
|
|
||||||
|
- [ ] This PR does **not** change how the app reads/writes feedpaks (manifest keys, pack files, folder layout)
|
||||||
|
- [ ] …or it does, and the spec change landed first via the [FEP process](https://github.com/got-feedback/feedpak-spec/blob/main/CONTRIBUTING.md) — FEP / spec PR: `got-feedback/feedpak-spec#___` (once it merges, re-run this PR's checks and the gate goes green)
|
||||||
|
|
||||||
|
## Checklist
|
||||||
|
|
||||||
|
- [ ] `CHANGELOG.md` `[Unreleased]` updated (user-visible changes)
|
||||||
|
- [ ] Tests added/updated for new behaviour
|
||||||
|
- [ ] Commits are DCO signed off (`git commit -s`)
|
||||||
@@ -124,6 +124,94 @@ jobs:
|
|||||||
print(f"Validated {len(manifests)} manifest(s) — OK")
|
print(f"Validated {len(manifests)} manifest(s) — OK")
|
||||||
EOF
|
EOF
|
||||||
|
|
||||||
|
feedpak-spec:
|
||||||
|
# Guard that core stays faithful to the feedpak format spec, which lives in
|
||||||
|
# its own repo (got-feedback/feedpak-spec) and is the contract third-party
|
||||||
|
# packers and players build against. Four surface checks: core reads/writes
|
||||||
|
# only manifest keys the spec declares (and the scanned-module list can't
|
||||||
|
# fall behind); the exception allowlist never grows, so the FEP process is
|
||||||
|
# the only way a new key lands; core ingests the spec's example packs; packs
|
||||||
|
# committed here pass the spec's reference validator. Motivated by
|
||||||
|
# #933, where a manifest key (`original_audio`) shipped in core without ever
|
||||||
|
# reaching the spec.
|
||||||
|
#
|
||||||
|
# The gate checks against the spec repo's HEAD, deliberately: the app must
|
||||||
|
# conform to the LIVING spec, always. The dev flow is self-serve — a gated
|
||||||
|
# PR opens a FEP, the spec PR merges, re-running this job goes green; no
|
||||||
|
# pin file to bump, nothing to maintain. Accepted trade-off: a BREAKING
|
||||||
|
# spec change (rare, deliberate, MAJOR per the spec's compatibility policy)
|
||||||
|
# reddens every PR here until core conforms — which is the correct
|
||||||
|
# org-wide signal that the app is out of conformance. The normal FEP is
|
||||||
|
# additive and can never redden this job.
|
||||||
|
name: feedpak-spec
|
||||||
|
runs-on: ubuntu-latest
|
||||||
|
steps:
|
||||||
|
- uses: actions/checkout@v4
|
||||||
|
# This job runs repository code (tools/check_spec_conformance.py) and
|
||||||
|
# never pushes; don't leave the token in git config for it.
|
||||||
|
# fetch-depth: 0 so the base branch is available — the gate must prove
|
||||||
|
# the exception allowlist didn't grow in this PR.
|
||||||
|
with:
|
||||||
|
persist-credentials: false
|
||||||
|
fetch-depth: 0
|
||||||
|
|
||||||
|
- uses: actions/setup-python@v5
|
||||||
|
with:
|
||||||
|
python-version: '3.12'
|
||||||
|
cache: 'pip'
|
||||||
|
|
||||||
|
- name: Check out feedpak-spec at HEAD
|
||||||
|
uses: actions/checkout@v4
|
||||||
|
with:
|
||||||
|
repository: got-feedback/feedpak-spec
|
||||||
|
ref: main
|
||||||
|
path: .feedpak-spec
|
||||||
|
persist-credentials: false
|
||||||
|
|
||||||
|
- name: Record the spec commit this run verified against
|
||||||
|
# HEAD-tracking means CI results can differ across time on the same
|
||||||
|
# commit. Log the exact spec SHA so a red run is reproducible.
|
||||||
|
run: git -C .feedpak-spec rev-parse HEAD
|
||||||
|
|
||||||
|
- name: Install dependencies
|
||||||
|
run: |
|
||||||
|
python -m pip install --upgrade pip
|
||||||
|
pip install -r requirements.txt
|
||||||
|
# CI-only: the spec's reference validator needs jsonschema. Not a
|
||||||
|
# runtime dependency — this gate never runs on the serve/Docker path
|
||||||
|
# (constitution Principle I). Pinned for the same reason the spec SHA
|
||||||
|
# is: an upstream release must not turn this job red on a PR that
|
||||||
|
# changed neither this repo nor the spec.
|
||||||
|
pip install 'jsonschema==4.26.0'
|
||||||
|
|
||||||
|
- name: Fetch the base branch's exception allowlist
|
||||||
|
id: baseline
|
||||||
|
run: |
|
||||||
|
# The allowlist is closed: it grandfathers keys that predate this gate
|
||||||
|
# and may only shrink. Prove that by diffing against the base branch —
|
||||||
|
# without this, anyone could append an entry and route around the FEP
|
||||||
|
# process from inside this repo.
|
||||||
|
#
|
||||||
|
# Resolve the base rather than hardcoding `main`: ship-ci.yml also runs
|
||||||
|
# this workflow for PRs into release/** and for pushes to release/**,
|
||||||
|
# where a main baseline would diff against the wrong branch.
|
||||||
|
# PR -> the branch it merges into
|
||||||
|
# push -> the branch itself (its tip already contains the change, so
|
||||||
|
# this is a no-op; enforcement happens at PR time)
|
||||||
|
BASE="${{ github.event.pull_request.base.ref || github.ref_name }}"
|
||||||
|
echo "diffing the allowlist against origin/$BASE"
|
||||||
|
git fetch --no-tags --depth=1 origin "$BASE"
|
||||||
|
if git cat-file -e FETCH_HEAD:feedpak-spec-exceptions.yml 2>/dev/null; then
|
||||||
|
git show FETCH_HEAD:feedpak-spec-exceptions.yml > "$RUNNER_TEMP/baseline-exceptions.yml"
|
||||||
|
echo "args=--baseline-exceptions $RUNNER_TEMP/baseline-exceptions.yml" >> "$GITHUB_OUTPUT"
|
||||||
|
else
|
||||||
|
# Only true until the PR that introduces this gate lands.
|
||||||
|
echo "args=--bootstrap-allowlist" >> "$GITHUB_OUTPUT"
|
||||||
|
fi
|
||||||
|
|
||||||
|
- name: Check feedpak spec conformance
|
||||||
|
run: python tools/check_spec_conformance.py --spec .feedpak-spec ${{ steps.baseline.outputs.args }}
|
||||||
|
|
||||||
lint:
|
lint:
|
||||||
# Maintainer/CI-only size + module-hygiene gate (constitution Principle I:
|
# Maintainer/CI-only size + module-hygiene gate (constitution Principle I:
|
||||||
# dev tooling, never on the serve/Docker path — same category as
|
# dev tooling, never on the serve/Docker path — same category as
|
||||||
|
|||||||
+26
-2
@@ -13,8 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
passport walls: genre badges computed on read from `song_stats` × the library's
|
passport walls: genre badges computed on read from `song_stats` × the library's
|
||||||
effective genre — Bronze = N genre songs at K★, data-driven in
|
effective genre — Bronze = N genre songs at K★, data-driven in
|
||||||
`plugins/career/passports.json`, default 5 songs at 2★ — plus qualifying-song
|
`plugins/career/passports.json`, default 5 songs at 2★ — plus qualifying-song
|
||||||
"ticket stubs", the library genre list, and drill status), `POST
|
"ticket stubs", the library genre list, and drill status), `POST /passports/commit`
|
||||||
/passports/commit` (instrument commitment), `POST /passports/open` (open a genre
|
(instrument commitment), `POST /passports/open` (open a genre
|
||||||
passport), and `POST /drill-state` (intake for the relayed Virtuoso
|
passport), and `POST /drill-state` (intake for the relayed Virtuoso
|
||||||
`virtuoso.progress` snapshot, so drill requirements can gate badges
|
`virtuoso.progress` snapshot, so drill requirements can gate badges
|
||||||
server-side). Badges are never stored; the only persisted state (commitments,
|
server-side). Badges are never stored; the only persisted state (commitments,
|
||||||
@@ -35,6 +35,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
localStorage snapshot to the drill-state intake on `virtuoso:progress` bus
|
localStorage snapshot to the drill-state intake on `virtuoso:progress` bus
|
||||||
events (debounced, plus a one-time bootstrap), closing the
|
events (debounced, plus a one-time bootstrap), closing the
|
||||||
fires-into-a-void seam without touching the virtuoso plugin.
|
fires-into-a-void seam without touching the virtuoso plugin.
|
||||||
|
- **CI gate: core must stay faithful to the feedpak spec (`feedpak-spec` job).** feedpak is published as
|
||||||
|
an open format with its own repo, normative spec, JSON Schemas, and reference validator — but nothing
|
||||||
|
stopped core from reading a manifest key the spec never defined, which is exactly what happened with
|
||||||
|
`original_audio` (#583 → #933). `tools/check_spec_conformance.py` now enforces four surface properties
|
||||||
|
in CI: (1) **key-coverage** — every manifest key core reads *or writes* is declared in the spec's
|
||||||
|
`manifest.schema.json`, found by walking the AST of `lib/sloppak.py`, `lib/enrichment.py`, and
|
||||||
|
`lib/songmeta.py`, `lib/gp2notation.py`, and `lib/routers/ws_highway.py` (writes are gated too — including
|
||||||
|
`setdefault()` — and reported separately: a key core writes lands in every pack we emit, so an undeclared
|
||||||
|
one seeds the ecosystem with non-spec data; a **readers-complete** guard fails the build if that module
|
||||||
|
list falls behind the codebase); (2) **allowlist-closed** — `feedpak-spec-exceptions.yml` never grows;
|
||||||
|
(3) **forward** — core's `load_song()` ingests every example pack the spec ships;
|
||||||
|
(4) **reverse** — every pack committed here passes the spec's own `tools/validate.py` (7/7 pass today).
|
||||||
|
The gate verifies against the spec repo's **HEAD** — the app must conform to the living spec, and the
|
||||||
|
flow is self-serve: a gated PR opens a FEP, the spec PR merges, re-running checks goes green. Nothing to
|
||||||
|
pin, nothing to bump. Each run logs the spec SHA it verified against so results are reproducible.
|
||||||
|
**There is no in-repo escape hatch, by design.** A blocked PR has exactly one route: land the key in the
|
||||||
|
spec via the [FEP process](https://github.com/got-feedback/feedpak-spec/blob/main/CONTRIBUTING.md), then
|
||||||
|
re-run the PR's checks — the gate verifies against the spec's HEAD, so it goes green once the key is real. `feedpak-spec-exceptions.yml` is a **closed
|
||||||
|
grandfather list** for keys that predate the gate, not a bypass: a fourth check (**allowlist-closed**)
|
||||||
|
diffs it against the base branch and fails any PR that *adds* an entry, so it may only shrink.
|
||||||
|
`original_audio` is grandfathered there against #933 so the gate lands green and starts blocking the
|
||||||
|
*next* instance immediately; the gate takes no position on how #933 resolves (the expected outcome is
|
||||||
|
removing the key, since the spec already carries the mixdown as a stem — not adopting it). Docs:
|
||||||
|
[docs/feedpak-spec-gate.md](docs/feedpak-spec-gate.md).
|
||||||
|
|
||||||
### Removed
|
### Removed
|
||||||
- **The classic v2 UI shell is gone — v3 is the only UI (R3a).** `static/index.html`, the
|
- **The classic v2 UI shell is gone — v3 is the only UI (R3a).** `static/index.html`, the
|
||||||
|
|||||||
@@ -588,6 +588,21 @@ tab, key/scale annotations, etc.). Published as **feedpak**; this codebase still
|
|||||||
**sloppak** name internally — same on-disk format. [docs/sloppak-spec.md](docs/sloppak-spec.md) is
|
**sloppak** name internally — same on-disk format. [docs/sloppak-spec.md](docs/sloppak-spec.md) is
|
||||||
a local pointer + code map.
|
a local pointer + code map.
|
||||||
|
|
||||||
|
**The spec is sacrosanct — read it BEFORE changing how this app reads or writes packs.** The
|
||||||
|
spec repo defines the format; this app merely implements it ("a change is not part of the format
|
||||||
|
until it lands here" — feedpak-spec/GOVERNANCE.md). Any new manifest key, file, or directory the
|
||||||
|
app touches must land in the spec **first**, via the
|
||||||
|
[FEP process](https://github.com/got-feedback/feedpak-spec/blob/main/CONTRIBUTING.md) (proposal
|
||||||
|
issue → one spec PR updating spec + schemas + example + changelog → then re-run your PR's checks
|
||||||
|
here; the gate verifies against the spec's HEAD, so it goes green the moment your key is real).
|
||||||
|
CI enforces this: the `feedpak-spec` job
|
||||||
|
([docs/feedpak-spec-gate.md](docs/feedpak-spec-gate.md)) fails any PR whose code touches a
|
||||||
|
manifest key the spec doesn't declare, and there is **no in-repo bypass** — the exceptions
|
||||||
|
file is a closed grandfather list that only shrinks. If the format seems to be missing something
|
||||||
|
you need, that's a FEP conversation, not a workaround. (Cautionary tale: `original_audio`, #933 —
|
||||||
|
shipped without a spec entry, and third-party packers reverse-engineered a folder convention out
|
||||||
|
of a code comment.)
|
||||||
|
|
||||||
**Key code:**
|
**Key code:**
|
||||||
- `lib/sloppak.py` — format detection, zip/directory resolution, metadata extraction, song loading
|
- `lib/sloppak.py` — format detection, zip/directory resolution, metadata extraction, song loading
|
||||||
- `lib/sloppak_convert.py` — sloppak assembly pipeline, Demucs stem splitting
|
- `lib/sloppak_convert.py` — sloppak assembly pipeline, Demucs stem splitting
|
||||||
|
|||||||
@@ -0,0 +1,132 @@
|
|||||||
|
# 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](https://github.com/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](https://github.com/got-feedback/feedback/issues/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](https://github.com/got-feedback/feedpak-spec/blob/main/CONTRIBUTING.md)):
|
||||||
|
|
||||||
|
1. **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.
|
||||||
|
2. **Discuss**, until it has a clear shape and rough consensus.
|
||||||
|
3. **Land one PR there** that updates the normative spec (`spec/feedpak-v1.md`), the relevant JSON
|
||||||
|
Schema(s), an example in `examples/` that exercises it, and the changelog — *together*. A PR touching
|
||||||
|
only one of those is incomplete.
|
||||||
|
4. **Back here**, just re-run your PR's checks. The gate verifies against the spec's HEAD, so the moment
|
||||||
|
your key is genuinely part of the format, your PR goes green — nothing to bump, nothing to maintain.
|
||||||
|
|
||||||
|
That's deliberately the only route — no experimental prefix, no self-serve allowlist — and it's usually a
|
||||||
|
quick one for additive keys. The reason it's worth the round-trip: the gate checks the whole repo against
|
||||||
|
the living spec, so if non-conformance ever lands, it shows up as red CI on *every* teammate's open PR, and
|
||||||
|
only the person who introduced it can clear it. Going through the FEP keeps your change clean and keeps
|
||||||
|
everyone else unblocked.
|
||||||
|
|
||||||
|
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](https://github.com/got-feedback/feedpak-spec/blob/main/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.
|
||||||
|
|
||||||
|
## Tracking the spec's HEAD
|
||||||
|
|
||||||
|
The gate checks out `feedpak-spec` at **HEAD**, on purpose: the app must conform to the *living* spec, and
|
||||||
|
nobody should have to maintain a pin. The dev flow is fully self-serve — gated PR → FEP → spec merge →
|
||||||
|
re-run checks → green.
|
||||||
|
|
||||||
|
Two properties to know about:
|
||||||
|
|
||||||
|
- **The normal FEP is additive** (a new optional key), which only ever makes the gate *looser* — it cannot
|
||||||
|
redden anyone's PR. Only a **breaking** spec change (removing/renaming a key the app uses, tightening the
|
||||||
|
validator against committed packs) turns PRs red repo-wide — and per the spec's compatibility policy that
|
||||||
|
is a rare, deliberate MAJOR event, exactly when an org-wide "the app is out of conformance" signal is the
|
||||||
|
right outcome. The CI job logs the exact spec SHA each run verified against, so a red run is reproducible.
|
||||||
|
- **CI results can change over time on the same commit** — that is inherent to tracking a living contract,
|
||||||
|
and it is the point: green means "conformant *now*", not "conformant when written".
|
||||||
|
|
||||||
|
## Limitations
|
||||||
|
|
||||||
|
Known, and worth fixing in follow-ups rather than blocking on:
|
||||||
|
|
||||||
|
- **Layer 1's receiver detection is heuristic.** Locals *assigned from* `load_manifest(...)` are discovered
|
||||||
|
flow-aware whatever they're called (chart.py's `m` taught us that), and the inline
|
||||||
|
`(load_manifest(p) or {}).get(...)` form is recognised — but a manifest that arrives as a **function
|
||||||
|
parameter** is only recognised by name (`MANIFEST_VARS`: `manifest`, `mf`). A parameter called something
|
||||||
|
else would slip. The hardening step is to route all manifest access through a single declared
|
||||||
|
`KNOWN_MANIFEST_KEYS` registry in `lib/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 `$ref` subschemas.
|
||||||
|
- **Layer 1 recognises `get`, `setdefault`, subscripts, and the known gap-fill helper** as key access.
|
||||||
|
`update()` and `pop()` aren't used against a feedpak manifest anywhere in the tree, so they're deliberately
|
||||||
|
not special-cased rather than speculatively handled. `readers-complete` reuses the same scanner
|
||||||
|
(`keys_touched()`), so this blind spot is shared, not doubled: a module using only unrecognised access forms
|
||||||
|
would evade both.
|
||||||
|
- **Layer 4 can't catch unknown keys**, because `manifest.schema.json` sets `additionalProperties: true` and
|
||||||
|
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 `--strict` mode). Until then,
|
||||||
|
layer 1 is the only thing standing between us and the next `original_audio`.
|
||||||
@@ -0,0 +1,50 @@
|
|||||||
|
# CLOSED grandfather list — manifest keys core reads or writes that predate the
|
||||||
|
# spec-conformance gate and that the feedpak spec does not define.
|
||||||
|
#
|
||||||
|
# Please don't add entries here — CI will flag any PR that grows this list, so
|
||||||
|
# it can only shrink over time. That's by design, not distrust: the moment the
|
||||||
|
# app touches a key the spec doesn't define, every teammate's PR starts failing
|
||||||
|
# the conformance gate too, and whoever added the key is the only person who
|
||||||
|
# can fix it. The FEP process below avoids putting anyone in that spot. The
|
||||||
|
# feedpak spec's own governance is explicit:
|
||||||
|
#
|
||||||
|
# "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."
|
||||||
|
# — got-feedback/feedpak-spec, GOVERNANCE.md
|
||||||
|
#
|
||||||
|
# So a new manifest key goes through the feedpak Enhancement Proposal (FEP)
|
||||||
|
# process — see feedpak-spec/CONTRIBUTING.md:
|
||||||
|
#
|
||||||
|
# 1. Open a FEP issue on got-feedback/feedpak-spec describing the problem, the
|
||||||
|
# on-disk shape, backward compatibility, and the version bump implied.
|
||||||
|
# 2. Land one PR there updating the normative spec, the JSON Schemas, an
|
||||||
|
# example that exercises it, and the changelog — together.
|
||||||
|
# 3. Back here, re-run this PR's checks. The gate verifies against the spec's
|
||||||
|
# HEAD, so once your key is in the spec, the gate goes green.
|
||||||
|
#
|
||||||
|
# That's the supported route — and usually a quick one for additive keys. If
|
||||||
|
# your PR is blocked by this gate, a FEP will get you unblocked properly; an
|
||||||
|
# entry here won't (CI rejects it).
|
||||||
|
#
|
||||||
|
# Entries below exist ONLY because they predate the gate. Each is debt with a
|
||||||
|
# tracking issue, and each disappears when its issue is fixed. The gate also
|
||||||
|
# fails if an entry goes stale — the spec caught up, or core no longer reads or
|
||||||
|
# writes the key — so this file cannot quietly become a place drift hides.
|
||||||
|
|
||||||
|
exceptions:
|
||||||
|
- key: original_audio
|
||||||
|
issue: https://github.com/got-feedback/feedback/issues/933
|
||||||
|
reason: >-
|
||||||
|
Added by #583 (the full mix played while every stem fader sits at unity,
|
||||||
|
since demucs recombination is lossy). Core, lib/enrichment.py, and the
|
||||||
|
stems plugin all depend on it, but it never went through a FEP and the
|
||||||
|
spec does not define it — the drift this gate exists to prevent.
|
||||||
|
|
||||||
|
The resolution is REMOVAL, not a FEP: the spec already carries the mixdown
|
||||||
|
as a stem ({id: full, file: stems/full.ogg}), so this key added a second,
|
||||||
|
redundant location for audio to a format that already had one. See #933.
|
||||||
|
|
||||||
|
Grandfathered so the gate can land green and start blocking the *next*
|
||||||
|
instance immediately, rather than blocking on #933. This entry goes away
|
||||||
|
when core no longer reads or writes the key.
|
||||||
@@ -0,0 +1,203 @@
|
|||||||
|
"""Regression tests for the feedpak spec-conformance gate.
|
||||||
|
|
||||||
|
The gate (tools/check_spec_conformance.py) is what keeps the app from drifting
|
||||||
|
away from the feedpak spec, so the gate itself must not be weakenable by a
|
||||||
|
quiet refactor: these tests pin its load-bearing behaviours — read/write
|
||||||
|
classification, the closed allowlist, and the duplicate/malformed-entry
|
||||||
|
rejections. If one of these fails, the spec's protection regressed.
|
||||||
|
"""
|
||||||
|
import importlib.util
|
||||||
|
import textwrap
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
_SPEC_GATE = Path(__file__).resolve().parent.parent / "tools" / "check_spec_conformance.py"
|
||||||
|
_spec = importlib.util.spec_from_file_location("check_spec_conformance", _SPEC_GATE)
|
||||||
|
gate = importlib.util.module_from_spec(_spec)
|
||||||
|
_spec.loader.exec_module(gate)
|
||||||
|
|
||||||
|
|
||||||
|
def _touch(tmp_path, source):
|
||||||
|
p = tmp_path / "mod.py"
|
||||||
|
p.write_text(textwrap.dedent(source), encoding="utf-8")
|
||||||
|
return gate.keys_touched(p)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------- keys_touched
|
||||||
|
|
||||||
|
def test_get_is_a_read(tmp_path):
|
||||||
|
reads, writes = _touch(tmp_path, 'x = manifest.get("title")')
|
||||||
|
assert reads == {"title"} and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_subscript_load_is_a_read(tmp_path):
|
||||||
|
reads, writes = _touch(tmp_path, 'x = manifest["artist"]')
|
||||||
|
assert reads == {"artist"} and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_subscript_store_is_a_write_not_a_read(tmp_path):
|
||||||
|
# The original scan ignored ctx and scored this as a read (lib/songmeta.py
|
||||||
|
# pattern). A regression here reopens the emitted-key blind spot.
|
||||||
|
reads, writes = _touch(tmp_path, 'manifest["year"] = 1999')
|
||||||
|
assert writes == {"year"} and reads == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_setdefault_is_a_write(tmp_path):
|
||||||
|
# lib/gp2notation.py stamps feedpak_version this way; a subscript-only scan
|
||||||
|
# missed it entirely.
|
||||||
|
reads, writes = _touch(tmp_path, 'manifest.setdefault("feedpak_version", "1.2.0")')
|
||||||
|
assert writes == {"feedpak_version"} and reads == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_manifest_wrapped_get_is_seen(tmp_path):
|
||||||
|
# lib/enrichment.py idiom: (load_manifest(p) or {}).get("key")
|
||||||
|
reads, writes = _touch(
|
||||||
|
tmp_path, 'rel = (sloppak_mod.load_manifest(p) or {}).get("original_audio")'
|
||||||
|
)
|
||||||
|
assert "original_audio" in reads
|
||||||
|
|
||||||
|
|
||||||
|
def test_flow_aware_receiver_any_name(tmp_path):
|
||||||
|
# lib/routers/chart.py binds `m = load_manifest(p) or {}` — a fixed name
|
||||||
|
# list missed it and the module's reads went entirely unscanned. Locals
|
||||||
|
# assigned from load_manifest must be receivers whatever they're called.
|
||||||
|
reads, writes = _touch(
|
||||||
|
tmp_path,
|
||||||
|
"""
|
||||||
|
pak_info = sloppak_mod.load_manifest(p) or {}
|
||||||
|
x = pak_info.get("stems")
|
||||||
|
pak_info["genres"] = ["metal"]
|
||||||
|
""",
|
||||||
|
)
|
||||||
|
assert reads == {"stems"} and writes == {"genres"}
|
||||||
|
|
||||||
|
|
||||||
|
def test_plain_dict_named_m_is_not_a_receiver(tmp_path):
|
||||||
|
# Flow-awareness must not make every short local a manifest: `m` bound to
|
||||||
|
# something other than load_manifest stays out of the scan.
|
||||||
|
reads, writes = _touch(tmp_path, 'm = {}\nx = m.get("title")')
|
||||||
|
assert reads == set() and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_unrelated_dicts_are_ignored(tmp_path):
|
||||||
|
reads, writes = _touch(tmp_path, 'x = config.get("title"); settings["artist"] = 1')
|
||||||
|
assert reads == set() and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_non_literal_keys_are_ignored(tmp_path):
|
||||||
|
reads, writes = _touch(tmp_path, 'x = manifest.get(key_var); manifest[key_var] = 1')
|
||||||
|
assert reads == set() and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
def test_manifest_key_read_helper_is_seen(tmp_path):
|
||||||
|
# lib/routers/song.py uses this helper for gap-fill proposals. If helpers
|
||||||
|
# are invisible, adding a new literal key through that path bypasses both
|
||||||
|
# key-coverage and readers-complete.
|
||||||
|
reads, writes = _touch(
|
||||||
|
tmp_path,
|
||||||
|
'_gap_fill_manifest_absent(manifest, "album")\n'
|
||||||
|
'_gap_fill_manifest_absent(manifest, dynamic_key)\n',
|
||||||
|
)
|
||||||
|
assert reads == {"album"} and writes == set()
|
||||||
|
|
||||||
|
|
||||||
|
# ------------------------------------------------------------ exceptions file
|
||||||
|
|
||||||
|
def test_duplicate_exception_key_is_rejected():
|
||||||
|
doc = """
|
||||||
|
exceptions:
|
||||||
|
- key: original_audio
|
||||||
|
issue: https://example.com/1
|
||||||
|
- key: original_audio
|
||||||
|
issue: https://example.com/2
|
||||||
|
"""
|
||||||
|
with pytest.raises(SystemExit):
|
||||||
|
gate._parse_exceptions(textwrap.dedent(doc), "test")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("doc", [
|
||||||
|
"- just\n- a\n- list\n", # list at top level
|
||||||
|
"exceptions: not-a-list\n", # scalar where list expected
|
||||||
|
"exceptions:\n - just-a-string\n", # non-mapping entry
|
||||||
|
"exceptions: [\n", # invalid YAML
|
||||||
|
])
|
||||||
|
def test_malformed_exceptions_fail_legibly(doc):
|
||||||
|
# Malformed shapes must exit with a ::error::, not an AttributeError
|
||||||
|
# traceback — CI output has to say what to fix.
|
||||||
|
with pytest.raises(SystemExit):
|
||||||
|
gate._parse_exceptions(doc, "test")
|
||||||
|
|
||||||
|
|
||||||
|
def test_exception_without_issue_is_rejected():
|
||||||
|
# No tracking issue, no exception — entries are debt and debt is tracked.
|
||||||
|
doc = """
|
||||||
|
exceptions:
|
||||||
|
- key: original_audio
|
||||||
|
"""
|
||||||
|
with pytest.raises(SystemExit):
|
||||||
|
gate._parse_exceptions(textwrap.dedent(doc), "test")
|
||||||
|
|
||||||
|
|
||||||
|
# --------------------------------------------------------- allowlist is CLOSED
|
||||||
|
|
||||||
|
def _yml(tmp_path, name, keys):
|
||||||
|
p = tmp_path / name
|
||||||
|
entries = "".join(
|
||||||
|
f" - key: {k}\n issue: https://example.com/{k}\n" for k in keys
|
||||||
|
)
|
||||||
|
p.write_text("exceptions:\n" + (entries or " []\n"), encoding="utf-8")
|
||||||
|
return p
|
||||||
|
|
||||||
|
|
||||||
|
def test_allowlist_may_not_grow(tmp_path, monkeypatch):
|
||||||
|
# THE core property: adding an entry must fail, or the FEP process has an
|
||||||
|
# in-repo bypass and the gate is a speed bump with a signed excuse note.
|
||||||
|
baseline = _yml(tmp_path, "base.yml", ["original_audio"])
|
||||||
|
current = _yml(tmp_path, "current.yml", ["original_audio", "sneaky_new_key"])
|
||||||
|
monkeypatch.setattr(gate, "EXCEPTIONS_FILE", current)
|
||||||
|
assert gate.check_allowlist_closed(baseline, bootstrap=False) is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_allowlist_may_shrink(tmp_path, monkeypatch):
|
||||||
|
baseline = _yml(tmp_path, "base.yml", ["original_audio"])
|
||||||
|
current = _yml(tmp_path, "current.yml", [])
|
||||||
|
monkeypatch.setattr(gate, "EXCEPTIONS_FILE", current)
|
||||||
|
assert gate.check_allowlist_closed(baseline, bootstrap=False) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_allowlist_steady_state_passes(tmp_path, monkeypatch):
|
||||||
|
baseline = _yml(tmp_path, "base.yml", ["original_audio"])
|
||||||
|
current = _yml(tmp_path, "current.yml", ["original_audio"])
|
||||||
|
monkeypatch.setattr(gate, "EXCEPTIONS_FILE", current)
|
||||||
|
assert gate.check_allowlist_closed(baseline, bootstrap=False) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_bootstrap_skips_the_diff(tmp_path, monkeypatch):
|
||||||
|
current = _yml(tmp_path, "current.yml", ["original_audio"])
|
||||||
|
monkeypatch.setattr(gate, "EXCEPTIONS_FILE", current)
|
||||||
|
assert gate.check_allowlist_closed(None, bootstrap=True) is True
|
||||||
|
|
||||||
|
|
||||||
|
# ------------------------------------------------------------ readers-complete
|
||||||
|
|
||||||
|
def test_readers_list_matches_the_codebase():
|
||||||
|
# If this fails, a module started touching feedpak manifests without being
|
||||||
|
# added to READERS — its keys are going unchecked. Same check CI runs.
|
||||||
|
assert gate.check_readers_complete() is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_no_undeclared_keys_beyond_the_grandfathered(monkeypatch):
|
||||||
|
# Every key core touches is either spec-declared or grandfathered with a
|
||||||
|
# tracking issue. New keys go through the FEP process, full stop.
|
||||||
|
reads, writes = set(), set()
|
||||||
|
for rel in gate.READERS:
|
||||||
|
r, w = gate.keys_touched(gate.REPO / rel)
|
||||||
|
reads |= r
|
||||||
|
writes |= w
|
||||||
|
grandfathered = set(gate.load_exceptions())
|
||||||
|
# Not asserting against the spec here (no spec checkout in unit tests) —
|
||||||
|
# asserting the *shape*: the only non-spec keys tolerated are grandfathered,
|
||||||
|
# and today that is exactly {original_audio}.
|
||||||
|
assert grandfathered == {"original_audio"}
|
||||||
|
assert "original_audio" in reads
|
||||||
@@ -0,0 +1,537 @@
|
|||||||
|
#!/usr/bin/env python3
|
||||||
|
"""feedpak spec-conformance gate.
|
||||||
|
|
||||||
|
feedpak is an open, versioned format with its own normative spec, JSON Schemas,
|
||||||
|
and reference validator (https://github.com/got-feedback/feedpak-spec). That
|
||||||
|
makes the spec a contract with everyone outside this repo: third-party packers,
|
||||||
|
converters, and players build against it. When core reads a manifest key the
|
||||||
|
spec never defined, the contract quietly breaks — a spec-compliant pack stops
|
||||||
|
being a fully-working pack, and the format's real definition migrates into our
|
||||||
|
source tree. See #933 for the instance that motivated this gate.
|
||||||
|
|
||||||
|
We cannot mechanically prove core *interprets* a key the way the spec means. We
|
||||||
|
can prove four surface properties, and those cover the drift that actually
|
||||||
|
happens:
|
||||||
|
|
||||||
|
1. key-coverage — every manifest key core reads OR WRITES is declared by the
|
||||||
|
spec. (Guarded by check_readers_complete(), so the list of
|
||||||
|
scanned modules cannot quietly fall behind the codebase.)
|
||||||
|
2. allowlist-closed— feedpak-spec-exceptions.yml never grows. It grandfathers
|
||||||
|
keys that predate this gate; it is not a way to merge a
|
||||||
|
new one. The only route for a new key is the FEP process.
|
||||||
|
3. forward — core ingests the spec's own example packs.
|
||||||
|
4. reverse — packs committed here satisfy the spec's reference validator.
|
||||||
|
|
||||||
|
Dev/CI tooling only: never imported on the serve or Docker path (constitution
|
||||||
|
Principle I — same category as scripts/build-tailwind.sh). `jsonschema` is
|
||||||
|
therefore a CI-only dependency, not a runtime requirement.
|
||||||
|
|
||||||
|
Usage:
|
||||||
|
python tools/check_spec_conformance.py --spec <path-to-feedpak-spec-checkout>
|
||||||
|
|
||||||
|
Exit status is 0 only when every layer passes.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import argparse
|
||||||
|
import ast
|
||||||
|
import json
|
||||||
|
import re
|
||||||
|
import subprocess
|
||||||
|
import sys
|
||||||
|
import tempfile
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
REPO = Path(__file__).resolve().parent.parent
|
||||||
|
|
||||||
|
# Modules that read or write a feedpak manifest dict. Explicit rather than
|
||||||
|
# globbed, because `manifest` is an overloaded name in this codebase: the
|
||||||
|
# loose-folder format (lib/loosefolder.py) and the diagnostics bundle
|
||||||
|
# (lib/diagnostics_bundle.py) both have their own unrelated `manifest`, and
|
||||||
|
# scanning those would flag *their* keys as feedpak drift.
|
||||||
|
#
|
||||||
|
# A hand-maintained list is itself a blind spot, so check_readers_complete()
|
||||||
|
# below re-derives the set and fails if this list has fallen behind. A missing
|
||||||
|
# file here is a hard error too, so a rename cannot silently disable the scan.
|
||||||
|
READERS = [
|
||||||
|
"lib/sloppak.py",
|
||||||
|
"lib/enrichment.py",
|
||||||
|
"lib/songmeta.py",
|
||||||
|
"lib/gp2notation.py", # rewrites manifest.yaml; stamps feedpak_version
|
||||||
|
"lib/routers/ws_highway.py", # reads `authors` off a feedpak manifest
|
||||||
|
"lib/routers/chart.py", # Get-info panel: binds `m = load_manifest(...)`
|
||||||
|
"lib/routers/song.py", # enrichment gap-fill: reads the manifest directly
|
||||||
|
]
|
||||||
|
|
||||||
|
# Where check_readers_complete() looks for modules READERS may have missed.
|
||||||
|
READER_SEARCH = ["lib/**/*.py", "server.py"]
|
||||||
|
|
||||||
|
# A module is handling a *feedpak* manifest (rather than some other manifest) if
|
||||||
|
# it shows one of these signals. lib/loosefolder.py and lib/diagnostics_bundle.py
|
||||||
|
# score zero on all of them, which is what keeps their keys out of the scan.
|
||||||
|
FEEDPAK_SIGNALS = re.compile(r"import sloppak|from sloppak|load_manifest|manifest\.yaml|feedpak")
|
||||||
|
|
||||||
|
# Locals assumed to hold a manifest dict by NAME. This is only the fallback for
|
||||||
|
# manifests that arrive as function parameters (ws_highway's `manifest` arg);
|
||||||
|
# locals ASSIGNED from load_manifest(...) are discovered flow-aware in
|
||||||
|
# keys_touched(), whatever they are called — chart.py's `m` taught us that a
|
||||||
|
# name list alone silently misses real readers.
|
||||||
|
MANIFEST_VARS = {"manifest", "mf"}
|
||||||
|
|
||||||
|
# Helper functions that take `(manifest, "literal_key", ...)` and read the
|
||||||
|
# manifest for that key. Keep this narrow: only helpers whose first argument is
|
||||||
|
# the manifest dict and whose second argument is a top-level manifest key belong
|
||||||
|
# here.
|
||||||
|
MANIFEST_KEY_READ_HELPERS = {"_gap_fill_manifest_absent"}
|
||||||
|
|
||||||
|
# Packs committed to this repo, checked against the spec's reference validator.
|
||||||
|
PACK_GLOBS = ["content/starter/*.feedpak", "docs/**/*.sloppak", "docs/**/*.feedpak"]
|
||||||
|
|
||||||
|
EXCEPTIONS_FILE = REPO / "feedpak-spec-exceptions.yml"
|
||||||
|
|
||||||
|
# How a new manifest key gets into core. There is no in-repo shortcut, by design:
|
||||||
|
# the spec's own governance says "a change is not part of the format until it
|
||||||
|
# lands here", and the FEP process is how it lands.
|
||||||
|
FEP = (
|
||||||
|
"New manifest keys go through the feedpak Enhancement Proposal process "
|
||||||
|
"(https://github.com/got-feedback/feedpak-spec/blob/main/CONTRIBUTING.md): land a PR on "
|
||||||
|
"feedpak-spec that updates the normative spec, the JSON Schemas, an example, and the "
|
||||||
|
"changelog together — then re-run this PR's checks; the gate verifies against the "
|
||||||
|
"spec's HEAD, so once your key is in the spec, this PR goes green. It matters beyond "
|
||||||
|
"this PR: the whole repo is checked against the living spec, so non-conformance that "
|
||||||
|
"slips in shows up as red CI on every teammate's PR until it's resolved — sorting it "
|
||||||
|
"out here keeps everyone else unblocked."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _fail(msg: str) -> None:
|
||||||
|
print(f"::error::{msg}")
|
||||||
|
|
||||||
|
|
||||||
|
def _manifest_locals(tree: ast.AST) -> set[str]:
|
||||||
|
"""Names of locals assigned from `load_manifest(...)` anywhere in `tree`.
|
||||||
|
|
||||||
|
Flow-aware receiver discovery: chart.py binds `m = load_manifest(p) or {}`,
|
||||||
|
and a fixed name list (`manifest`, `mf`) silently missed it — the module's
|
||||||
|
reads went entirely unscanned. Whatever the local is called, an assignment
|
||||||
|
whose right-hand side mentions load_manifest marks it as a manifest dict.
|
||||||
|
"""
|
||||||
|
names: set[str] = set()
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if not isinstance(node, (ast.Assign, ast.AnnAssign)):
|
||||||
|
continue
|
||||||
|
try:
|
||||||
|
rhs = ast.unparse(node.value) if node.value else ""
|
||||||
|
except Exception:
|
||||||
|
continue
|
||||||
|
if "load_manifest" not in rhs:
|
||||||
|
continue
|
||||||
|
targets = node.targets if isinstance(node, ast.Assign) else [node.target]
|
||||||
|
for t in targets:
|
||||||
|
if isinstance(t, ast.Name):
|
||||||
|
names.add(t.id)
|
||||||
|
return names
|
||||||
|
|
||||||
|
|
||||||
|
def _is_manifest_receiver(node: ast.expr, receivers: set[str]) -> bool:
|
||||||
|
"""True when `node` evaluates to a manifest dict.
|
||||||
|
|
||||||
|
Covers named receivers (fixed names + flow-discovered locals) plus the
|
||||||
|
inline wrapped form used in lib/enrichment.py:
|
||||||
|
`(sloppak_mod.load_manifest(p) or {}).get("key")`.
|
||||||
|
"""
|
||||||
|
if isinstance(node, ast.Name) and node.id in receivers:
|
||||||
|
return True
|
||||||
|
try:
|
||||||
|
src = ast.unparse(node)
|
||||||
|
except Exception:
|
||||||
|
return False
|
||||||
|
return "load_manifest" in src
|
||||||
|
|
||||||
|
|
||||||
|
def keys_touched(path: Path) -> tuple[set[str], set[str]]:
|
||||||
|
"""Literal top-level manifest keys `path` reads and writes, separately.
|
||||||
|
|
||||||
|
Writes matter as much as reads: `manifest["k"] = v` means core *emits* `k`
|
||||||
|
into a pack it ships, so an undeclared key there puts non-spec surface into
|
||||||
|
the wild — the same drift, pointed outward. `manifest["k"]` in a subscript
|
||||||
|
is a read only when its context is a Load; an `ast.walk` that ignores `ctx`
|
||||||
|
would score `manifest["year"] = ...` (lib/songmeta.py) as a read.
|
||||||
|
"""
|
||||||
|
reads: set[str] = set()
|
||||||
|
writes: set[str] = set()
|
||||||
|
tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path))
|
||||||
|
receivers = MANIFEST_VARS | _manifest_locals(tree)
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if (
|
||||||
|
isinstance(node, ast.Call)
|
||||||
|
and isinstance(node.func, ast.Attribute)
|
||||||
|
# `setdefault("k", v)` writes k when absent — lib/gp2notation.py
|
||||||
|
# stamps feedpak_version that way, and a subscript-only scan misses
|
||||||
|
# it entirely, letting an emitted key slip past the gate.
|
||||||
|
and node.func.attr in ("get", "setdefault")
|
||||||
|
and _is_manifest_receiver(node.func.value, receivers)
|
||||||
|
and node.args
|
||||||
|
and isinstance(node.args[0], ast.Constant)
|
||||||
|
and isinstance(node.args[0].value, str)
|
||||||
|
):
|
||||||
|
bucket = writes if node.func.attr == "setdefault" else reads
|
||||||
|
bucket.add(node.args[0].value)
|
||||||
|
elif (
|
||||||
|
isinstance(node, ast.Call)
|
||||||
|
and isinstance(node.func, ast.Name)
|
||||||
|
and node.func.id in MANIFEST_KEY_READ_HELPERS
|
||||||
|
and len(node.args) >= 2
|
||||||
|
and _is_manifest_receiver(node.args[0], receivers)
|
||||||
|
and isinstance(node.args[1], ast.Constant)
|
||||||
|
and isinstance(node.args[1].value, str)
|
||||||
|
):
|
||||||
|
reads.add(node.args[1].value)
|
||||||
|
elif (
|
||||||
|
isinstance(node, ast.Subscript)
|
||||||
|
and _is_manifest_receiver(node.value, receivers)
|
||||||
|
and isinstance(node.slice, ast.Constant)
|
||||||
|
and isinstance(node.slice.value, str)
|
||||||
|
):
|
||||||
|
target = writes if isinstance(node.ctx, ast.Store) else reads
|
||||||
|
target.add(node.slice.value)
|
||||||
|
return reads, writes
|
||||||
|
|
||||||
|
|
||||||
|
def _parse_exceptions(text: str, origin: str) -> dict[str, str]:
|
||||||
|
"""Parse an exceptions document into {key: tracking issue}."""
|
||||||
|
import yaml # runtime dep (PyYAML is already in requirements.txt)
|
||||||
|
|
||||||
|
try:
|
||||||
|
data = yaml.safe_load(text) or {}
|
||||||
|
except yaml.YAMLError as e:
|
||||||
|
_fail(f"{origin}: not valid YAML — {e}")
|
||||||
|
sys.exit(1)
|
||||||
|
# A malformed shape (list/string at top level, non-mapping entry) must fail
|
||||||
|
# with a CI-legible error, not an AttributeError traceback.
|
||||||
|
if not isinstance(data, dict):
|
||||||
|
_fail(f"{origin}: top level must be a mapping with an 'exceptions' list, got {type(data).__name__}")
|
||||||
|
sys.exit(1)
|
||||||
|
entries = data.get("exceptions") or []
|
||||||
|
if not isinstance(entries, list):
|
||||||
|
_fail(f"{origin}: 'exceptions' must be a list, got {type(entries).__name__}")
|
||||||
|
sys.exit(1)
|
||||||
|
out: dict[str, str] = {}
|
||||||
|
for entry in entries:
|
||||||
|
if not isinstance(entry, dict):
|
||||||
|
_fail(f"{origin}: each exception must be a mapping with 'key' and 'issue', got {type(entry).__name__}")
|
||||||
|
sys.exit(1)
|
||||||
|
key, issue = entry.get("key"), entry.get("issue")
|
||||||
|
if not key or not issue:
|
||||||
|
_fail(f"{origin}: every exception needs both 'key' and 'issue'")
|
||||||
|
sys.exit(1)
|
||||||
|
# A duplicate would silently take the last issue link, quietly retargeting
|
||||||
|
# the debt this file exists to track. Fail instead.
|
||||||
|
if key in out:
|
||||||
|
_fail(
|
||||||
|
f"{origin}: '{key}' is listed more than once. "
|
||||||
|
f"Keep one entry per key so the tracking issue is unambiguous."
|
||||||
|
)
|
||||||
|
sys.exit(1)
|
||||||
|
out[key] = issue
|
||||||
|
return out
|
||||||
|
|
||||||
|
|
||||||
|
def load_exceptions() -> dict[str, str]:
|
||||||
|
"""Map of grandfathered key -> tracking issue URL, as of this working tree."""
|
||||||
|
if not EXCEPTIONS_FILE.exists():
|
||||||
|
return {}
|
||||||
|
return _parse_exceptions(
|
||||||
|
EXCEPTIONS_FILE.read_text(encoding="utf-8"), EXCEPTIONS_FILE.name
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def check_allowlist_closed(baseline: Path | None, bootstrap: bool) -> bool:
|
||||||
|
"""The allowlist is CLOSED: it may shrink, never grow.
|
||||||
|
|
||||||
|
`feedpak-spec-exceptions.yml` grandfathers keys that predate this gate. It is
|
||||||
|
not a way to merge a new one. Without this check the gate would be a speed
|
||||||
|
bump with a signed excuse note — anyone could append an entry and route
|
||||||
|
around the FEP process from inside this repo, which is exactly the drift that
|
||||||
|
produced #933.
|
||||||
|
|
||||||
|
So: removing an entry is fine (that's the debt being paid down); adding one
|
||||||
|
fails the build, and the error points at the FEP process instead.
|
||||||
|
"""
|
||||||
|
if bootstrap:
|
||||||
|
print(" allowlist-closed: bootstrapping (no baseline on the base branch) — skipped")
|
||||||
|
return True
|
||||||
|
if baseline is None:
|
||||||
|
print(" allowlist-closed: no baseline supplied (local run) — skipped")
|
||||||
|
return True
|
||||||
|
|
||||||
|
if not baseline.is_file():
|
||||||
|
_fail(
|
||||||
|
f"--baseline-exceptions {baseline} does not exist. CI derives this from the base "
|
||||||
|
f"branch; for a local run, omit the flag to skip the allowlist diff."
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
base_keys = set(
|
||||||
|
_parse_exceptions(baseline.read_text(encoding="utf-8"), f"{EXCEPTIONS_FILE.name} (base)")
|
||||||
|
)
|
||||||
|
now_keys = set(load_exceptions())
|
||||||
|
added = sorted(now_keys - base_keys)
|
||||||
|
removed = sorted(base_keys - now_keys)
|
||||||
|
|
||||||
|
for key in added:
|
||||||
|
_fail(
|
||||||
|
f"{EXCEPTIONS_FILE.name}: this PR adds an exception for '{key}', and the allowlist "
|
||||||
|
f"can't take new entries — it only grandfathers keys that predate the gate. {FEP}"
|
||||||
|
)
|
||||||
|
if removed:
|
||||||
|
print(f" allowlist shrank (debt paid down): {', '.join(removed)}")
|
||||||
|
print(f" allowlist-closed: {'FAILED' if added else 'OK'}")
|
||||||
|
return not added
|
||||||
|
|
||||||
|
|
||||||
|
def check_readers_complete() -> bool:
|
||||||
|
"""READERS must not fall behind the codebase.
|
||||||
|
|
||||||
|
The key-coverage scan is only as good as the list of modules it scans, and a
|
||||||
|
hand-maintained list rots: `lib/routers/ws_highway.py` and
|
||||||
|
`lib/gp2notation.py` both touched feedpak manifests for a while without being
|
||||||
|
on it. So re-derive the set — any module that both touches manifest keys and
|
||||||
|
shows a feedpak signal must be listed — and fail if one is missing.
|
||||||
|
|
||||||
|
This is a guard on the gate itself, not on the format.
|
||||||
|
"""
|
||||||
|
listed = set(READERS)
|
||||||
|
missing: list[str] = []
|
||||||
|
for pattern in READER_SEARCH:
|
||||||
|
for path in sorted(REPO.glob(pattern)):
|
||||||
|
rel = path.relative_to(REPO).as_posix()
|
||||||
|
if rel in listed:
|
||||||
|
continue
|
||||||
|
src = path.read_text(encoding="utf-8", errors="replace")
|
||||||
|
if not FEEDPAK_SIGNALS.search(src):
|
||||||
|
continue
|
||||||
|
# Same scanner the coverage check uses — a separate "does it touch
|
||||||
|
# keys" regex diverged from it once already (`m = load_manifest(...)`
|
||||||
|
# in chart.py matched neither `manifest` nor `mf`, so the module
|
||||||
|
# went unlisted AND unscanned). One detector, one truth.
|
||||||
|
try:
|
||||||
|
reads, writes = keys_touched(path)
|
||||||
|
except SyntaxError:
|
||||||
|
continue
|
||||||
|
if reads or writes:
|
||||||
|
missing.append(rel)
|
||||||
|
|
||||||
|
for rel in missing:
|
||||||
|
_fail(
|
||||||
|
f"{rel} touches feedpak manifest keys but is not in READERS "
|
||||||
|
f"({Path(__file__).name}) — its keys are going unchecked. Add it."
|
||||||
|
)
|
||||||
|
print(f" scanning {len(listed)} modules; readers-complete: {'FAILED' if missing else 'OK'}")
|
||||||
|
return not missing
|
||||||
|
|
||||||
|
|
||||||
|
def check_key_coverage(spec: Path) -> bool:
|
||||||
|
"""Layer 1 — core must not read or write a manifest key the spec does not declare."""
|
||||||
|
schema = json.loads((spec / "schemas" / "manifest.schema.json").read_text(encoding="utf-8"))
|
||||||
|
declared = set(schema.get("properties") or {})
|
||||||
|
if not declared:
|
||||||
|
_fail("spec manifest.schema.json declares no properties — wrong path or bad checkout?")
|
||||||
|
return False
|
||||||
|
|
||||||
|
reads: set[str] = set()
|
||||||
|
writes: set[str] = set()
|
||||||
|
for rel in READERS:
|
||||||
|
path = REPO / rel
|
||||||
|
if not path.exists():
|
||||||
|
_fail(f"reader {rel} not found — was it renamed? Update READERS in {Path(__file__).name}.")
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
r, w = keys_touched(path)
|
||||||
|
except SyntaxError as e:
|
||||||
|
# A module that doesn't parse can't be scanned — but it also can't
|
||||||
|
# pass pytest, so this is belt-and-braces for a CI-legible message
|
||||||
|
# rather than a traceback if this job runs first.
|
||||||
|
_fail(f"could not scan {rel}: {e}")
|
||||||
|
return False
|
||||||
|
reads |= r
|
||||||
|
writes |= w
|
||||||
|
|
||||||
|
exceptions = load_exceptions()
|
||||||
|
ok = True
|
||||||
|
|
||||||
|
def _undeclared(keys: set[str]) -> list[str]:
|
||||||
|
return sorted((keys - declared) - set(exceptions))
|
||||||
|
|
||||||
|
for key in _undeclared(reads):
|
||||||
|
_fail(f"core reads manifest key '{key}', which the feedpak spec does not define. {FEP}")
|
||||||
|
ok = False
|
||||||
|
|
||||||
|
for key in _undeclared(writes):
|
||||||
|
_fail(
|
||||||
|
f"core writes manifest key '{key}', which the feedpak spec does not define — that "
|
||||||
|
f"puts non-spec surface into every pack we emit. {FEP}"
|
||||||
|
)
|
||||||
|
ok = False
|
||||||
|
|
||||||
|
# A stale exception is its own bug: it means the spec caught up and nobody
|
||||||
|
# cleaned up, so the allowlist slowly becomes a place drift hides.
|
||||||
|
touched = reads | writes
|
||||||
|
for key, issue in exceptions.items():
|
||||||
|
if key in declared:
|
||||||
|
_fail(
|
||||||
|
f"'{key}' is listed in {EXCEPTIONS_FILE.name} but the spec now declares it. "
|
||||||
|
f"Remove the exception and close {issue}."
|
||||||
|
)
|
||||||
|
ok = False
|
||||||
|
elif key not in touched:
|
||||||
|
_fail(
|
||||||
|
f"'{key}' is listed in {EXCEPTIONS_FILE.name} but core no longer reads or writes "
|
||||||
|
f"it. Remove the exception."
|
||||||
|
)
|
||||||
|
ok = False
|
||||||
|
|
||||||
|
print(f" spec declares {len(declared)} keys; core reads {len(reads)}, writes {len(writes)}")
|
||||||
|
if exceptions:
|
||||||
|
print(f" grandfathered (tracked debt): {', '.join(sorted(exceptions))}")
|
||||||
|
print(f" key-coverage: {'OK' if ok else 'FAILED'}")
|
||||||
|
return ok
|
||||||
|
|
||||||
|
|
||||||
|
def check_forward(spec: Path) -> bool:
|
||||||
|
"""Layer 3 — core must ingest every example pack the spec ships."""
|
||||||
|
examples_dir = spec / "examples"
|
||||||
|
if not examples_dir.is_dir():
|
||||||
|
_fail(f"{examples_dir} is missing — wrong path or bad checkout?")
|
||||||
|
return False
|
||||||
|
# rglob, not iterdir: the contract is "every example pack the spec ships", so
|
||||||
|
# a pack nested under examples/<group>/ must not slip through.
|
||||||
|
#
|
||||||
|
# Deliberately NOT filtered by is_file(): a feedpak is dual-form — a zip
|
||||||
|
# (`foo.feedpak`) *or* a directory (`foo.feedpak/`) — and the spec's own
|
||||||
|
# examples ship as directories today. An is_file() guard here would silently
|
||||||
|
# match zero packs. Matching on the suffix covers both forms, and rglob does
|
||||||
|
# not smuggle in a pack's innards because files inside a pack don't carry a
|
||||||
|
# pack suffix.
|
||||||
|
examples = sorted(
|
||||||
|
p for p in examples_dir.rglob("*")
|
||||||
|
if p.suffix in (".feedpak", ".sloppak")
|
||||||
|
)
|
||||||
|
if not examples:
|
||||||
|
_fail("spec ships no example packs — wrong path or bad checkout?")
|
||||||
|
return False
|
||||||
|
|
||||||
|
sys.path.insert(0, str(REPO / "lib"))
|
||||||
|
try:
|
||||||
|
import sloppak # noqa: E402 (path must be set first — flat imports, no package)
|
||||||
|
except Exception as e:
|
||||||
|
_fail(
|
||||||
|
f"could not import core's sloppak loader ({type(e).__name__}: {e}). "
|
||||||
|
f"Are requirements.txt deps installed?"
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
|
||||||
|
ok = True
|
||||||
|
with tempfile.TemporaryDirectory() as tmp:
|
||||||
|
cache = Path(tmp)
|
||||||
|
for pack in examples:
|
||||||
|
try:
|
||||||
|
loaded = sloppak.load_song(pack.name, pack.parent, cache)
|
||||||
|
except Exception as e:
|
||||||
|
_fail(
|
||||||
|
f"core failed to load the spec's own example pack {pack.name}: "
|
||||||
|
f"{type(e).__name__}: {e}. A spec-valid pack must load."
|
||||||
|
)
|
||||||
|
ok = False
|
||||||
|
continue
|
||||||
|
if not loaded.song.arrangements:
|
||||||
|
_fail(f"core loaded {pack.name} but found no arrangements")
|
||||||
|
ok = False
|
||||||
|
continue
|
||||||
|
print(f" loaded {pack.name}: {len(loaded.song.arrangements)} arrangement(s)")
|
||||||
|
print(f" forward: {'OK' if ok else 'FAILED'}")
|
||||||
|
return ok
|
||||||
|
|
||||||
|
|
||||||
|
def check_reverse(spec: Path) -> bool:
|
||||||
|
"""Layer 4 — packs committed here must pass the spec's reference validator."""
|
||||||
|
packs = sorted({p for g in PACK_GLOBS for p in REPO.glob(g)})
|
||||||
|
if not packs:
|
||||||
|
print(" reverse: no committed packs — skipped")
|
||||||
|
return True
|
||||||
|
|
||||||
|
try:
|
||||||
|
proc = subprocess.run(
|
||||||
|
[sys.executable, str(spec / "tools" / "validate.py"), *[str(p) for p in packs]],
|
||||||
|
capture_output=True,
|
||||||
|
text=True,
|
||||||
|
# The validator takes seconds for all committed packs; a pathological
|
||||||
|
# pack or validator bug must fail the job, not hang the runner until
|
||||||
|
# the Actions-level timeout.
|
||||||
|
timeout=300,
|
||||||
|
)
|
||||||
|
except subprocess.TimeoutExpired:
|
||||||
|
_fail("the spec's reference validator did not finish within 300s — pathological pack or validator bug?")
|
||||||
|
print(" reverse: FAILED")
|
||||||
|
return False
|
||||||
|
sys.stdout.write("".join(f" {ln}\n" for ln in proc.stdout.splitlines() if ln.strip()))
|
||||||
|
if proc.returncode != 0:
|
||||||
|
_fail(
|
||||||
|
"a pack committed to this repo does not satisfy the feedpak spec "
|
||||||
|
"(see the reference validator output above)."
|
||||||
|
)
|
||||||
|
if proc.stderr.strip():
|
||||||
|
sys.stderr.write(proc.stderr)
|
||||||
|
print(f" reverse: {'OK' if proc.returncode == 0 else 'FAILED'}")
|
||||||
|
return proc.returncode == 0
|
||||||
|
|
||||||
|
|
||||||
|
def main() -> int:
|
||||||
|
ap = argparse.ArgumentParser(description=__doc__.splitlines()[0])
|
||||||
|
ap.add_argument(
|
||||||
|
"--spec",
|
||||||
|
required=True,
|
||||||
|
type=Path,
|
||||||
|
help="path to a feedpak-spec checkout (CI checks out the spec repo's HEAD)",
|
||||||
|
)
|
||||||
|
ap.add_argument(
|
||||||
|
"--baseline-exceptions",
|
||||||
|
type=Path,
|
||||||
|
help="the exceptions file as it exists on the base branch. Supplied by CI so the "
|
||||||
|
"allowlist can be proven to have not grown. Omit for a local run.",
|
||||||
|
)
|
||||||
|
ap.add_argument(
|
||||||
|
"--bootstrap-allowlist",
|
||||||
|
action="store_true",
|
||||||
|
help="the base branch has no exceptions file yet (this PR introduces the gate), so "
|
||||||
|
"there is nothing to diff against. CI passes this only in that case.",
|
||||||
|
)
|
||||||
|
args = ap.parse_args()
|
||||||
|
|
||||||
|
spec = args.spec.resolve()
|
||||||
|
if not (spec / "schemas" / "manifest.schema.json").exists():
|
||||||
|
_fail(f"{spec} does not look like a feedpak-spec checkout")
|
||||||
|
return 1
|
||||||
|
|
||||||
|
print("[1/4] key-coverage — core reads/writes only keys the spec declares")
|
||||||
|
# Both run, always: a stale READERS list and an undeclared key are separate
|
||||||
|
# failures, and reporting only the first would hide the second. Hence two
|
||||||
|
# calls and an explicit `and` over the results, not a short-circuiting one.
|
||||||
|
readers_ok = check_readers_complete()
|
||||||
|
coverage_ok = check_key_coverage(spec)
|
||||||
|
ok1 = readers_ok and coverage_ok
|
||||||
|
print("[2/4] allowlist-closed — the grandfather list may shrink, never grow")
|
||||||
|
ok2 = check_allowlist_closed(args.baseline_exceptions, args.bootstrap_allowlist)
|
||||||
|
print("[3/4] forward — core ingests the spec's example packs")
|
||||||
|
ok3 = check_forward(spec)
|
||||||
|
print("[4/4] reverse — committed packs satisfy the reference validator")
|
||||||
|
ok4 = check_reverse(spec)
|
||||||
|
|
||||||
|
if ok1 and ok2 and ok3 and ok4:
|
||||||
|
print("\nfeedpak spec conformance: OK")
|
||||||
|
return 0
|
||||||
|
print("\nfeedpak spec conformance: FAILED")
|
||||||
|
return 1
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
sys.exit(main())
|
||||||
Reference in New Issue
Block a user