Skip to content

feat: implement package management system with recipe support, regist… - #111

Open
Grantlinkz wants to merge 7 commits into
embeddedos-org:masterfrom
Grantlinkz:Remote-Package-Index-And-Ecosystem
Open

feat: implement package management system with recipe support, regist…#111
Grantlinkz wants to merge 7 commits into
embeddedos-org:masterfrom
Grantlinkz:Remote-Package-Index-And-Ecosystem

Conversation

@Grantlinkz

Copy link
Copy Markdown

…ry, and build synchronization.

Summary

Type of Change

  • eat — New feature
  • ix — Bug fix
  • docs — Documentation only
  • style — Formatting, no code change
  • [ ]
    efactor — Code restructuring without behavior change
  • est — Add or fix tests
  • �uild — Build system or dependency changes
  • ci — CI/CD pipeline changes
  • perf — Performance improvement

Changes

Testing

  • Unit tests pass (ctest --test-dir build --output-on-failure)
  • Integration tests pass
  • Manual testing performed
  • New tests added for new functionality

Pre-Submission Checklist

  • Code compiles without warnings (-Wall -Wextra -Werror for C)
  • All existing tests pass
  • New tests added for new functionality
  • Documentation updated if API changed
  • Commit messages follow (): convention
  • Branch is rebased on latest master

Related Issues

Screenshots / Logs

Additional Notes

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#111 "feat: implement package management system with recipe support, registry…"

head: 77f1d9f author: Grantlinkz ci: none reported · mergeable: CONFLICTING

Verdict: The shape is right — a remote index client in eBuild with HTTPS-only fetching,
strict name sanitisation, a size cap that survives a lying Content-Length, an atomic cache
replace, and an offline mode. Three things block it. A cached remote recipe silently
overwrites a project's own pinned url and checksum (reproduced). Nothing authenticates the
index, so checksum pins bytes without proving provenance — §10.1 asks for signatures. And
the default index URL points at a repository that does not exist, so the feature has never run
against a real index. The branch is 7 commits behind master, conflicts, and re-implements two
things already merged there.

Findings

# Severity File:line Finding Recommended fix
1 High ebuild/packages/repository.py:118 vs :126-129 A cached remote recipe overrides the project's own pinned recipe, including its checksum. load_index is careful — if name not in self._index at :126, with the comment "Local recipe directory overrides remote index if already loaded". add_recipe_directory is not: :118 does an unconditional self._index[recipe.name] = info. load_all_sources calls it in the order project → shipped → cached remote (:157-160), so step 3 overwrites steps 1 and 2. Reproduced: a project shipping recipes/cjson.yaml (url: …/cjson-PROJECT.tar.gz, checksum: sha256:111…) alongside a cached ~/.ebuild/index/recipes/cjson.yaml (url: …/cjson-REMOTE.tar.gz, checksum: sha256:222…) resolves to version 9.9.9, the REMOTE url and the REMOTE checksum. So ebuild update-index can replace what a project has pinned in its own tree, and because the checksum is replaced too, fetcher.py's verification passes against the substituted archive. This is the §9.2 "reproducible lockfiles/manifests for production builds" guarantee, inverted. Make add_recipe_directory respect precedence the way load_index does — either if recipe.name not in self._index there too, or give load_all_sources an explicit priority so the first source to define a name wins. Then add the test that would have caught it: three sources defining one package, asserting the project-local one resolves. That test does not exist today, which is why this is invisible to a green suite.
2 High ebuild/packages/index_sync.py:127-232 The index is fetched and trusted with no authenticity or integrity check. sync() verifies the URL scheme is HTTPS, bounds the size, and parses JSON — then writes packages.json and derives recipe YAML from it. There is no signature over the index and no digest it is checked against. The per-package checksum is copied straight out of that same document (:207), so it pins the bytes of whatever url the same document supplied: it proves the download was not corrupted in transit, and nothing about where the package came from. --url (commands.py:1705) accepts any HTTPS origin with no allowlist or pinning. Master design §10.1 lists Integrity — "Hashes, signatures and provenance" as a component-contract field; §11 is the Registry and §15.1 requires "signed metadata and package provenance". Combined with finding 1, a single unauthenticated document can redirect and re-pin a project's dependencies. Not all of it belongs in this PR, but the boundary has to be drawn here rather than left implicit. Minimum: detach the pin from the index — refuse to overwrite a checksum that a project-local recipe already states (finding 1 covers the mechanism), and record the index's own digest in packages.json so a changed index is visible. Then state in the docs that the index is unauthenticated, so nobody builds a release path on it before signing exists. The full answer is a detached signature over index.json verified against a key shipped with eBuild, which is §14.1's "integrate key management across eBoot, eSec, eOTA and release signing" extended to the registry, and is worth its own design discussion.
3 High ebuild/packages/index_sync.py:32-34 DEFAULT_INDEX_URL points at a repository that does not exist. https://raw.githubusercontent.com/embeddedos-org/recipes/main/index.jsongh api repos/embeddedos-org/recipes returns 404, and so does the index.json path. It also names branch main, while .github/STANDARDS.md states every repo has exactly master and release; this branch's own history contains fix/setup-clones-master-not-main (#99) for the same mistake. Simulated the fresh-machine path: no cache plus a 404 gives IndexSyncError: Failed to fetch remote package index and no cache is available: HTTP Error 404: Not Found, and no cache is written, so the next run fails identically. Every test mocks urllib.request.urlopen, so the feature has never been exercised against a real index. Either create embeddedos-org/recipes with an index.json on master and point the constant at master, or make the default empty and require --url until the registry exists — with ebuild search saying so rather than "Try running 'ebuild update-index'" (commands.py:1692-1696), which today is advice to run a command that cannot succeed. §28's claims policy applies: this is Planned, not Implemented, until the index it reads exists.
4 Medium branch state mergeStateStatus: DIRTY, mergeable: CONFLICTING. The branch sits on 562d28d (merge of #99); master is at e5d8052, 7 commits ahead. git apply of the PR diff onto master fails on ebuild/cli/commands.py and ebuild/packages/registry.py. Two of the conflicts are re-implementations of work already merged: master already has subprocess.run(argv, cwd=str(cwd), capture_output=True, text=True) and _NO_TESTS_MARKERS in test() (commands.py:2451, 2649), and master already has a working _board_config() with the docstring "A project that states its part's real capacity should not be measured against the reference part for its family." This PR's _board_config is an independent reimplementation that drops that docstring — merged as-is it would replace documented code with undocumented code. Rebase on master and drop the test() and _board_config() changes entirely; they are already there and better documented. That should also shrink the diff and remove both conflicts.
5 Medium pr body The body is the unfilled template. No summary, - placeholders under Changes, every Testing and Pre-Submission box unchecked, no command output, no related issue — for 927 added lines that include a network-fetching subsystem and five new recipes. The brief treats an unsupported "verified" as a finding; a change of this size with no claim at all is harder to review, not easier. (The garbled type labels — eat, ix, efactor, est, uild — are not the author's doing; see Architecture conformance.) Fill in Summary, Changes, and Testing with what was actually run. If the answer is "the 11 new tests", say that and say what is not covered — several of the findings above are honest gaps rather than mistakes, and naming them is faster than having a reviewer find them.
6 Medium ebuild/packages/index_sync.py:186-190 vs repository.py:113-115 The cache is written before per-entry validation, and the reader does not sanitise. packages.json is atomically replaced at :186-190 with the raw downloaded array; only afterwards does the loop apply sanitize_package_name. So an entry the guard rejects still lands in the cached index, and load_index at repository.py:113 takes name = str(entry["name"]) with no sanitisation before building a PackageInfo. Reproduced: an index containing good, urlless and bad name! logs "Skipping unsafe package entry" and writes only good.yaml, while packages.json holds all three — and bad name! is then searchable. The path-traversal guard protects the recipe filenames and nothing else. Filter the array before writing it: build a validated list in the loop and dump that, or apply sanitize_package_name in load_index too. A name that was refused once should not be reachable by a second path.
7 Medium ebuild/packages/index_sync.py:130, 138; commands.py:1707 --force does nothing. force appears only in sync()'s signature and its docstring ("If True, re-download even if recently synced"); no line reads it, and there is no staleness check anywhere — sync() always re-downloads. The CLI advertises "Force refresh even if cache is up-to-date." Either implement the staleness check the docstring describes (an mtime or a recorded fetch timestamp in packages.json) or remove both the parameter and the flag. A flag that is documented and inert is worse than an absent one.
8 Medium commands.py:1712-1717; index_sync.py:229-232 ebuild update-index reports success and exits 0 when the sync failed. On any network error with a cache present, sync() returns a normal (count, message) tuple, and the CLI calls log.success(msg) and returns. The message text does say "Network sync failed (…); fell back to cached index", but it is rendered as a success and the exit status is 0, so a CI step that runs ebuild update-index before a build cannot tell a fresh index from a stale one. .ai/tooling.md: "Exit non-zero on failure, always." Return the fallback distinguishably — a third element, or a dedicated exception the CLI catches to log.warn and exit non-zero (or 0 only under --offline, where using the cache is the request rather than a fallback).
9 Low ebuild/packages/index_sync.py:203-232 Two smaller edges in the same block. (a) synced_count counts entries that produced nothing: the recipe is only written if recipe_dict["url"] (:215), but synced_count += 1 runs regardless. Reproduced — "Successfully synchronized 2 packages" with one recipe file on disk. (b) The except (URLError, HTTPError, OSError, TimeoutError) at :229 spans the cache writes too, so a disk-full while writing packages.json is reported as "Network sync failed (…)" and falls back to the stale cache, blaming the network for a local fault. §9.2 asks for actionable diagnostics. (a) Move the increment inside the if, or count written and skipped separately and report both. (b) Narrow the try to the urlopen/read, and let cache-write failures surface as themselves.
10 Low ebuild/packages/index_sync.py:216-221 recipe = _parse_recipe(recipe_dict) is assigned and never read — it is used only for its exception — and the unvalidated recipe_dict is what gets written to disk (:218). Whatever _parse_recipe normalises or defaults is validated and then discarded, so the cached YAML is the raw remote shape rather than the canonical one. _parse_recipe is also a module-private name imported across a module boundary (:26). yaml.safe_dump(asdict(recipe), …) — write the thing that passed validation. If PackageRecipe needs a public constructor for this, add one rather than importing the underscore.
11 Low commands.py:1, 871, 1621, 1641, 1687 etc. An undisclosed encoding change rides along: # -*- coding: utf-8 -*- is added at :1 and six em-dashes in user-facing strings become ASCII hyphens ("ebuild — A unified embedded OS build system.""ebuild - …", log.header("ebuild — Package Registry")"ebuild - …", f" — {recipe.description}"f" - …"). Counted: 39 em-dashes on master, 33 here — so 33 remain and the CLI's own output becomes inconsistent within one file. The pattern (a coding cookie plus selective em-dash loss) is the signature of a cp1252 editor round-trip rather than an intended change. Revert all of it. Python 3 source is UTF-8 by default, so the cookie is noise, and the visual identity of the CLI output is not this PR's subject.
12 Low pytest.ini:27 -p no:faker is added to addopts with no comment, in a file that comments every other section. Checked: faker is not imported anywhere under tests/ or ebuild/, and it is not installed here — so it changes nothing today and looks like a leftover from the author's environment. Recording it because a global test-runner flag arriving inside a feature PR is the shape worth catching even when this instance is harmless. Drop it, or keep it with a one-line comment naming the conflict it avoids.
13 Low ebuild/packages/repository.py:151-155 (search) The license parameter shadows the builtin. Harmless in this scope, but the module already uses lic_filter for the same thing at the CLI boundary (commands.py:1670). license_filter, matching the CLI.

Test coverage gaps (not scored separately; they are why findings 1, 6 and 7 are invisible):
the 11 new tests cover sanitisation, offline mode, insecure-URL rejection, corrupted JSON and
network fallback-with-cache — good choices — but there is nothing for source precedence, the
MAX_INDEX_SIZE_BYTES cap, a lying Content-Length, duplicate index entries, url-less
entries, or --force. Every network test mocks urlopen, so nothing exercises a real fetch.

Architecture conformance

Master design §10 (component and manifest system), §10.1 (component contract — Identity,
Compatibility, Dependencies, Capabilities, Permissions, Resources, Integrity, Compliance),
§11 and §11.1 (Registry and artifact types), §9.1–9.2 (eBuild engine and SDK design rules),
§15.1 (signed metadata and package provenance), §14.1, §21 tiers and §21.1 split policy.
Tier placement conforms; the component contract is only partly satisfied.

Placement is right. A remote index client in ebuild is Tier 1 – Foundation reading a
Tier 4 – Developer Ecosystem service, which is the direction §5.1 permits: eBuild "understands
the complete graph but is not a runtime dependency", and §11 names ebuild search mqtt /
ebuild add embeddedos/mqtt as the CLI surface for exactly this. §9.1's engine diagram already
puts "Packages / Registry" inside eBuild. Nothing in the diff points up a tier, and §21.1 is
not triggered — no new repository is proposed, and embeddedos-org/recipes would be data, not
a subsystem.

Where §10.1 is not met. The contract has eight fields; the recipe schema this PR caches
covers Identity, Dependencies and part of Compliance (license), and reduces Integrity to a
transit checksum with no signature or provenance (finding 2). Compatibility — "EmbeddedOS
API/ABI, architecture, SoC and target constraints" — and Resources — "Flash/RAM/storage" — are
absent from recipe_dict (index_sync.py:203-214) and from PackageInfo
(repository.py:26-40) entirely. For an embedded package manager those are the fields that
decide whether a package can go in an image at all; §10's own worked example carries
resources: flash_max / ram_max. That is a gap to name now, while the schema is new and
cheap to extend, rather than after recipes exist in the wild. Not scored as a finding because
this PR does not claim to implement the full contract — but the docs it adds should say which
fields are and are not carried.

Adjacent, not this PR's doing: .github/PULL_REQUEST_TEMPLATE.md on origin/master is
corrupted, which is why finding 5's type labels read eat/ix/efactor/est/uild. cat -A
shows - [ ] ^Leat — a literal formfeed where \feat was written, and the same for \fix
(FF), \refactor (CR), \test (TAB), \build (BS). Present in eBoot, ebuild, eos and
EoSim
(two control characters each); the org-level template in embeddedos-org/.github is
correct. .github/STANDARDS.md says repos that ship no override inherit the org file, so
the fix is to delete the four local copies or repair them. Worth an issue against the org: the
template that tells contributors the Conventional Commit type names currently shows five
mangled ones, in the four most active repos.

Verified by running:

git apply of the PR diff onto origin/master
  -> error: ebuild/cli/commands.py: patch does not apply
  -> error: ebuild/packages/registry.py: patch does not apply       (finding 4)
git rev-list --count pr111..master -> 7                              (finding 4)
master already has: commands.py:2451 capture_output=True, text=True
                    commands.py:2649 _NO_TESTS_MARKERS
                    commands.py     _board_config() with its docstring and body

load_all_sources precedence, project + shipped + cached-remote all defining cjson:
  resolved version 9.9.9 · url …/cjson-REMOTE.tar.gz · checksum sha256:222…
  -> the cached remote recipe won                                    (finding 1)

sync() over an index of {good, urlless, "bad name!"}:
  "Skipping unsafe package entry: Invalid package name 'bad name!'"
  reported count: 2 · recipe files written: ['good.yaml']
  entries in packages.json: 3   ("bad name!" cached and searchable)  (findings 6, 9a)

gh api repos/embeddedos-org/recipes            -> 404 Not Found
gh api …/recipes/contents/index.json           -> 404 Not Found
fresh machine, no cache, default URL:
  IndexSyncError: Failed to fetch remote package index and no cache is available:
                  HTTP Error 404: Not Found      · cache file exists: False   (finding 3)

grep force  -> index_sync.py:130 (signature), :138 (docstring) only    (finding 7)
grep faker  -> no hits under tests/ or ebuild/; module not installed    (finding 12)
em-dashes in commands.py: master 39 · pr111 33                          (finding 11)
docs/architecture.md: build/orchestrator.py -> build/dispatch.py — correct,
  ebuild/build/ holds dispatch.py and no orchestrator.py

Worth crediting, because they are the parts that are easy to get wrong: HTTPS-only enforcement
(:151-155); response.read(MAX + 1) after the Content-Length check, so a lying header does
not defeat the cap; temp_json.replace() for an atomic cache swap; sanitize_package_name
with a strict allowlist rather than a blocklist; and fetcher.py:53-56 already refusing a
recipe with no checksum, so an empty checksum field cannot silently skip verification. The
docs/architecture.md correction is a real fix to a stale diagram, not churn.

Proposed changes

In order, because the first four gate the rest:

  1. Rebase on master; drop the test() and _board_config() changes as already merged
    (finding 4). This removes both conflicts.
  2. Make add_recipe_directory respect source precedence, and add the three-source test
    (finding 1).
  3. Point DEFAULT_INDEX_URL at something that exists, or make it empty and say so in
    ebuild search's empty-state text (finding 3).
  4. Fill in the PR body (finding 5).
  5. Filter packages.json before writing it and sanitise in load_index (finding 6).
  6. Implement or remove --force (finding 7); make the fallback exit non-zero (finding 8).
  7. Findings 9–13 are small and can travel together.
  8. Separately: state in docs/dependency-management.md that the index is unauthenticated and
    which §10.1 fields the recipe schema does not carry (finding 2, and the §10.1 note above).

No fix PR opened. Every finding is on this PR's branch, which the brief puts out of bounds, and
the branch needs a rebase before anything else is worth doing to it.

Not checked

  • No CI has run on this head at all. actions/runs?head_sha=77f1d9f2… returns
    total_count: 0 — not even a queued-and-unapproved run, unlike #109 and #110 which have
    action_required runs. commits/77f1d9f2…/status is {"state":"pending","count":0}. So
    nothing in this PR has been verified by the project's pipeline, and the PR body claims
    nothing either.
  • pytest is not installed in this environment, so none of the 11 new tests was run. Every
    result above comes from importing the modules directly and driving them, with
    urllib.request.urlopen mocked where the network would be reached. Whether the suite passes
    is unknown.
  • No real network fetch. Findings 2 and 3 rest on the 404 from gh api plus a simulated
    HTTPError; I did not attempt to fetch the URL itself.
  • The five new recipes' checksums were not verified. recipes/{cjson,lvgl,nanopb,tinyusb,unity}.yaml
    each carry a sha256: for an upstream tarball. Confirming those would mean downloading five
    archives from the network, which this run did not do. They are pins on third-party code and
    someone should check them before merge — a wrong one fails closed, but a copied-from-elsewhere
    one would not.
  • No package was actually fetched, built, or installed. fetcher.py and builder.py were read
    where finding 2 depends on them, not exercised.
  • ebuild search and ebuild update-index were driven through their library layer, not through
    the click CLI, so argument parsing, --json output shape and exit codes were reasoned from
    the source rather than observed.
  • The local ebuild clone was left alone — the sync step reported it dirty (4 files, on
    branch v90), and the PR head was not present locally. I cloned it to /tmp with
    git clone --shared --no-checkout and fetched pull/111/head there, so the user's working
    tree, index and refs were never written to.

Automated architecture review of 77f1d9f22daf — 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.

GrantLinkz added 4 commits September 4, 2026 17:56
…ry, and build synchronization.

# Conflicts:
#	ebuild/cli/commands.py
…ecipe repository

- Add IndexSyncManager for HTTPS-only index downloading with 24h cache TTL
- Enforce local recipe precedence to guarantee project-local pins override remote
- Add �build search and �build update-index CLI commands
- Ship verified recipes for cjson, lvgl, nanopb, tinyusb, and unity
- Add offline fallback, digest tracking, and strict package name sanitization

fix(packages): enforce local recipe precedence, cache sanitization, and fallback exit codes
…om/Grantlinkz/ebuild into Remote-Package-Index-And-Ecosystem

# Conflicts:
#	docs/dependency-management.md
#	ebuild/cli/commands.py
#	ebuild/packages/index_sync.py
#	ebuild/packages/repository.py
#	pytest.ini
#	tests/unit/test_index_sync.py
#	tests/unit/test_package_search.py

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#111 "feat: implement package management system with recipe support, registry…"

head: 7e76e05 author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED

Verdict: Follow-up to the review of 77f1d9f2. Ten of the thirteen prior findings are
resolved and the branch is now merged up to master with no conflicts — the rebase, the
empty DEFAULT_INDEX_URL, the pre-write entry filtering, a real --force, a non-zero exit
on network fallback, the public parse_recipe, and the reverted encoding churn are all
done. Finding 1 is the exception: it was fixed in PackageRepository (the ebuild search
surface) and not in the build path, which this PR newly wires to the remote cache. So a
cached remote recipe still replaces a project's pinned url and checksum in the tree that
actually gets built — and docs/dependency-management.md:325 now states the opposite as a
guarantee. That, three defects introduced by the new --force/TTL and Content-Length code,
and the still-empty PR body are what is left.

Findings

# Severity File:line Finding Recommended fix
1 High ebuild/cli/commands.py:64-68 + ebuild/packages/registry.py:125-129; claim at docs/dependency-management.md:325 Finding 1 was fixed on the search path only; the build path still lets a cached remote recipe replace a project's pinned url and checksum — and this PR is what connects the remote cache to it. PackageRepository.add_recipe_directory now guards with if recipe.name not in self._index (repository.py:78), and test_package_source_precedence_project_wins proves it. But ebuild build does not go through PackageRepository. It goes _install_packages_find_recipe_dirscreate_registryPackageRegistry._register, and _register at registry.py:129 is self._recipes[name][version] = recipelast write wins. _find_recipe_dirs is scanned project → shipped → cached-remote, and lines 64-68 added by this PR put ~/.ebuild/index/recipes/ on that list for the first time. Reproduced: a project pinning cjson 1.7.18 to …/cjson-PROJECT.tar.gz + sha256:111…, with a cached remote cjson 1.7.18 present, resolves on the build path to …/cjson-REMOTE.tar.gz + sha256:222… — while the search path, in the same process, correctly returns the project's. Because the checksum is replaced with the url, fetcher.py:61-70 verifies the substituted archive and passes. ebuild.lock is then rewritten with the substituted pin (commands.py:178-180), and nothing ever reads it back (see Architecture conformance). docs/dependency-management.md:325 says "project-local recipes in ./recipes/ take absolute precedence over remote index definitions. An index update will never override pinned URLs or checksums defined in your project repository." For ebuild build that sentence is not true. Rated High rather than Critical because the last-wins in _register predates this PR (shipped recipes already shadow project ones) and reaching it needs the user to have run update-index against the substituting index themselves — but this PR extends its reach to the network and documents a guarantee against it, so do not round it down further. Two changes, both small. (a) Apply the same first-wins rule in PackageRegistry._register: skip when recipe.version is already registered for that name, so the earliest search path wins — add_search_path already preserves order. (b) Delete _find_recipe_dirs and have _install_packages use PackageRepository.load_all_sources, or vice versa; there are now two recipe-directory discovery implementations (commands.py:52-70 and repository.py:139-165) with the same three sources and opposite precedence, which is what made this survive. Then extend test_package_source_precedence_project_wins with a second assertion that drives _find_recipe_dirs + create_registry — the existing test cannot fail on this bug. Until (a) lands, docs/dependency-management.md:325 overstates what the code does and should be softened.
2 Medium ebuild/packages/index_sync.py:174-186 --url is silently ignored while the cache is under 24h old, and the tool reports success. The new staleness check runs before target_url is looked at, and the cache is not keyed by origin. Reproduced: sync(url="https://a.example/index.json") then sync(url="https://b.example/index.json") — the second call never opens a connection (urlopen.called == False), returns "Cache is up-to-date (synced recently). Use --force to re-download. (1 packages)", exits 0, and leaves index A's entries in place. Since DEFAULT_INDEX_URL is now "" (correctly, per finding 3), --url is the only way to sync, so the one argument that selects the source is the one the freshness check disregards. The message is an affirmative claim about a URL that was never fetched. Key the freshness check to the origin: record the fetched URL in packages.json (or a sibling index-meta.json alongside the digest from finding 5) and treat a cache fetched from a different URL as stale regardless of age. Cheaper interim fix: skip the staleness short-circuit whenever url was passed explicitly.
3 Medium ebuild/packages/index_sync.py:207-208 A malformed Content-Length crashes the CLI with an unhandled ValueError. int(content_length) sits inside the try, but the handlers are IndexSyncError and (URLError, HTTPError, TimeoutError, OSError)ValueError matches neither, so it escapes sync(), escapes update_index's except IndexSyncError (commands.py:1759), and reaches the user as a traceback. Reproduced with Content-Length: not-a-number: UNHANDLED ValueError: invalid literal for int() with base 10: 'not-a-number'. Malformed remote input is one of the failure paths the review brief requires to be handled, and the size cap at :212-216 already makes the header advisory — the code does not need to trust it. Wrap the parse: try: declared = int(content_length) except (TypeError, ValueError): declared = None, and only compare when it parsed. The response.read(MAX + 1) cap that follows already enforces the limit for a missing or unparseable header.
4 Medium pr body The body is still the unfilled template — unchanged since the last review. No summary, - placeholders under Changes, every Testing and Pre-Submission box unchecked, no command output, no linked issue, for 1,150 added lines that include a network-fetching subsystem. This is the one prior finding with no movement at all. (The garbled type labels — eat/ix/efactor — are still not the author's doing; .github/PULL_REQUEST_TEMPLATE.md on origin/master carries literal control characters. Unchanged from the last review.) Fill in Summary, Changes and Testing with what was actually run. The 24 tests in tests/unit/test_index_sync.py and tests/unit/test_package_search.py are worth naming, along with what they do not cover — findings 1, 2 and 3 are all in that gap, and naming them is faster than having a reviewer find them.
5 Low ebuild/packages/index_sync.py:258-264 vs docs/dependency-management.md:327 The index digest is write-only, and the documented path for it is wrong. :259-264 computes sha256(raw_bytes) and writes it out; nothing in ebuild/ or tests/ ever reads it back, compares it, or prints it — the only reference is test_index_sync.py:106 asserting the file exists. So it satisfies "record the index's own digest so a changed index is visible" in letter but not effect: nothing makes it visible. Separately, self.packages_json.with_suffix(".sha256") replaces the .json suffix and yields ~/.ebuild/index/**packages.sha256**, while the docs point users at ~/.ebuild/index/packages.json.sha256, which never exists. The test mirrors the implementation, so it cannot catch the mismatch. Use packages_json.with_name(packages_json.name + ".sha256") to match the documented name, and give the digest a consumer: have update-index print it, and compare it against the previous value so a changed index produces a line of output rather than a silent file.
6 Low ebuild/packages/index_sync.py:266-296 Duplicate names in one index over-report and leave unreachable entries in the cache. Two entries named dup (1.0.0 and 9.9.9) both pass the filter and both land in packages.json; the second overwrites the first's dup.yaml. Reproduced: "Successfully synchronized 2 packages", 2 entries in packages.json, one dup.yaml on disk holding 9.9.9 — and because load_all_sources reads recipe dirs before the JSON index, the 1.0.0 entry is permanently unreachable. Same class as prior finding 9(a), which was fixed for url-less entries but not for collisions. Deduplicate by name when building valid_entries — keep the first and log the discard — so the count, packages.json and recipes/ all describe the same set.
7 Low docs/dependency-management.md:359 "Set environment variable EBUILD_OFFLINE=1 or pass --offline to commands." Only update-index has an --offline flag, and EBUILD_OFFLINE is read in exactly one place (index_sync.py:72) — it governs index sync and nothing else. ebuild build still downloads package archives with no offline gate. For a section headed "Offline & Air-Gapped Operation" that reads as a broader promise than the code makes. Scope the sentence to ebuild update-index, and say plainly that package archive fetching is not yet offline-gated.
8 Low ebuild/packages/repository.py:151-155 Prior finding 13 is half done: license_filter was added and is preferred (effective_lic = license_filter or license), but the license parameter was kept as a backward-compatible alias and still shadows the builtin. No caller uses it — commands.py:1719 passes license_filter=, and search() is new in this PR, so there is no released signature to stay compatible with. Drop the license parameter.

Resolved since 77f1d9f2 — one line each, no further treatment:

  • Finding 3 (default index URL 404): resolved in ce4cb18. DEFAULT_INDEX_URL = "" (index_sync.py:31) with an explicit error at :188-191; ebuild search's empty state now points at --url (commands.py:1723-1727). Driven through the CLI: ebuild update-index with no URL exits 1 with that message.
  • Finding 4 (conflicts, 7 behind): resolved in 7e76e05. Merged e5d8052; git rev-list --count pr111..origin/master → 0, mergeable: MERGEABLE. registry.py is out of the diff and the duplicate test() / _board_config() reimplementations are gone.
  • Finding 6 (cache written before validation): resolved. index_sync.py:240-256 filters into valid_entries before the atomic write; repository.py:117-120 sanitises in load_index too.
  • Finding 7 (--force inert): resolved — TTL check at :174-186, covered by test_index_sync_force_and_staleness. See finding 2 for what the implementation introduced.
  • Finding 8 (success exit on fallback): resolved. SyncResult.is_fallback (:50-65) → log.warning + SystemExit(1) (commands.py:1753-1756), covered by test_cli_update_index_fallback_exits_nonzero.
  • Finding 9(a) (count over-reports) and 9(b) (broad except): resolved. synced_count += 1 is inside the if recipe_dict["url"] block (:288-293); the try now spans only the fetch (:201-229), so cache-write failures surface as themselves.
  • Finding 10 (discarded _parse_recipe): resolved. parse_recipe is public (recipe.py:118), and recipe.to_dict() — the validated shape — is what gets written (:290-292).
  • Finding 11 (encoding churn): resolved. No coding cookie; em-dashes in commands.py went 39 → 42, so none were lost.
  • Finding 12 (-p no:faker): addressed as offered — kept with a comment naming the conflict (pytest.ini:26-27).
  • The docs/architecture.md orchestrator.pydispatch.py correction survived the merge.

Also credited, because they were asked for and delivered: the §10.1 field-coverage table and the "Index Authenticity & Provenance Notice" in docs/dependency-management.md:320-337 are exactly what prior finding 2 asked for — the index is now stated to be unauthenticated at the point of use. The one sentence in that notice that is not yet true is finding 1.

Architecture conformance

Master design §9.1–9.2 (eBuild engine and SDK design rules), §10 and §10.1 (component model and contract), §11/§11.1 (Registry), §14.1, §15.1, §21 tiers and §21.1 split policy.
Tier placement conforms, unchanged from the last review; §9.2's reproducibility rule does not hold on the build path.

Placement is right and nothing in this diff points up a tier: a remote index client inside ebuild is Tier 1 – Foundation reading a Tier 4 – Developer Ecosystem service, which §5.1 permits ("eBuild understands the complete graph but is not a runtime dependency"), and §11 names ebuild search as the CLI surface for exactly this. §21.1 is not triggered — no repository is proposed.

Where it deviates is §9.2, "Reproducible lockfiles/manifests for production builds". Finding 1 is one half of that; the other half is that ebuild.lock is written and never read. Lockfile is constructed once at commands.py:110 and only lock() and save() are called (:178-180); load(), is_locked(), get_locked_entry(), get_locked_version() and locked_packages have zero call sites anywhere in ebuild/. So every build re-resolves from whatever recipes are on disk and then overwrites the lockfile with the result. That is pre-existing on master and not this PR's defect — but it is why finding 1 has no backstop, and it is the reason for the proposal appended below. §10.1's Integrity field remains satisfied only as a transit checksum; the PR now says so in its own docs, which is the right treatment for this PR.

Verified by running:

git rev-list --count pr111..origin/master  -> 0        (finding 4 resolved)
git merge-base origin/master pr111         -> e5d8052  (merged, MERGEABLE)
diff vs merge-base: 16 files, +1150 -96; registry.py absent

precedence, project + shipped + cached-remote all defining cjson 1.7.18:
  _find_recipe_dirs + create_registry  -> url …/cjson-REMOTE.tar.gz  checksum sha256:222…
  PackageRepository.load_all_sources   -> url …/cjson-PROJECT.tar.gz checksum sha256:111…
  same process, same inputs, opposite answers                        (finding 1)
  with no remote cache at all, the shipped recipe still wins over the project's
  -> the last-wins in registry.py:129 predates this PR; lines 64-68 extend its reach

sync(url=A) then sync(url=B), cache 0s old:
  "Cache is up-to-date (synced recently)…"  urlopen called for B: False
  cached names still ['alpha']                                       (finding 2)

Content-Length: "not-a-number"  -> UNHANDLED ValueError, not IndexSyncError  (finding 3)
index with two entries named 'dup' -> reported 2, packages.json 2, dup.yaml 1 (finding 6)
Path('~/.ebuild/index/packages.json').with_suffix('.sha256')
  -> packages.sha256, docs say packages.json.sha256                  (finding 5)
grep Lockfile.load/is_locked/get_locked_* in ebuild/ -> no call sites

CLI driven through click.testing.CliRunner:
  ebuild update-index            -> exit 1, "No remote package index URL configured…"
  ebuild update-index --offline  -> exit 0, "using cached index (0 packages)"
  ebuild search --json           -> exit 0, 10 packages, 8 keys, schema unchanged
  ebuild search json             -> exit 0, cjson v1.7.18
  ebuild search nosuchpkg        -> exit 0, empty state points at --url

the five new recipes' checksums, downloaded from upstream and hashed:
  cjson 1.7.18     MATCH  3aa806844a03442c00769b83e99970be70fbef03735ff898f4811dd03b9f5ee5
  lvgl 9.2.2       MATCH  129b4e00e06639fa79d7e8a6cab3c1ecce2445b1a246652ccd34f22e7b17ad6f
  nanopb 0.4.9.1   MATCH  4575944a468718ef25f05eb01d994364650b581563089a9841986bb1e460eac3
  tinyusb 0.18.0   MATCH  e7fa1bd723213749a0362c79eaccc99e84c8adea8f0a63588c4e4812608b7aa9
  unity 2.6.1      MATCH  b41a66d45a6b99758fb3202ace6178177014d52fc524bf1f72687d93e9867292
  all five verify — the CHANGELOG's "5 verified recipes" claim is supported

CI: actions/runs?head_sha=7e76e056 -> 3 runs, all conclusion "action_required"
    commits/7e76e056/status -> {"state":"pending","count":0}; check-runs -> 0

One durability note on those five pins, not a finding: they are github.com/<org>/<repo>/archive/refs/tags/*.tar.gz URLs, which GitHub generates on demand. Those archives are stable today but have changed byte-for-byte across a toolchain change before, and a release-asset URL is the sturdier pin where upstream publishes one.

Proposed changes

In order — 1 is the only one that blocks:

  1. Finding 1. Make PackageRegistry._register first-wins, then collapse _find_recipe_dirs and load_all_sources into one implementation so there is a single answer to "where do recipes come from and in what order". Add the build-path assertion to test_package_source_precedence_project_wins. Until that lands, soften docs/dependency-management.md:325 — a documented guarantee the code does not provide is worse than no sentence.
  2. Finding 2, then finding 3 — both are inside sync() and can travel together with a test each: a second --url against a warm cache, and a non-integer Content-Length.
  3. Finding 4: fill in the PR body.
  4. Findings 5–8 are small and can go in one commit.
  5. Ask a maintainer to approve the three action_required workflow runs. Nothing in this PR has been executed by the project's CI, and this is the third head where that is true.

No fix PR opened. Finding 1's trigger (commands.py:64-68) is on this PR's branch, which the brief puts out of bounds; the half that lives on master (registry.py:129) is a change to package-resolution precedence, which is a behaviour change to what gets built rather than the small provable class of fix the brief permits — it belongs to the author of this PR or to a maintainer, with the design rule settled first.

Not checked

  • No CI has run on this head. Three workflow runs exist for 7e76e056CI — ebuild, CodeQL, Simulation Test — and all three are conclusion: action_required, i.e. queued awaiting maintainer approval for a fork contribution. Combined status is pending with zero statuses and there are zero check-runs. So no build, lint, type check or test in this PR has been executed by the project's pipeline.
  • The test suite was not run. pytest is not installed in this environment and there is no pip to install it (python3 -m pip → no module named pip; a venv builds without pip). All 24 new tests in tests/unit/test_index_sync.py and tests/unit/test_package_search.py were read, not executed. Whether the suite passes is unknown — every result above comes from importing the modules directly, driving the CLI through click.testing.CliRunner, and mocking urllib.request.urlopen where the network would be reached.
  • pytest.ini:26's inline comment inside addopts was not executed. iniconfig skips any line whose first non-space character is #, so it should not reach pytest's argument list — that is inferred from the parser's behaviour, not observed, because neither pytest nor iniconfig is installed here.
  • No real index was fetched. DEFAULT_INDEX_URL is empty and no deployed index exists, so every sync path was exercised against a mocked urlopen. The five recipe archives were really downloaded and hashed; the index document was not.
  • No package was fetched, built or installed end to end. Finding 1's consequence at fetcher.py:61-70 was read, and the recipe that reaches the fetcher was reproduced; the download-and-extract itself was not run.
  • ebuild build was not run, so finding 1 was demonstrated at _find_recipe_dirs + create_registry — the exact functions _install_packages calls at commands.py:105-110 — rather than through a full build.
  • The local ebuild clone was left untouched. The sync step reported it dirty (4 files, branch v90) and the PR head was not present locally. I made a git clone --shared --no-checkout under /tmp and fetched pull/111/head there, so the user's working tree, index and refs were never written to.

Automated architecture review of 7e76e0565f81 — 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.

…d in the review of PR embeddedos-org#111 (7e76e05), ensuring full compliance with EmbeddedOS Master Design §9.2 (reproducibility) and §10.1 (component contract).
@Grantlinkz
Grantlinkz requested a review from srpatcha September 5, 2026 04:52

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#111 "feat: implement package management system with recipe support, registry…"

head: 005a69a author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED

Verdict: Follow-up to the review of 7e76e056. One commit, 005a69a. Five of the eight
prior findings are resolved and verified by running them: the origin-keyed cache, the
malformed Content-Length, index deduplication, the offline-scope doc fix and the removed
license alias. The structural half of finding 1 is genuinely delivered — there is now one
recipe-directory discovery function instead of two. What is left is that the other half of
finding 1 was fixed only for the case where both sources name the same version. A cached
remote recipe that declares a higher version still replaces a project's pinned url and
checksum on the build path, for any package without an explicit version: in build.yaml
and for every transitive dependency. The precedence test was edited in the same commit —
its remote fixture went from 9.9.9 to 1.7.18 — which removes the only version-differing
scenario in the suite, and the added build-path assertion pins the version explicitly rather
than resolving the way the resolver does. I ran the original fixture against this head: it
passes every assertion in the new test, so the edit was not needed to make it green.
docs/dependency-management.md:325 still states the guarantee the code does not provide, and
the commit subject claims "full compliance with §9.2 and §10.1", which the PR's own docs
contradict.

Findings

# Severity File:line Finding Recommended fix
1 High ebuild/packages/registry.py:125-130 and :144; ebuild/packages/resolver.py:116-117, 129; claim at docs/dependency-management.md:325 Prior finding 1 is fixed for equal versions only; a cached remote recipe with a higher version still overrides a project's pinned url and checksum on the build path. _register is now first-wins (:128-130) but its key is (name, version), so two sources naming one package at two versions both land in the index. get(name) with no version then returns sorted(versions, key=version_sort_key)[-1] (:144) — the highest, wherever it came from. PackageResolver._collect calls self.registry.get(name, pins.get(name)) (:116-117), and pins is built only from the top-level requested list, so every transitive dependency resolves unpinned (:129). version in a packages: entry is Optional[str] = None (ebuild/core/config.py:40), so the unpinned top-level case is a supported configuration, not a misuse. Reproduced on this head, project recipes/ + shipped recipes/ + cached ~/.ebuild/index/recipes/: project lvgl 9.2.2 vs cached remote lvgl 9.9.9 → ebuild build resolves v9.9.9, the REMOTE url, the REMOTE checksum, while ebuild search in the same process reports v9.2.2 with the project's url. With the top-level package explicitly pinned, its dependency mbedtls (project 3.6.0, remote 9.9.9) still resolved to the remote 9.9.9 url and checksum. Because the checksum travels with the url, fetcher.py:94's verification passes against the substituted archive. docs/dependency-management.md:325 — "project-local recipes in ./recipes/ take absolute precedence over remote index definitions. An index update will never override pinned URLs or checksums defined in your project repository" — is still not true for ebuild build. Precedence has to dominate version selection, not sit beside it. Record the source rank alongside each recipe at registration and make get(name) pick the highest version within the highest-ranked source that defines the name, rather than the highest version across all sources. That keeps "newest wins" inside a source and "project wins" between sources, which is what §9.2 and :325 both describe. Same treatment for list_packages() (:151-158), which has the identical [-1]. Until it lands, soften :325 to say precedence holds for equal versions and that an unpinned or transitive package may still resolve to a registry definition — a documented guarantee the code does not provide is worse than no sentence.
2 High tests/unit/test_package_search.py:73 and :108-116 The precedence test was weakened in the same commit that claims to fix precedence. test_package_source_precedence_project_wins previously wrote the cached remote cjson as version: "9.9.9" against a project 1.7.18; 005a69a changes the remote fixture to 1.7.18 (:73, and the matching packages.json entry at :87), deleting the only version-differing scenario in the suite. The build-path assertion added at :112-116 calls registry.get("cjson", "1.7.18") — an explicit version — which is the case that cannot fail; the resolver's actual call is registry.get(name, pins.get(name)) with pins.get returning None for unpinned and transitive packages. I restored the original 9.9.9 fixture and ran it against this head: the search-path assertions at :100-106 still pass and registry.get("cjson", "1.7.18") still returns the project recipe, so the fixture change was not required to make the new test green. Its only effect is that the suite can no longer observe finding 1's remaining half. Per the brief, a narrowed assertion is a finding regardless of intent, and severity is not rounded down when it conceals an open High. Restore version: "9.9.9" at :73 and :87, and add assert registry.get("cjson").url == "https://custom-project.org/cjson-PROJECT.tar.gz" — no version argument — next to the existing pinned assertion. That test fails today; it is the one that proves finding 1. Add a third case for a transitive dependency: package app pinned, its dependency present at 3.6.0 in the project and 9.9.9 in the cache.
3 Medium commit 005a69a subject; pr body Two unsupported claims, one new and one carried over. (a) The commit subject asserts it resolves "all open architectural and functional findings … ensuring full compliance with EmbeddedOS Master Design §9.2 (reproducibility) and §10.1 (component contract)". §10.1 compliance is not full and the PR says so itself — docs/dependency-management.md:331-333 lists Compatibility and Resources as Deferred Contract Fields. §9.2 reproducibility is not met either: finding 1 stands, and Lockfile.load / is_locked / get_locked_entry / get_locked_version / locked_packages still have zero call sites anywhere in ebuild/ (grepped on this head), so ebuild.lock is written every build and never read back. Prior finding 4 is also untouched, so "all open findings" is not accurate. (b) The PR body is still the unfilled template — no summary, - placeholders under Changes, every Testing and Pre-Submission box unchecked, no command output, no linked issue, across four heads and 1,273 added lines. The brief treats an unsupported "verified" as itself the finding. (The garbled type labels eat/ix/efactor remain .github/PULL_REQUEST_TEMPLATE.md's corruption on origin/master, not the author's doing — unchanged from both prior reviews.) Restate the commit subject as what it did: origin-keyed cache freshness, malformed Content-Length, index deduplication, unified recipe-directory discovery, same-version source precedence. Name what remains open rather than claiming compliance the docs contradict. Then fill in the body: the 26 tests in tests/unit/test_index_sync.py and tests/unit/test_package_search.py are worth naming, along with what they do not cover — findings 1, 2 and 5 all sit in that gap.
4 Medium CHANGELOG.md:10 vs docs/dependency-management.md:360 This commit added a doc note that contradicts the CHANGELOG entry the same PR added, and left the CHANGELOG alone. CHANGELOG.md:10: "Fully supports air-gapped/offline execution via --offline and EBUILD_OFFLINE=1." docs/dependency-management.md:360, added by 005a69a for prior finding 7: "Package archive source fetching (ebuild build) … is not yet gated by the --offline flag." The docs are right — is_offline() is consulted at exactly one place, index_sync.py:169, and fetcher.py:94 calls urlretrieve(recipe.url, …) unconditionally with no offline gate. The CHANGELOG is the entry that reaches a release note, so this is the copy that will mislead. Per the brief, a change that makes existing documentation wrong is not finished. Scope the CHANGELOG line to the index: "Index synchronization supports air-gapped operation via --offline and EBUILD_OFFLINE=1; package archive fetching is not yet offline-gated."
5 Medium ebuild/packages/index_sync.py:303-330 Cached recipe YAMLs are never pruned, and the finding-2 fix makes that reachable. The sync loop writes <name>.yaml for every entry in the new index but removes nothing, so a package withdrawn from the index — or belonging to a previous index origin, which 005a69a newly allows you to switch away from — stays on disk in ~/.ebuild/index/recipes/ indefinitely. Reproduced: sync(url=A) with [alpha], then sync(url=B) with [beta]packages.json holds ['beta'] while recipes/ holds ['alpha.yaml', 'beta.yaml']. find_recipe_dirs (registry.py:216-227) puts that directory on the build path, so alpha remains resolvable and buildable from a pin nobody publishes any more, and --force does not clear it. Combined with finding 1, a stale remote recipe at a high version can outrank a project's own. Prune before or after the write: delete *.yaml in self.recipes_dir whose stem is not in seen_names, or write into a temp directory and swap it in, matching the atomic replace already used for packages.json at :276-280. Add a test that syncs two different indices in sequence and asserts recipes/ matches the second.
6 Low ebuild/packages/index_sync.py:301; ebuild/cli/commands.py update_index Prior finding 5 is half resolved: the filename is fixed, the digest still has no consumer. with_name(name + ".sha256") now produces ~/.ebuild/index/packages.json.sha256, matching docs/dependency-management.md:327 — verified, the file exists under that name — and index-meta.json records the digest too. But the only thing that "makes it visible" is logger.info("Remote index SHA-256 digest: %s", …) at :301, and no module under ebuild/ calls logging.basicConfig (grepped: no hits), so at the root logger's default level that line never reaches the user. Confirmed in a live sync: the module's logger.warning output appeared on stderr and its logger.info lines did not. update_index prints the message and the cache path, not the digest, and nothing compares the digest to the previous value — so a changed index still produces no output. Have update_index read index-meta.json's previous sha256 before syncing and log.info the new digest, plus a line when it differs. That is the difference between recording the digest and making a changed index visible.
7 Low ebuild/packages/registry.py:216-227 find_recipe_dirs wraps the whole remote-index-directory resolution in except Exception: pass. A bad EBUILD_INDEX_PATH or EBUILD_CACHE_DIR, or an import failure in index_sync, silently yields a build with no remote recipes rather than an error naming the cause — §9.2 asks for actionable diagnostics, and the same broad catch would also swallow a genuine bug in get_default_index_dir. Catch (ImportError, OSError) and logger.warning the exception before continuing, so the degradation is at least stated.

Resolved since 7e76e056 — one line each, no further treatment:

  • Finding 2 (--url ignored against a warm cache): resolved in 005a69a. index-meta.json records the origin URL (index_sync.py:120, :290-299) and same_origin gates the TTL short-circuit (:194-196). Verified: sync(url=A) then sync(url=B)urlopen called, cached names become ['beta']; a second sync(url=B) short-circuits with urlopen not called.
  • Finding 3 (malformed Content-Length → unhandled ValueError): resolved. :221-230 parses inside its own try with except ValueError: pass. Verified: Content-Length: not-a-number now syncs cleanly, and a declared length over MAX_INDEX_SIZE_BYTES is still rejected with IndexSyncError (IndexSyncError derives from Exception, not ValueError, so the guard's own raise is not swallowed).
  • Finding 6 (duplicate index names): resolved. seen_names dedup at :262-271. Verified: two entries named dup → "Successfully synchronized 1 packages", 1 entry in packages.json, one dup.yaml holding the first (1.0.0).
  • Finding 7 (offline over-promise in docs): resolved at docs/dependency-management.md:359-360. See finding 4 for the CHANGELOG copy that was not updated with it.
  • Finding 8 (license shadows the builtin): resolved. The parameter is gone from repository.py:156-172 and the two alias assertions were removed from the test.
  • Finding 5 (digest path): the packages.json.sha256 half is resolved; see finding 6 above for the half that is not.
  • The structural half of finding 1 is delivered: _find_recipe_dirs is deleted from commands.py and both paths now call registry.find_recipe_dirs (commands.py:48, repository.py:147). There is one answer to "where do recipes come from" for the first time; what remains is that the two layers still key their indexes differently.
  • pytest.ini:25 — the comment moved out of addopts. The last review could only infer that an in-list comment was skipped by iniconfig; this removes the question rather than answering it, which is the better fix.

Verified by running:

git rev-list --count pr111..origin/master   -> 0   (still merged up, MERGEABLE)
commits since 7e76e056                      -> 1   (005a69a)
diffstat 7e76e056..005a69ad -> 8 files, +189 -86

precedence, project + shipped + cached-remote:
  cjson  project 1.7.18 vs remote 1.7.18, unpinned request
    -> v1.7.18 url …/cjson-PROJECT.tar.gz  cks sha256:111…   FIXED
  lvgl   project 9.2.2  vs remote 9.9.9,  unpinned request
    -> v9.9.9  url …/lvgl-REMOTE.tar.gz    cks sha256:444…   STILL BROKEN
  lvgl   same, request pinned to "9.2.2"
    -> v9.2.2  url …/lvgl-PROJECT.tar.gz                     ok when pinned
  app pinned 1.0.0, dependency mbedtls project 3.6.0 vs remote 9.9.9
    -> mbedtls v9.9.9 url …/mbedtls-REMOTE.tgz cks sha256:222…  (finding 1)
  same lvgl inputs through PackageRepository.search
    -> v9.2.2 project url — search and build still disagree

original 9.9.9 fixture replayed against this head:
  repo.info("cjson")            -> 1.7.18, PROJECT url    (assertions :100-106 pass)
  registry.get("cjson","1.7.18")-> PROJECT url            (assertion :112-116 passes)
  registry.get("cjson")         -> 9.9.9, REMOTE url      (nothing asserts this)  (finding 2)

sync(url=A) then sync(url=B), cache 0s old:
  urlopen called for B: True · cached names ['beta'] · meta url b.example  (finding 2 resolved)
  second sync(url=B): urlopen called False, "Cache is up-to-date"
  recipes/ after the A->B switch: ['alpha.yaml', 'beta.yaml']              (finding 5)

Content-Length "not-a-number" -> synced, no exception                      (finding 3 resolved)
Content-Length MAX+1          -> IndexSyncError "exceeds maximum allowed size"
index with two 'dup' entries  -> reported 1, packages.json 1, dup.yaml 1.0.0 (finding 6 resolved)
cache dir contents: index-meta.json, packages.json, packages.json.sha256, recipes/
logger.warning surfaced on stderr; logger.info did not; no basicConfig in ebuild/  (finding 6)

grep is_offline/EBUILD_OFFLINE in ebuild/ -> index_sync.py:68,72,169 only
fetcher.py:94 urlretrieve(recipe.url, ...) — no offline gate               (finding 4)
grep Lockfile.load/is_locked/get_locked_*/locked_packages in ebuild/ -> no call sites
grep _RECIPE_DIRS -> no hits (cleanly removed)

CLI driven through click.testing.CliRunner:
  ebuild update-index                       -> exit 1, "No remote package index URL configured…"
  ebuild update-index --offline             -> exit 0, "using cached index (0 packages)"
  ebuild update-index --url http://insecure -> exit 1, "only HTTPS URLs are permitted"
  ebuild search json / nosuchpkg / --json / --all -> exit 0, output shape unchanged

CI: actions/runs?head_sha=005a69ad -> 3 runs, all conclusion "action_required"
    commits/005a69ad/status -> {"state":"pending","total":0}; check-runs -> 0

Architecture conformance

Master design §9.1–9.2 (eBuild engine and SDK design rules), §10 and §10.1 (component model
and contract), §11/§11.1 (Registry), §15.1, §21 tiers and §21.1 split policy.
Tier placement conforms, unchanged across all three reviews; §9.2's reproducibility rule
still does not hold on the build path.

Placement is right and nothing in this diff points up a tier. A remote index client inside
ebuild is Tier 1 – Foundation reading a Tier 4 – Developer Ecosystem service, which §5.1
permits — "eBuild understands the complete graph but is not a runtime dependency" — and §11
names ebuild search / ebuild add as the CLI surface for exactly this. §21.1 is not
triggered: no repository is proposed. Collapsing the two recipe-directory implementations into
registry.find_recipe_dirs moves the diff toward §9.2's "one source of truth for CLI, VS
Code and EoStudio", and is the right call.

The deviation is the same one, narrowed. §9.2 asks for "reproducible lockfiles/manifests for
production builds". After 005a69a, a project's committed recipe is authoritative when the
registry names the same version, and is not when the registry names a newer one, or when the
package is reached as a dependency. That is the design question the 2026-09-03 proposal
already raises: precedence between sources and version ordering within a source are two
different rules, and the code currently lets the second override the first. The proposal's
text — "a later registry synchronisation must not change that component's source location,
version or digest" — already answers it, so no new proposal is appended; this head is
additional evidence for the one that stands.

The lockfile is still the missing backstop and is still pre-existing on master, not this
PR's defect: ebuild.lock is written at commands.py:157-158 and no code path reads it back.
Finding 1 has nothing to catch it downstream. That is the subject of the 2026-09-04 proposal
already filed against §9.2.

§10.1's Integrity field remains satisfied as a transit checksum only, and the PR's own
"Index Authenticity & Provenance Notice" says so at docs/dependency-management.md:321-327
that is the right treatment for this PR, and it is why finding 3(a)'s "full compliance with
§10.1" claim is contradicted by the PR's own documentation two sections later.

Proposed changes

In order — 1 and 2 travel together and are the only ones that block:

  1. Finding 1. Rank recipe sources at registration and make get(name) / list_packages()
    choose the highest version within the highest-ranked source that defines the name. This is
    ~10 lines in registry.py and needs no change to resolver.py.
  2. Finding 2. Restore the 9.9.9 fixture, add the unpinned registry.get("cjson")
    assertion and a transitive-dependency case. Write these first — they fail on this head, and
    they are what makes step 1 provable.
  3. Finding 3. Rewrite the commit subject to what the commit did, and fill in the PR body.
  4. Finding 4 (CHANGELOG line) and finding 5 (prune stale recipe YAMLs) — one commit each.
  5. Findings 6 and 7 are small and can travel together.
  6. Ask a maintainer to approve the three action_required workflow runs. Four consecutive
    heads have now been reviewed with no CI execution at all
    , and this PR has grown to 1,273
    added lines. Nothing here has been compiled, linted or tested by the project's pipeline.

No fix PR opened, for the same two reasons as the last review. Findings 2, 3, 4 and 5 are on
this PR's branch, which the brief puts out of bounds. Finding 1's fix is a change to
package-resolution semantics — it changes which archive a build downloads — which is not the
small, provable class the brief permits an unattended agent to open, and the governing design
rule is still an open proposal awaiting a human.

Not checked

  • No CI has run on this head. Three workflow runs exist for 005a69adCI — ebuild,
    CodeQL, Simulation Test — all conclusion: action_required, i.e. queued awaiting
    maintainer approval for a fork contribution. Combined status pending with zero statuses,
    zero check-runs. No build, lint, type check or test in this PR has been executed by the
    project's pipeline, on this or any previous head.
  • The test suite was not run. pytest is not installed in this environment and there is no
    pip to install it
    (python3 -m pip → no module named pip). All 26 tests were read, and
    the two that bear on findings 1 and 2 were replayed by hand against the modules; the suite as
    a whole was NOT RUN and whether it passes is unknown. Every result above comes from
    importing the modules directly, driving the CLI through click.testing.CliRunner, and
    mocking urllib.request.urlopen where the network would be reached.
  • No real index was fetched. DEFAULT_INDEX_URL is still "" and no deployed index
    exists, so every sync path was exercised against a mocked urlopen.
  • No package was fetched, built or installed end to end. ebuild build was not run;
    finding 1 was demonstrated at _find_recipe_dirs + create_registry + PackageResolver
    the exact functions _install_packages calls at commands.py:72-84 — not through a build.
    fetcher.py:94's consequence was read, not executed.
  • The five recipe checksums were not re-verified this run. All five were downloaded and
    hashed at head 7e76e056 and matched; recipes/ is unchanged since, so the prior result
    carries — but it was not re-run here.
  • Finding 5's pruning behaviour was demonstrated with two mocked indices, not against a
    real registry withdrawing a package.
  • index-meta.json has no schema validation; a hand-edited or truncated one falls back to
    {} via except Exception (index_sync.py:186-191), which I read but did not exercise for
    every malformed shape.
  • The local ebuild clone was left untouched. The sync step reported it dirty (4 files,
    branch v90) and skipped it. I made a git clone --shared --no-checkout under /tmp and
    fetched pull/111/head there, so the user's working tree, index, stashes and refs were never
    written to.

Automated architecture review of 005a69adb03c — 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.

… and surface index digest

- Rank recipe search paths in PackageRegistry so higher-priority sources
  (project > shipped > remote) dominate version selection for unpinned
  and transitive packages (§9.2 reproducibility).
- Restore 9.9.9 remote fixture and test unpinned & transitive precedence.
- Prune stale cached YAML recipes when synchronizing package index.
- Surface index SHA-256 digest and change notifications in update-index.
- Catch narrow (ImportError, OSError) with logging in find_recipe_dirs.
- Scope CHANGELOG offline capability statement to index synchronization.
@Grantlinkz
Grantlinkz force-pushed the Remote-Package-Index-And-Ecosystem branch from e713993 to afb768f Compare September 5, 2026 05:39

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#111 "feat: implement package management system with recipe support, registry…"

head: e713993 author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED

Verdict: Follow-up to the review of 005a69ad. One commit, e713993. Both High findings
are resolved and I verified them by running them
— source-ranked precedence now holds on the
build path for unpinned and transitive packages, and the precedence test is restored to the
9.9.9 fixture with the two assertions that make the fix provable. Findings 4, 6 and 7 are also
resolved. docs/dependency-management.md:325 is now a true statement for the first time in this
PR's history. This run also had a working pytest that the previous three did not: the full
suite passes, 599 tests, 0 failures
— so the three earlier "test suite NOT RUN" caveats are now
answered. What is left is the prune added for finding 5. It works for the case it was written
for, and it introduces three new defects: it deletes the last known-good recipe for a package
that is still in the index, it deletes files it did not write with no user-visible output, and
its stem check defeats its own *.yml glob so a stale .yml can still outrank a fresh .yaml.
The PR body is still the unfilled template, now across five heads.

Findings

# Severity File:line Finding Recommended fix
1 Medium ebuild/packages/index_sync.py:332-341 and :343-351 The prune deletes the last known-good recipe for a package that is still listed in the index, leaving ebuild search and ebuild build disagreeing about whether it exists. seen_recipe_stems.add(pkg_name) only runs at :337, i.e. inside if recipe_dict["url"] and after parse_recipe succeeded. An entry that is still in the index but whose url disappeared, or whose fields no longer validate, therefore never reaches seen_recipe_stems — and the prune at :346 unlinks its cached YAML. valid_entries still contains it, so it is still written to packages.json at :285. Reproduced twice on this head, --force both syncs: (a) alpha synced with a url, then re-synced with the url key removed → recipes/ becomes [] while packages.json still holds ['alpha']; (b) beta re-synced with version: ""Skipping invalid recipe entry beta: Package 'beta' must have a 'version' field, then Pruned stale cached recipe: beta.yaml. In case (a) I then drove both surfaces: PackageRepository.load_index(packages.json).search("alpha") returns [('alpha','1.0.0')] while create_registry(recipes_dir).get("alpha") returns None. Before e713993 the previous good recipe survived, so this is a behaviour regression introduced by the prune: one malformed upstream entry now destroys a working local definition instead of leaving it in place, and it does so on a cache that --offline builds depend on. Prune on absence from the index, not on failure to write a recipe this run. Seed the set from the entries rather than the writes: seen_recipe_stems = {sanitize_package_name(str(e["name"])) for e in valid_entries} computed before the loop, and let the write loop keep only synced_count. A package the index still names then keeps whatever recipe it already had, and only a package the index actually dropped is removed. Add the test: sync alpha with a url, re-sync with the url removed, assert alpha.yaml still exists.
2 Medium ebuild/packages/index_sync.py:343-351 update-index now deletes every *.yaml/*.yml in the cache recipe directory that it did not write this run, and says nothing about it. The only report is logger.debug at :349, and no module under ebuild/ calls logging.basicConfig, so at the root logger's default level that line is unreachable — logger.warning at :351 surfaces via the last-resort handler, logger.debug does not. Driven through the CLI on this head: a hand-written my-own-recipe.yaml placed in the cache dir is gone after one update-index --force, and the command's entire output is the header, [ok] Successfully synchronized 1 packages, the digest line and the cache path — no mention of the deletion, exit 0. The directory is also user-selectable: EBUILD_INDEX_PATH sets index_dir (used in this reproduction), so update-index is an unannounced unlink pass over *.yaml/*.yml in a path the user chose. .ai/tooling.md asks for actionable diagnostics; a delete is the one action that must be visible. Report it at a level the user sees: logger.warning, or better, count the prunes and have update_index print Pruned N stale cached recipes next to the digest line the same commit added. If the directory is to be treated as exclusively ebuild-owned, say so in docs/dependency-management.md where the cache path is documented — see finding 7.
3 Medium ebuild/packages/index_sync.py:345-346 The prune globs *.yml but matches on .stem, so a stale .yml is never pruned when a .yaml of the same name is written — and it can still outrank it. existing_file.stem strips the extension, and seen_recipe_stems holds bare package names, so delta.yml and delta.yaml both match the stem delta. Reproduced: a pre-existing delta.yml pinning version: "9.9.9", url: https://STALE/d.tgz, then a sync writing delta.yaml at 2.0.0 from https://FRESH/d.tgz → both files remain on disk, and because PackageRegistry.scan (registry.py:112-125) gives every file in one directory the same rank, get("delta") falls through to highest-version-within-rank and returns 9.9.9 with the STALE url. That is precisely the stale-recipe-outranks-current case the prune was added to close, still reachable, and the new rank ordering cannot help because both files sit at the same rank. Key the prune on the filename the sync would have written, not the stem: track seen_recipe_files = {f"{pkg_name}.yaml"} and prune any *.yaml/*.yml whose name is not in it. That removes a delta.yml shadow whether or not delta.yaml was written this run. Add the assertion to test_index_sync_prunes_stale_cached_recipes.
4 Medium pr body The PR body is still the unfilled template — five heads, 1,477 added lines, no movement. No summary, - placeholders under Changes, every Testing and Pre-Submission box unchecked, no command output, no linked issue. This is now the only prior finding with zero progress across four consecutive reviews. It is also the one that costs least to fix and would have saved the most: the suite I ran this session is green and the author could simply have said so. The brief treats an unsupported "verified" as itself the finding; a change of this size asserting nothing is the same gap from the other side. (The garbled type labels eat/ix/efactor remain .github/PULL_REQUEST_TEMPLATE.md's corruption on origin/master, not the author's doing — unchanged across all four reviews.) State what this PR does, and name the verification: python -m pytest tests/ -q → 599 passed on this head. Then name what it does not cover — findings 1, 2 and 3 are all in that gap.
5 Low commit e713993 subject The commit subject is scrambled and does not follow Conventional Commits. git log -1 --format=%s returns a 260-character line beginning tifications in update-index. - Catch narrow (ImportError, OSError) with logging in find_recipe_dirs. - Scope CHANGELOG offline capability statement to index synchronization.fix(packages): enforce source-ranked precedence, prune stale recipes, and surface index digest — the tail of the body has been spliced in front of the real subject, and the body's own last line is truncated mid-word at and change no. .github/STANDARDS.md:56 requires Conventional Commits 1.0.0, and the PR's own checklist has a "Commit messages follow <type>(<scope>): <description>" box. The actual message underneath is a good one; the mangling looks like an editor or -m quoting accident, not a lack of care. git commit --amend with the subject the message clearly meant: fix(packages): enforce source-ranked precedence, prune stale recipes, and surface index digest, and restore the truncated final bullet.
6 Low CHANGELOG.md:143 The same edit that scoped the offline claim (prior finding 4, correctly resolved) removed the file's trailing newline: the link-reference line [0.1.0]: …/tag/v0.1.0 now ends the file with no \n. Confirmed by hexdump — the last byte is 0. The line's content is byte-identical to before, so the whole hunk is a no-op except for the lost newline. Re-add the newline; the second CHANGELOG hunk should not be in the diff at all.
7 Low docs/dependency-management.md:315-327, 359-363; CHANGELOG.md Unreleased Two user-visible behaviours landed in e713993 and neither is documented. Grepped this head: no occurrence of "prune" or "stale" anywhere in docs/ or CHANGELOG.md. Nothing tells a user that update-index now deletes cached recipes, which matters most to exactly the air-gapped audience the "Offline & Air-Gapped Operation" section at :359 addresses — after this change one online update-index can remove a definition an offline build was relying on (finding 1). The digest line the CLI now prints is likewise unrecorded; the CHANGELOG's only edit in this commit was the offline scoping. Per the brief, behaviour changed without the docs changing with it. One bullet under "Offline & Air-Gapped Operation" stating that a successful sync removes cached recipes no longer present in the index, and one Unreleased CHANGELOG line covering the prune, the digest output and the source-ranked precedence — the precedence change in particular is the most significant behaviour change in this commit and the CHANGELOG does not mention it.
8 Low ebuild/packages/index_sync.py:53-57; ebuild/cli/commands.py:1738 SyncResult subclasses Tuple and its count attribute shadows tuple.count. Ran the CI type check on the changed modules — mypy ebuild/packages/registry.py ebuild/packages/index_sync.py --ignore-missing-imports --no-strict-optionalindex_sync.py:53: error: Incompatible types in assignment (expression has type "int", base class "tuple" defined the type as "Callable[[Any], int]"), 1 error. Attribute access works, but res.count(x) — the inherited method — is unreachable on every SyncResult. This commit added a fourth field (sha256) to the same class rather than moving off the pattern. Adjacent, in code this commit edited: commands.py:1738's count, msg = res[0], res[1] binds count and never reads it (ruff F841). Both predate e713993 within this PR; recorded now because this is the first run that could execute ruff and mypy, and CI has never executed either on any head. SyncResult wants to be a NamedTuple or a small @dataclass with count/message/is_fallback/sha256 and no tuple base — the two positional unpackings at commands.py:1737 are the only thing relying on tuple-ness, and both would read better as attributes. Drop the unused count binding while there.

Resolved since 005a69ad — one line each, no further treatment:

  • Finding 1 (High, cached remote recipe overrides a project's pinned url/checksum for unpinned and transitive packages): resolved in e713993. PackageRegistry now stores (rank, recipe) keyed by search-path order (registry.py:95, 111, 128-137), and get(name) picks the highest version within min_rank (:153-158), with list_packages() delegating to it (:164-171). Verified on this head with project + shipped + cached-remote all on the path: unpinned lvgl (project 9.2.2 vs remote 9.9.9) → v9.2.2, PROJECT url, PROJECT checksum; the transitive case, app pinned to 1.0.0 with dependency lvgl unpinned → v9.2.2, PROJECT url; and the search path (PackageRepository.load_all_sources) and the build path (find_recipe_dirs + create_registry) return the same answer for the same inputs for the first time in this PR. The 9.9.9 scenario the last review recorded as STILL BROKEN is fixed.
  • Finding 2 (High, precedence test weakened): resolved in e713993. The remote fixture is back to 9.9.9 at test_package_search.py:73 and :87, the unpinned assertion the last review asked for is there verbatim at :116-121, and test_transitive_dependency_source_precedence_project_wins (:126-186) adds the third case. Both files pass; I confirmed the new assertions exercise the unpinned path rather than the pinned one.
  • Finding 3(a) (the "full compliance with §9.2 and §10.1" claim): resolved. e713993's message makes no compliance claim — it lists what it changed. See finding 5 for the message's remaining problem, and finding 4 for 3(b), which is untouched.
  • Finding 4 (CHANGELOG contradicts the docs): resolved in e713993. CHANGELOG.md:10 now reads "Index synchronization supports air-gapped operation via --offline and EBUILD_OFFLINE=1; package archive fetching is not yet offline-gated", matching docs/dependency-management.md:361. See finding 6 for the newline the same hunk dropped.
  • Finding 6 (index digest had no consumer): resolved in e713993. SyncResult carries sha256 on all four return paths (index_sync.py:57, 210, 252, 357) and update_index reads index-meta.json before the sync to compare (commands.py:1728-1734, 1745-1758). Verified through click.testing.CliRunner: first sync prints [info] Index SHA-256 digest: f7439d04…; a second sync of a changed index prints the new digest and [info] Index updated (previous digest: f7439d04…). It uses the CLI Logger, not the stdlib one, so unlike the logger.info the last review flagged, this genuinely reaches the user.
  • Finding 7 (broad except Exception in find_recipe_dirs): resolved. registry.py:240-241 is now except (ImportError, OSError) as e: with logger.warning, which does surface on stderr.

Verified by running — first run in this PR's history with a working test environment:

environment: uv venv + uv pip install pytest click pyyaml ninja ruff mypy
  (the previous three reviews reported pytest unavailable; /home/srpatcha/.local/bin/uv
   resolves it, so the three "suite NOT RUN" caveats are answered here)

python -m pytest tests/ -q                -> 599 passed, 0 failed        (PR head e713993)
python -m pytest tests/unit/test_index_sync.py tests/unit/test_package_search.py -q
                                          -> 21 passed
  (an earlier run showed 1 failure, test_end_to_end_build_from_outside_produces_the_binary,
   "No module named ninja" — environment, not the PR; green after installing ninja)

ruff check . --select=E,F,W --ignore=E501   origin/master 382 · pr111 393
ruff, PR-touched files only                 11 errors, all F401/F841
ruff on the same files at 005a69ad vs e713993 -> identical count; this commit adds none
  (CI's ruff and mypy steps are both continue-on-error: true, so none of this is CI-blocking)
mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional
                                          -> 1 error, index_sync.py:53   (finding 8)

precedence, project + shipped + cached-remote on the build path:
  registry.get("lvgl")               -> 9.2.2  https://custom-project.org/lvgl-PROJECT…
  registry.get("lvgl","9.9.9")       -> 9.9.9  remote url (explicit pin, expected)
  list_all_versions("lvgl")          -> [9.2.2 PROJECT, 9.9.9 REMOTE]  (no prod caller)
  list_packages() lvgl               -> 9.2.2  PROJECT
  resolve([{app 1.0.0}]) -> lvgl     -> 9.2.2  PROJECT       (transitive, finding 1 fixed)
  search path vs build path, same inputs -> both 1.7.18 PROJECT   (they now agree)

prune probes, --force on every sync:
  alpha(url) then alpha(no url)  -> recipes [] · packages.json ['alpha']   (finding 1)
    search sees ('alpha','1.0.0') · registry.get('alpha') -> None
  beta(url) then beta(version:"") -> "Skipping invalid recipe entry beta" then
                                     "Pruned stale cached recipe: beta.yaml"  (finding 1)
  handwritten.yaml present, sync gamma -> handwritten.yaml deleted, CLI output silent,
                                     exit 0                                   (finding 2)
  stale delta.yml 9.9.9 + fresh delta.yaml 2.0.0 -> both survive,
                                     get("delta") -> 9.9.9 STALE url          (finding 3)
  alpha->beta index switch (the prior review's repro) -> recipes ['beta.yaml'] (finding 5 fixed)

CLI through click.testing.CliRunner:
  update-index --url … --force        -> exit 0, digest line printed
  update-index, changed index         -> exit 0, digest + "Index updated (previous digest:)"

git log -1 --format=%s e713993       -> 260 chars, body spliced before subject (finding 5)
tail -c CHANGELOG.md | xxd           -> last byte "0", no trailing newline    (finding 6)
grep -i "prune\|stale" docs/ CHANGELOG.md -> no hits                          (finding 7)
grep Lockfile.load/is_locked/get_locked_*/locked_packages in ebuild/ -> still no call sites

git rev-list --count pr111..origin/master -> 0   (merged up, MERGEABLE)
diffstat 005a69ad..e713993 -> 6 files, +214 -30
CI: actions/runs?head_sha=e7139935 -> 3 runs (CI — ebuild, CodeQL, Simulation Test),
    all conclusion "action_required"; commits/e7139935/status -> pending, total 0;
    check-runs -> 0

Architecture conformance

Master design §9.1–9.2 (eBuild engine and SDK design rules), §10 and §10.1 (component model
and contract), §11/§11.1 (Registry), §21 tiers and §21.1 split policy.
Tier placement conforms, unchanged across all four reviews. §9.2's reproducibility rule now
holds on the build path — the deviation the previous three reviews recorded is closed.

Placement is unchanged and nothing in this diff points up a tier: a remote index client
inside ebuild is Tier 1 – Foundation reading a Tier 4 – Developer Ecosystem service, which
§5.1 permits ("eBuild understands the complete graph but is not a runtime dependency"), and §11
names ebuild search / ebuild add as the CLI surface for exactly this. §21.1 is not
triggered; no repository is proposed.

The §9.2 deviation is resolved. "Reproducible lockfiles/manifests for production builds" was
failing because precedence between sources and version ordering within a source were the same
rule; e713993 separates them, and the ranked get() at registry.py:153-158 is the shape the
2026-09-03 proposal against §10 described — "the definition committed in the consuming project,
the definition shipped with the SDK, the definition obtained from the registry", in that fixed
order. That proposal stands unmerged and is not duplicated here; this head is the implementation
arriving ahead of the design text, which is the argument for merging it rather than against.
One clause of it is still unimplemented: "Tooling must be able to report, for every resolved
component, which source its definition came from." The rank is now known at resolution time and
discarded — get() returns a bare PackageRecipe — so ebuild build still cannot say which of
three documents supplied the url it fetched. Not scored as a finding, because the proposal that
would require it has not been accepted.

The lockfile remains the missing backstop and remains pre-existing on master, not this PR's
defect: Lockfile.load / is_locked / get_locked_entry / get_locked_version /
locked_packages still have zero call sites in ebuild/ on this head, so ebuild.lock is
written every build and never read. That is the subject of the 2026-09-04 proposal against §9.2,
which also stands.

§10.1's Integrity field is still satisfied as a transit checksum only, and the PR's own "Index
Authenticity & Provenance Notice" (docs/dependency-management.md:320-327) says so — the right
treatment for this PR. With finding 1 fixed, the sentence at :325 — "project-local recipes in
./recipes/ take absolute precedence over remote index definitions. An index update will never
override pinned URLs or checksums defined in your project repository" — is now true as
written
; I verified it on the build path, the search path and through a transitive dependency.

One design gap is new with this commit and is not covered by any standing proposal: nothing in
the master design says whether a registry client may remove locally held component
definitions. §11 defines the registry and its artifact types and never mentions a local cache;
§9.2 promises "No mandatory cloud connection" without saying what the offline path is entitled to
keep. e713993 makes a successful online update-index delete cached definitions, which can
break a build that worked offline a moment earlier (finding 1). A proposal is appended.

Proposed changes

In order — findings 1 and 3 are one edit and are the only ones that should block:

  1. Findings 1 and 3 together. Build the prune's keep-set from valid_entries before the
    write loop, and key it on filename rather than stem:
    keep = {f"{sanitize_package_name(str(e['name']))}.yaml" for e in valid_entries}, then prune
    any *.yaml/*.yml whose .name is not in keep. That fixes both — a package the index
    still names keeps its recipe, and a delta.yml shadow is removed. Two assertions on
    test_index_sync_prunes_stale_cached_recipes cover it.
  2. Finding 2. Count the prunes and print the count from update_index, next to the digest
    line this commit already added.
  3. Finding 4. Fill in the PR body. python -m pytest tests/ -q → 599 passed on this head is
    the sentence that has been missing for five heads.
  4. Findings 5, 6, 7 — one commit: amend the subject, restore the newline, document the prune
    and the precedence change.
  5. Finding 8 is a small refactor and can wait for a follow-up.
  6. Ask a maintainer to approve the three action_required workflow runs. Five consecutive
    heads have now been reviewed with no CI execution at all.
    I ran the suite, ruff and mypy
    myself this session and they are reported above, but that is a reviewer's sandbox, not the
    project's pipeline, and it does not cover the Windows and macOS matrix legs, CodeQL, or the
    simulation job.

No fix PR opened. Every finding is on this PR's branch, which the brief puts out of bounds, and
findings 1 and 3 change which files update-index deletes — not the small, provable class an
unattended agent may open unreviewed.

Not checked

  • No CI has run on this head, or on any of the five. Three workflow runs exist for
    e7139935CI — ebuild, CodeQL, Simulation Test — all conclusion: action_required,
    queued awaiting maintainer approval for a fork contribution. Combined status pending with
    zero statuses, zero check-runs. Everything reported under "Verified by running" was executed
    in a local sandbox on Linux/CPython 3.12.14, not by the project's pipeline: the windows-2022
    and macOS matrix legs, CodeQL and the simulation job are NOT RUN and their result is
    unknown.
  • The --cov-fail-under and coverage gates were not exercised. I ran pytest tests/ -q,
    not CI's --cov=ebuild --cov-report=xml --cov-fail-under=0 invocation; pytest-cov and
    pytest-benchmark were not installed, so tests/performance/ ran without the benchmark
    plugin's JSON output.
  • The ruff and mypy numbers are from ruff 0.16.6 and the current mypy, resolved fresh; CI
    installs both unpinned, so its versions will differ from a future run. Both steps are
    continue-on-error: true in .github/workflows/ci.yml:53, 65, so neither can fail the job
    either way.
  • No real index was fetched. DEFAULT_INDEX_URL is still "" and no deployed index exists,
    so every sync path — including all four prune probes — ran against a mocked
    urllib.request.urlopen.
  • No package was fetched, built or installed end to end. ebuild build was not run; finding
    1's precedence result was demonstrated at _find_recipe_dirs + create_registry +
    PackageResolver.resolve — the exact functions _install_packages calls — and fetcher.py's
    consequence was read, not executed.
  • The five recipe checksums were not re-verified this run. All five were downloaded and
    hashed at head 7e76e056 and matched; recipes/ is unchanged since, so that result carries,
    but it was not re-run.
  • Finding 2's EBUILD_INDEX_PATH reproduction used a temporary directory, not a directory
    holding work a user cared about; the deletion is real, the consequence is inferred.
  • index-meta.json still has no schema validation and update_index's two reads of it wrap
    json.load in a bare except Exception (commands.py:1731-1734, 1742-1747). I exercised the
    absent-file and well-formed cases; truncated and hand-edited shapes were read, not run.
  • The local ebuild clone was left untouched. The sync step reported it dirty (3 modified
    files plus smart-sensor/, on branch v90, 100 behind origin/master) and skipped it. I made
    a git clone --shared --no-checkout under /tmp and fetched pull/111/head there, so the
    user's working tree, index, stashes and refs were never written to.

Automated architecture review of e71399354475 — 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants