ci: gate core against the feedpak spec

feedpak is published as an open format with its own repo, normative spec,
JSON Schemas, and reference validator. That makes the spec a contract with
everyone outside this repo: third-party packers, converters, and players
build against it, and it is meant to be the complete description of a pack.

Nothing enforced that. #583 added a manifest key (`original_audio`) that
core, lib/enrichment.py, and the stems plugin all now depend on, but which
was never added to the spec — so a spec-compliant pack stopped being a
fully-working pack, the reference validator could not warn authors about a
key it had never heard of, and third-party tooling began emitting an
`original/` directory reverse-engineered from an example in a code comment.
See #933.

We cannot mechanically prove core interprets a key the way the spec means.
We can prove three surface properties, and they cover the drift that
actually happens:

  1. key-coverage — every manifest key core reads is declared in the spec's
     manifest.schema.json (AST scan of lib/sloppak.py, lib/enrichment.py,
     lib/songmeta.py).
  2. forward — core's load_song() ingests every example pack the spec ships.
  3. reverse — every pack committed here passes the spec's own
     tools/validate.py (7/7 pass today).

The spec is pinned by SHA in .feedpak-spec-ref rather than tracked from its
default branch, so a change over there cannot redden an unrelated PR here;
bump it in its own PR, where a red result is precisely the signal that core
does not satisfy the new spec.

A gate with no legitimate way to say "yes, deliberately, not yet" gets
switched off the first time it blocks a release, so there are two escape
hatches: the reserved `x-` key prefix (always permitted, and it tells every
third-party packer the key is not stable surface), and
feedpak-spec-exceptions.yml, which requires a tracking issue per entry. An
exception that goes stale — the spec caught up, or core stopped reading the
key — fails the build, so the allowlist cannot become somewhere drift
quietly accumulates. `original_audio` is seeded there against #933 so the
gate lands green and starts blocking the next instance immediately, rather
than requiring #933 to be resolved first.

Dev/CI tooling only; never on the serve or Docker path (constitution
Principle I). jsonschema is installed in the CI job, not added to
requirements.txt.

Signed-off-by: topkoa <topkoa@gmail.com>

.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

.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

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

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`.

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.

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 <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 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())