From 22332bef22f1df04cdc98b6c4c46976b219d39a8 Mon Sep 17 00:00:00 2001 From: topkoa Date: Sun, 12 Jul 2026 23:30:45 -0400 Subject: [PATCH 01/15] ci: gate core against the feedpak spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .feedpak-spec-ref | 9 ++ .github/workflows/ci.yml | 49 ++++++ CHANGELOG.md | 16 ++ docs/feedpak-spec-gate.md | 78 +++++++++ feedpak-spec-exceptions.yml | 30 ++++ tools/check_spec_conformance.py | 271 ++++++++++++++++++++++++++++++++ 6 files changed, 453 insertions(+) create mode 100644 .feedpak-spec-ref create mode 100644 docs/feedpak-spec-gate.md create mode 100644 feedpak-spec-exceptions.yml create mode 100644 tools/check_spec_conformance.py 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()) From 0dc9fd7ba80fb9208528e3808f6ff9f2fc85b480 Mon Sep 17 00:00:00 2001 From: topkoa Date: Sun, 12 Jul 2026 23:35:49 -0400 Subject: [PATCH 02/15] docs: the fix for original_audio is removal, not adoption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec already carries the pre-separation mixdown as a stem ({id: full, file: stems/full.ogg}), so the key added a second, redundant location for audio to a format that already had one. Adopting it into the spec would make that permanent; the resolution in #933 is to remove it. No behaviour change — the gate is agnostic about which way a violation resolves, and only insists that one of the two happens deliberately and in the open before the code merges. This just stops the exception entry, the changelog, and the docs from presupposing adoption. Signed-off-by: topkoa --- CHANGELOG.md | 4 +++- docs/feedpak-spec-gate.md | 7 +++++++ feedpak-spec-exceptions.yml | 13 ++++++++++--- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f37b5d..f8c0555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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). + 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/docs/feedpak-spec-gate.md b/docs/feedpak-spec-gate.md index ecbef26..4285d57 100644 --- a/docs/feedpak-spec-gate.md +++ b/docs/feedpak-spec-gate.md @@ -23,6 +23,13 @@ The moment core reads a manifest key the spec doesn't define, that promise break 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. +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 three surface diff --git a/feedpak-spec-exceptions.yml b/feedpak-spec-exceptions.yml index 2f47610..b0ef1a2 100644 --- a/feedpak-spec-exceptions.yml +++ b/feedpak-spec-exceptions.yml @@ -25,6 +25,13 @@ exceptions: 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. + drift this gate exists to prevent. + + The expected resolution is REMOVAL, not adoption: 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. + + Seeded here so the gate lands green and starts blocking the *next* instance + immediately, rather than blocking on #933. The entry goes away when core + stops reading the key. From d0626f5618dfa57ddeaa703f49c99e40ee5999f0 Mon Sep 17 00:00:00 2001 From: topkoa Date: Sun, 12 Jul 2026 23:44:15 -0400 Subject: [PATCH 03/15] =?UTF-8?q?ci:=20address=20review=20=E2=80=94=20chec?= =?UTF-8?q?k=20writes=20too,=20pin=20deps,=20harden=20the=20spec=20pin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback from CodeRabbit and Copilot on #934. All six findings were valid; one is fixed the other way round from how it was suggested. Key-coverage now checks manifest WRITES as well as reads. Copilot correctly spotted that `ast.walk` ignored subscript context, so `manifest["year"] = ...` (lib/songmeta.py) scored as a read — but the fix is not to drop writes. A key core *writes* is spec surface pointed outward: it lands in every pack we emit, so an undeclared one seeds the ecosystem with non-spec data. Subscripts are now classified by ctx (Store = write, Load = read) and both sets are checked, with distinct error messages. Today: 19 reads, 2 writes, all declared. Workflow: - persist-credentials: false on the repo checkout — the job runs repository code and never pushes (CodeRabbit / zizmor artipacked). - .feedpak-spec-ref must be a full 40-char SHA. actions/checkout resolves branches and tags in `ref` too, so a non-SHA there would silently un-pin the spec — precisely what the file exists to prevent. - Pin jsonschema==4.26.0, for the same reason the spec SHA is pinned: an upstream release must not redden this job on a PR that changed neither this repo nor the spec. Script: - check_forward() guards a missing examples/ dir instead of raising an unhandled FileNotFoundError. - TemporaryDirectory() instead of mkdtemp(), so a local run doesn't leak. Signed-off-by: topkoa --- .github/workflows/ci.yml | 17 +++++- docs/feedpak-spec-gate.md | 11 ++-- tools/check_spec_conformance.py | 96 +++++++++++++++++++++------------ 3 files changed, 85 insertions(+), 39 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b63901..8f30ec3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,6 +137,10 @@ jobs: 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. + with: + persist-credentials: false - uses: actions/setup-python@v5 with: @@ -151,6 +155,13 @@ jobs: echo "::error file=.feedpak-spec-ref::no commit SHA found in .feedpak-spec-ref" exit 1 fi + # actions/checkout resolves branches and tags in `ref` too, so a + # non-SHA here would silently un-pin the spec — the one thing this + # file exists to prevent. Demand a full 40-char SHA. + if ! printf '%s' "$sha" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error file=.feedpak-spec-ref::expected a full 40-character commit SHA, got '$sha' — a branch or tag name would defeat the pin" + exit 1 + fi echo "sha=$sha" >> "$GITHUB_OUTPUT" - name: Check out feedpak-spec at the pinned commit @@ -167,8 +178,10 @@ jobs: 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 + # (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: Check feedpak spec conformance run: python tools/check_spec_conformance.py --spec .feedpak-spec diff --git a/docs/feedpak-spec-gate.md b/docs/feedpak-spec-gate.md index 4285d57..7793d02 100644 --- a/docs/feedpak-spec-gate.md +++ b/docs/feedpak-spec-gate.md @@ -37,14 +37,19 @@ 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. | +| 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. 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 +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. Three ways forward, in order of preference: diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py index 0357beb..46126e3 100644 --- a/tools/check_spec_conformance.py +++ b/tools/check_spec_conformance.py @@ -83,9 +83,17 @@ def _is_manifest_receiver(node: ast.expr) -> bool: 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() +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)) for node in ast.walk(tree): if ( @@ -97,15 +105,16 @@ def keys_read(path: Path) -> set[str]: and isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str) ): - found.add(node.args[0].value) + reads.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 + target = writes if isinstance(node.ctx, ast.Store) else reads + target.add(node.slice.value) + return reads, writes def load_exceptions() -> dict[str, str]: @@ -133,22 +142,25 @@ def check_key_coverage(spec: Path) -> bool: _fail("spec manifest.schema.json declares no properties — wrong path or bad checkout?") return False - read: set[str] = set() + 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 - read |= keys_read(path) + r, w = keys_touched(path) + reads |= r + writes |= w 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: + def _undeclared(keys: set[str]) -> list[str]: + flagged = {k for k in (keys - declared) if not k.startswith(EXPERIMENTAL_PREFIX)} + return sorted(flagged - set(exceptions)) + + for key in _undeclared(reads): _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, " @@ -157,8 +169,19 @@ def check_key_coverage(spec: Path) -> bool: ) ok = False + for key in _undeclared(writes): + _fail( + f"core writes manifest key '{key}', which the feedpak spec does not define — " + f"that puts non-spec surface into every pack we emit. Add it to the spec " + f"(github.com/got-feedback/feedpak-spec) before merging, rename it to " + f"'{EXPERIMENTAL_PREFIX}{key}' if it is deliberately pre-spec, or record it in " + f"{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. + touched = reads | writes for key, issue in exceptions.items(): if key in declared: _fail( @@ -166,14 +189,14 @@ def check_key_coverage(spec: Path) -> bool: f"Remove the exception and close {issue}." ) ok = False - elif key not in read: + elif key not in touched: _fail( - f"'{key}' is listed in {EXCEPTIONS_FILE.name} but core no longer reads it. " - f"Remove the exception." + 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(read)}") + print(f" spec declares {len(declared)} keys; core reads {len(reads)}, writes {len(writes)}") if exceptions: print(f" allowlisted (pending spec): {', '.join(sorted(exceptions))}") print(f" key-coverage: {'OK' if ok else 'FAILED'}") @@ -182,8 +205,12 @@ def check_key_coverage(spec: Path) -> bool: def check_forward(spec: Path) -> bool: """Layer 2 — 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 examples = sorted( - p for p in (spec / "examples").iterdir() + p for p in examples_dir.iterdir() if p.suffix in (".feedpak", ".sloppak") ) if not examples: @@ -193,23 +220,24 @@ def check_forward(spec: Path) -> bool: 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)") + 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 From ceb1e143cd5d82c2acfb77038c83e6358de5eb08 Mon Sep 17 00:00:00 2001 From: topkoa Date: Sun, 12 Jul 2026 23:54:07 -0400 Subject: [PATCH 04/15] =?UTF-8?q?ci:=20second=20review=20pass=20=E2=80=94?= =?UTF-8?q?=20recurse=20examples,=20reject=20duplicate=20exceptions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six more findings from CodeRabbit and Copilot on #934. All valid; four were my own docs lagging the write-checking change in d0626f5. check_forward() now discovers example packs recursively, so a pack nested under examples// can't slip past the "every example pack" contract. Taken WITHOUT the suggested is_file() filter, which would have broken it: a feedpak is dual-form — a zip (foo.feedpak) or a directory (foo.feedpak/) — and the spec's own examples ship as directories, so is_file() would have matched zero packs. Suffix matching covers both forms. load_exceptions() rejects duplicate keys instead of silently keeping the last one, which would quietly retarget the tracking issue for a piece of debt this file exists to track. The sloppak import is wrapped so a missing dependency produces a CI-legible ::error:: rather than a bare traceback. Docs caught up with the code: the exceptions file header, its stale-entry rule, and the changelog all said "reads" when the gate checks reads AND writes. Signed-off-by: topkoa --- CHANGELOG.md | 6 ++++-- feedpak-spec-exceptions.yml | 11 +++++++++-- tools/check_spec_conformance.py | 30 +++++++++++++++++++++++++++--- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8c0555..67e65e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 + 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`; (2) **forward** — core's `load_song()` ingests every example pack the spec ships; + `lib/songmeta.py` (writes are gated too, and reported separately: a key core writes lands in every pack + we emit, so an undeclared one seeds the ecosystem with non-spec data); (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. diff --git a/feedpak-spec-exceptions.yml b/feedpak-spec-exceptions.yml index b0ef1a2..b2ccd52 100644 --- a/feedpak-spec-exceptions.yml +++ b/feedpak-spec-exceptions.yml @@ -1,4 +1,9 @@ -# Manifest keys core reads that the feedpak spec does not (yet) define. +# Manifest keys core reads OR WRITES that the feedpak spec does not (yet) define. +# +# Both directions are gated, and both are allowlisted here. A key core *writes* +# (`manifest["k"] = v`) is spec surface pointed outward — it lands in every pack +# we emit — so an undeclared one needs an entry here just as much as one core +# reads. # # 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 @@ -10,8 +15,10 @@ # - 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. +# - One entry per key. A duplicate is an error, not a last-one-wins. # - 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. +# no longer reads or writes 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 diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py index 46126e3..769c485 100644 --- a/tools/check_spec_conformance.py +++ b/tools/check_spec_conformance.py @@ -130,12 +130,20 @@ def load_exceptions() -> dict[str, str]: if not key or not issue: _fail(f"{EXCEPTIONS_FILE.name}: 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"{EXCEPTIONS_FILE.name}: '{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 check_key_coverage(spec: Path) -> bool: - """Layer 1 — core must not read a manifest key the spec does not declare.""" + """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: @@ -209,8 +217,17 @@ def check_forward(spec: Path) -> bool: 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.iterdir() + p for p in examples_dir.rglob("*") if p.suffix in (".feedpak", ".sloppak") ) if not examples: @@ -218,7 +235,14 @@ def check_forward(spec: Path) -> bool: return False sys.path.insert(0, str(REPO / "lib")) - import sloppak # noqa: E402 (path must be set first — flat imports, no package) + 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: From 32d723b774c11c0289e46f82829138de1704029d Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 00:14:57 -0400 Subject: [PATCH 05/15] =?UTF-8?q?ci:=20close=20the=20escape=20hatches=20?= =?UTF-8?q?=E2=80=94=20the=20FEP=20process=20is=20the=20only=20route?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate's purpose is to make a non-conforming change *not merge*, so the person merging must stop and decide whether to take it through the format process. The escape hatches defeated exactly that: a developer who did not want to write a FEP could name their key `x-whatever`, or append an entry to feedpak-spec-exceptions.yml with any issue link, and merge. Both were self-serve and in-repo. That is a speed bump with a signed excuse note, not a gate. The relief valve is the FEP process itself, not something in this repo. The spec's governance already says so: "A change is not part of the format until it lands here." Removed the `x-` prefix bypass. It was invented here, not in the spec — the spec reserves no experimental namespace. Its "unknown keys are reserved for forward-compatibility" rule is about *tolerating* other implementations' keys, not a licence for core to mint its own. feedpak-spec-exceptions.yml is now a CLOSED grandfather list. A new check (allowlist-closed) diffs it against the base branch and fails any PR that ADDS an entry; removal stays allowed, so the list can only shrink. Deleting an entry does not by itself pass the gate — key-coverage still fails while core reads the key, so the entry goes when the code goes. Every failure message now points at the FEP process and at bumping .feedpak-spec-ref to the merged spec SHA, which is the one supported way a new manifest key reaches core. CI fetches the base branch to diff the allowlist; the bootstrap flag covers the one case with no baseline — the PR introducing the gate. Signed-off-by: topkoa --- .github/workflows/ci.yml | 21 +++++- CHANGELOG.md | 15 +++-- docs/feedpak-spec-gate.md | 48 +++++++++++--- feedpak-spec-exceptions.yml | 68 ++++++++++--------- tools/check_spec_conformance.py | 114 +++++++++++++++++++++++--------- 5 files changed, 187 insertions(+), 79 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f30ec3..65c5abe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,8 +139,11 @@ jobs: - 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: @@ -183,8 +186,24 @@ jobs: # 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. + git fetch --no-tags --depth=1 origin main + 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 + 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: diff --git a/CHANGELOG.md b/CHANGELOG.md index 67e65e1..247966b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,12 +20,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (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; 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). + **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 + bump `.feedpak-spec-ref` to the merged SHA in the same PR. `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/docs/feedpak-spec-gate.md b/docs/feedpak-spec-gate.md index 7793d02..31ad336 100644 --- a/docs/feedpak-spec-gate.md +++ b/docs/feedpak-spec-gate.md @@ -38,8 +38,9 @@ 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. 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. | +| 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 @@ -52,16 +53,41 @@ context — `Store` is a write, `Load` is a read — so `manifest["year"] = ...` ## When it fails -You added a manifest key. Three ways forward, in order of preference: +You added a manifest key the spec doesn't define. **There is exactly one way forward, and it is not in this +repo.** -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. +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**, bump `.feedpak-spec-ref` to that merged SHA, in the same PR as your code. The gate goes + green, because the key is now genuinely part of the format. + +That is deliberately the only route. There is **no in-repo escape hatch** — no experimental prefix, no +self-serve allowlist. If your PR is blocked, the answer is a FEP, not a workaround. The person merging has +to stop and decide whether the change is worth taking through the format process, which is the whole point. + +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. ## Pinning diff --git a/feedpak-spec-exceptions.yml b/feedpak-spec-exceptions.yml index b2ccd52..0614ee1 100644 --- a/feedpak-spec-exceptions.yml +++ b/feedpak-spec-exceptions.yml @@ -1,29 +1,36 @@ -# Manifest keys core reads OR WRITES that the feedpak spec does not (yet) define. +# CLOSED grandfather list — manifest keys core reads or writes that predate the +# spec-conformance gate and that the feedpak spec does not define. # -# Both directions are gated, and both are allowlisted here. A key core *writes* -# (`manifest["k"] = v`) is spec surface pointed outward — it lands in every pack -# we emit — so an undeclared one needs an entry here just as much as one core -# reads. +# ┌─────────────────────────────────────────────────────────────────────────┐ +# │ THIS IS NOT AN ESCAPE HATCH. You cannot add to it. │ +# │ CI fails any PR that adds an entry here. The list may only SHRINK. │ +# └─────────────────────────────────────────────────────────────────────────┘ # -# 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. +# There is deliberately no in-repo way to merge a manifest key the spec doesn't +# define. The feedpak spec's own governance is explicit: # -# 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. -# - One entry per key. A duplicate is an error, not a last-one-wins. -# - The gate fails if an entry goes stale — i.e. the spec caught up, or core -# no longer reads or writes the key. The allowlist must never become a -# hiding place. +# "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 # -# 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. +# 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, bump `.feedpak-spec-ref` to that merged SHA in the same PR that +# adds your code. The gate then goes green, because the key is now declared. +# +# That is the only route. If your PR is blocked by this gate, the answer is a +# FEP, not an entry in this file. +# +# 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 @@ -31,14 +38,13 @@ exceptions: 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. + 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 expected resolution is REMOVAL, not adoption: 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. + 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. - Seeded here so the gate lands green and starts blocking the *next* instance - immediately, rather than blocking on #933. The entry goes away when core - stops reading the key. + 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 stops reading the key. diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py index 769c485..30534c8 100644 --- a/tools/check_spec_conformance.py +++ b/tools/check_spec_conformance.py @@ -58,10 +58,15 @@ PACK_GLOBS = ["content/starter/*.feedpak", "docs/**/*.sloppak", "docs/**/*.feedp 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-" +# 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 = ( + "A new manifest key must 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 bump .feedpak-spec-ref to the merged SHA in this PR." +) def _fail(msg: str) -> None: @@ -117,24 +122,22 @@ def keys_touched(path: Path) -> tuple[set[str], set[str]]: return reads, writes -def load_exceptions() -> dict[str, str]: - """Map of allowlisted key -> tracking issue URL.""" - if not EXCEPTIONS_FILE.exists(): - return {} +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) - data = yaml.safe_load(EXCEPTIONS_FILE.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(text) 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'") + _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"{EXCEPTIONS_FILE.name}: '{key}' is listed more than once. " + f"{origin}: '{key}' is listed more than once. " f"Keep one entry per key so the tracking issue is unambiguous." ) sys.exit(1) @@ -142,6 +145,52 @@ def load_exceptions() -> dict[str, str]: 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 + + 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}'. The allowlist is " + f"closed — it grandfathers keys that predate this gate and may only shrink. {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_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")) @@ -165,25 +214,16 @@ def check_key_coverage(spec: Path) -> bool: ok = True def _undeclared(keys: set[str]) -> list[str]: - flagged = {k for k in (keys - declared) if not k.startswith(EXPERIMENTAL_PREFIX)} - return sorted(flagged - set(exceptions)) + 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. " - 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." - ) + _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 — " - f"that puts non-spec surface into every pack we emit. Add it to the spec " - f"(github.com/got-feedback/feedpak-spec) before merging, rename it to " - f"'{EXPERIMENTAL_PREFIX}{key}' if it is deliberately pre-spec, or record it in " - f"{EXCEPTIONS_FILE.name} with a tracking issue." + 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 @@ -298,6 +338,18 @@ def main() -> int: type=Path, help="path to a feedpak-spec checkout (CI pins the SHA in .feedpak-spec-ref)", ) + 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() @@ -305,14 +357,16 @@ def main() -> int: _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") + print("[1/4] key-coverage — core reads/writes 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) + 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: + if ok1 and ok2 and ok3 and ok4: print("\nfeedpak spec conformance: OK") return 0 print("\nfeedpak spec conformance: FAILED") From d806d12c2292c84a36b5e29e24ea4cc29dc6b67d Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 00:23:36 -0400 Subject: [PATCH 06/15] ci: close two blind spots in the key scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the gate was scanning less than it claimed. READERS missed two modules that genuinely touch feedpak manifests: lib/routers/ws_highway.py (reads `authors`) and lib/gp2notation.py (loads manifest.yaml, stamps feedpak_version, writes the file back). Keys touched there were going entirely unchecked. The scan also only recognised writes done via subscript, so `manifest.setdefault("k", v)` — exactly how gp2notation.py stamps feedpak_version — was invisible. setdefault with a literal key now counts as a write. The deeper problem is that READERS is hand-maintained, and a hand-maintained list rots; that is how both modules went unnoticed. check_readers_complete() now re-derives the set: any module under lib/ (or server.py) that both touches manifest keys and shows a feedpak signal must be listed, or the build fails. It is a guard on the gate itself. The list stays explicit rather than becoming a glob, because `manifest` is overloaded here: lib/loosefolder.py (the loose-folder manifest.json) and lib/diagnostics_bundle.py (the diagnostics bundle manifest) have their own unrelated manifests, and scanning those would flag *their* keys as feedpak drift. Both score zero on the feedpak signals, which is what keeps them out. Now scanning 5 modules: 20 reads, 2 writes, all spec-declared. Signed-off-by: topkoa --- tools/check_spec_conformance.py | 813 +++++++++++++++++--------------- 1 file changed, 436 insertions(+), 377 deletions(-) diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py index 30534c8..b420935 100644 --- a/tools/check_spec_conformance.py +++ b/tools/check_spec_conformance.py @@ -1,377 +1,436 @@ -#!/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" - -# 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 = ( - "A new manifest key must 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 bump .feedpak-spec-ref to the merged SHA in this PR." -) - - -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_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)) - 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) - ): - reads.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) - ): - 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) - - data = yaml.safe_load(text) 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"{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 - - 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}'. The allowlist is " - f"closed — it grandfathers keys that predate this gate and may only shrink. {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_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 - r, w = keys_touched(path) - 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" 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_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 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)", - ) - 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") - ok1 = check_key_coverage(spec) - 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()) +#!/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 +] + +# 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") + +# Does this module touch manifest keys at all? +KEY_OPS = re.compile(r"(manifest|mf)\.(get|setdefault)\(|(manifest|mf)\[") + +# 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" + +# 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 = ( + "A new manifest key must 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 bump .feedpak-spec-ref to the merged SHA in this PR." +) + + +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_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)) + 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) + 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.Subscript) + and _is_manifest_receiver(node.value) + 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) + + data = yaml.safe_load(text) 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"{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 + + 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}'. The allowlist is " + f"closed — it grandfathers keys that predate this gate and may only shrink. {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 KEY_OPS.search(src) and FEEDPAK_SIGNALS.search(src): + 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 + r, w = keys_touched(path) + 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" 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_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 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)", + ) + 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") + ok1 = check_readers_complete() & check_key_coverage(spec) + 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()) From ab2e68a6381ea8f8a158f9017322ae47a190ad5a Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 00:32:24 -0400 Subject: [PATCH 07/15] ci: resolve the allowlist baseline against the real base branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allowlist-closed diff hardcoded `origin main`, but ship-ci.yml also runs this workflow for PRs into release/** and for pushes to release/**, where a main baseline diffs against the wrong branch and can fail changes that have nothing to do with the allowlist. It now resolves the base: PR -> github.event.pull_request.base.ref (the branch it merges into) push -> github.ref_name (the branch itself; its tip already contains the change, so the diff is a no-op — enforcement happens at PR time) Also from review, all documentation drift introduced by my own earlier commits: - The layer count said "three" in the module docstring, the workflow comment, the docs, and the changelog. There are four (allowlist-closed was added). - The changelog listed three scanned modules; there are five. - The docs and changelog stated the rule for keys core *reads*, omitting writes — which are equally gated, and land in every pack we emit. - The CI summary line labelled grandfathered keys "pending spec", implying adoption is the only resolution. For original_audio it is not: the fix is removal. Relabelled "grandfathered (tracked debt)". - feedpak-spec-exceptions.yml said an entry clears when core "stops reading" the key; the rule is "no longer reads or writes". - Replaced a bitwise `&` over two bools with two named results and an explicit `and` — both checks must run (a stale READERS list and an undeclared key are separate failures; short-circuiting would hide one), and `&` reads like a typo. Signed-off-by: topkoa --- .github/workflows/ci.yml | 473 ++++++++++----------- CHANGELOG.md | 706 ++++++++++++++++---------------- docs/feedpak-spec-gate.md | 238 +++++------ feedpak-spec-exceptions.yml | 100 ++--- tools/check_spec_conformance.py | 9 +- 5 files changed, 775 insertions(+), 751 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 65c5abe..7c1ab42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,231 +1,242 @@ -name: ci - -# Runs only as a reusable workflow invoked by ship-ci.yml (for PRs into main -# and release/**). It deliberately has no standalone pull_request trigger: a -# direct run would publish unprefixed "" checks, but the org rulesets -# require the "ci / " names produced when ship-ci.yml calls this workflow. -on: - workflow_call: - -permissions: - contents: read - pull-requests: read - checks: read - -jobs: - - test: - name: test - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-python@v5 - with: - python-version: '3.12' - cache: 'pip' - - - name: Guard against print() / traceback.print_exc() in server.py, lib/, and bundled plugin routes - run: | - # git grep: tracked files only — no .pyc / __pycache__ noise from - # later pytest runs. Covers both audited patterns from #155 / #242. - # `(^|[^A-Za-z0-9_])` anchor avoids matching suffixes like `myprint(`; - # POSIX leaves `\b` undefined, so we use an explicit character class. - hits=$(git grep -nE '(^|[^A-Za-z0-9_])(print|traceback\.print_exc)[[:space:]]*\(' \ - -- server.py lib/ \ - $(git ls-files 'plugins/*/routes.py') || true) - if [ -n "$hits" ]; then - echo "$hits" - first=$(printf '%s\n' "$hits" | head -n1) - file=$(printf '%s' "$first" | cut -d: -f1) - line=$(printf '%s' "$first" | cut -d: -f2) - echo "::error file=${file},line=${line}::print() or traceback.print_exc() found in server.py, lib/, or a bundled plugin routes.py. Use the feedBack logger (lib/logging_setup.py) — see issues #155 / #242." - exit 1 - fi - - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install -r requirements.txt -r requirements-test.txt - - - name: Run pytest - run: pytest - - - name: Run JS plugin-API tests - run: node --test tests/js/*.test.js 'tests/plugins/*/js/*.test.js' 'plugins/*/tests/*.test.js' - - tailwind-fresh: - # Guard that the committed static/tailwind.min.css is in sync with source. - # The Play CDN's runtime JIT was removed (feedBack-desktop#110); a prebuilt - # stylesheet only contains classes the scanner saw at build time, so stale - # CSS silently ships unstyled elements. Rebuild and fail on any diff. - name: tailwind-fresh - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-node@v4 - with: - node-version: '20' - - - name: Rebuild Tailwind CSS - run: bash scripts/build-tailwind.sh - - - name: Verify committed static/tailwind.min.css is fresh - run: | - # Hard-fail (matches the print() guard convention) — do NOT auto-commit. - if ! git diff --quiet -- static/tailwind.min.css; then - echo "::error file=static/tailwind.min.css::static/tailwind.min.css is stale. Run 'bash scripts/build-tailwind.sh' and commit the regenerated file." - git diff -- static/tailwind.min.css - exit 1 - fi - - manifest-validation: - name: manifest-validation - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - - name: Validate plugin manifests - run: | - python - <<'EOF' - import json, sys - from pathlib import Path - - errors = [] - manifests = sorted(Path("plugins").glob("*/plugin.json")) - - for manifest in manifests: - try: - data = json.loads(manifest.read_text()) - except json.JSONDecodeError as e: - errors.append(f"{manifest}: invalid JSON — {e}") - continue - for field in ("id", "name"): - if field not in data: - errors.append(f"{manifest}: missing required field '{field}'") - pid = data.get("id", "") - if pid and pid != pid.lower(): - errors.append(f"{manifest}: 'id' must be lowercase, got '{pid}'") - # The plugin loader treats each plugins/ as a Python module, - # so the manifest 'id' must match its directory name. - dirname = manifest.parent.name - if pid and pid != dirname: - errors.append(f"{manifest}: 'id' ({pid!r}) must match directory name ({dirname!r})") - - if errors: - for e in errors: - print(f"::error::{e}") - sys.exit(1) - 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 - # 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: 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 - # actions/checkout resolves branches and tags in `ref` too, so a - # non-SHA here would silently un-pin the spec — the one thing this - # file exists to prevent. Demand a full 40-char SHA. - if ! printf '%s' "$sha" | grep -qE '^[0-9a-fA-F]{40}$'; then - echo "::error file=.feedpak-spec-ref::expected a full 40-character commit SHA, got '$sha' — a branch or tag name would defeat the pin" - 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). 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. - git fetch --no-tags --depth=1 origin main - 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 - # scripts/build-tailwind.sh). max-lines WARNS (the 1,500-line size ratchet; - # non-blocking), while import-x/no-unresolved + no-cycle HARD-ERROR on the - # ES-module graphs the refactor produces. Exemptions: docs/size-exemptions.md. - name: lint - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - # npm ci runs third-party postinstall scripts; don't leave the token in - # git config for them (this job never pushes). - with: - persist-credentials: false - - - uses: actions/setup-node@v4 - with: - node-version: '20' - - - name: Install dependencies - run: npm ci - - - name: ESLint (size norm + module hygiene) - run: npm run lint +name: ci + +# Runs only as a reusable workflow invoked by ship-ci.yml (for PRs into main +# and release/**). It deliberately has no standalone pull_request trigger: a +# direct run would publish unprefixed "" checks, but the org rulesets +# require the "ci / " names produced when ship-ci.yml calls this workflow. +on: + workflow_call: + +permissions: + contents: read + pull-requests: read + checks: read + +jobs: + + test: + name: test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + cache: 'pip' + + - name: Guard against print() / traceback.print_exc() in server.py, lib/, and bundled plugin routes + run: | + # git grep: tracked files only — no .pyc / __pycache__ noise from + # later pytest runs. Covers both audited patterns from #155 / #242. + # `(^|[^A-Za-z0-9_])` anchor avoids matching suffixes like `myprint(`; + # POSIX leaves `\b` undefined, so we use an explicit character class. + hits=$(git grep -nE '(^|[^A-Za-z0-9_])(print|traceback\.print_exc)[[:space:]]*\(' \ + -- server.py lib/ \ + $(git ls-files 'plugins/*/routes.py') || true) + if [ -n "$hits" ]; then + echo "$hits" + first=$(printf '%s\n' "$hits" | head -n1) + file=$(printf '%s' "$first" | cut -d: -f1) + line=$(printf '%s' "$first" | cut -d: -f2) + echo "::error file=${file},line=${line}::print() or traceback.print_exc() found in server.py, lib/, or a bundled plugin routes.py. Use the feedBack logger (lib/logging_setup.py) — see issues #155 / #242." + exit 1 + fi + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt -r requirements-test.txt + + - name: Run pytest + run: pytest + + - name: Run JS plugin-API tests + run: node --test tests/js/*.test.js 'tests/plugins/*/js/*.test.js' 'plugins/*/tests/*.test.js' + + tailwind-fresh: + # Guard that the committed static/tailwind.min.css is in sync with source. + # The Play CDN's runtime JIT was removed (feedBack-desktop#110); a prebuilt + # stylesheet only contains classes the scanner saw at build time, so stale + # CSS silently ships unstyled elements. Rebuild and fail on any diff. + name: tailwind-fresh + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Rebuild Tailwind CSS + run: bash scripts/build-tailwind.sh + + - name: Verify committed static/tailwind.min.css is fresh + run: | + # Hard-fail (matches the print() guard convention) — do NOT auto-commit. + if ! git diff --quiet -- static/tailwind.min.css; then + echo "::error file=static/tailwind.min.css::static/tailwind.min.css is stale. Run 'bash scripts/build-tailwind.sh' and commit the regenerated file." + git diff -- static/tailwind.min.css + exit 1 + fi + + manifest-validation: + name: manifest-validation + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Validate plugin manifests + run: | + python - <<'EOF' + import json, sys + from pathlib import Path + + errors = [] + manifests = sorted(Path("plugins").glob("*/plugin.json")) + + for manifest in manifests: + try: + data = json.loads(manifest.read_text()) + except json.JSONDecodeError as e: + errors.append(f"{manifest}: invalid JSON — {e}") + continue + for field in ("id", "name"): + if field not in data: + errors.append(f"{manifest}: missing required field '{field}'") + pid = data.get("id", "") + if pid and pid != pid.lower(): + errors.append(f"{manifest}: 'id' must be lowercase, got '{pid}'") + # The plugin loader treats each plugins/ as a Python module, + # so the manifest 'id' must match its directory name. + dirname = manifest.parent.name + if pid and pid != dirname: + errors.append(f"{manifest}: 'id' ({pid!r}) must match directory name ({dirname!r})") + + if errors: + for e in errors: + print(f"::error::{e}") + sys.exit(1) + 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. 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 + # 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: 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 + # actions/checkout resolves branches and tags in `ref` too, so a + # non-SHA here would silently un-pin the spec — the one thing this + # file exists to prevent. Demand a full 40-char SHA. + if ! printf '%s' "$sha" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error file=.feedpak-spec-ref::expected a full 40-character commit SHA, got '$sha' — a branch or tag name would defeat the pin" + 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). 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 + # scripts/build-tailwind.sh). max-lines WARNS (the 1,500-line size ratchet; + # non-blocking), while import-x/no-unresolved + no-cycle HARD-ERROR on the + # ES-module graphs the refactor produces. Exemptions: docs/size-exemptions.md. + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # npm ci runs third-party postinstall scripts; don't leave the token in + # git config for them (this job never pushes). + with: + persist-credentials: false + + - uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Install dependencies + run: npm ci + + - name: ESLint (size norm + module hygiene) + run: npm run lint diff --git a/CHANGELOG.md b/CHANGELOG.md index 247966b..636a428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,352 +1,354 @@ -# Changelog - -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - -## [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 *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` (writes are gated too, and reported separately: a key core writes lands in every pack - we emit, so an undeclared one seeds the ecosystem with non-spec data); (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. - **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 - bump `.feedpak-spec-ref` to the merged SHA in the same PR. `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 - `/v2` route, and the `FEEDBACK_UI` v2/legacy opt-out are deleted; `/` and `/v3` both serve - `static/v3/index.html`, which has been the default since 0.3.0. This is the first step of - the core-frontend ES-module migration (R3a): both shells load the same `static/app.js`, so - every subsequent step of that migration would otherwise have to be made, and verified, - twice. Removing the fallback now halves that surface before any of it is touched. - Incidentally fixes a latent bug in the old `index()` route — its guard read - `if getenv_compat("FEEDBACK_UI") or getenv_compat("FEEDBACK_UI") in ("v2", "legacy")`, - whose left operand is truthy for *any* non-empty value, so `FEEDBACK_UI=v3` actually served - the **v2** shell. `static/tailwind.min.css` is regenerated (the content globs scanned the - deleted file, so v2-only utility classes are now purged). Constitution amended to 1.3.0: - Principle II's frontend file list now names `static/v3/index.html`. - **Migration notes:** if you set `FEEDBACK_UI=v2` (or `=legacy`), or bookmarked `/v2`, there - is no longer a classic shell to fall back to — unset the variable and use `/`. The env var - itself is no longer read; the `SLOPSMITH_*`→`FEEDBACK_*` compat shim is unaffected. No - chart, settings, or plugin data changes, and no plugin API changes: v3 reuses the same - engine (`app.js`, `highway.js`, `playSong`, `showScreen`, the capability registry). - -### Fixed -- **The packaged desktop app could not start (`ModuleNotFoundError: No module named - 'appstate'`).** feedback-desktop's `scripts/bundle-slopsmith.sh` copies a *hardcoded - list* of core files into the app bundle — `server.py`, `VERSION`, `lib/`, `data/`, - `static/`, `plugins/__init__.py`. The root-level `appstate.py` and `routers/` added in - R3 shipped correctly in Docker and passed every test, and were then silently dropped - from the packaged app, which died at startup. Both now live under **`lib/`** — the one - core directory the Dockerfile (`COPY lib/`), `docker-compose.yml`, and the desktop - bundler (`cp -r lib`) all copy wholesale, and that all three put on `sys.path` (on - Windows via the embeddable-Python `._pth`, where `PYTHONPATH` is ignored). This needs no - change in feedback-desktop and no new release to take effect. Placing them there is also - correct under Principle V: with the injection seam, `appstate.py` constructs nothing and - does no import-time IO, and a route module only builds an `APIRouter`. The - `Dockerfile` / `.dockerignore` / `docker-compose.yml` entries added for the root layout - are reverted. New `tests/test_packaging.py` walks `server.py`'s module-level imports and - fails if any first-party module resolves outside a directory the packagers copy, so the - next root-level module can't ship broken. - -### Added -- **Perf harness now measures 2D-highway frame time (R3c gate).** `scripts/perf-baseline.mjs` gains a `--song` mode that reports per-frame draw-cost p50/p95/p99 (draw-tagged via `highway.addDrawHook`), the metric that gates the `highway.js` split. Maintainer/CI-only; baseline recorded in `docs/perf-baseline.md`. -- **`routers/` — extracting `server.py`'s route layer, cheapest-first (R3).** Each PR moves a cohesive route group into a `fastapi.APIRouter` under `lib/routers/`, mounted with `app.include_router(...)` at its original site (FastAPI matches in registration order; the full route table stays byte-identical). Bodies are verbatim — only the decorator receiver (`@app` → `@router`) and singleton reads (`meta_db` → `appstate.meta_db`, resolved at call time) change. So far: `audio_effects` (5), `artist_aliases` (5), `loops` (3), `playlists` (12 + covers), `ws_highway` (the 902-line highway chart WebSocket), `chart` (split/unsplit/work/fileinfo — unblocked by the DLC-path substrate), `library_extras`, `wanted`, `shop`, `progression`, `profile`, `stats` (the `/api/stats/{path}` catch-all stays registered last so it can't shadow `/recent` `/best` `/top`), `version` (`/api/version`; VERSION-file lookup adjusted for the router subdir depth), `art` (the `/api/song/{f}/art*` serve/cover-search/candidates/upload/url + `/api/art/{f}/override` routes; the shared `_song_pack_art_exists`/`_art_override_paths`/`_art_safe_name` helpers stay in `server.py` for the song/delete routes and are reached through the `appstate` seam, the CAA/release transport as `enrichment.X`), and `settings` (`GET`/`POST /api/settings`, `/reset`, and the two-phase atomic export/import bundle `/api/settings/export|import`; the shared `_default_settings` builder stays in `server.py` and is reached through the `appstate` seam), and `song` (upload/delete + the metadata write-back, user-meta, overrides, gap-fill, and per-song info routes; the scan/ingest helpers stay in `server.py` and are reached through new `appstate` seams — `kick_scan`, `invalidate_song_caches`, `stat_for_cache`, and a `scan_status()` getter — the `get_song_info` catch-all mounts after the art routes so it can't shadow them), and `library` + collections (the provider list/art/sync endpoints, the library query surface, and collection CRUD → `lib/routers/library.py`; the `LibraryProviderRegistry`/`LocalLibraryProvider`/`SmartCollectionProvider` classes + shared query/collection helpers move to `lib/library_registry.py`, and the registry instance + local provider ride the `appstate` seam — server.py still constructs the singleton and exposes `register_library_provider`/`unregister_library_provider` to plugins via `plugin_context` unchanged), and the `enrichment` route handlers (`/api/enrichment/*`: status, kick/cancel, per-song state, the Match-Review queue, and AcoustID identify → `lib/routers/enrichment.py`; the engine already lives in `lib/enrichment.py` and is reached as `enrichment.X`), and `media` (the file-serving routes — song audio `/audio/{f}`, the local-audio-path resolver `/api/audio-local-path`, and raw sloppak-member serving `/api/sloppak/{f}/file/{rel}` → `lib/routers/media.py`; the cache/static path seams were already in `appstate`), and `artist` (the artist page + external-links payload `/api/artist/{name}/page|links|links/refresh` → `lib/routers/artist.py`; MB link enrichment reached as `enrichment.X`), and `diagnostics` (`/api/diagnostics/export|preview|hardware`; the plugins-root lookup adjusted for the router subdir depth, `_running_version` reached through the `appstate` seam, pure payload-cap helpers re-exported for the `server._diag_*` tests), and `tunings` (`/api/tunings`; the pure `config.json` reader moved to `lib/appconfig.py`, the tuning-provider registry read through the `appstate` seam so plugin-contributed tunings still merge). The DLC library-path resolution (`_get_dlc_dir`, pure `_resolve_dlc_path`) moved to `lib/dlc_paths.py`, reading paths through the seam; `config_dir`/`dlc_dir`/`dlc_dir_env` now ride the `appstate` seam (env-derived, so the pop-and-reimport fixtures reconfigure it for free), and the shared request-field sanitizer `_clean_str` moved to `lib/reqfields.py`. The next cut is picked by a dependency-closure scan that ranks groups by how many `monkeypatch.setattr(server, …)` targets they'd drag along. -- **`routers/` — the first extracted route module (R3).** The five audio-effects mapping - endpoints move out of `server.py` into `lib/routers/audio_effects.py` as a - `fastapi.APIRouter`, mounted with `app.include_router(...)` **at the point in the file - where they used to be defined** — FastAPI matches routes in registration order, so the - mount site preserves it. Verified: the full 143-route table (paths, methods, *and* - order) is byte-for-byte identical to `main`. Bodies are verbatim; the only edits are - the decorator receiver (`@app.get` → `@router.get`) and the singleton read - (`audio_effect_mappings` → `appstate.audio_effect_mappings`, a module attribute - resolved at call time). This proves the seam from #833 under a real consumer, including - the second slot. The `_demo_mode_guard` middleware still blocks all four moved write - routes with 403, and `Query(...)` validation still 422s — both checked against a running - server. `server.py`: **9,445 → 9,386 lines**. -- **`lib/enrichment.py` — the metadata-enrichment subsystem leaves `server.py` (R3, move-only).** MusicBrainz / Cover-Art-Archive / AcoustID transport, the match-scorer glue, and the background enrichment worker (~930 lines, 61 defs) move out as one cohesive unit. Bodies are verbatim; the only changes are seam reads — `meta_db`/`config_dir`/`sloppak_cache_dir`/`art_cache_dir` and the two shared art helpers (`song_pack_art_exists`, `art_override_paths`, which stay in `server.py` for the art/delete routes) are reached through `appstate` at call time, and the User-Agent VERSION lookup is corrected for the module's new depth. `server.py` drives the worker through the module (`import enrichment`; the routes + scan lifecycle call `enrichment.X`); tests that faked the network on `server` now patch the same names on `enrichment` (module attribute resolved at call time, so one `setattr` reaches both the routes and the worker's internal callers). Acyclic — `enrichment` imports no `server`. Route table byte-identical; full suite green. `server.py`: 6,917 → 5,988. -- **`appstate.py` — the router seam (R3).** Route modules moving out of `server.py` - need `meta_db` and friends but must not `import server`, or the import graph goes - circular the moment `server` imports them back. So `server.py` keeps *constructing* - its singletons and now **injects** them once — `appstate.configure(meta_db=…, - audio_effect_mappings=…)` — and a router reads them back as module attributes at call - time (`import appstate; appstate.meta_db.…`). This is the Python analogue of the - frontend refactor's injected `configureX({…})` seams and of the plugin - `setup(app, context)` contract: dependencies flow one way, `server → routers → - appstate`. Two properties are load-bearing and pinned by `tests/test_appstate.py`: - (1) `import appstate` constructs nothing and touches no disk, so the ~49 test fixtures - that `sys.modules.pop("server")` + re-import (to rebuild `meta_db` under a patched - `CONFIG_DIR`) keep working untouched — a singleton *owned* by `appstate` would survive - that pop and go stale; (2) reads must be late-bound (`appstate.meta_db`, never - `from appstate import meta_db`), since a `from` import freezes the binding and defeats - both a later `configure()` and `monkeypatch.setattr` — the same read-only-binding trap - as ES `import`. `configure()` rejects an unknown slot rather than silently creating a - global nothing reads, and the suite asserts `server` actually calls it (a seam whose - wiring can no-op undetected is worse than no seam). Lives at `lib/appstate.py`. - -### Changed -- **`AudioEffectsMappingDB` moved out of `server.py` into `lib/audio_effects_db.py` - (R3, move-only).** The core-owned song/tone → provider routing index follows - `MetadataDB` out of the host file, byte-identical apart from the same constructor - seam (`__init__` takes `config_dir`; `audio_effect_mappings = AudioEffectsMappingDB(CONFIG_DIR)`), - so the module does no IO at import. The singleton stays in `server.py`; no route, - no test and no `monkeypatch.setattr(server, …)` target moves. `server.py`: - **9,705 → 9,433 lines**. -- **`MetadataDB` moved out of `server.py` into `lib/metadata_db.py` (R3, move-only).** - The library metadata cache — the `MetadataDB` class (4,018 lines) plus the query - helpers it owns (keyset paging cursors, the tuning grouping key, smart-arrangement - naming, tag normalisation, the startup DB-restore swap) — now lives in its own flat - `lib/` module. `server.py` drops from **14,037 → 9,705 lines** and keeps the - `meta_db` singleton, so `server.meta_db` and `server.app` resolve exactly as before - and every route is untouched. The only non-verbatim change is the seam that lets the - class leave `server.py`: `MetadataDB.__init__` now takes `config_dir` explicitly - (`meta_db = MetadataDB(CONFIG_DIR)`) instead of reading the module-level `CONFIG_DIR`, - which also means `lib/metadata_db.py` performs no IO at import (Principle V). Logging - still goes through the `feedBack.server` logger, so existing log filters and `caplog` - assertions resolve to the same logger object. `tests/test_settings_export_library_db.py` - now imports `_apply_pending_db_restore` from `metadata_db` (the test moved with its - subject); no other test changed. Every moved block is byte-identical to its - `server.py` original. - -### Added -- **Plugins can ship an ES-module `src/` tree (module-migration rails, R0).** The host gains three things so a plugin can move off a single global-scope `screen.js` IIFE onto native ES modules with **no build step**: (1) a new sandboxed `GET /api/plugins/{id}/src/{path}` route that serves a plugin's `src/` source subtree, containment-checked by the same `safe_join` guard as `assets/` (traversal/absolute/NUL → 404); (2) the live-edit cache contract — `Cache-Control: no-cache` + a weak mtime/size `ETag` + `If-None-Match`→`304` — applied to `src/`, `screen.js`, and `assets/` (previously `screen.js` sent no cache headers and `assets/` emitted an ETag but never revalidated), so an edited module reloads on refresh while unchanged ones `304`; and (3) `scriptType`/`minHost` passthrough from `plugin.json` to `/api/plugins`, with the loader injecting a plugin that declares `"scriptType":"module"` as `