mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-29 08:01:22 +00:00
ci: legible failures for malformed exceptions file; docs catch up
_parse_exceptions() now validates the document shape — top level must be a mapping, 'exceptions' must be a list, each entry a mapping, and YAML parse errors are caught — each failing with a ::error:: instead of an AttributeError traceback. CI output must say what to fix. Parametrised tests cover all four malformed shapes. docs/feedpak-spec-gate.md: the Limitations section still described the pre-flow-aware scanner (KEY_OPS, name-list-only receivers). Now states the actual residual gaps: function-parameter manifests are recognised by name only, and helper-mediated literal keys (song.py's _gap_fill_manifest_absent(manifest, "album")) are unseen by both the scan and the readers-complete guard, since they share one detector. Signed-off-by: topkoa <topkoa@gmail.com>
This commit is contained in:
parent
0158286d06
commit
203f82b6fe
@ -104,18 +104,21 @@ branch.
|
||||
|
||||
Known, and worth fixing in follow-ups rather than blocking on:
|
||||
|
||||
- **Layer 1 is name-heuristic.** It recognises manifest dicts bound to locals named in `MANIFEST_VARS`
|
||||
(`manifest`, `mf`) plus the `load_manifest(...)` call form. This works because the loaders use a uniform
|
||||
idiom, but it is fragile against a refactor that renames the local. The `readers-complete` guard limits the
|
||||
blast radius — a module that touches manifest keys can't go unscanned — but a *renamed local inside a
|
||||
scanned module* would still slip. The hardening step is to route all manifest access through a single
|
||||
declared `KNOWN_MANIFEST_KEYS` registry in `lib/sloppak.py`; the gate then compares registry against schema
|
||||
exactly instead of inferring.
|
||||
- **Layer 1's receiver detection is heuristic.** Locals *assigned from* `load_manifest(...)` are discovered
|
||||
flow-aware whatever they're called (chart.py's `m` taught us that), and the inline
|
||||
`(load_manifest(p) or {}).get(...)` form is recognised — but a manifest that arrives as a **function
|
||||
parameter** is only recognised by name (`MANIFEST_VARS`: `manifest`, `mf`). A parameter called something
|
||||
else would slip. The hardening step is to route all manifest access through a single declared
|
||||
`KNOWN_MANIFEST_KEYS` registry in `lib/sloppak.py`; the gate then compares registry against schema exactly
|
||||
instead of inferring.
|
||||
- **Layer 1 covers top-level keys only.** Nested structure (`arrangements[].file`, `.id`, `.notation`) isn't
|
||||
checked. Extending to it means walking the schema's `$ref` subschemas.
|
||||
- **Layer 1 recognises `get`, `setdefault`, and subscripts** as key access. `update()` and `pop()` aren't
|
||||
used against a feedpak manifest anywhere in the tree, so they're deliberately not special-cased rather than
|
||||
speculatively handled — but note `KEY_OPS` (which drives `readers-complete`) doesn't look for them either.
|
||||
speculatively handled. `readers-complete` reuses the same scanner (`keys_touched()`), so this blind spot is
|
||||
shared, not doubled: a module using only unrecognised access forms would evade both. Keys passed as literal
|
||||
*call arguments* to helpers (`_gap_fill_manifest_absent(manifest, "album")` in `lib/routers/song.py`) are
|
||||
likewise unseen.
|
||||
- **Layer 4 can't catch unknown keys**, because `manifest.schema.json` sets `additionalProperties: true` and
|
||||
the reference validator deliberately "treats unknown keys/files as forward-compatible". Fixing this
|
||||
properly belongs in the spec (tighten the schema, or give the validator a `--strict` mode). Until then,
|
||||
|
||||
@ -105,6 +105,19 @@ def test_duplicate_exception_key_is_rejected():
|
||||
gate._parse_exceptions(textwrap.dedent(doc), "test")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("doc", [
|
||||
"- just\n- a\n- list\n", # list at top level
|
||||
"exceptions: not-a-list\n", # scalar where list expected
|
||||
"exceptions:\n - just-a-string\n", # non-mapping entry
|
||||
"exceptions: [\n", # invalid YAML
|
||||
])
|
||||
def test_malformed_exceptions_fail_legibly(doc):
|
||||
# Malformed shapes must exit with a ::error::, not an AttributeError
|
||||
# traceback — CI output has to say what to fix.
|
||||
with pytest.raises(SystemExit):
|
||||
gate._parse_exceptions(doc, "test")
|
||||
|
||||
|
||||
def test_exception_without_issue_is_rejected():
|
||||
# No tracking issue, no exception — entries are debt and debt is tracked.
|
||||
doc = """
|
||||
|
||||
@ -182,9 +182,25 @@ 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(text) or {}
|
||||
try:
|
||||
data = yaml.safe_load(text) or {}
|
||||
except yaml.YAMLError as e:
|
||||
_fail(f"{origin}: not valid YAML — {e}")
|
||||
sys.exit(1)
|
||||
# A malformed shape (list/string at top level, non-mapping entry) must fail
|
||||
# with a CI-legible error, not an AttributeError traceback.
|
||||
if not isinstance(data, dict):
|
||||
_fail(f"{origin}: top level must be a mapping with an 'exceptions' list, got {type(data).__name__}")
|
||||
sys.exit(1)
|
||||
entries = data.get("exceptions") or []
|
||||
if not isinstance(entries, list):
|
||||
_fail(f"{origin}: 'exceptions' must be a list, got {type(entries).__name__}")
|
||||
sys.exit(1)
|
||||
out: dict[str, str] = {}
|
||||
for entry in data.get("exceptions") or []:
|
||||
for entry in entries:
|
||||
if not isinstance(entry, dict):
|
||||
_fail(f"{origin}: each exception must be a mapping with 'key' and 'issue', got {type(entry).__name__}")
|
||||
sys.exit(1)
|
||||
key, issue = entry.get("key"), entry.get("issue")
|
||||
if not key or not issue:
|
||||
_fail(f"{origin}: every exception needs both 'key' and 'issue'")
|
||||
|
||||
Loading…
Reference in New Issue
Block a user