From 0158286d063bf2d0461e1d6624fcff3d7e5fd3b7 Mon Sep 17 00:00:00 2001 From: topkoa Date: Mon, 13 Jul 2026 00:45:38 -0400 Subject: [PATCH] =?UTF-8?q?ci:=20flow-aware=20manifest=20discovery=20?= =?UTF-8?q?=E2=80=94=20a=20name=20list=20missed=20real=20readers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found lib/routers/chart.py binding `m = load_manifest(p) or {}` and reading eight manifest keys through it. `m` was not in MANIFEST_VARS and `m.get` did not match the readers-complete regex, so the module was invisible to BOTH halves of the gate — unlisted and unscanned. Same for lib/routers/song.py (binds `manifest` from load_manifest for enrichment gap-fill). Both are now in READERS. The structural fix, not the name-list patch: keys_touched() now discovers receivers flow-aware — any local assigned from load_manifest(...) is a manifest dict, whatever it is called. MANIFEST_VARS remains only as the fallback for manifests that arrive as function parameters (ws_highway). A plain `m = {}` is not a receiver; test pins that. readers-complete now reuses keys_touched() itself instead of a parallel KEY_OPS regex — the two detectors diverged once already (that is exactly how chart.py slipped through), so now there is one detector and one truth. check_reverse() gets a 300s subprocess timeout: the validator executes at a pinned SHA, but a pathological pack or validator bug should fail the job, not hang the runner to the Actions-level timeout. Tests: flow-aware receiver under an arbitrary name (read + write), and the negative — a plain dict named `m` stays out of the scan. 17 pass. Signed-off-by: topkoa --- tests/test_spec_gate.py | 22 +++++++++ tools/check_spec_conformance.py | 83 ++++++++++++++++++++++++++------- 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/tests/test_spec_gate.py b/tests/test_spec_gate.py index 562d728..29027a2 100644 --- a/tests/test_spec_gate.py +++ b/tests/test_spec_gate.py @@ -59,6 +59,28 @@ def test_load_manifest_wrapped_get_is_seen(tmp_path): assert "original_audio" in reads +def test_flow_aware_receiver_any_name(tmp_path): + # lib/routers/chart.py binds `m = load_manifest(p) or {}` — a fixed name + # list missed it and the module's reads went entirely unscanned. Locals + # assigned from load_manifest must be receivers whatever they're called. + reads, writes = _touch( + tmp_path, + """ + pak_info = sloppak_mod.load_manifest(p) or {} + x = pak_info.get("stems") + pak_info["genres"] = ["metal"] + """, + ) + assert reads == {"stems"} and writes == {"genres"} + + +def test_plain_dict_named_m_is_not_a_receiver(tmp_path): + # Flow-awareness must not make every short local a manifest: `m` bound to + # something other than load_manifest stays out of the scan. + reads, writes = _touch(tmp_path, 'm = {}\nx = m.get("title")') + assert reads == set() and writes == set() + + def test_unrelated_dicts_are_ignored(tmp_path): reads, writes = _touch(tmp_path, 'x = config.get("title"); settings["artist"] = 1') assert reads == set() and writes == set() diff --git a/tools/check_spec_conformance.py b/tools/check_spec_conformance.py index f5746d4..cfa31d0 100644 --- a/tools/check_spec_conformance.py +++ b/tools/check_spec_conformance.py @@ -59,6 +59,8 @@ READERS = [ "lib/songmeta.py", "lib/gp2notation.py", # rewrites manifest.yaml; stamps feedpak_version "lib/routers/ws_highway.py", # reads `authors` off a feedpak manifest + "lib/routers/chart.py", # Get-info panel: binds `m = load_manifest(...)` + "lib/routers/song.py", # enrichment gap-fill: reads the manifest directly ] # Where check_readers_complete() looks for modules READERS may have missed. @@ -69,12 +71,11 @@ READER_SEARCH = ["lib/**/*.py", "server.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. +# Locals assumed to hold a manifest dict by NAME. This is only the fallback for +# manifests that arrive as function parameters (ws_highway's `manifest` arg); +# locals ASSIGNED from load_manifest(...) are discovered flow-aware in +# keys_touched(), whatever they are called — chart.py's `m` taught us that a +# name list alone silently misses real readers. MANIFEST_VARS = {"manifest", "mf"} # Packs committed to this repo, checked against the spec's reference validator. @@ -97,13 +98,39 @@ def _fail(msg: str) -> None: print(f"::error::{msg}") -def _is_manifest_receiver(node: ast.expr) -> bool: +def _manifest_locals(tree: ast.AST) -> set[str]: + """Names of locals assigned from `load_manifest(...)` anywhere in `tree`. + + Flow-aware receiver discovery: chart.py binds `m = load_manifest(p) or {}`, + and a fixed name list (`manifest`, `mf`) silently missed it — the module's + reads went entirely unscanned. Whatever the local is called, an assignment + whose right-hand side mentions load_manifest marks it as a manifest dict. + """ + names: set[str] = set() + for node in ast.walk(tree): + if not isinstance(node, (ast.Assign, ast.AnnAssign)): + continue + try: + rhs = ast.unparse(node.value) if node.value else "" + except Exception: + continue + if "load_manifest" not in rhs: + continue + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + for t in targets: + if isinstance(t, ast.Name): + names.add(t.id) + return names + + +def _is_manifest_receiver(node: ast.expr, receivers: set[str]) -> 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")`. + Covers named receivers (fixed names + flow-discovered locals) plus the + inline 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: + if isinstance(node, ast.Name) and node.id in receivers: return True try: src = ast.unparse(node) @@ -124,6 +151,7 @@ def keys_touched(path: Path) -> tuple[set[str], set[str]]: reads: set[str] = set() writes: set[str] = set() tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + receivers = MANIFEST_VARS | _manifest_locals(tree) for node in ast.walk(tree): if ( isinstance(node, ast.Call) @@ -132,7 +160,7 @@ def keys_touched(path: Path) -> tuple[set[str], set[str]]: # 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 _is_manifest_receiver(node.func.value, receivers) and node.args and isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str) @@ -141,7 +169,7 @@ def keys_touched(path: Path) -> tuple[set[str], set[str]]: bucket.add(node.args[0].value) elif ( isinstance(node, ast.Subscript) - and _is_manifest_receiver(node.value) + and _is_manifest_receiver(node.value, receivers) and isinstance(node.slice, ast.Constant) and isinstance(node.slice.value, str) ): @@ -238,7 +266,17 @@ def check_readers_complete() -> bool: if rel in listed: continue src = path.read_text(encoding="utf-8", errors="replace") - if KEY_OPS.search(src) and FEEDPAK_SIGNALS.search(src): + if not FEEDPAK_SIGNALS.search(src): + continue + # Same scanner the coverage check uses — a separate "does it touch + # keys" regex diverged from it once already (`m = load_manifest(...)` + # in chart.py matched neither `manifest` nor `mf`, so the module + # went unlisted AND unscanned). One detector, one truth. + try: + reads, writes = keys_touched(path) + except SyntaxError: + continue + if reads or writes: missing.append(rel) for rel in missing: @@ -372,11 +410,20 @@ def check_reverse(spec: Path) -> bool: 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, - ) + try: + proc = subprocess.run( + [sys.executable, str(spec / "tools" / "validate.py"), *[str(p) for p in packs]], + capture_output=True, + text=True, + # The validator takes seconds for all committed packs; a pathological + # pack or validator bug must fail the job, not hang the runner until + # the Actions-level timeout. + timeout=300, + ) + except subprocess.TimeoutExpired: + _fail("the spec's reference validator did not finish within 300s — pathological pack or validator bug?") + print(" reverse: FAILED") + return False sys.stdout.write("".join(f" {ln}\n" for ln in proc.stdout.splitlines() if ln.strip())) if proc.returncode != 0: _fail(