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