mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 12:52:29 +00:00
ci: second review pass — recurse examples, reject duplicate exceptions
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/<group>/ 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 <topkoa@gmail.com>
This commit is contained in:
parent
d0626f5618
commit
ceb1e143cd
@ -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.
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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/<group>/ 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:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user