diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..b31237d --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,17 @@ +## What + + + +## feedpak surface + + + +- [ ] 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`) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5190973..666b122 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,6 +124,94 @@ jobs: print(f"Validated {len(manifests)} manifest(s) — OK") 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: # Maintainer/CI-only size + module-hygiene gate (constitution Principle I: # dev tooling, never on the serve/Docker path — same category as diff --git a/CHANGELOG.md b/CHANGELOG.md index 13e556e..8e68fdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 effective genre — Bronze = N genre songs at K★, data-driven in `plugins/career/passports.json`, default 5 songs at 2★ — plus qualifying-song - "ticket stubs", the library genre list, and drill status), `POST - /passports/commit` (instrument commitment), `POST /passports/open` (open a genre + "ticket stubs", the library genre list, and drill status), `POST /passports/commit` + (instrument commitment), `POST /passports/open` (open a genre passport), and `POST /drill-state` (intake for the relayed Virtuoso `virtuoso.progress` snapshot, so drill requirements can gate badges 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 events (debounced, plus a one-time bootstrap), closing the 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 - **The classic v2 UI shell is gone — v3 is the only UI (R3a).** `static/index.html`, the diff --git a/CLAUDE.md b/CLAUDE.md index 9f6f973..0cf042e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 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:** - `lib/sloppak.py` — format detection, zip/directory resolution, metadata extraction, song loading - `lib/sloppak_convert.py` — sloppak assembly pipeline, Demucs stem splitting diff --git a/docs/feedpak-spec-gate.md b/docs/feedpak-spec-gate.md new file mode 100644 index 0000000..ace58ed --- /dev/null +++ b/docs/feedpak-spec-gate.md @@ -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`. diff --git a/feedpak-spec-exceptions.yml b/feedpak-spec-exceptions.yml new file mode 100644 index 0000000..aa6f4d1 --- /dev/null +++ b/feedpak-spec-exceptions.yml @@ -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. diff --git a/tests/test_spec_gate.py b/tests/test_spec_gate.py new file mode 100644 index 0000000..bac8a12 --- /dev/null +++ b/tests/test_spec_gate.py @@ -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 diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py new file mode 100644 index 0000000..da2ec66 --- /dev/null +++ b/tools/check_spec_conformance.py @@ -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 + +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// 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())