From d0626f5618dfa57ddeaa703f49c99e40ee5999f0 Mon Sep 17 00:00:00 2001 From: topkoa Date: Sun, 12 Jul 2026 23:44:15 -0400 Subject: [PATCH] =?UTF-8?q?ci:=20address=20review=20=E2=80=94=20check=20wr?= =?UTF-8?q?ites=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