feat: implement package management system with recipe support, regist… - #111
Conversation
…ry, and build synchronization.
srpatcha
left a comment
There was a problem hiding this comment.
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.json — gh 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:
- Rebase on
master; drop thetest()and_board_config()changes as already merged
(finding 4). This removes both conflicts. - Make
add_recipe_directoryrespect source precedence, and add the three-source test
(finding 1). - Point
DEFAULT_INDEX_URLat something that exists, or make it empty and say so in
ebuild search's empty-state text (finding 3). - Fill in the PR body (finding 5).
- Filter
packages.jsonbefore writing it and sanitise inload_index(finding 6). - Implement or remove
--force(finding 7); make the fallback exit non-zero (finding 8). - Findings 9–13 are small and can travel together.
- Separately: state in
docs/dependency-management.mdthat 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_requiredruns.commits/77f1d9f2…/statusis{"state":"pending","count":0}. So
nothing in this PR has been verified by the project's pipeline, and the PR body claims
nothing either. pytestis 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.urlopenmocked 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 apiplus 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 asha256: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.pyandbuilder.pywere read
where finding 2 depends on them, not exercised. ebuild searchandebuild update-indexwere driven through their library layer, not through
theclickCLI, so argument parsing,--jsonoutput shape and exit codes were reasoned from
the source rather than observed.- The local
ebuildclone was left alone — the sync step reported it dirty (4 files, on
branchv90), and the PR head was not present locally. I cloned it to/tmpwith
git clone --shared --no-checkoutand fetchedpull/111/headthere, 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.
…ry, and build synchronization. # Conflicts: # ebuild/cli/commands.py
…ry, and build synchronization.
…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
left a comment
There was a problem hiding this comment.
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_dirs → create_registry → PackageRegistry._register, and _register at registry.py:129 is self._recipes[name][version] = recipe — last 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-indexwith no URL exits 1 with that message. - Finding 4 (conflicts, 7 behind): resolved in
7e76e05. Mergede5d8052;git rev-list --count pr111..origin/master→ 0,mergeable: MERGEABLE.registry.pyis out of the diff and the duplicatetest()/_board_config()reimplementations are gone. - Finding 6 (cache written before validation): resolved.
index_sync.py:240-256filters intovalid_entriesbefore the atomic write;repository.py:117-120sanitises inload_indextoo. - Finding 7 (
--forceinert): resolved — TTL check at:174-186, covered bytest_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 bytest_cli_update_index_fallback_exits_nonzero. - Finding 9(a) (count over-reports) and 9(b) (broad
except): resolved.synced_count += 1is inside theif recipe_dict["url"]block (:288-293); thetrynow spans only the fetch (:201-229), so cache-write failures surface as themselves. - Finding 10 (discarded
_parse_recipe): resolved.parse_recipeis public (recipe.py:118), andrecipe.to_dict()— the validated shape — is what gets written (:290-292). - Finding 11 (encoding churn): resolved. No coding cookie; em-dashes in
commands.pywent 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.mdorchestrator.py→dispatch.pycorrection 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:
- Finding 1. Make
PackageRegistry._registerfirst-wins, then collapse_find_recipe_dirsandload_all_sourcesinto one implementation so there is a single answer to "where do recipes come from and in what order". Add the build-path assertion totest_package_source_precedence_project_wins. Until that lands, softendocs/dependency-management.md:325— a documented guarantee the code does not provide is worse than no sentence. - Finding 2, then finding 3 — both are inside
sync()and can travel together with a test each: a second--urlagainst a warm cache, and a non-integerContent-Length. - Finding 4: fill in the PR body.
- Findings 5–8 are small and can go in one commit.
- Ask a maintainer to approve the three
action_requiredworkflow 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
7e76e056—CI — ebuild,CodeQL,Simulation Test— and all three areconclusion: action_required, i.e. queued awaiting maintainer approval for a fork contribution. Combined status ispendingwith 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.
pytestis not installed in this environment and there is nopipto install it (python3 -m pip→ no module named pip; a venv builds without pip). All 24 new tests intests/unit/test_index_sync.pyandtests/unit/test_package_search.pywere read, not executed. Whether the suite passes is unknown — every result above comes from importing the modules directly, driving the CLI throughclick.testing.CliRunner, and mockingurllib.request.urlopenwhere the network would be reached. pytest.ini:26's inline comment insideaddoptswas not executed.iniconfigskips 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 neitherpytestnoriniconfigis installed here.- No real index was fetched.
DEFAULT_INDEX_URLis empty and no deployed index exists, so every sync path was exercised against a mockedurlopen. 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-70was read, and the recipe that reaches the fetcher was reproduced; the download-and-extract itself was not run. ebuild buildwas not run, so finding 1 was demonstrated at_find_recipe_dirs+create_registry— the exact functions_install_packagescalls atcommands.py:105-110— rather than through a full build.- The local
ebuildclone was left untouched. The sync step reported it dirty (4 files, branchv90) and the PR head was not present locally. I made agit clone --shared --no-checkoutunder/tmpand fetchedpull/111/headthere, 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).
srpatcha
left a comment
There was a problem hiding this comment.
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 (
--urlignored against a warm cache): resolved in005a69a.index-meta.jsonrecords the origin URL (index_sync.py:120,:290-299) andsame_origingates the TTL short-circuit (:194-196). Verified:sync(url=A)thensync(url=B)→urlopencalled, cached names become['beta']; a secondsync(url=B)short-circuits withurlopennot called. - Finding 3 (malformed
Content-Length→ unhandledValueError): resolved.:221-230parses inside its owntrywithexcept ValueError: pass. Verified:Content-Length: not-a-numbernow syncs cleanly, and a declared length overMAX_INDEX_SIZE_BYTESis still rejected withIndexSyncError(IndexSyncErrorderives fromException, notValueError, so the guard's own raise is not swallowed). - Finding 6 (duplicate index names): resolved.
seen_namesdedup at:262-271. Verified: two entries nameddup→ "Successfully synchronized 1 packages", 1 entry inpackages.json, onedup.yamlholding 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 (
licenseshadows the builtin): resolved. The parameter is gone fromrepository.py:156-172and the two alias assertions were removed from the test. - Finding 5 (digest path): the
packages.json.sha256half is resolved; see finding 6 above for the half that is not. - The structural half of finding 1 is delivered:
_find_recipe_dirsis deleted fromcommands.pyand both paths now callregistry.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 ofaddopts. The last review could only infer that an in-list comment was skipped byiniconfig; 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:
- 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 inregistry.pyand needs no change toresolver.py. - Finding 2. Restore the
9.9.9fixture, add the unpinnedregistry.get("cjson")
assertion and a transitive-dependency case. Write these first — they fail on this head, and
they are what makes step 1 provable. - Finding 3. Rewrite the commit subject to what the commit did, and fill in the PR body.
- Finding 4 (CHANGELOG line) and finding 5 (prune stale recipe YAMLs) — one commit each.
- Findings 6 and 7 are small and can travel together.
- Ask a maintainer to approve the three
action_requiredworkflow 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
005a69ad—CI — ebuild,
CodeQL,Simulation Test— allconclusion: action_required, i.e. queued awaiting
maintainer approval for a fork contribution. Combined statuspendingwith 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.
pytestis not installed in this environment and there is no
pipto 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 throughclick.testing.CliRunner, and
mockingurllib.request.urlopenwhere the network would be reached. - No real index was fetched.
DEFAULT_INDEX_URLis still""and no deployed index
exists, so every sync path was exercised against a mockedurlopen. - No package was fetched, built or installed end to end.
ebuild buildwas not run;
finding 1 was demonstrated at_find_recipe_dirs+create_registry+PackageResolver—
the exact functions_install_packagescalls atcommands.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 head7e76e056and 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.jsonhas no schema validation; a hand-edited or truncated one falls back to
{}viaexcept Exception(index_sync.py:186-191), which I read but did not exercise for
every malformed shape.- The local
ebuildclone was left untouched. The sync step reported it dirty (4 files,
branchv90) and skipped it. I made agit clone --shared --no-checkoutunder/tmpand
fetchedpull/111/headthere, 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.
e713993 to
afb768f
Compare
srpatcha
left a comment
There was a problem hiding this comment.
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-optional → index_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.PackageRegistrynow stores(rank, recipe)keyed by search-path order (registry.py:95, 111, 128-137), andget(name)picks the highest version withinmin_rank(:153-158), withlist_packages()delegating to it (:164-171). Verified on this head with project + shipped + cached-remote all on the path: unpinnedlvgl(project 9.2.2 vs remote 9.9.9) → v9.2.2, PROJECT url, PROJECT checksum; the transitive case,apppinned to 1.0.0 with dependencylvglunpinned → 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. The9.9.9scenario the last review recorded as STILL BROKEN is fixed. - Finding 2 (High, precedence test weakened): resolved in
e713993. The remote fixture is back to9.9.9attest_package_search.py:73and:87, the unpinned assertion the last review asked for is there verbatim at:116-121, andtest_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:10now reads "Index synchronization supports air-gapped operation via--offlineandEBUILD_OFFLINE=1; package archive fetching is not yet offline-gated", matchingdocs/dependency-management.md:361. See finding 6 for the newline the same hunk dropped. - Finding 6 (index digest had no consumer): resolved in
e713993.SyncResultcarriessha256on all four return paths (index_sync.py:57, 210, 252, 357) andupdate_indexreadsindex-meta.jsonbefore the sync to compare (commands.py:1728-1734, 1745-1758). Verified throughclick.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 CLILogger, not the stdlib one, so unlike thelogger.infothe last review flagged, this genuinely reaches the user. - Finding 7 (broad
except Exceptioninfind_recipe_dirs): resolved.registry.py:240-241is nowexcept (ImportError, OSError) as e:withlogger.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:
- Findings 1 and 3 together. Build the prune's keep-set from
valid_entriesbefore 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/*.ymlwhose.nameis not inkeep. That fixes both — a package the index
still names keeps its recipe, and adelta.ymlshadow is removed. Two assertions on
test_index_sync_prunes_stale_cached_recipescover it. - Finding 2. Count the prunes and print the count from
update_index, next to the digest
line this commit already added. - 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. - Findings 5, 6, 7 — one commit: amend the subject, restore the newline, document the prune
and the precedence change. - Finding 8 is a small refactor and can wait for a follow-up.
- Ask a maintainer to approve the three
action_requiredworkflow 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
e7139935—CI — ebuild,CodeQL,Simulation Test— allconclusion: action_required,
queued awaiting maintainer approval for a fork contribution. Combined statuspendingwith
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: thewindows-2022
and macOS matrix legs, CodeQL and the simulation job are NOT RUN and their result is
unknown. - The
--cov-fail-underand coverage gates were not exercised. I ranpytest tests/ -q,
not CI's--cov=ebuild --cov-report=xml --cov-fail-under=0invocation;pytest-covand
pytest-benchmarkwere not installed, sotests/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: truein.github/workflows/ci.yml:53, 65, so neither can fail the job
either way. - No real index was fetched.
DEFAULT_INDEX_URLis 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 buildwas not run; finding
1's precedence result was demonstrated at_find_recipe_dirs+create_registry+
PackageResolver.resolve— the exact functions_install_packagescalls — andfetcher.py's
consequence was read, not executed. - The five recipe checksums were not re-verified this run. All five were downloaded and
hashed at head7e76e056and matched;recipes/is unchanged since, so that result carries,
but it was not re-run. - Finding 2's
EBUILD_INDEX_PATHreproduction used a temporary directory, not a directory
holding work a user cared about; the deletion is real, the consequence is inferred. index-meta.jsonstill has no schema validation andupdate_index's two reads of it wrap
json.loadin a bareexcept 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
ebuildclone was left untouched. The sync step reported it dirty (3 modified
files plussmart-sensor/, on branchv90, 100 behindorigin/master) and skipped it. I made
agit clone --shared --no-checkoutunder/tmpand fetchedpull/111/headthere, 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.
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#111 "feat: implement package management system with recipe support, registry…"
head: afb768f author: Grantlinkz ci: none run (4 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED
Verdict: Follow-up to the review of e7139935. afb768f is a git commit --amend of
e713993 and nothing else — git diff e7139935 afb768fb is empty, the two trees are
byte-identical. It resolves finding 5 (the scrambled commit subject) cleanly and correctly,
including the bullet that was truncated mid-word. It changes no code, so findings 1, 2, 3,
6, 7 and 8 are untouched, and the PR body is still the unfilled template, now across six
heads. The suite is still green at this head: 599 passed. Nothing new is wrong.
Status of the previous findings (ebuild-111-e7139935.md)
git diff --stat e71399354475ad577683e0f3c8fb7aa7be950951 afb768fb3351e3b2dc9015869a3eda209487437a
produces no output. That single fact settles seven of the eight rows below; each is
re-confirmed at this head by the line citation given.
| Prev # | Sev | Status | Evidence at afb768fb |
|---|---|---|---|
| 1 | Medium | Untouched | index_sync.py:337 — seen_recipe_stems.add(pkg_name) is still inside if recipe_dict["url"]: (:332) and still after parse_recipe succeeds. An index entry that loses its url or stops validating is still written to packages.json and still has its cached recipe unlinked at :346. |
| 2 | Medium | Untouched | index_sync.py:349 is still logger.debug("Pruned stale cached recipe: %s", …). No module under ebuild/ calls logging.basicConfig, so the deletion is still silent at the CLI. |
| 3 | Medium | Untouched | index_sync.py:345-346 still globs *.yaml + *.yml and matches on existing_file.stem, so a stale delta.yml still survives a sync that writes delta.yaml, and still outranks it. |
| 4 | Medium | Untouched — sixth head | pr.json body is 1,404 bytes of unmodified template: empty Summary, - placeholders under Changes, every Testing and Pre-Submission box unchecked, no linked issue. additions: 1477, deletions: 136, changedFiles: 17. |
| 5 | Low | Resolved in afb768f |
See below. |
| 6 | Low | Untouched | tail -c 1 CHANGELOG.md | xxd → 00000000: 30 — last byte is 0, still no trailing newline. |
| 7 | Low | Untouched | grep -rin "prune|stale" docs/ CHANGELOG.md returns three pre-existing hits about stale object files and a stale core/ tree; nothing documents that update-index now deletes cached recipes, and nothing records the source-ranked precedence change. |
| 8 | Low | Untouched | index_sync.py:50 is still class SyncResult(Tuple[int, str]). mypy ebuild/packages/registry.py ebuild/packages/index_sync.py --ignore-missing-imports --no-strict-optional → 1 error, index_sync.py:53. commands.py:1738's unused count binding is still there. |
Finding 5 — resolved, and worth showing because it is exactly right. The old subject was
268 characters with the message's own tail spliced in front of it, and the body stopped
mid-word at and change no:
old (e713993) subject:
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
old body last line:
- Surface index SHA-256 digest and change no
new (afb768f) subject: 94 chars
fix(packages): enforce source-ranked precedence, prune stale recipes, and surface index digest
new body last bullets:
- 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.
Conventional Commits 1.0.0 as .github/STANDARDS.md:56 requires, the splice is gone, and the
truncated bullet is restored to what it evidently meant. No CI check would have caught this —
there is no commitlint or PR-title workflow in .github/ — so it was fixed by reading, which
is the right outcome.
Findings
None new. afb768f touches no file. The six open findings above are carried from
ebuild-111-e7139935.md and are not restated here; that report holds the reproductions,
severities and recommended fixes, and they all still apply verbatim at this head.
For convenience, the order to work them in is unchanged:
- Findings 1 and 3 together — one edit in
index_sync.py. Build the keep-set from
valid_entriesbefore 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/*.ymlwhose.nameis not inkeep. These are the only two that
should block. - Finding 2 — count the prunes and print the count from
update_index, beside the digest
line the same commit already added. - Finding 4 — the PR body.
python -m pytest tests/ -q→ 599 passed atafb768fbis the
one sentence that has been missing for six heads, and it is a sentence the author can write
truthfully today. - Findings 6, 7 — one commit: restore the
CHANGELOG.mdnewline, and document the prune
and the source-ranked precedence change. - Finding 8 — small refactor, follow-up is fine.
Verified by running (at afb768fb)
git diff --stat e7139935 afb768fb -> empty; trees identical
git rev-list --count afb768fb..origin/master -> 0 (origin/master e5d8052; 7 ahead)
env: uv venv + uv pip install pytest click pyyaml ninja ruff mypy
CPython 3.12.14, Linux x86-64
python -m pytest tests/ -q -> 599 passed, 0 failed, 4.34s
mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional
-> 1 error, index_sync.py:53 (finding 8)
ruff check <7 PR-touched files> --select=E,F,W --ignore=E501
-> 11 errors, all F401/F841 (unchanged)
(ci.yml:53 and :65 make both ruff and mypy continue-on-error: true, so neither gates)
tail -c 1 CHANGELOG.md | xxd -> 30 (finding 6)
grep -rin "prune|stale" docs/ CHANGELOG.md -> 3 unrelated pre-existing hits (finding 7)
CI: actions/runs?head_sha=afb768fb -> 4 runs (CI — ebuild, CodeQL, Simulation Test,
Claude Code Review), every conclusion "action_required"
commits/afb768fb/status -> pending, total_count 0 · check-runs -> total_count 0
The local ebuild clone was left untouched: the sync step reported it dirty (4 files,
branch v90) and skipped it. I worked in 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.
The prune probes, precedence probes and CLI drives from the previous review were not
re-run. The tree is byte-identical to the one they were run against, which is stronger
evidence than a repeat run, and the line citations above confirm the code is where those
reproductions found it.
Architecture conformance
Unchanged from ebuild-111-e7139935.md, and nothing in this head can have changed it: the
amend altered a commit message, not a byte of source. §5.1 holds, §21 tier placement holds,
§21.1 is not triggered, and §9.2's reproducibility rule on the build path remains satisfied by
the source-ranked get() at registry.py:153-158. docs/dependency-management.md:325 is
still a true statement.
The design gap this PR surfaced — §11 defines the registry and never says what a client may
hold locally, so a synchronisation is free to delete the definitions an offline build depends
on — was appended as a proposal on 2026-09-04 and stands. No new proposal from this head;
an amended commit message reveals nothing new about the master design.
Blocked / stale
Blocked, and this is now the defining fact about this PR. Six consecutive heads —
77f1d9f2, 7e76e056, 005a69ad, e7139935, afb768fb and the one before them — have been
reviewed with zero CI executions between them. Every workflow run on this branch, on every
head, has conclusion: action_required: the first-time-fork-contributor approval gate, which
is not the author's to clear. commits/afb768fb/status is pending with total_count: 0;
check-runs is total_count: 0.
The author is not stalled — they have responded to four consecutive reviews, and this head
answers the last one's finding 5 within a day. mergeStateStatus: BLOCKED is
reviewDecision: REVIEW_REQUIRED, not a red check, and the branch is MERGEABLE with 0
commits behind origin/master.
What unblocks it: a maintainer approving the four action_required workflow runs. Until
then the Windows and macOS matrix legs of CI — ebuild, CodeQL, the Simulation Test job and
the coverage gate have said nothing about 1,477 added lines. I have run the suite, ruff and
mypy in a sandbox on Linux/CPython 3.12 and reported the numbers above; that is a reviewer's
machine, not the project's pipeline.
Not checked
- No CI has run on this head, or on any of the six. The
windows-2022and macOS matrix
legs, CodeQL, the Simulation Test job and the--cov-fail-undercoverage step are all
NOT RUN and their results are unknown. - The previous review's prune, precedence and CLI reproductions were not re-executed. I
relied on tree identity plus line-level confirmation instead. If the identity check is
wrong, so is that inheritance — the command and its empty output are quoted above so it can
be re-run. pytest-covandpytest-benchmarkwere not installed, sotests/performance/ran
without the benchmark plugin and CI's--cov=ebuild --cov-report=xmlinvocation was not
reproduced.- ruff 0.16.6 and the current mypy, resolved fresh; CI installs both unpinned, so its
versions will differ. - No real index was fetched and no package was built end to end.
DEFAULT_INDEX_URLis
still""and no deployed index exists. - The five shipped recipe checksums were not re-verified. They were downloaded and hashed
at head7e76e056and matched;recipes/is unchanged since, so that result carries, but
it was not re-run. - Whether the amend was intended to also address anything else. The commit message is the
only change; if the author meant to include the finding-1/3 fix in the same amend and it did
not make it into the push, that would explain the shape of this head, and I cannot tell from
here.
Automated architecture review of afb768fb3351 — 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.
…ng, typing, and docs feat: implement on-demand repository cloning and remote package index synchronization
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#111 "feat: implement package management system with recipe support, registry…"
head: 9ad48e1 author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED
Verdict: Follow-up to the review of afb768fb. One commit, 9ad48e1, and it is the
substantive one this PR has been waiting for. Findings 1, 2, 3 and 7 are resolved and I
verified each by running the previous review's own reproductions — the prune now keys its
keep-set on the index entries rather than on the writes it managed this run, and on filename
rather than stem, so a package the index still names keeps its recipe and a stale .yml
shadow is finally removed. The prune count reaches the user. The docs and CHANGELOG now
describe both the precedence rule and the prune. The suite is green at 601. Finding 8 is
half-done: SyncResult is a NamedTuple now, but mypy reports the identical error at the
identical line, and the conversion silently changed the tuple's arity from 2 to 5 while
leaving two docstrings asserting the old shape. Finding 4 is untouched for the seventh head.
Finding 6 is untouched and TASKS.md now records it as verified, which is a worse state
than leaving it alone. One new defect: an index that returns an empty array deletes every
cached recipe.
Status of the previous findings (ebuild-111-e7139935.md, carried through afb768fb)
| Prev # | Sev | Status | Evidence at 9ad48e1e |
|---|---|---|---|
| 1 | Medium | Resolved in 9ad48e1 |
index_sync.py:300 — keep = {f"{sanitize_package_name(str(e['name']))}.yaml" for e in valid_entries}, built before the write loop; seen_recipe_stems is gone. Both reproductions re-run: (a) alpha synced with a url then re-synced with url removed → recipes/ is still ['alpha.yaml'], pruned=0, packages.json still ['alpha'] — search and build now agree; (b) beta re-synced with version: "" → Skipping invalid recipe entry beta and beta.yaml survives, pruned=0. |
| 2 | Medium | Resolved in 9ad48e1 |
index_sync.py:333, 339, 349 count the prunes onto SyncResult.pruned; commands.py:1741, 1760-1761 print it. Driven through CliRunner with EBUILD_INDEX_PATH and a hand-written my-own-recipe.yaml: output now carries [info] Pruned 1 stale cached recipe(s) between the digest line and the cache path, exit 0. |
| 3 | Medium | Resolved in 9ad48e1 |
index_sync.py:336 is if existing_file.name not in keep. Reproduction re-run: pre-existing delta.yml at 9.9.9 + sync writing delta.yaml at 2.0.0 → recipes/ is ['delta.yaml'], pruned=1. The stale-outranks-fresh path the prune was added to close is closed. |
| 4 | Medium | Untouched — seventh head | gh pr view 111 --json body → 1,404 bytes, byte-identical to the template. Summary empty, - placeholders under Changes, 19 unchecked boxes and 0 checked, no linked issue. additions: 1538, deletions: 136, changedFiles: 18. See finding 2 below — TASKS.md:21 now claims this one was addressed. |
| 5 | Low | Resolved in afb768f |
Subject at this head is 76 chars and Conventional-Commits clean. See finding 5 for what the body now says. |
| 6 | Low | Untouched, and now claimed done | tail -c 1 CHANGELOG.md | xxd → 00000000: 30. Last byte is still the 0 of [0.1.0]: …/tag/v0.1.0; still no trailing newline. Folded into finding 2 below because it is no longer just an omission. |
| 7 | Low | Resolved in 9ad48e1 |
docs/dependency-management.md:325-332 documents the 3-tier precedence and states that update-index prunes cached .yaml/.yml recipes absent from the updated index. CHANGELOG.md:16 and :18 add Unreleased entries for source-ranked precedence and for the prune, including the surfaced count. Both of the behaviours the last review said were undocumented are now documented. |
| 8 | Low | Partially addressed | SyncResult is a NamedTuple (index_sync.py:50-57) and the unused count binding at commands.py:1738 is gone (msg = res.message). But mypy ebuild/packages/registry.py ebuild/packages/index_sync.py --ignore-missing-imports --no-strict-optional still returns 1 error at index_sync.py:53 — the same error, the same line: the count field still shadows tuple.count, and res.count(x) still raises TypeError: 'int' object is not callable. See findings 3 and 4 for the two things the conversion introduced. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | ebuild/packages/index_sync.py:300 with :333-341 |
An index that returns an empty array deletes every cached recipe. keep is derived from valid_entries with no floor, so when valid_entries is empty the prune loop matches every *.yaml/*.yml in the cache. Reproduced twice on this head: seed alpha.yaml + bravo.yaml, then re-sync against [] → recipes/ is [], count=0, pruned=2, exit 0; same result when the payload is a list whose every element is unparseable ([{"nope":1},"str",5]), because those are filtered out at :258-270 and the prune sees the same empty keep-set. packages.json is overwritten with [] in the same pass at :274-277. This is the whole offline cache, and the trigger is a well-formed HTTP 200 — an empty bucket, a partially-deployed index, a CDN error page that happens to be [], or a substituted document, which the PR's own notice at docs/dependency-management.md:320-321 says is possible because the index is unauthenticated. Not introduced by 9ad48e1 — the prune has behaved this way since e713993 — but it is this PR's code, it is the residue of finding 1 that the keep-set fix does not reach, and it is the case where the consequence is total. Rated Medium rather than higher for two reasons stated plainly: the removal is now reported (Pruned N stale cached recipe(s)), and DEFAULT_INDEX_URL is still "" with no deployed index, so today it requires an explicit --url. Neither will be true once an index ships. |
Give the prune a floor. Skip it and warn when the sync produced nothing to keep: if not valid_entries: logger.warning("Index contained no usable entries; keeping %d cached recipes", …) before the prune block, and leave packages.json alone on that path too — an index with zero valid entries is far more likely to be a delivery fault than a registry that genuinely retired every package. Add the test: seed two recipes, sync [], assert both survive and pruned == 0. |
| 2 | Medium | TASKS.md:21 |
TASKS.md T-003 records verification that was not performed, for two of the seven findings it claims to address. The row is Address PR #111 findings 1, 2, 3, 4, 6, 7, 8 with Evidence: "Unit tests in tests/unit/test_index_sync.py verify keep-set filename matching, .yml pruning, lack-of-URL recipe preservation, CLI prune reporting, and trailing newline in CHANGELOG.md." The first four are true and I confirmed each by running them. The fifth is not: grep -rn CHANGELOG tests/ returns nothing — no test in the repository reads CHANGELOG.md — and the file still has no trailing newline at this head (tail -c 1 → 0x30). The row also names finding 4, the PR body, which is unchanged at 1,404 template bytes; no unit test could have addressed it and none claims to. TASKS.md is this repo's evidence table, with a column literally headed "Evidence", so a false entry in it is the failure mode .ai/reviewer.md singles out — an unsupported "verified" is itself the finding — and it is a step down from finding 6's previous state, where the newline was merely missing rather than reported as fixed. This is not a judgement about intent; it reads like a checklist written from the plan rather than from the run. |
Two edits. Add the newline to CHANGELOG.md — that makes the claim true and costs one byte. Then correct the row to name only what a test covers: drop 4 from the ID list and drop the CHANGELOG clause, or keep it and cite the check that proves it. If a claim in TASKS.md is meant to be machine-checkable, python -m pytest tests/ -q → 601 passed at 9ad48e1e is the sentence that belongs in that column. |
| 3 | Low | ebuild/packages/index_sync.py:51 and :155 |
The NamedTuple conversion changed SyncResult's tuple arity from 2 to 5, and both docstrings still describe the old shape. SyncResult(Tuple[int, str]) built a genuine 2-tuple in __new__; NamedTuple with five fields does not. Probed on this head: len(SyncResult(3, "msg")) → 5, tuple(r) → (3, 'msg', False, None, 0), and count, msg = r now raises ValueError: too many values to unpack (expected 2). Line 51 still reads "preserving tuple unpacking (count, message)" and :155 still documents the return as "SyncResult of (package_count, status_message, is_fallback)". Both are now false, and :51 is false in the specific way that invites the next contributor to write the statement that raises. Nothing in-tree breaks — commands.py:1738 was migrated to res.message in this same commit and no test unpacks it, which is why the suite is green — and SyncResult has no consumer outside ebuild, which is why this is Low rather than an API finding. But the diff changes a public struct's shape and says nothing about it, and per brief item 8 silence about that is the finding. |
Fix the two docstrings in the same breath as the code they describe: :51 → "Result of index synchronization." and :155 → "SyncResult with count, message, is_fallback, sha256 and pruned." If 2-tuple unpacking is still wanted anywhere, say so and keep a __iter__; if not — and nothing needs it — the docstring is the only thing still promising it. |
| 4 | Low | ebuild/packages/index_sync.py:22 |
The commit leaves typing.Tuple imported and unused, a new lint error. ruff check … --select=E,F,W --ignore=E501 → index_sync.py:22:59: F401 'typing.Tuple' imported but unused; grep -n Tuple finds only the import and the NamedTuple on :50. The PR-touched-file total is 11 both before and after, which conceals the change: 9ad48e1 removed the F841 on commands.py:1738 (correctly, finding 8) and added this one. Neither shows up in CI — .github/workflows/ci.yml:53 and :65 are continue-on-error: true for ruff and mypy — so the only thing that will catch it is a reader. |
Drop Tuple from the import on :22. ruff check ebuild/packages/index_sync.py --select=F401 --fix does it. |
| 5 | Low | ebuild/packages/index_sync.py:336 with :300 |
Finding 1's fix covers .yaml and not .yml: a cached .yml for a package the index still names is deleted with nothing written in its place. keep only ever holds {name}.yaml, and the write loop only ever emits .yaml, but the prune globs both. Reproduced: cache holds eps.yml, index still lists eps but its entry has no url → nothing is written and eps.yml is unlinked, pruned=1, cache empty. That is exactly the loss finding 1 described, reached through the other extension. It matters because PackageRegistry.scan reads both (registry.py:112 globs *.yaml, :119 globs *.yml), so a .yml there is a real definition, not a stray file. Low, not Medium, because update-index itself never writes .yml — such a file is hand-placed or left by another tool, which is the narrow case finding 1's fix legitimately deprioritised. |
Make the keep-set carry both extensions for entries the index still names: keep = {f"{n}.yaml", f"{n}.yml"} for each n, and prune a .yml only when the sync actually wrote the .yaml that supersedes it. One line, and it closes the last corner of finding 1. |
| 6 | Low | commit 9ad48e1 body |
The commit body describes a different change from the subject. Subject is correct and Conventional-Commits clean — fix(packages): resolve PR #111 review findings on pruning, typing, and docs, 76 characters — but the entire body is the single line feat: implement on-demand repository cloning and remote package index synchronization, which is this PR's original headline, not what 9ad48e1 did. So the one commit in this range explains itself as neither the four resolutions it contains nor the two it left open. Same family as finding 5 in the earlier chain and much less severe; noted because git log is where the next reader will look for why the prune's keep-set changed shape. |
Amend the body to the four bullets this commit actually earned: keep-set from index entries, filename-keyed pruning, pruned count surfaced through SyncResult and the CLI, SyncResult converted to NamedTuple. |
Verified by running (at 9ad48e1e)
env: uv venv + uv pip install pytest click pyyaml ninja ruff mypy
CPython 3.12.14, Linux x86-64 · ruff 0.16.6 · mypy 2.3.1
git log --oneline afb768fb..9ad48e1e -> 1 commit (9ad48e1)
git diff --stat afb768fb..9ad48e1e -> 6 files, +83 -22
git rev-list --count 9ad48e1e..origin/master -> 0 (origin/master e5d8052)
python -m pytest tests/ -q -> 601 passed, 0 failed, 3.73s (599 at afb768fb; +2 new)
mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional
-> 1 error, index_sync.py:53 (finding 8 unchanged)
ruff check <7 PR-touched files> --select=E,F,W --ignore=E501
-> 11 errors at 9ad48e1e, 11 at afb768fb, but not the
same 11: -F841 commands.py:1738, +F401 index_sync.py:22
(ci.yml:53 and :65 keep ruff and mypy continue-on-error: true, so neither gates)
prune probes, --force on every sync, mocked urlopen — all four re-run from the last review:
alpha(url) then alpha(no url) -> recipes ['alpha.yaml'] · pruned 0 · packages.json ['alpha']
(prev finding 1 FIXED)
beta(url) then beta(version:"") -> "Skipping invalid recipe entry beta", beta.yaml SURVIVES
pruned 0 (prev finding 1 FIXED)
handwritten my-own-recipe.yaml, sync gamma -> deleted, and CLI now prints
"[info] Pruned 1 stale cached recipe(s)", exit 0
(prev finding 2 FIXED)
stale delta.yml 9.9.9 + fresh delta.yaml 2.0.0 -> recipes ['delta.yaml'] · pruned 1
(prev finding 3 FIXED)
new probes this run:
alpha+bravo cached, re-sync against [] -> recipes [] · pruned 2 · exit 0 (finding 1)
alpha+bravo cached, re-sync [{"nope":1},"str",5] -> recipes [] · pruned 2 · exit 0 (finding 1)
cached eps.yml, index still lists eps w/o url -> eps.yml deleted · pruned 1 (finding 5)
len(SyncResult(3,"msg")) -> 5 · tuple(r) -> (3,'msg',False,None,0)
"count, msg = r" -> ValueError: too many values to unpack (expected 2) (finding 3)
r.count("msg") -> TypeError: 'int' object is not callable (prev finding 8 residue)
tail -c 1 CHANGELOG.md | xxd -> 30 (prev finding 6 OPEN)
grep -rn CHANGELOG tests/ -> no hits (finding 2)
grep -n "prune|stale" docs/dependency-management.md CHANGELOG.md
-> docs :325-332, CHANGELOG :16, :18 (prev finding 7 FIXED)
gh pr view 111 --json body -> 1404 bytes, 19 "- [ ]", 0 "- [x]" (prev finding 4 OPEN)
CI: actions/runs?head_sha=9ad48e1e -> 3 runs (CI — ebuild, CodeQL, Simulation Test),
every conclusion "action_required"
commits/9ad48e1e/status -> {"state":"pending","total_count":0} · check-runs -> 0
The local ebuild clone was left untouched: the sync step reported it dirty (TASKS.md,
ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py modified,
smart-sensor/ untracked, branch v90, 100 behind origin/master) and skipped it. I worked in 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.
Architecture conformance
Master design §9.1–9.2 (eBuild engine and SDK design rules), §10/§10.1 (component model and
contract), §11/§11.1 (Registry), §21 tiers and §21.1 split policy.
Conforms, and the §9.2 position is stronger at this head than at any previous one.
Tier placement is unchanged and nothing in this diff points up a tier. The whole commit is
internal to ebuild plus its own docs; the only import it adds is typing.NamedTuple. 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 it.
§21.1 is not triggered; no repository is proposed.
§9.2's "reproducible lockfiles/manifests for production builds" was satisfied on the build
path at e713993 and remains so — registry.py:153-158's source-ranked get() is untouched
by this commit. What 9ad48e1 adds is that the rule is now written down where a user will
find it: docs/dependency-management.md:325-330 states the three tiers in order, and
CHANGELOG.md:16 records the change. §29's documentation architecture asks for exactly this
and the previous four reviews could not point at it.
The design gap this PR surfaced — §11 defines the registry and never says what a client may
hold locally — was appended as a proposal on 2026-09-04. 9ad48e1 implements all three of
that proposal's clauses: a per-entry failure no longer removes the cached definition, the
removal is reported, and the cache directory's ownership is now documented at
docs/dependency-management.md:332. The proposal stands unmerged; this head is the
implementation arriving ahead of the design text, which argues for merging it.
One clause of the 2026-09-03 precedence proposal is still unimplemented — "Tooling must be
able to report, for every resolved component, which source its definition came from." The
rank is known at resolution time and discarded, so ebuild build still cannot say which of
three documents supplied the url it fetched. Not scored as a finding; the proposal that would
require it has not been accepted.
The lockfile remains the missing backstop and remains pre-existing on master:
Lockfile.load / is_locked / get_locked_entry / get_locked_version / locked_packages
still have zero call sites in ebuild/, 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.
Finding 1 exposes a gap in the 2026-09-04 proposal's own text — it authorises removal whenever
"the index it synchronised against no longer names that component", and an empty index names
nothing, so a total wipe is permitted by the rule as drafted. An addendum is appended.
Proposed changes
In order. Only finding 1 should block:
- Finding 1. Refuse to prune when
valid_entriesis empty, warn, and leave
packages.jsonalone on that path. Three lines inindex_sync.pybefore:333, plus one
test: seed two recipes, sync[], assert both survive andpruned == 0. - Finding 2 and prior finding 6 together — one commit. Add the trailing newline to
CHANGELOG.md, then correctTASKS.md:21to claim only what ran. This is the cheapest
item on the list and it is currently the one that misleads. - Prior finding 4. Fill in the PR body.
python -m pytest tests/ -q→ 601 passed at
9ad48e1eis the sentence that has been missing for seven heads, and this head is the
one where it is most worth saying: four review findings resolved, each with a test. - Findings 3, 4, 5, 6 — one tidy-up commit: the two stale docstrings, the unused
Tuple
import, the.ymlcorner of the keep-set, and the commit body. - Ask a maintainer to approve the three
action_requiredworkflow runs. Seven consecutive
heads have now been reviewed with no CI execution at all.
No fix PR opened. Every finding is on this PR's branch, which the brief puts out of bounds,
and finding 1 changes which files update-index deletes — not the small, provable class an
unattended agent may open unreviewed.
Blocked / stale
Blocked, unchanged in cause and now seven heads deep. 77f1d9f2, 7e76e056, 005a69ad,
e7139935, afb768fb, 9ad48e1e and the head before them have all been reviewed with zero
CI executions between them. Every workflow run on this branch, on every head, has
conclusion: action_required — the first-time-fork-contributor approval gate, which is not
the author's to clear. commits/9ad48e1e/status is pending with total_count: 0;
check-runs is total_count: 0.
The author is not stalled and this head is the clearest evidence of that in the PR's history:
four findings closed with reproductions that hold up, two of them with new regression tests
(test_index_sync.py:378-397 and :416-438). mergeStateStatus: BLOCKED is
reviewDecision: REVIEW_REQUIRED, not a red check; the branch is MERGEABLE and 0 commits
behind origin/master.
What unblocks it: a maintainer approving the three action_required workflow runs. Until
then the Windows and macOS matrix legs of CI — ebuild, CodeQL, the Simulation Test job and
the coverage step have said nothing about 1,538 added lines. I ran the suite, ruff and mypy in
a sandbox on Linux/CPython 3.12 and reported the numbers above; that is a reviewer's machine,
not the project's pipeline.
Not checked
- No CI has run on this head, or on any of the seven. The
windows-2022and macOS matrix
legs, CodeQL, the Simulation Test job and the--cov-fail-undercoverage step are all
NOT RUN and their results are unknown. pytest-covandpytest-benchmarkwere not installed, sotests/performance/ran
without the benchmark plugin and CI's--cov=ebuild --cov-report=xmlinvocation was not
reproduced.- ruff 0.16.6 and mypy 2.3.1, resolved fresh; CI installs both unpinned, so its versions
will differ. Both steps arecontinue-on-error: true, so neither gates either way. - No real index was fetched and no package was built end to end.
DEFAULT_INDEX_URLis
still""and no deployed index exists, so every sync path — including all seven prune
probes — ran against a mockedurllib.request.urlopen.ebuild buildwas not run this
session; the precedence result is inherited frome7139935, where it was demonstrated at
_find_recipe_dirs+create_registry+PackageResolver.resolve, andregistry.pyis
unchanged since. - Finding 1's consequence for a real air-gapped user is inferred, not observed. The wipe
is reproduced; that a developer then cannot build is read from the code path, not run. - The five shipped recipe checksums were not re-verified. Downloaded and hashed at head
7e76e056and matched;recipes/is unchanged since, so that result carries, but it was
not re-run. index-meta.jsonstill has no schema validation andupdate_index's two reads of it
wrapjson.loadin a bareexcept Exception(commands.py:1742-1758). Absent-file and
well-formed cases were exercised; truncated and hand-edited shapes were read, not run.- Whether
TASKS.md:21was written before or after the CHANGELOG edit was attempted. The
claim is false either way; which it is, I cannot tell from here, and finding 2 is written
about the record, not the intent.
Automated architecture review of 9ad48e1e5e1a — 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.
… index floor, yml retention, and typing - Refuse to prune cached recipes and preserve packages.json when index sync produces no valid entries - Preserve cached .yml recipes for index entries without download URLs, pruning .yml only when superseded by fresh .yaml - Update SyncResult and sync() docstrings to reflect 5-field NamedTuple and fix mypy assignment error - Drop unused imports in index_sync.py and test_index_sync.py - Add trailing newline to CHANGELOG.md and record machine-verified test suite evidence in TASKS.md - Add comprehensive regression tests in test_index_sync.py
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#111 "feat: implement package management system with recipe support, registry…"
head: 21523b1 author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED
Verdict: Follow-up to the review of 9ad48e1e. One commit, 21523b1, +125 −18 across four
files. Five of the six findings are resolved and I verified each by re-running the previous
review's own reproductions: the empty-index wipe, the .yml corner of the keep-set, both stale
docstrings, the unused Tuple import and the mismatched commit body. The CHANGELOG.md newline
that had been open since e713993 is added, and there is now a test that asserts it. The suite is
green at 605 (601 at 9ad48e1e, +4). What is left is that finding 2 — a TASKS.md row claiming
verification that was not performed — was answered by replacing one unverified claim with another:
the row now asserts "Static analysis with mypy and ruff passes with 0 errors", and on the PR's
changed files ruff still reports 3 errors while mypy's 0 is produced by a # type: ignore this
commit added over the error rather than by fixing it. One new defect: the empty-index floor
correctly keeps the cached data, then records the discarded document's digest and sync timestamp
anyway, so the next update-index short-circuits for 24 hours and reports the empty document's
digest as the digest of a cache that describes something else. The PR body is still the unfilled
template, now across eight heads.
Status of the previous findings (ebuild-111-9ad48e1e.md)
| Prev # | Sev | Status | Evidence at 21523b18 |
|---|---|---|---|
| 1 | Medium | Resolved in 21523b1 |
index_sync.py:335-345 — if not valid_entries: warns and skips the prune entirely, and :273-278 guards the packages.json write on the same condition. Both reproductions re-run: seed alpha.yaml + bravo.yaml, re-sync [] → both survive, pruned=0, count=0, packages.json still holds ['alpha','bravo']; re-sync [{"nope":1},"str",5] → identical. The warning reaches stderr (Index contained no usable entries; keeping 2 cached recipes). Two new tests cover both. See finding 2 for what the floor does not cover. |
| 2 | Medium | Partially addressed — one unverified claim replaced by another | The false clause is gone in the right way: CHANGELOG.md:147 now ends with \n (tail -c 1 → 0x0a), and test_changelog_trailing_newline (test_index_sync.py:522-528) asserts it, so the row's CHANGELOG claim is now backed by a test. Finding 4 is dropped from the ID list. But the row gained "Static analysis with mypy and ruff passes with 0 errors", which does not hold — see finding 1. |
| 3 | Low | Resolved in 21523b1 |
index_sync.py:51 → """Result of index synchronization."""; :155 → "SyncResult with count, message, is_fallback, sha256, and pruned.". Both now describe the 5-field shape. |
| 4 | Low | Resolved in 21523b1 |
typing.Tuple dropped from :22; PackageRecipe dropped from :26 as well. ruff check ebuild/packages/index_sync.py --select=F401 → clean. |
| 5 | Low | Resolved in 21523b1 |
:347-352 — keep now carries {name}.yml for every index entry whose .yaml was not written this run (written_yaml_stems, :300, :330). Reproduced: cache eps.yml + delta.yml + zeta.yml, index names eps (no url) and delta (with url) → eps.yml survives, delta.yml pruned and replaced by delta.yaml, zeta.yml pruned, pruned=2. The parse-failure path behaves the same: index entry beta with version: "" → nothing written, both beta.yaml and beta.yml kept, pruned=0. |
| 6 | Low | Resolved in 21523b1 |
git log -1 --format=%s → 94 chars, fix(packages): resolve PR #111 review findings on empty index floor, yml retention, and typing; the body is six bullets that each name something this commit did. The previous head's spliced headline is gone. |
| (carried) 4 | Medium | Untouched — eighth head | gh pr view 111 --json body → 1,424 bytes, still the template. 19 - [ ], 0 - [x], empty Summary, - placeholders under Changes. additions: 1644, deletions: 135, changedFiles: 18. See finding 3. |
| (carried) 6 | Low | Resolved in 21523b1 |
The CHANGELOG.md trailing newline is restored. Open since e713993. |
| (carried) 8 | Low | Suppressed, not fixed | mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional → Success: no issues found (1 error at 9ad48e1e), but only because :51 is now count: int # type: ignore[assignment]. SyncResult(3,"msg").count("msg") still raises TypeError: 'int' object is not callable. See finding 4. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | TASKS.md:21 |
T-003's evidence column again states a check result that does not hold, for the second consecutive head. The row now reads "Unit tests in tests/unit/test_index_sync.py (22 passed) … Static analysis with mypy and ruff passes with 0 errors." The test half is true and I confirmed it: python -m pytest tests/unit/test_index_sync.py -q → 22 passed, and each named behaviour has a test that exercises it. The static-analysis half is not true as written, and it is unscoped, so it reads as a statement about the change. Ran both: ruff check <7 PR-touched .py files> --select=E,F,W --ignore=E501 — the CI invocation restricted to what this PR touches — reports 3 errors, all F401, all in files this PR adds or edits: ebuild/packages/repository.py:18 (get_default_index_dir imported but unused), tests/unit/test_package_search.py:7 (pathlib.Path) and :12 (PackageInfo). CI's actual full-repo ruff check . goes 380 on origin/master → 383 at this head, and the +3 is exactly these. mypy ebuild/ --ignore-missing-imports --no-strict-optional → 9 errors in 4 files; it is 0 only when narrowed to the two modules this commit edited, and only via the suppression in finding 4. The commit's own cleanup pass ("Drop unused imports in index_sync.py and test_index_sync.py") did exactly those two files and the row generalises it to all of them. .ai/reviewer.md is explicit that an unsupported claim is itself the finding, and TASKS.md is this repo's evidence table — the column is headed "Evidence". Severity is held at Medium rather than lowered because this is a repeat: the previous head's row claimed a CHANGELOG test that did not exist, and the correction introduced a new claim in the same cell. |
Two options, both one edit. Either delete the three F401s — ruff check ebuild/packages/repository.py tests/unit/test_package_search.py --select=F401 --fix does it and makes the sentence true — or scope the claim to what was run: "ruff check ebuild/packages/index_sync.py tests/unit/test_index_sync.py --select=E,F,W --ignore=E501 → 0 errors; mypy ebuild/packages/{registry,index_sync}.py → 0 errors." The first is better, and it is three deletions. Then add the sentence that is machine-checkable and currently missing: python -m pytest tests/ -q → 605 passed at 21523b18. |
| 2 | Medium | ebuild/packages/index_sync.py:273-278 and :279-297 |
The empty-index floor declines to apply the index and then records it as applied. :273 correctly skips the packages.json write when valid_entries is empty, and :335 correctly skips the prune. But :279-297 runs unconditionally: the SHA-256 of the discarded document is written to packages.json.sha256, and index-meta.json records that document's sha256 and a fresh synced_at. Three consequences, all reproduced on this head with --force and a mocked urlopen. (a) After a good sync of one package followed by an empty one, index-meta.json.sha256 is 4f53cda1… — sha256("[]") — while packages.json still holds the previous entry; the sidecar is written in sha256sum format naming packages.json and no longer describes anything on disk. (b) :187-192 keys freshness on synced_at and the origin URL, both of which the empty sync just refreshed, so the next update-index without --force does not fetch at all for 24 hours (CACHE_TTL_SECONDS = 86400): driven through CliRunner, urlopen called: False, output [ok] Cache is up-to-date (synced recently). Use --force to re-download. (1 packages) followed by [info] Index SHA-256 digest: 4f53cda1… — the tool reports a cache describing one package and, on the next line, the digest of the empty document. A transient empty publish therefore freezes re-synchronisation for a day rather than being retried. (c) The empty sync itself still exits 0 under [ok] Successfully synchronized 0 packages from remote index, and because the recorded digest changed, update_index prints [info] Index updated (previous digest: …) for a synchronisation that updated nothing. The standing 2026-09-05 proposal this commit implements asks for three things — remove no cached definition, do not overwrite the cached index document, and "report the condition as a failure rather than a success"; the first two are delivered and the third is not. Separately, on a cold cache the empty path writes index-meta.json and packages.json.sha256 but no packages.json at all, leaving a cache directory whose only content is metadata for a document that was thrown away. |
Move the digest and metadata writes inside the same if valid_entries: guard as the packages.json write, so a synchronisation that changes nothing records nothing, and the next update-index retries instead of short-circuiting. Then return the empty case the way the fallback case is already returned: SyncResult(..., is_fallback=True, ...) with a message naming the condition — commands.py:1755-1760 already turns that into log.warning plus SystemExit(1), so the CLI needs no change. Extend test_index_sync_empty_index_preserves_cache_and_warns with two assertions: index-meta.json's sha256 and synced_at are unchanged, and a following non-forced sync() calls urlopen. |
| 3 | Medium | pr body | The PR body is still the unfilled template — eighth head, 1,644 added lines, no movement. 1,424 bytes, byte-identical to .github/PULL_REQUEST_TEMPLATE.md except for a truncated title fragment prepended to the first line (…ry, and build synchronization.). Empty Summary, - placeholders under Changes, 19 unchecked boxes, 0 checked, no linked issue. This is the only finding with zero movement across all eight reviews, and it is the cheapest one on the list: the suite is green at 605 on this head and the author could simply say so. The brief treats an unsupported "verified" as itself the finding; a change of this size asserting nothing is the same gap approached from the other side. (The garbled type labels eat/ix/efactor/est/uild are still .github/PULL_REQUEST_TEMPLATE.md's corruption on origin/master — literal control characters where \feat, \fix etc. were written — not the author's doing. Unchanged across all eight reviews.) |
Write Summary and Changes from git log on this branch, and put the verification in Testing: python -m pytest tests/ -q → 605 passed at 21523b18. Then name what it does not cover — no CI has executed on any head, no real index has been fetched, and ebuild build has never been run end to end against a remote recipe. Naming those is faster than having a reviewer find them, and all three are honest gaps rather than mistakes. |
| 4 | Low | ebuild/packages/index_sync.py:51 |
Prior finding 8 was closed by silencing the diagnostic, not by fixing what it reported. count: int # type: ignore[assignment] makes mypy report Success: no issues found on the two modules, where 9ad48e1e reported one error at this line. The condition the error described is unchanged: count still shadows tuple.count, and SyncResult(3, "msg").count("msg") still raises TypeError: 'int' object is not callable — verified on this head. The suppression is also silent about why, so the next reader sees a clean type check over a field that has an unreachable inherited method. Per the brief a suppressed check is a finding regardless of the reason given; Low because SyncResult has no consumer outside ebuild/ and nothing in-tree calls .count(). It becomes finding 1's problem when TASKS.md presents the resulting 0 as evidence. |
Rename the field to package_count (or synced) and drop the type: ignore; there are five call sites, all in index_sync.py and commands.py, and commands.py:1738 already reads res.message rather than unpacking. If the name must stay, keep the ignore and put the reason next to it: # type: ignore[assignment] # shadows tuple.count; SyncResult is never used as a sequence. |
| 5 | Low | docs/dependency-management.md:332; CHANGELOG.md:18-19 |
The safety floor added by this commit is a user-visible behaviour change and neither document mentions it. Both describe the prune as unconditional — docs: "ebuild update-index automatically prunes stale cached .yaml and .yml recipes … that are absent from the updated remote index"; CHANGELOG: the same sentence. Grepped this head: no occurrence of "empty", "no usable" or "floor" in docs/ or CHANGELOG.md. So the sentence a reader relies on still describes 9ad48e1's behaviour, and the one guarantee that matters most to the air-gapped audience the same file addresses at :359 — that a bad index cannot empty their cache — is the one that is not written down. Per brief item 11, behaviour changed without the documentation changing with it. The same paragraph's closing line, "the SHA-256 digest of the downloaded index is recorded in ~/.ebuild/index/packages.json.sha256", is also the sentence finding 2 makes untrustworthy on the empty path. |
One clause on each. Docs :332: "… absent from the updated remote index. An index that yields no usable entries is treated as a delivery fault: nothing is pruned, the cached index is left in place, and the condition is reported." CHANGELOG :19: the same, one line. Both are cheaper than the code they describe and this is the commit that earned them. |
Verified by running (at 21523b18)
env: uv venv + uv pip install pytest click pyyaml ninja ruff mypy
CPython 3.12.14, Linux x86-64 · ruff 0.16.6 · mypy 2.3.1
git log --oneline 9ad48e1e..21523b18 -> 1 commit (21523b1)
git diff --stat 9ad48e1e 21523b18 -> 4 files, +125 -18
git rev-list --count 21523b18..origin/master -> 0 (origin/master e5d8052)
git log -1 --format=%s -> 94 chars, Conventional Commits clean (prev 6 FIXED)
python -m pytest tests/ -q -> 605 passed, 0 failed, 3.75s (601 at 9ad48e1e)
python -m pytest tests/unit/test_index_sync.py -q -> 22 passed (TASKS.md's count is right)
mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional
-> Success, 0 errors (1 at 9ad48e1e; via type: ignore)
mypy ebuild/ --ignore-missing-imports --no-strict-optional -> 9 errors in 4 files
(10 at 9ad48e1e; the one removed is index_sync.py:53)
ruff check <7 PR-touched .py> --select=E,F,W --ignore=E501
-> 3 errors (11 at 9ad48e1e), all F401:
repository.py:18 get_default_index_dir
test_package_search.py:7 Path, :12 PackageInfo
ruff check . --select=E,F,W --ignore=E501 -> origin/master 380 · 21523b18 383 (the same +3)
(ci.yml:53 and :65 keep ruff and mypy continue-on-error: true, so neither gates)
empty-index floor, --force on every sync, mocked urlopen:
alpha+bravo cached, re-sync [] -> both survive · pruned 0 · count 0
packages.json still ['alpha','bravo'] (prev 1 FIXED)
alpha+bravo cached, re-sync [{"nope":1},"str",5] -> identical (prev 1 FIXED)
warning on stderr: "Index contained no usable entries; keeping 2 cached recipes"
after the empty sync: index-meta sha256 -> 4f53cda1… = sha256("[]"), synced_at advanced
packages.json.sha256 -> 4f53cda1… packages.json (describes neither) (f2)
then sync() without --force -> urlopen called: False
"[ok] Cache is up-to-date (synced recently)… (1 packages)"
"[info] Index SHA-256 digest: 4f53cda1…" (finding 2)
empty sync through CliRunner -> exit 0, "[ok] Successfully synchronized 0 packages",
"[info] Index updated (previous digest: 3501798f…)" (finding 2)
cold cache + empty index -> exit 0; files: index-meta.json,
packages.json.sha256, recipes/ — no packages.json at all (finding 2)
.yml keep-set:
cached eps.yml + delta.yml + zeta.yml, index names eps (no url) and delta (url)
-> eps.yml SURVIVES · delta.yml pruned, delta.yaml written · zeta.yml pruned · pruned 2
(prev 5 FIXED)
cached beta.yml + beta.yaml, index names beta with version:"" (parse fails)
-> "Skipping invalid recipe entry beta" · both files kept · pruned 0 (prev 5 FIXED)
len(SyncResult(3,"msg")) -> 5 · tuple(r) -> (3,'msg',False,None,0)
r.count("msg") -> TypeError: 'int' object is not callable (prev 8, finding 4)
tail -c 1 CHANGELOG.md | xxd -> 0a (prev 6 FIXED)
grep -i "empty\|no usable\|floor" docs/ CHANGELOG.md -> no hits (finding 5)
gh pr view 111 --json body -> 1424 bytes, 19 "- [ ]", 0 "- [x]" (finding 3)
CI: actions/runs?head_sha=21523b18 -> 3 runs (CI — ebuild, CodeQL, Simulation Test),
every conclusion "action_required"
commits/21523b18/status -> {"state":"pending","total_count":0} · check-runs -> 0
The local ebuild clone was left untouched: the sync step reported it dirty (4 files) and
skipped it. I worked in 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.
Architecture conformance
Master design §9.1–9.2 (eBuild engine and SDK design rules), §10/§10.1 (component model and
contract), §11/§11.1 (Registry), §21 tiers and §21.1 split policy.
Conforms. Tier placement is unchanged and §9.2's position is unchanged from 9ad48e1e.
Nothing in this diff points up a tier. The commit is internal to ebuild plus its own
CHANGELOG.md, TASKS.md and tests; it adds no import and removes two. 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 it. §21.1 is not triggered; no repository is
proposed.
§9.2's "reproducible lockfiles/manifests for production builds" was satisfied on the build path at
e713993 by the source-ranked get() at registry.py:153-158, and neither that function nor
resolver.py is touched here, so it holds. docs/dependency-management.md:325 remains a true
statement. This commit strengthens the offline half of the same rule: §9.2's "No mandatory cloud
connection" is only worth something if a network answer cannot destroy what the offline path
depends on, and the floor at :335 is what makes that so for the total-wipe case.
The design gap this head sits on is already filed. The 2026-09-05 addendum to §11 — "a
synchronisation whose index yields zero usable entries must make no destructive change" — is what
21523b1 implements, and it implements two of its three clauses. The third, "reports the
condition as a failure rather than a success", is finding 2(c). That proposal stands unmerged;
this head is the implementation arriving ahead of the design text, which argues for merging it.
What this head exposes that the drafted rule does not reach is the record rather than the data.
The rule says a refused synchronisation leaves the client "resolving exactly what it resolved
before" — which this code satisfies — and says nothing about the freshness and digest metadata the
client writes while refusing. 21523b1 writes both, so the refusal is recorded as a completed
synchronisation and suppresses the next attempt for the TTL. An addendum is appended.
Two standing items are unchanged and remain pre-existing on master, not this PR's defects:
Lockfile.load / is_locked / get_locked_entry / get_locked_version / locked_packages still
have zero call sites in ebuild/, so ebuild.lock is written every build and never read (subject
of the 2026-09-04 proposal against §9.2); and the 2026-09-03 precedence proposal's clause that
"tooling must be able to report, for every resolved component, which source its definition came
from" is still unimplemented — the rank is known at resolution time and discarded. Neither is
scored as a finding, because the proposals that would require them have not been accepted.
Proposed changes
In order. Only finding 2 should block:
- Finding 2. Move the digest and
index-meta.jsonwrites inside the existing
if valid_entries:guard, and return the empty case asis_fallback=Trueso the CLI already
in place warns and exits non-zero. Two assertions on the test this commit added. - Finding 1.
ruff check ebuild/packages/repository.py tests/unit/test_package_search.py --select=F401 --fix— three deletions — then either leave theTASKS.mdsentence as written
or scope it, and addpython -m pytest tests/ -q→ 605 passed at21523b18as the
machine-checkable evidence. - Finding 3. Fill in the PR body. Eight heads.
- Findings 4 and 5 — one tidy-up commit: rename
SyncResult.countand drop the
type: ignore, and add the floor todocs/dependency-management.md:332andCHANGELOG.md:19. - Ask a maintainer to approve the three
action_requiredworkflow runs. Eight consecutive
heads have now been reviewed with no CI execution at all.
No fix PR opened. Every finding is on this PR's branch, which the brief puts out of bounds —
including the three F401s, which would otherwise be exactly the small provable class the brief
permits.
Blocked / stale
Blocked, unchanged in cause, now eight heads deep. 77f1d9f2, 7e76e056, 005a69ad,
e7139935, afb768fb, 9ad48e1e, 21523b18 and the head before them have all been reviewed
with zero CI executions between them. Every workflow run on this branch, on every head, has
conclusion: action_required — the first-time-fork-contributor approval gate, which is not the
author's to clear. commits/21523b18/status is pending with total_count: 0; check-runs is
total_count: 0.
The PR is not stale in any other sense: MERGEABLE, 0 commits behind origin/master, and
mergeStateStatus: BLOCKED is reviewDecision: REVIEW_REQUIRED rather than a red check. The
author closed five of six findings in one commit with four new regression tests, which is the
second consecutive head where that is true.
What unblocks it: a maintainer approving the three action_required workflow runs. Until
then the Windows and macOS matrix legs of CI — ebuild, CodeQL, the Simulation Test job and the
coverage step have said nothing about 1,644 added lines. I ran the suite, ruff and mypy in a
sandbox on Linux/CPython 3.12 and reported the numbers above; that is a reviewer's machine, not
the project's pipeline.
Not checked
- No CI has run on this head, or on any of the eight. The
windows-2022and macOS matrix
legs (ci.yml:26matrixes Python 3.10/3.11/3.12), CodeQL, the Simulation Test job and the
coverage step are all NOT RUN and their results are unknown. Everything above ran on
Linux/CPython 3.12.14 only. pytest-covandpytest-benchmarkwere not installed, sotests/performance/ran without
the benchmark plugin and CI's--cov=ebuild --cov-report=xmlinvocation was not reproduced.- ruff 0.16.6 and mypy 2.3.1, resolved fresh; CI installs both unpinned, so its versions will
differ. Both steps arecontinue-on-error: true(ci.yml:53,:65), so neither gates either
way and the +3 ruff delta cannot fail the job. - No real index was fetched and no package was built end to end.
DEFAULT_INDEX_URLis still
""and no deployed index exists, so every probe — the floor, the.ymlkeep-set, the TTL
short-circuit — ran against a mockedurllib.request.urlopen.ebuild buildwas not run this
session; the precedence result is inherited frome7139935, andregistry.pyandresolver.py
are unchanged since. - Finding 2's consequence for a real user is inferred beyond what I ran. The metadata write,
the 24-hour short-circuit and the misreported digest are each reproduced; that a developer then
fails to notice their index is stale for a day is read from the code path, not observed. - The five shipped recipe checksums were not re-verified. Downloaded and hashed at head
7e76e056and all five matched;recipes/is unchanged since, so that result carries, but it
was not re-run. index-meta.jsonstill has no schema validation andupdate_index's two reads of it wrap
json.loadin a bareexcept Exception(commands.py:1728-1734,:1742-1752). Absent-file
and well-formed cases were exercised; truncated and hand-edited shapes were read, not run.requires-python = ">=3.8"(pyproject.toml:10) is not exercised below 3.10. The
written_yaml_stems: set[str]annotation this commit adds is a local variable annotation and so
is not evaluated at runtime, which is why it is not raised as a finding — that is read from
PEP 526, not run on 3.8, and no 3.8 or 3.9 interpreter was available here.- Whether the
TASKS.mdstatic-analysis claim was written from a narrower command than the one
it implies.ruffandmypyrestricted to the two files this commit edited do return 0, which
would explain it exactly; which command was run I cannot tell from here, and finding 1 is written
about the record, not the intent.
Automated architecture review of 21523b18c8d9 — 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.
…update docs - Guard packages.json, sha256 sidecar, and index-meta.json writes behind if valid_entries - Return is_fallback=True on empty or unparseable remote index to report failure and allow retries - Rename SyncResult.count to package_count, eliminating tuple.count method shadowing and type ignore - Remove remaining unused F401 imports in repository.py and test_package_search.py - Document empty-index safety floor in docs/dependency-management.md and CHANGELOG.md - Record machine-verified test suite and static analysis evidence in TASKS.md - Expand unit tests with metadata preservation and network retry assertions
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#111 "feat: implement package management system with recipe support, registry…"
head: 7a67ae4 author: Grantlinkz ci: none run (3 workflow runs, all action_required) · mergeable: MERGEABLE · mergeStateStatus: BLOCKED
Verdict: Follow-up to the review of 21523b18. One commit, 7a67ae4, +82 −39 across seven
files. Four of the five previous findings are resolved and I verified each by re-running the
previous review's own reproductions. The empty-index floor now refuses the document and the
record: packages.json, packages.json.sha256 and index-meta.json all survive untouched, the
next non-forced update-index fetches again instead of short-circuiting for 24 hours, and the CLI
exits 1 with a warning rather than reporting success — all three clauses of the standing
2026-09-05 addendum, which this head is the first to implement in full. SyncResult.count is
renamed to package_count and the # type: ignore is gone rather than moved; every call site
follows. The three F401s are deleted, so ruff on the PR-touched files is clean and the full-repo
count is back to origin/master's 380. TASKS.md's evidence column is scoped and, for the first
time in three heads, every clause in it holds when run. What is left is one defect the fix itself
introduced: the freshness check now reads synced_at out of index-meta.json and passes it to
float() inside a try that only catches OSError, so a cache file whose synced_at is not a
number aborts ebuild update-index with an unhandled traceback instead of falling through to a
re-fetch. The PR body is still the unfilled template, now across nine heads.
Status of the previous findings (ebuild-111-21523b18.md)
| Prev # | Sev | Status | Evidence at 7a67ae42 |
|---|---|---|---|
| 1 | Medium | Resolved in 7a67ae4 |
The three F401s are deleted (repository.py:19 no longer imports get_default_index_dir — grepped, it has no use in the file; test_package_search.py drops Path and PackageInfo — likewise unused). ruff check <7 PR-touched .py> --select=E,F,W --ignore=E501 → All checks passed (3 errors at 21523b18); ruff check . --select=E,F,W --ignore=E501 → 380, the same number as origin/master, so the PR's delta is now +0. mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional → 0 errors, and no type: ignore anywhere in the PR's package files (the one remaining in the repo is commands.py:2871, import serial, pre-existing). TASKS.md:21 is now scoped to exactly those two commands and adds the machine-checkable line the last review asked for: python -m pytest tests/ -q → 605 passed, confirmed. |
| 2 | Medium | Resolved in 7a67ae4 |
All three consequences closed, each re-run. (a) index_sync.py:275-298 returns before any write, so after a good sync followed by an empty one index-meta.json still reads {"sha256": "INITIAL", "synced_at": 1000.0} and packages.json.sha256 still reads INITIAL packages.json — both describe the cache that is still on disk. (b) Because synced_at is not advanced, a following non-forced sync() calls urlopen (urlopen called: True) instead of short-circuiting for the 24-hour TTL. (c) is_fallback=True on the empty path, and commands.py:1748-1753 renders it as [warn] + SystemExit(1): driven through CliRunner, exit 1, no [ok], and the digest printed is the cached one. Cold cache + empty index now leaves the directory with recipes/ only — no orphan metadata for a document that was thrown away. |
| 3 | Medium | Untouched — ninth head | pr.json body → 1,424 bytes, still byte-identical to the template with a truncated title fragment prepended. Empty Summary, - placeholders under Changes, 19 - [ ], 0 - [x], no linked issue. additions: 1687, deletions: 135, changedFiles: 18. See finding 2. |
| 4 | Low | Resolved in 7a67ae4 |
index_sync.py:53 is package_count: int with no suppression; the shadowed tuple.count is gone. All call sites follow: :164 and :379 pass positionally, :293 by keyword, commands.py reads only .message / .is_fallback / .sha256 / .pruned, and the 13 test assertions are updated. Grepped for a surviving .count on a SyncResult — none; registry.package_count (commands.py:78) and repository.package_count (:155) are an unrelated property. Suite green at 605. |
| 5 | Low | Resolved in 7a67ae4 |
docs/dependency-management.md:332 and CHANGELOG.md:19 both gained the sentence: "An index that yields no usable entries is treated as a delivery fault: nothing is pruned, the cached index is left in place, and the condition is reported." Verified against the code — all three clauses now hold, including "reported", which is what finding 2(c) was. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | ebuild/packages/index_sync.py:191-194 and :205-206 |
The fix for finding 2 widened this expression's input to an on-disk value and left the narrow guard around it, so a corrupt cache file now crashes update-index instead of causing a re-fetch. The freshness check was cache_age = time.time() - self.packages_json.stat().st_mtime; st_mtime is always a float and except OSError was a complete guard. 7a67ae4 replaces it with synced_at = cached_meta.get("synced_at") (:191) followed by float(synced_at) (:194) — cached_meta is json.load of ~/.ebuild/index/index-meta.json (:180-184), i.e. whatever is in a user-writable cache directory. float() raises ValueError on a non-numeric string and TypeError on a list or dict, and except OSError (:205) catches neither. Reproduced on this head: synced_at: "2026-09-05T20:00:00Z" → ValueError: could not convert string to float, propagating out of sync(); through CliRunner, ebuild update-index prints its header and nothing else, and the exception reaches the top with no diagnostic naming the file. synced_at: {} and [] → TypeError, same path. (null correctly falls back to the mtime; true is harmless.) This is the only unguarded read of index-meta.json in the tree — :180-184, :286-290, commands.py:1728-1734 and :1742-1752 all wrap it in except Exception, so the file's handling is now inconsistent with itself. §9.2 asks for "actionable diagnostics with remediation guidance"; a traceback on a cache file the user can simply delete is the opposite. Medium and not Low because it is a regression introduced by this commit in a guard whose evident purpose is to make this path total; Medium and not High because it needs out-of-band corruption to trigger and rm -rf ~/.ebuild/index recovers it. |
Widen the guard to match the value it now protects: except (OSError, TypeError, ValueError): pass. That is one line and restores the previous property — an unusable freshness record means "stale", which re-fetches, which is the safe direction. A test alongside the two this commit added: seed index-meta.json with "synced_at": "not-a-number", call sync(force=False), assert urlopen was called. |
| 2 | Medium | pr body | The PR body is still the unfilled template — ninth head, 1,687 added lines, no movement. Empty Summary, - placeholders under Changes, 19 unchecked boxes, 0 checked, no linked issue. This is now the only finding with zero movement across all nine reviews, and it is by some distance the cheapest one on the list: the suite is green at 605 on this head, ruff on the touched files is clean, and the author has already written all of that into TASKS.md:21 — the body could quote it. 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/est/uild are still .github/PULL_REQUEST_TEMPLATE.md's corruption on origin/master — literal control characters where \feat, \fix etc. were written — not the author's doing. Unchanged across all nine reviews.) |
Summary and Changes from git log on this branch; Testing gets the numbers already recorded in TASKS.md. Then name what is not covered — no CI has executed on any of the nine heads, no real index has ever been fetched (DEFAULT_INDEX_URL is ""), and ebuild build has never been run end to end against a remote recipe. All three are honest gaps rather than mistakes, and naming them is faster than having a reviewer find them. |
| 3 | Low | ebuild/packages/index_sync.py:284-290, :293 |
The empty-index return re-reads a file it already has parsed, and reports a count that does not match its own message. (a) :284-290 opens and json.loads index-meta.json a second time to recover sha256, inside a bare except Exception: pass, while cached_meta (:178-184) already holds that exact parse from earlier in the same call — the network-error fallback at :239-240 gets this right and reads cached_meta.get("sha256"). Two reads of one file in one function is the duplication the brief's item 10 describes, and the bare except here can only hide a fault the first read already handled. (b) package_count=len(cached_entries) (:293) is the number of entries in the cached packages.json, while the message on the line above counts recipe files in recipes/. They are different quantities and the PR's own tests show them diverging: test_index_sync_empty_index_preserves_cache_and_warns:468 asserts package_count == 2 and test_index_sync_unparseable_entries_preserves_cache:507 asserts 0, for the same condition with the same two cached recipes. Nothing user-visible depends on it today — update_index prints only res.message on this path — which is why this is Low rather than higher. |
(a) cached_meta_sha = cached_meta.get("sha256"), deleting :284-290 — six lines out, and it matches :239. (b) Pick one meaning and use it in both places; cached_count is already computed at :277-281 and is the number the message quotes. |
| 4 | Low | ebuild/packages/repository.py:1; ebuild/packages/index_sync.py:386 |
Two stray whitespace edits ride along, unmentioned in a commit message that is otherwise itemised. repository.py gains a blank first line, pushing # SPDX-License-Identifier: MIT to line 2 — checked every *.py in the repo, and this is now the only file where the identifier is not on line 1. index_sync.py gains a blank line at EOF (:386, after the closing )). Neither is caught by CI: ruff's W391 requires --preview, which ci.yml:52 does not pass, so ruff check . --select=E,F,W reports nothing. Of the three *.py files in the repo that end with a blank line, all three are files this PR adds or edits. No functional consequence — recorded because the brief asks whether anything was changed without explanation, and because an SPDX header that moves off line 1 is the kind of thing a licence scanner notices long after the PR that did it. |
Delete both lines. git diff origin/master...HEAD -- ebuild/packages/repository.py should show only the import change. |
Verified by running (at 7a67ae42)
env: uv venv -p 3.12 + uv pip install pytest click pyyaml ninja ruff mypy
CPython 3.12.14, Linux x86-64 · ruff 0.16.6 · mypy 2.3.1
git log --oneline 21523b18..7a67ae42 -> 1 commit (7a67ae4)
git diff --stat 21523b18 7a67ae42 -> 7 files, +82 -39
git rev-list --count 7a67ae42..origin/master -> 0 (origin/master e5d8052)
git log -1 --format=%s -> 81 chars, Conventional Commits clean
python -m pytest tests/ -q -> 605 passed, 3.74s (605 at 21523b18)
python -m pytest tests/unit/test_index_sync.py -q -> 22 passed (TASKS.md's count is right)
ruff check <7 PR-touched .py> --select=E,F,W --ignore=E501
-> All checks passed (3 at 21523b18) prev1 FIXED
ruff check . --select=E,F,W --ignore=E501 -> 380 · origin/master 380 · delta +0 prev1 FIXED
mypy ebuild/packages/{registry,index_sync}.py --ignore-missing-imports --no-strict-optional
-> Success, 0 errors, no type: ignore prev4 FIXED
mypy ebuild/ --ignore-missing-imports --no-strict-optional -> 9 errors in 4 files
(deliverable_packager.py, cli/integration.py, eos_ai/__init__.py, plugins/__init__.py —
none touched by this PR; unchanged from 21523b18)
grep "type: ignore" ebuild/packages/ ebuild/cli/commands.py -> commands.py:2871 only (pre-existing)
empty-index floor, --force, mocked urlopen, seeded meta {"sha256":"INITIAL","synced_at":1000.0}:
alpha+bravo cached, re-sync []
-> SyncResult(package_count=2, is_fallback=True, sha256='INITIAL', pruned=0)
index-meta.json -> sha256 INITIAL, synced_at 1000.0 UNCHANGED prev2(a) FIXED
packages.json.sha256 -> "INITIAL packages.json" UNCHANGED prev2(a) FIXED
packages.json -> still ['alpha','bravo'] · recipes/ -> both survive
then sync(force=False) -> urlopen called: True prev2(b) FIXED
through CliRunner -> exit 1, "[warn] Remote package index contained no usable entries;
keeping 1 cached recipes", "[info] Index SHA-256 digest: INITIAL"
no "[ok]", no "Index updated" prev2(c) FIXED
cold cache + empty index -> SyncResult(0, is_fallback=True, sha256=None)
dir contains recipes/ only — no orphan metadata prev2 FIXED
malformed index-meta.json synced_at, sync(force=False):
"2026-09-05T20:00:00Z" -> ValueError: could not convert string to float (finding 1)
"abc" -> ValueError (finding 1)
{} / [] -> TypeError: float() argument must be a string or a real number
null -> ok, urlopen False (mtime fallback) · true -> ok, urlopen True
through CliRunner -> header printed, no diagnostic, ValueError to the top (finding 1)
source-ranked precedence regression check (registry.py untouched since e713993):
project lvgl 9.2.2 vs cached-remote lvgl 9.9.9, unpinned registry.get("lvgl")
-> 9.2.2, PROJECT url, PROJECT checksum precedence still dominates version
registry.get("lvgl","9.9.9") -> 9.9.9 REMOTE (explicit pin still reachable, as designed)
SPDX-License-Identifier not on line 1: ebuild/packages/repository.py only (1 of 1) (finding 4)
*.py ending in a blank line: index_sync.py, test_index_sync.py, test_package_search.py
— all three are this PR's (finding 4)
ruff --select=W391 -> "has no effect because preview is not enabled" (finding 4)
get_default_index_dir in repository.py after the import removal -> no uses (safe)
Lockfile.load / is_locked / get_locked_* / locked_packages in ebuild/ -> still 0 call sites
recipes/ unchanged since 7e76e056 -> empty diff
CI: actions/runs?head_sha=7a67ae42 -> 3 runs (CI — ebuild, CodeQL, Simulation Test),
every conclusion "action_required"
commits/7a67ae42/status -> {"state":"pending","total_count":0} · check-runs -> 0
gh pr view 111 -> 0 issue comments, 7 reviews (all this pipeline's)
The local ebuild clone was left untouched: the sync step reported it dirty (4 files) and
skipped it. I worked in 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.
Architecture conformance
Master design §9.1–9.2 (eBuild engine and SDK design rules), §10/§10.1 (component model and
contract), §11/§11.1 (Registry), §21 tiers and §21.1 split policy.
Conforms. Tier placement is unchanged and §9.2's position is unchanged from 21523b18.
Nothing in this diff points up a tier. The commit is internal to ebuild plus its own
CHANGELOG.md, TASKS.md, docs/ and tests; it adds no import and removes three. 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 it. §21.1 is not triggered; no
repository is proposed.
§9.2's "reproducible lockfiles/manifests for production builds" was satisfied on the build path at
e713993 by the source-ranked get() at registry.py:139-158, and neither that function nor
resolver.py is touched here. I re-ran the reproduction anyway, because the fix's blast radius was
adjacent: a project lvgl 9.2.2 against a cached remote 9.9.9 still resolves unpinned to the
project's url and checksum. docs/dependency-management.md:325 remains a true statement.
This head completes the 2026-09-05 addendum rather than deviating from it. That proposal's
clause — a synchronisation the client refuses to apply "does not advance the freshness timestamp
the client uses to decide whether to fetch again, and it does not record the refused document's
digest as the digest of the index the client currently holds" — is implemented here in full, and
its parent clause's third requirement, "reports the condition as a failure rather than a success",
is implemented with it. Three consecutive heads have now landed the design text ahead of the
.docx, which is the strongest argument yet for merging the §11 retention clause and both of its
addenda. No new proposal is appended for any of that; the standing ones cover it.
What this head does expose that the drafted rule does not reach is the read side of the record
it governs. The rule makes the freshness timestamp, the digest and the cached document one unit
written together — and says nothing about a client that cannot parse the unit back. Finding 1 is
precisely that gap arriving in code: two of the three reads of index-meta.json in this tree
degrade to "assume stale", the one this commit added raises. An addendum is appended.
Two standing items are unchanged and remain pre-existing on master, not this PR's defects:
Lockfile.load / is_locked / get_locked_entry / get_locked_version / locked_packages still
have zero call sites in ebuild/, so ebuild.lock is written every build and never read (subject
of the 2026-09-04 proposal against §9.2); and the 2026-09-03 precedence proposal's clause that
"tooling must be able to report, for every resolved component, which source its definition came
from" is still unimplemented — registry.py:95 keeps the rank in _recipes and get() discards
it. Neither is scored as a finding, because the proposals that would require them are unmerged.
Proposed changes
In order. Only finding 1 should block, and it is one line:
- Finding 1.
except (OSError, TypeError, ValueError): passatindex_sync.py:205, plus the
one test named above. This is the smallest possible change and it restores the property the
guard had before7a67ae4. - Finding 2. Fill in the PR body. Ninth head, and
TASKS.md:21already contains the text. - Findings 3 and 4 — one tidy-up commit: use
cached_meta.get("sha256")at:284, settle
whatpackage_countmeans on the refusal path, and delete the two stray blank lines. - Ask a maintainer to approve the three
action_requiredworkflow runs. Nine consecutive heads
have now been reviewed with no CI execution at all.
No fix PR opened. Every finding is on this PR's branch, which the brief puts out of bounds —
including finding 1, which would otherwise be exactly the small, provable class the brief permits.
Blocked / stale
Blocked, unchanged in cause, now nine heads deep. 77f1d9f2, 7e76e056, 005a69ad,
e7139935, afb768fb, 9ad48e1e, 21523b18, 7a67ae42 and the head before them have all been
reviewed with zero CI executions between them. Every workflow run on this branch, on every head,
has conclusion: action_required — the first-time-fork-contributor approval gate, which is not
the author's to clear. commits/7a67ae42/status is pending with total_count: 0; check-runs
is total_count: 0.
The PR is not stale in any other sense: MERGEABLE, 0 commits behind origin/master, and
mergeStateStatus: BLOCKED is reviewDecision: REVIEW_REQUIRED rather than a red check. This is
the third consecutive head on which the author closed nearly every open finding in a single
commit, with new regression tests for each.
What unblocks it: a maintainer approving the three action_required workflow runs. Until then
the Windows and macOS matrix legs of CI — ebuild, CodeQL, the Simulation Test job and the
coverage step have said nothing about 1,687 added lines. I ran the suite, ruff and mypy in a
sandbox on Linux/CPython 3.12 and reported the numbers above; that is a reviewer's machine, not
the project's pipeline.
Not checked
- No CI has run on this head, or on any of the nine. The
windows-2022and macOS matrix legs
(ci.yml:26matrixes Python 3.10/3.11/3.12), CodeQL, the Simulation Test job and the coverage
step are all NOT RUN and their results are unknown. Everything above ran on Linux/CPython
3.12.14 only. pytest-covandpytest-benchmarkwere not installed, sotests/performance/ran without
the benchmark plugin and CI's--cov=ebuild --cov-report=xmlinvocation was not reproduced.- ruff 0.16.6 and mypy 2.3.1, resolved fresh; CI installs both unpinned, so its versions will
differ. Both steps arecontinue-on-error: true(ci.yml:53,:65), so neither gates either
way — the clean ruff result is an improvement that CI could not have failed on regardless. - No real index was fetched and no package was built end to end.
DEFAULT_INDEX_URLis still
""and no deployed index exists, so every probe — the floor, the metadata preservation, the
TTL retry, the malformedsynced_at— ran against a mockedurllib.request.urlopen.
ebuild buildwas not run this session; the precedence result above isregistry.get()driven
directly, which is what_install_packagescalls, not a build. - Finding 1's trigger was injected, not observed.
sync()only ever writestime.time()into
synced_at, so reaching the crash needs a hand-edited, restored, or foreign-written
index-meta.json. I wrote those files myself; I have not seen one arise on its own, and I did
not test a partially-written meta (the write is atomic via.replace(), so truncation should
not occur through this code path). index-meta.jsonstill has no schema validation. Theurl,sha256and absent-file cases
were exercised; other malformed shapes were read, not run.requires-python = ">=3.8"(pyproject.toml:10) is not exercised below 3.10. No 3.8 or 3.9
interpreter was available here.- The five shipped recipe checksums were not re-verified. All five were downloaded and hashed
at head7e76e056and matched;git diff 7e76e056 HEAD -- recipes/is empty, so that result
carries, but it was not re-run. - The local
ebuildclone was left untouched. The sync step reported it dirty (4 files) and
skipped it; all work above happened in a throwaway--sharedclone under/tmp.
Automated architecture review of 7a67ae42ed31 — 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.
…ry, and build synchronization.
Summary
Type of Change
efactor — Code restructuring without behavior change
Changes
Testing
Pre-Submission Checklist
Related Issues
Screenshots / Logs
Additional Notes