diff --git a/.feedpak-spec-ref b/.feedpak-spec-ref new file mode 100644 index 0000000..4ee7faf --- /dev/null +++ b/.feedpak-spec-ref @@ -0,0 +1,9 @@ +# Pinned commit of github.com/got-feedback/feedpak-spec that this repo is +# verified against by the `feedpak-spec` CI job (tools/check_spec_conformance.py). +# +# Pinned by SHA rather than tracking the spec's default branch on purpose: a +# change over there must never turn CI red on an unrelated PR here. When the +# spec moves, bump this SHA in its own PR — if that PR is red, the spec changed +# in a way core does not satisfy, which is exactly the signal we want, delivered +# as a reviewable PR instead of a surprise. +15e13e02062842d6660897623b41e04fd99ece58 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5190973..9b63901 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,6 +124,55 @@ 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. Three surface checks: core reads only + # manifest keys the spec declares; 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. Pinned by SHA in .feedpak-spec-ref so a change over + # there can't redden an unrelated PR here. + name: feedpak-spec + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + cache: 'pip' + + - name: Read the pinned spec commit + id: spec + run: | + sha=$(grep -vE '^[[:space:]]*(#|$)' .feedpak-spec-ref | head -n1 | tr -d '[:space:]') + if [ -z "$sha" ]; then + echo "::error file=.feedpak-spec-ref::no commit SHA found in .feedpak-spec-ref" + exit 1 + fi + echo "sha=$sha" >> "$GITHUB_OUTPUT" + + - name: Check out feedpak-spec at the pinned commit + uses: actions/checkout@v4 + with: + repository: got-feedback/feedpak-spec + ref: ${{ steps.spec.outputs.sha }} + path: .feedpak-spec + persist-credentials: false + + - 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). + pip install jsonschema + + - name: Check feedpak spec conformance + run: python tools/check_spec_conformance.py --spec .feedpak-spec + 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 2550ab0..3f37b5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- **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 three surface properties + in CI: (1) **key-coverage** — every manifest key core reads 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`; (2) **forward** — core's `load_song()` ingests every example pack the spec ships; + (3) **reverse** — every pack committed here passes the spec's own `tools/validate.py` (7/7 pass today). + The spec is pinned by SHA in `.feedpak-spec-ref` so a change over there can't redden an unrelated PR + here; bump it in its own PR, and a red result is the signal that core doesn't satisfy the new spec. + A key that must ship ahead of the spec uses the reserved `x-` prefix (always allowed) or is recorded in + `feedpak-spec-exceptions.yml` with a tracking issue — and the gate fails if such an exception goes stale, + so the allowlist can't become somewhere drift hides. `original_audio` is seeded there against #933 so the + gate lands green and starts blocking the *next* instance immediately. 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 `/v2` route, and the `FEEDBACK_UI` v2/legacy opt-out are deleted; `/` and `/v3` both serve diff --git a/docs/feedpak-spec-gate.md b/docs/feedpak-spec-gate.md new file mode 100644 index 0000000..ecbef26 --- /dev/null +++ b/docs/feedpak-spec-gate.md @@ -0,0 +1,78 @@ +# 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 must be in the spec before core ships code that +depends on it.** Spec first, implementation second. + +## What it checks + +We can't mechanically prove core *interprets* a key the way the spec means. We can prove three surface +properties, and they cover the drift that actually occurs. + +| Layer | Check | Catches | +|---|---|---| +| 1. key-coverage | Every manifest key core reads is declared in the spec's `manifest.schema.json`. | Core growing a key the spec never defined — the #933 class. | +| 2. forward | Core's `load_song()` ingests every example pack the spec ships. | The spec adding or tightening something core ignores or breaks on. | +| 3. 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 read +off a manifest dict (`manifest.get("x")`, `manifest["x"]`, and the wrapped +`(load_manifest(p) or {}).get("x")` form used in `lib/enrichment.py`). + +## When it fails + +You added a manifest key. Three ways forward, in order of preference: + +1. **Land it in the spec first.** Open a PR against `feedpak-spec` adding the key to + `schemas/manifest.schema.json` and `spec/feedpak-v1.md`, bump `.feedpak-spec-ref` here to the merged + SHA, and your key passes. This is the intended path. +2. **Mark it experimental.** Prefix the key `x-` (e.g. `x-my_new_key`). The gate permits `x-`-prefixed keys + unconditionally, and the prefix signals to every third-party packer that the key is not stable surface. +3. **Record an exception.** Add it to `feedpak-spec-exceptions.yml` with a tracking issue. This is debt, and + the gate treats it as such: an exception goes stale (and fails the build) the moment the spec catches up + or core stops reading the key, so the allowlist can't become somewhere drift quietly accumulates. + +## Pinning + +`.feedpak-spec-ref` holds the SHA of the `feedpak-spec` commit this repo is verified against. Pinned rather +than tracking the spec's default branch on purpose — a change over there must never turn CI red on an +unrelated PR here. + +When the spec moves, bump the SHA in its own PR. If that PR is red, the spec changed in a way core doesn't +satisfy — exactly the signal we want, delivered as a reviewable PR rather than a surprise on someone else's +branch. + +## Limitations + +Known, and worth fixing in follow-ups rather than blocking on: + +- **Layer 1 is name-heuristic.** It recognises manifest dicts bound to locals named in `MANIFEST_VARS` + (`manifest`, `mf`) plus the `load_manifest(...)` call form. This works because the loaders use a uniform + idiom, but it is fragile against a refactor that renames the local. 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 3 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..2f47610 --- /dev/null +++ b/feedpak-spec-exceptions.yml @@ -0,0 +1,30 @@ +# Manifest keys core reads that the feedpak spec does not (yet) define. +# +# This file exists so the spec-conformance gate (tools/check_spec_conformance.py) +# can be honest instead of being switched off. A gate with no legitimate way to +# say "yes, deliberately, not yet" gets commented out the first time it blocks a +# release — so drift that is *known and tracked* is allowed to sit here, and +# only drift that is unknown and untracked fails the build. +# +# Rules: +# - Every entry needs a tracking issue. No issue, no exception. +# - Entries are debt, not policy. The fix is to land the key in the spec +# (github.com/got-feedback/feedpak-spec) and delete the entry. +# - The gate fails if an entry goes stale — i.e. the spec caught up, or core +# stopped reading the key. The allowlist must never become a hiding place. +# +# For a key that is genuinely experimental and not yet ready for the spec, +# prefer the reserved `x-` prefix (e.g. `x-my_new_key`) over an exception: the +# gate permits `x-`-prefixed keys unconditionally, and the prefix tells every +# third-party packer that the key is not stable surface. + +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 was never added to the spec — the + drift this gate exists to prevent. Seeded here so the gate lands green and + starts blocking the *next* instance immediately; remove once the spec + adopts the key. diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py new file mode 100644 index 0000000..0357beb --- /dev/null +++ b/tools/check_spec_conformance.py @@ -0,0 +1,271 @@ +#!/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 three surface properties, and those cover the drift that actually +happens: + + 1. key-coverage — every manifest key core reads is declared by the spec. + 2. forward — core ingests the spec's own example packs. + 3. 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 subprocess +import sys +import tempfile +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent + +# Modules that read a feedpak manifest dict. Listed explicitly rather than +# globbed so that adding a new reader is a deliberate act that shows up in +# review — a new reader is exactly when key drift gets introduced. A missing +# file here is a hard error, so a rename cannot silently disable the scan. +READERS = [ + "lib/sloppak.py", + "lib/enrichment.py", + "lib/songmeta.py", +] + +# Locals that hold a manifest dict. The loaders use a uniform idiom +# (`manifest.get("key")`), so binding by name is sufficient today. See +# "Limitations" in docs/feedpak-spec-gate.md for the hardening path. +MANIFEST_VARS = {"manifest", "mf"} + +# 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" + +# Keys under this prefix are reserved for pre-spec experimentation and are +# always permitted. Anything else undeclared must be listed in the exceptions +# file with a tracking issue, or the build fails. +EXPERIMENTAL_PREFIX = "x-" + + +def _fail(msg: str) -> None: + print(f"::error::{msg}") + + +def _is_manifest_receiver(node: ast.expr) -> bool: + """True when `node` evaluates to a manifest dict. + + Covers the plain `manifest.get(...)` idiom plus the wrapped form used in + lib/enrichment.py: `(sloppak_mod.load_manifest(p) or {}).get("key")`. + """ + if isinstance(node, ast.Name) and node.id in MANIFEST_VARS: + return True + try: + src = ast.unparse(node) + except Exception: + return False + return "load_manifest" in src + + +def keys_read(path: Path) -> set[str]: + """Every literal top-level manifest key read by `path`.""" + found: set[str] = set() + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "get" + and _is_manifest_receiver(node.func.value) + and node.args + and isinstance(node.args[0], ast.Constant) + and isinstance(node.args[0].value, str) + ): + found.add(node.args[0].value) + elif ( + isinstance(node, ast.Subscript) + and _is_manifest_receiver(node.value) + and isinstance(node.slice, ast.Constant) + and isinstance(node.slice.value, str) + ): + found.add(node.slice.value) + return found + + +def load_exceptions() -> dict[str, str]: + """Map of allowlisted key -> tracking issue URL.""" + if not EXCEPTIONS_FILE.exists(): + return {} + import yaml # runtime dep (PyYAML is already in requirements.txt) + + data = yaml.safe_load(EXCEPTIONS_FILE.read_text(encoding="utf-8")) or {} + out: dict[str, str] = {} + for entry in data.get("exceptions") or []: + key, issue = entry.get("key"), entry.get("issue") + if not key or not issue: + _fail(f"{EXCEPTIONS_FILE.name}: every exception needs both 'key' and 'issue'") + sys.exit(1) + out[key] = issue + return out + + +def check_key_coverage(spec: Path) -> bool: + """Layer 1 — core must not read 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 + + read: 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 + read |= keys_read(path) + + exceptions = load_exceptions() + undeclared = { + k for k in (read - declared) if not k.startswith(EXPERIMENTAL_PREFIX) + } + unexcused = sorted(undeclared - set(exceptions)) + ok = True + + for key in unexcused: + _fail( + f"core reads manifest key '{key}', which the feedpak spec does not define. " + f"Add it to the spec (github.com/got-feedback/feedpak-spec) before merging, " + f"rename it to '{EXPERIMENTAL_PREFIX}{key}' if it is deliberately pre-spec, or " + f"record it in {EXCEPTIONS_FILE.name} with a tracking issue." + ) + 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. + 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 read: + _fail( + f"'{key}' is listed in {EXCEPTIONS_FILE.name} but core no longer reads it. " + f"Remove the exception." + ) + ok = False + + print(f" spec declares {len(declared)} keys; core reads {len(read)}") + if exceptions: + print(f" allowlisted (pending spec): {', '.join(sorted(exceptions))}") + print(f" key-coverage: {'OK' if ok else 'FAILED'}") + return ok + + +def check_forward(spec: Path) -> bool: + """Layer 2 — core must ingest every example pack the spec ships.""" + examples = sorted( + p for p in (spec / "examples").iterdir() + 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")) + import sloppak # noqa: E402 (path must be set first — flat imports, no package) + + cache = Path(tempfile.mkdtemp()) + ok = True + 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 3 — 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 + + proc = subprocess.run( + [sys.executable, str(spec / "tools" / "validate.py"), *[str(p) for p in packs]], + capture_output=True, + text=True, + ) + 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 pins the SHA in .feedpak-spec-ref)", + ) + 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/3] key-coverage — core reads only keys the spec declares") + ok1 = check_key_coverage(spec) + print("[2/3] forward — core ingests the spec's example packs") + ok2 = check_forward(spec) + print("[3/3] reverse — committed packs satisfy the reference validator") + ok3 = check_reverse(spec) + + if ok1 and ok2 and ok3: + print("\nfeedpak spec conformance: OK") + return 0 + print("\nfeedpak spec conformance: FAILED") + return 1 + + +if __name__ == "__main__": + sys.exit(main())