got-feedBack/feedBack
1
0
Fork 0
mirror of https://github.com/got-feedBack/feedBack.git synced 2026-07-21 20:31:21 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

ci: address review — check writes too, pin deps, harden the spec pin

Browse Source 
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 <topkoa@gmail.com>
This commit is contained in:
topkoa 2026-07-12 23:44:15 -04:00
parent 0dc9fd7ba8
commit d0626f5618
3 changed files with 85 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
.github/workflows/ci.yml vendored
View File

@ -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

11
docs/feedpak-spec-gate.md
View File

@ -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:

96
tools/check_spec_conformance.py
View File

@ -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