fix(packages): validate recipe list fields - #112
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#112 "fix(packages): validate recipe list fields"
head: fa27e25 author: moomal2026 ci: none ran (checks.txt is empty)
Verdict: The right defect to fix, and the validation itself is sound — I probed it with scalars, dicts, nested lists, bools and floats and it rejects all of them. But YAML's ordinary way of writing an empty list, a bare configure_args:, is now a RecipeError, and the registry swallows RecipeError without a word. The net effect is that a recipe which loads today disappears from the registry with no output at all.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | ebuild/packages/recipe.py:44-45 |
configure_args: written with no value parses as None, not [], so isinstance(value, list) is false and the recipe is rejected. That is the standard YAML spelling of "this field is empty", and it used to load. Worse than the rejection is where it lands: ebuild/packages/registry.py:113 and :120 are except (RecipeError, FileNotFoundError): continue — no log, no counter, no re-raise. So the recipe is not reported invalid, it is silently absent, and the next thing the user sees is ebuild add demo failing with "not found" for a package whose recipe is sitting right there. Verified with one recipe differing only in configure_args: having no value: PackageRegistry.scan() returns 1 on origin/master and 0 on this head, printing nothing either way. |
In _parse_string_list, before the type check: if value is None: value = []. That is what the field means and it removes the whole exposure. Add the case to both parametrised tests — it is the one input most likely to be written by hand. Separately, registry.py's bare continue deserves its own PR: validation whose only product is a message is worth nothing if the caller drops the message. |
| 2 | Medium | ebuild/packages/recipe.py:45,48,130 |
The new errors are 'configure_args' must be a list. — no package name, no file path. Every other RecipeError in this module is prefixed Package '{self.name}': … (:78, :84, :91). The path is available and thrown away: load_recipe passes it at :181, _parse_recipe declares source_path: Optional[Path] = None at :130, and the parameter is referenced nowhere in the file. Master design §9.2 requires "actionable diagnostics with remediation guidance"; "one of your recipes has a bad field" is neither. |
Thread it through: _parse_string_list(raw, field, source_path=source_path, package=...) and prefix Package 'demo' ({path}): 'configure_args' must be a list, got str. Naming the observed type costs nothing and is usually the whole answer. |
| 3 | Medium | PR body, "Testing" | "Ran the full pytest suite successfully with all tests passing." Not reproducible: tests/ebuild/test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary fails on this head. It also fails identically on origin/master, with ninja, cmake and gcc all present on the host, so it is pre-existing and not caused by this PR — but the suite does not pass, and the claim is made with no command and no output. The brief treats an unsupported "verified" as itself the finding. "39 tests passed" for the package suite is likewise unattributable; the new file contains 13. |
State the command, the counts, and the one known-failing test with a note that it predates the change. That is a stronger position than an unqualified pass, not a weaker one. |
| 4 | Low | ebuild/packages/recipe.py:45 |
A recipe written with the legacy alias, depends: zlib, raises 'dependencies' must be a list. — naming a key the author did not write and cannot find in their file. test_depends_alias_must_be_a_list pins this behaviour with match="dependencies". |
Report the key that was actually present. Pass it down, or use fallback_field when that is the branch taken. |
| 5 | Low | ebuild/packages/recipe.py:37-42 |
When both dependencies and depends are present, depends is silently ignored — verified, dependencies: [a] + depends: [b] yields ['a'] with no diagnostic. The precedence is inherited from the raw.get("dependencies", raw.get("depends", [])) it replaces, so it is not new; but this PR is what centralises the alias handling and adds the alias tests, which makes it the natural place to pin it. |
Either a test asserting the precedence, or a RecipeError on both-present. Silently dropping one of two conflicting declarations of the same thing is the failure mode this PR exists to prevent, one field over. |
| 6 | Low | tests/ebuild/test_package_recipe.py:107 |
No trailing newline (last byte is 0x29, )). |
Add one. |
Architecture conformance
Conforms. ebuild is Tier 1 Foundation (§21), and package recipes are squarely eBuild's "Packages / Registry" branch of the §9.1 engine diagram. No dependency direction is touched — recipe.py imports only yaml, dataclasses, pathlib, re, typing. Nothing here is a runtime dependency, so §5.1's "eBuild understands the complete graph but is not a runtime dependency" holds.
The change serves §9.2 in intent: an SDK that accepts a malformed recipe and fails later during command construction is the opposite of "actionable diagnostics". Finding 1 is where the implementation inverts that intent — the diagnostic exists and never reaches a human.
Worth noting for context, not as a finding against this PR: recipes/*.yaml is a third-party source recipe, not a §10.1 component manifest, and the design never specifies its schema. §10.1 defines Identity / Compatibility / Dependencies / Capabilities / Permissions / Resources / Integrity / Compliance for components; this format carries name, version, url, checksum, build system, dependencies, patches, three argument lists, description and license, and its validation is being assembled one field at a time by PRs like this one. The 2026-09-03 proposal already in proposals/2026-09.md ("§11 has no provenance rule, so a package's checksum is only as trustworthy as the document that names its URL", trigger ebuild#111) covers the adjacent half of this. I am not filing a second proposal for the schema gap; it belongs with that one when a human picks it up.
Proposed changes
Smallest sequence, in order:
ebuild/packages/recipe.py, inside_parse_string_list, before theisinstancecheck:and addif value is None: # `configure_args:` with nothing after it value = []
""(the empty key) as a case to both@pytest.mark.parametrizelists, asserting it loads as[]rather than raising. This is finding 1 and it is two lines.- Prefix the two error messages with the package name and, when known, the source path — finding 2. Use the
source_paththat_parse_recipealready receives and currently ignores. - Findings 4, 5, 6: report the present key, pin the alias precedence, add the newline.
- Rewrite the Testing section against a command you ran, naming
test_end_to_end_build_from_outside_produces_the_binaryas a pre-existing failure.
Only finding 1 blocks merge. Everything else is worth doing in the same pass because the file is already open.
Not checked
- Not run: any CI.
checks.txtfor this head is empty — no workflow has executed onfa27e25. Everything below is from my machine. - Not verified: that downstream build code actually consumes these five fields the way the PR assumes. The premise is "downstream code expects
configure_argsto be a list"; I confirmed the fields exist onPackageRecipeand are populated, but I did not trace them into command construction or run a package build, so I cannot say what a scalar would really have done there. - Not verified: the
39 tests passedfigure. I could not find a selection of the package-related tests that yields 39, and the body does not give the command. - Not checked: whether any recipe outside this repository — a vendor or community recipe not in
recipes/— uses the empty-key form. I checked the five shipped recipes only; all five still load. The exposure in finding 1 is to hand-written recipes, which is precisely the population that cannot be checked from here. - Not checked:
tests/functional,tests_backup/, and therecipes/linting story. I rantests/unitandtests/ebuild. - Not attempted: a fix PR. Finding 1's fix belongs in this branch, which is not mine to touch; the
registry.pyswallow is an independentmasterdefect but fixing it means choosing a reporting mechanism, which is a design decision rather than a provable one-line repair.
Evidence
fresh venv (pytest, click, pyyaml), read-only git archive extraction of fa27e25:
pytest tests/ebuild/test_package_recipe.py -q -> 13 passed in 0.04s
pytest tests/unit tests/ebuild -q -> 1 failed, 567 passed, 3 skipped
the failure: tests/ebuild/test_build_dir_resolution.py::
test_end_to_end_build_from_outside_produces_the_binary
same test on origin/master (e5d8052) -> 1 failed, 7 passed
ninja, cmake, gcc all present -> pre-existing, not this PR
all 5 shipped recipes/*.yaml load through the new parser -> 0 failures
input probes against this head:
configure_args: (empty key) -> RecipeError: 'configure_args' must be a list.
depends: zlib (scalar) -> RecipeError: 'dependencies' must be a list. <- names the wrong key
dependencies:[a] + depends:[b] -> OK, dependencies == ['a'], depends dropped silently
patches: [[a, b]] (nested) -> RecipeError: 'patches' must contain only strings.
build_args: [true] (bool) -> RecipeError: 'build_args' must contain only strings.
install_args: {k: v} (mapping) -> RecipeError: 'install_args' must be a list.
finding 1, end to end, one recipe whose only unusual line is `configure_args:` with no value:
PackageRegistry().add_search_path(dir); scan()
origin/master : recipes loaded: 1 demo found: True
this head : recipes loaded: 0 demo found: False
neither run printed anything.
So the type checks all do what they claim. The gap is the empty key, and the reason it matters is that registry.py turns the resulting error into silence.
Automated architecture review of fa27e2581ca2 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Summary
This PR improves package recipe validation by ensuring that fields expected to contain lists of strings are validated when the recipe is parsed.
Previously, malformed YAML such as configure_args: "-DENABLE_AI=ON" was accepted by the recipe loader even though downstream code expects configure_args to be a list. This could lead to runtime errors when build commands are constructed. A similar issue could occur with scalar dependency values.
Type of Change
Bug fix and tests.
Changes
Added validation for dependencies, patches, configure_args, build_args, and install_args so they must be lists containing only strings.
Preserved support for the existing depends alias.
Added regression tests for scalar values, non-string list items, valid list inputs, and the depends alias.
Testing
Reproduced the original issue using a scalar configure_args value and confirmed that it was accepted by the parser even though downstream build code expects a list.
Ran the package-related pytest suite successfully: 39 tests passed.
Ran the full pytest suite successfully with all tests passing.
Related Issues
None.
Screenshots / Logs
Not applicable.