fix: repair master — the dropped to_dict() fields, vendored drift, scorecard pins (lint landed via #122) - #132
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
srpatcha
left a comment
There was a problem hiding this comment.
Thank you for the forensic write-up — it made this easy to verify. I re-ran each check on the branch: ruff clean, mypy loses exactly the two code errors (plugins/__init__.py:46, index_sync.py:354), pytest 674 passed with only the environment-only e2e build test failing, and check_vendor_drift.py is back to 44/44 where master fails at 45. The three-way-alignment.md blob hash matches the pinned 7f9c8c1, and I confirmed no other YAML file carried a CR before the .gitattributes pin.
Two optional follow-ups on the restored method, neither blocking:
ebuild/packages/recipe.py:92-114—to_dict()predatesinstall_args, so that field is dropped on round-trip throughindex_sync. Addingif self.install_args: data["install_args"] = list(self.install_args)would make it complete, and wrapping the other lists inlist(...)avoids handing out live references.-
- Since this method was already lost once in a replay, a tiny test asserting
parse_recipe(r.to_dict()) == rwould make the next disappearance a red test rather than nineAttributeErrors.
Approving — this should land first in ebuild. For the maintainers: this supersedes #122 (identical hunks) and #119/#124 (to_dict), and it will conflict with #127's unrelatedrecipe.py/pluginshunks.
- Since this method was already lost once in a replay, a tiny test asserting
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#132 "fix: repair master — lint, a dropped method, vendored drift, scorecard"
head: f8cc357 author: Kartikey1306 ci: pass (30 checks green, Create GitHub Release skipped)
Verdict: The most complete diagnosis of master's red state in the current queue, and the only PR in this batch with a full green matrix behind it — nine Test legs, CodeQL, vendor drift, eleven EoSim targets. Every claim in the body that I could check independently held. One defect: the restored PackageRecipe.to_dict() does not emit install_args, so it is not the round-tripping serializer the body says it is. That is latent rather than live today, and the reasoning for why is below.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | ebuild/packages/recipe.py, to_dict() |
install_args is dropped. parse_recipe() reads ten optional fields; to_dict() emits nine. Verified: a recipe with install_args=["DESTDIR=/stage","--strip"] serializes to ['build','dependencies','package','patches','url','version'] and reconstructs with install_args == [] — parse_recipe(yaml.safe_load(yaml.safe_dump(r.to_dict()))) != r. This matters because the write and the read are two halves of one loop inside the product: index_sync.py:354 writes these files into recipes_dir, get_recipe_dirs() at :122 hands that directory to RecipeRegistry, and registry.py:114 loads them back through load_recipe(). No data is lost today, because the only producer — the entry→dict mapping at index_sync.py:335-347 — never populates install_args either, so the field is always empty by the time to_dict() sees it. The defect is that both halves silently agree to drop a field the schema supports, and the first recipe that carries one loses it with no error. The body's claim that this "emits the package:/build: keys that parse_recipe() reads back" is right about the key names and wrong about completeness. |
Add if self.install_args: data["install_args"] = list(self.install_args) alongside the other conditional fields. #124's implementation emits all twelve and round-trips exactly — I verified that one too — so if the restoration-vs-reimplementation argument is what is keeping this hunk here, #124 already has the correct behaviour and could simply be taken instead. Add the missing key to index_sync.py:335-347 in the same pass, or the field can never arrive. |
| 2 | Medium | core/eos/docs/three-way-alignment.md |
Reverting the vendored snapshot to the pin is the correct call and I am not disputing it — core/UPSTREAM.yaml says a vendored copy must match its pin, and the guard did its job. But the content being reverted away from is a correction: #109 replaced "25 YAML files / 25 board ports / ✅ Aligned" with "84 vs 83 vs 14/138 — three inventories describe the same set and nothing cross-checks them". After this merges, the repository again ships a table asserting ✅ Aligned for a set of inventories that #109 found do not agree. The body acknowledges this ("worth keeping; it belongs in ebuild's own docs or upstream in eos") but nothing in the diff or the PR carries it anywhere. A correction that is reverted with only a sentence in a PR body is a correction that is lost. |
Open the upstream eos issue or PR before merging this, and reference it from the diff — a line in ebuild's own docs, or the issue number in the CHANGELOG. The revert then reads as "moved", not "dropped". |
| 3 | Low | .github/workflows/scorecard.yml:27 |
ossf/[email protected] is pinned by tag. A mutable tag is exactly what Scorecard's own Pinned-Dependencies check penalises, and this repo already knows the pattern — .github/workflows/linked-issue.yml on master pins its reusable workflow to @92cb596c773496ec4df76717e8acf0e6b7700f73. Raising the Scorecard job from the floor while leaving the action itself unpinned leaves points on the table in the same file. |
Pin to the commit SHA for the v2.4.3 tag, with # v2.4.3 trailing so the version stays readable. Same for actions/checkout@v4 and github/codeql-action/upload-sarif@v3 at :24 and :32 if you want the check fully satisfied. Out of scope if you would rather keep this PR to the four named repairs — say so and it stands. |
| 4 | Low | .gitattributes (new) |
*.yml/*.yaml text eol=lf is the right fix and the reasoning in the header comment is exactly right. One operational gap: .gitattributes governs future checkouts and commits, not files already in a contributor's working tree. Anyone with an existing Windows clone keeps CRLF locally until git add --renormalize . or a fresh checkout, so "it still fails for me" reports are likely. |
Add one line to the CHANGELOG or CONTRIBUTING telling Windows contributors to run git add --renormalize . once after pulling this. |
| 5 | Low | ebuild/packages/recipe.py, to_dict() |
The conditional-emission branches have no test in this PR — which is how finding 1 survives review, and is corroborated by the coverage bot's report on this PR. A single round-trip assertion would have caught it. | python\ndef test_to_dict_round_trips():\n r = PackageRecipe(name="d", version="1", url="https://e/x.tgz",\n build_system="cmake", install_args=["DESTDIR=/s"])\n assert parse_recipe(yaml.safe_load(yaml.safe_dump(r.to_dict()))) == r\n Fails today, passes with finding 1 fixed. |
Architecture conformance
Conforms. §21 places ebuild in Tier 1 — Foundation. Every code change is inside that repo, and the one file under core/ is a revert toward the pinned upstream, which strengthens rather than crosses a boundary. §5.1 is not engaged by any hunk: no import, link line or manifest entry changes direction, and eBuild understands the complete graph but is not a runtime dependency still holds.
The to_dict() restoration is the §23.2 "Package format — versioned .epkg/.eapp metadata schema" contract, and finding 1 is a conformance gap against it: the serializer does not emit the whole schema the parser accepts. The .gitattributes and Scorecard hunks are infrastructure, which §21 assigns to the Infrastructure tier and .github/STANDARDS.md §"Security" names OpenSSF Scorecard and CodeQL explicitly — moving a Scorecard job from permanently-red to green is direct compliance work under that section, and the body's evidence (gcr.io now requiring billing, v2.4.3 pulling from ghcr.io, eos already pinned there) is the kind of concrete justification STANDARDS.md §"Standards-compliance assertions" asks for.
No API break (brief item 8): to_dict() is a new method, purely additive; cast(Mapping[str, Any], ...) is a runtime no-op; the .gitattributes and workflow changes touch no interface. No performance concern (brief item 9). Documentation (brief item 11): the _PIC_FLAGS-style stale comment in plugins/__init__.py is corrected in place, and .gitattributes documents its own reasoning — but there is no CHANGELOG entry for a PR that changes five subsystems, which is inconsistent with #125, #126 and #131 in the same queue, all of which added one.
On duplication (brief item 10): the body is straight about it — three test hunks and the plugins/__init__.py hunk are stated as byte-identical to #122's, and I confirmed the blob hashes match (e08444f, 3c25922, b1d5e0f, 54605f0). That is the honest way to declare an overlap. It does still mean #122 and this PR cannot both merge meaningfully, and that to_dict() now has four competing implementations in flight — #119 (asdict, emits internal key names), #124 (all twelve keys, round-trips), #127 (conditional, includes install_args) and this one. Only #124 and #127 preserve install_args.
Proposed changes
- Fix finding 1 — one
if self.install_args:branch, plus the matching key atindex_sync.py:335-347. - Add the round-trip test from finding 5. It is the assertion that makes "restored verbatim from #111" a checkable claim rather than a provenance argument.
- Resolve finding 2 before merge: file the upstream correction somewhere durable and reference it.
- Decide the overlap with #122, #119, #124 and #127 as one decision rather than four. This PR plus #124's
to_dict()is the smallest combination that leaves master green and loses nothing. - Findings 3 and 4 are optional polish.
Not checked
- pytest — NOT RUN by me. pytest is not importable on this host, and the local
ebuildclone has a dirty working tree, left untouched per the rules of engagement. Unlike every other PR in this batch, this one does not need me to: nineTest (Python 3.x, os)legs are green on this head in CI, which is stronger evidence than a single local run. The body's680 passed, 1 skippedis consistent with that but I did not reproduce the count. - mypy — NOT RUN by me. Not installed. The body's "no issues in 107 source files" is unverified locally; the green
Testlegs imply theType check (mypy)step passed, sinceci.ymlruns it before the test step with nocontinue-on-error. - yamllint — NOT RUN by me. Not installed. The Windows CRLF diagnosis is the one part of this PR I could not check at all: I have no Windows runner, did not reproduce
core.autocrlf=truebehaviour, and did not confirm that the stray CR inauto-assign.ymlwas the specific file that broke thenew-lines: platformattempt. I am taking the body's account on that, and the greenTest (Python 3.x, windows-2022)legs are consistent with it. - Vendor drift — NOT RUN by me.
python scripts/check_vendor_drift.pywas not executed here;Compare core/ against pinned upstreamsis green in CI on this head. I did not independently verify that the reverted file matches blob7f9c8c1at eos pin5544c98. - Scorecard — NOT VERIFIED. I did not confirm that
v2.4.3pulls from ghcr.io, that gcr.io returns the billing error, or that eos's Scorecard job is green. All three are the body's claims. TheCodeQLandAnalyze (Python)checks green on this head do not cover the Scorecard workflow, which runs on a schedule. - The
existing-comments.txtfor this PR contains only the coverage bot; no human review points existed to avoid repeating.
Verified locally, against a git archive export of head f8cc3574:
ruff 0.16.5 check .→All checks passed!The four findings present onorigin/masterare cleared.to_dict()emits['build','dependencies','package','patches','url','version']for a recipe carryinginstall_args;parse_recipe(yaml.safe_load(yaml.safe_dump(...)))does not compare equal to the original, andinstall_argscomes back[]. Finding 1.- The four blobs the body cites as byte-identical to #122 match.
origin/masterstill carries all four ruff findings, so this PR's premise is current, not stale.
Automated architecture review of f8cc357421ac — 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.
f8cc357 to
43b0337
Compare
|
Thanks — finding 1 was a real gap and the round-trip test would have caught it. Follow-ups on the branch, as separate commits:
Current head: ruff, yamllint, mypy clean; |
|
@srpatcha — your approval at |
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#132 "fix: repair master — lint, a dropped method, vendored drift, scorecard"
head: 43b0337 author: Kartikey1306 ci: pass (31 green, 0 red; only Create GitHub Release skipped)
Verdict: Follow-up review. All five previous findings are resolved, and I verified each
one rather than taking the summary: the round-trip test fails with exactly the reported
diagnostic when the fix is reverted, and all three Scorecard SHA pins resolve to the tags their
comments claim. One new Low, on a brittle assertion inside the new test. The unresolved item is
not in this diff — it is that four open PRs still implement the same method.
Status of the previous findings (ebuild-132-f8cc3574.md)
| # | Prev. severity | Status | Evidence |
|---|---|---|---|
| 1 | Medium — to_dict() drops install_args |
Resolved | 70632ca. ebuild/packages/recipe.py:115-116 emits it; ebuild/packages/index_sync.py:346 carries it through the entry mapping so the field can actually arrive. Negative control: removing the two emission lines makes test_to_dict_round_trips_every_field fail at assert reloaded == recipe with install_args: [] != ['DESTDIR=/tmp/stage'] — the precise failure the previous review predicted — and test_index_sync_caches_install_args fail with KeyError: 'install_args'. Both restored afterwards. |
| 2 | Medium — the reverted correction goes nowhere | Resolved | Filed as embeddedos-org/eos#149 (open: "docs/three-way-alignment.md asserts ✅ Aligned for board inventories that disagree (84 vs 83 vs 14/136)"), and re-measured in eos#151, which is open and reviewed in this same run. The CHANGELOG names the issue inline, so the revert reads as moved rather than dropped — which is what was asked. |
| 3 | Low — Scorecard actions pinned by mutable tag | Resolved, and correct | 8f4123b. All three now pin by commit with a # vX comment. I resolved every one against the GitHub API: actions/checkout@11d5960a… is the v4 ref; ossf/scorecard-action@4eaacf05… is what the annotated v2.4.3 tag object dereferences to; github/codeql-action/upload-sarif@faaca9a8… is what v3 dereferences to. No transcription error. |
| 4 | Low — .gitattributes does not fix existing Windows clones |
Resolved, and the author corrected themselves | b29f7ea added a CONTRIBUTING note; 43b0337 then fixed it, because the command the previous review suggested — git add --renormalize . — updates the index and never the working tree, so it would not have solved the reported problem. The note now says git rm --cached -r . && git reset --hard HEAD from a clean tree, or re-clone. That is right: emptying the index and hard-resetting re-checks-out every file, which is when the new eol=lf attribute applies. The previous review's recommendation was wrong and this PR is right. |
| 5 | Low — no test on the conditional-emission branches | Resolved | tests/ebuild/test_package_recipe.py:135-176 adds test_to_dict_round_trips_every_field over a twelve-field recipe and test_to_dict_returns_copies_not_live_lists. |
Beyond what was asked: to_dict() now returns list(...) copies of all five list fields,
not the live lists. Nobody raised that; it is a real aliasing defect — a caller appending to
data["dependencies"] was mutating the recipe — and it is pinned by its own test. Fixing an
adjacent defect found while in the file, and saying so in the CHANGELOG, is the right instinct.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | tests/ebuild/test_package_recipe.py:157-158 |
assert keys.index("install_args") == keys.index("build_args") + 1 pins the emission order of to_dict(), and the docstring six lines above it in recipe.py:106-109 says order does not matter: "parse_recipe() reads every key by name, so a dump and a reload agree field for field regardless of order." So the test will fail on a reordering that the code it tests explicitly declares harmless, and a future contributor will have to decide whether they broke something or the assertion is decorative. The intent — keep the key order aligned with index_sync's recipe_dict so diffs of cached YAML stay stable — is legitimate, it just is not what the assertion communicates. |
Either drop the line, or keep it and say why in the test: # cached recipe YAML is diffed by humans; keep the key order stable against index_sync's recipe_dict. One comment turns a mystery failure into an intentional one. |
One finding, Low, on 339 lines added across seven commits. I looked for more in the new work
and did not find it.
Verification performed for this review
Detached worktree at .ai/autoreview/state/scratch/ebuild-132 off refs/pull/132/head, with
a throwaway venv. The ebuild checkout has a dirty working tree (TASKS.md,
ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py, and an
untracked smart-sensor/) and I left it exactly as it was — verified still 4 entries after
all work. Nothing was committed or pushed.
| Check | Result |
|---|---|
pytest tests/ -q |
PASS — 683 passed, 1 skipped, 5.96s. Matches the PR comment's count exactly. The previous review could not run this at all. |
pytest tests/ebuild/test_package_recipe.py tests/unit/test_index_sync.py -q |
PASS — 39 passed |
ruff check . |
PASS — All checks passed! |
Negative control, finding 1: deleted the if self.install_args: emission and re-ran |
2 failed — test_to_dict_round_trips_every_field at assert reloaded == recipe, reporting install_args: [] != ['DESTDIR=/tmp/stage']; test_index_sync_caches_install_args with KeyError: 'install_args'. The tests genuinely pin the fix. recipe.py restored; git diff --stat clean. |
Scorecard pin actions/checkout@11d5960a… = v4 |
CONFIRMED via repos/actions/checkout/git/ref/tags/v4 |
Scorecard pin ossf/scorecard-action@4eaacf05… = v2.4.3 |
CONFIRMED — v2.4.3 is an annotated tag 99c09fe9… whose target commit is 4eaacf0543bb3f2c246792bd56e8cdeffafb205a |
Scorecard pin github/codeql-action@faaca9a8… = v3 |
CONFIRMED — annotated tag c20e34f4… → faaca9a8f6edddba5725ffe5adefdab6669a2eca |
| eos#149 exists and says what the CHANGELOG says it says | CONFIRMED — open, titled for the 84 vs 83 vs 14/136 discrepancy |
Was anything weakened? No. Across the seven commits: no test disabled, no lint rule
loosened, no assertion removed, no || true added, no permission widened. The workflow changes
tighten supply-chain posture rather than relax it.
Still open, and not this PR's to fix alone
The previous review's fourth "Proposed change" — that to_dict() has competing implementations
in flight — is unresolved and I am restating it because it is unresolved, not to repeat a
point. Open right now in ebuild: #119 (asdict, emits internal key names), #122
(overlapping lint repairs, four blobs byte-identical to this PR's), #124 (all twelve keys,
round-trips) and this one. #127 no longer appears to carry a to_dict() — it is now titled
fix(eos_ai): LLMClient URL normalization, so the previous review's count of four
implementations is down to three. This PR's version is now the most complete of them: it
round-trips every field and returns copies, which #124 does not. That is an argument for
taking this one and closing the others, but it is a maintainer decision about four PRs, not a
defect in this diff.
Architecture conformance
Conforms, and nothing in the new commits changes the previous assessment. §21 places ebuild
in Tier 1 — Foundation; every code change is inside the owning repo, and the one file under
core/ remains a revert toward the pinned upstream, which strengthens the boundary. §5.1 is
untouched: no import, link line or manifest entry changes direction, and "eBuild understands the
complete graph but is not a runtime dependency" still holds. §23.2's "Package format — versioned
.epkg/.eapp metadata schema" contract is the one the previous finding 1 was a gap against,
and it is now closed: the serializer emits the whole schema the parser accepts, and a test
asserts it. No API break (brief item 8) — to_dict() gains a key and returns copies, both
strictly additive to what callers could rely on. .github/STANDARDS.md §Security names OpenSSF
Scorecard explicitly, and pinning by commit is the Pinned-Dependencies check it scores; §"Org-wide
files" is the home of the linked-issue.yml pattern this follows.
Proposed changes
None blocking. Optional:
test_package_recipe.py:157 comment the key-order assertion, or drop it
Maintainer decision, not this PR's:
resolve #119 / #122 / #124 / #132 as one call. This PR's to_dict() is the
most complete of the three remaining implementations.
Not checked
- mypy — NOT RUN. Not installed in the scratch env and I did not add it. The PR comment's
"mypy clean" is unverified locally; the nine greenTest (Python 3.x, os)legs imply the
Type check (mypy)step passed, sinceci.ymlruns it before the tests with no
continue-on-error. In particular thecast(Mapping[str, Any], ...)in
ebuild/plugins/__init__.py— the 3.10/3.11entry_points()fallback — is Inferred
correct from CI, not verified here. - yamllint — NOT RUN. Not installed.
- The Windows CRLF story — NOT REPRODUCED. No Windows runner, no
core.autocrlf=true
scratch clone. I did not reproduce the author's before/after CR counts, and I did not verify
thatgit rm --cached -r . && git reset --hard HEADfixes an existing Windows clone — I am
reasoning from what those commands do to the index and working tree, which is Inferred.
The greenTest (…, windows-2022)legs are consistent with the fix but do not test the
documented recovery procedure, because CI always clones fresh. check_vendor_drift.py— NOT RUN. The comment's "44/44 and 46/46" is unverified here;
Compare core/ against pinned upstreamsis green in CI on this head. I did not independently
confirm the revertedthree-way-alignment.mdmatches blob7f9c8c1at the eos pin.- Scorecard behaviour — NOT VERIFIED. I verified the three pins resolve correctly, which is
the part that could have been wrong. I did not confirm the resulting Scorecard job is
green, that gcr.io returns a billing error, or that the Pinned-Dependencies score actually
moves — that workflow runs on a schedule and did not run on this head. pyteston Python 3.10 and 3.11 — NOT RUN. My venv is 3.12. Theentry_points()
fallback that the plugins fix targets only executes on 3.10/3.11, so the one code path this
PR changes for those versions is the one my local run did not exercise. CI covers it.
Automated architecture review of 43b03374aaf8 — 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.
{ |
|
Finding 1 done at The four-way That is the last change I intend to push here. @srpatcha — your approval at |
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#132 "fix: repair master — lint, a dropped method, vendored drift, scorecard"
head: f4788f2 author: Kartikey1306 ci: pass (30 green, 0 red; only Create GitHub Release skipped)
Verdict: One commit since the last review (43b03374..f4788f2c), six lines, all comment.
The one open Low is resolved and I have no new findings. Nothing blocks this from my side.
Previous finding — status
| # | Previous finding | Status | Evidence |
|---|---|---|---|
| 1 | Low — assert keys.index("install_args") == keys.index("build_args") + 1 pins an emission order that recipe.py:106-109 explicitly declares harmless, so a deliberate reordering fails a test that gives no reason |
Resolved in f4788f2 |
tests/ebuild/test_package_recipe.py:154-159. Keeping the line and explaining it was the better of the two options I offered, and the comment is accurate rather than decorative — I checked the claim it makes: index_sync.py:335-347's recipe_dict orders the keys package, version, description, license, url, checksum, build, dependencies, configure_args, build_args, install_args, patches, recipe.py:100-121 emits the same order with install_args after build_args, and index_sync.py:355 dumps with sort_keys=False, so the stable-diff property the comment cites is the one that actually holds. It also says what to do on a deliberate reordering, which is the part that turns a mystery failure into an intentional one. |
Findings
None. Six lines of comment, no behaviour change, and the suite is unchanged at 683 passed / 1
skipped. I looked for a reason to say more and there is not one.
Still open, unchanged, and not this PR's to fix
Restating only because it is still unresolved, not to repeat it: to_dict() has competing
implementations open in #119 (asdict, internal key names), #122 (overlapping lint
repairs, four blobs byte-identical to this PR's) and #124 (all twelve keys, round-trips),
alongside this one. That is a maintainer decision across four PRs, not a defect in this diff. For
whoever makes it, the fact I can attest to from having run the negative controls last round: this
PR's version is the only one of the three that both round-trips every field and returns list
copies rather than the live lists, and both properties fail their tests when reverted.
The author also notes they held this one-line fix back deliberately rather than restack #144 and
#146, whose bodies cite ranges against 43b0337. That reasoning was sound and the push has now
happened anyway, so #144 and #146's stated base..head ranges are stale against this head —
worth a glance from whoever reviews those two, and the reason I am mentioning it rather than
filing it.
Verification performed for this review
Detached worktree at .ai/autoreview/state/scratch/ebuild-132 on refs/pull/132/head, using the
run's shared venv. The ebuild checkout has a dirty working tree — TASKS.md,
ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py, and an
untracked smart-sensor/ — and I left it exactly as found; re-checked after all work and it is
still those four entries. Nothing was committed or pushed.
| Check | Result |
|---|---|
pytest tests/ -q |
PASS — 683 passed, 1 skipped, 5.98s — identical to the previously reviewed head, so the comment changed nothing |
pytest tests/ebuild/test_package_recipe.py -q |
PASS — 16 passed |
ruff 0.16.5 check . |
PASS — All checks passed! |
Comment's factual claim, read against index_sync.py:335-347 and recipe.py:100-121 |
CONFIRMED — same key order, install_args after build_args, yaml.safe_dump(…, sort_keys=False) |
| Was anything weakened? | No. The diff is six comment lines; no test disabled, no assertion removed, no lint rule loosened, no || true, no permission widened. |
| CI on this head | 30 green, 0 red; Create GitHub Release skipped. mergeStateStatus: BLOCKED is REVIEW_REQUIRED — it needs a human approval, not a fix. |
Architecture conformance
Conforms; unchanged from the previous two reviews and not re-litigated here. The commit adds
comment lines to one test file in ebuild, Tier 1 — Foundation (§21). No import, link line or
manifest entry moves, so §5.1 is not engaged, and "eBuild understands the complete graph but is
not a runtime dependency" still holds. §23.2's package-format contract is where the earlier
install_args finding sat and it stays closed.
Proposed changes
None. The one open finding is closed.
Maintainer decision, still outstanding and not this PR's:
resolve #119 / #122 / #124 / #132 as one call
#144 and #146 cite base..head ranges against 43b0337, now stale
Not checked
- mypy — NOT RUN. Not installed in the scratch env. The body's "mypy clean" is unverified
locally; the nine greenTest (Python 3.x, os)legs imply theType check (mypy)step passed,
sinceci.ymlruns it before the tests with nocontinue-on-error. Inferred. - yamllint — NOT RUN. Not installed.
check_vendor_drift.py— NOT RUN.Compare core/ against pinned upstreamsis green in CI
on this head; I did not reproduce the 44/44 and 46/46 counts.- Windows CRLF recovery — NOT REPRODUCED. No Windows runner. Unchanged from the previous
review; this commit does not touch it. - Scorecard behaviour — NOT VERIFIED. The three SHA pins were confirmed correct last round and
are untouched here; I did not confirm the resulting job is green, because that workflow runs on a
schedule and did not run on this head. pyteston Python 3.10 and 3.11 — NOT RUN. My venv is 3.12. Irrelevant to this commit, which
is comment-only, but theentry_points()fallback this PR changed earlier still executes only on
those versions. CI covers it.
Automated architecture review of f4788f2c6042 — 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.
|
Thanks — nothing further from me on this branch; head stays On the one thing you mentioned rather than filed: both dependents were restacked within the hour and neither range is stale any more. Measured just now, in the same session as this comment:
#144's CI on the restacked head is 5/5 green. So the ordering concern is closed: land this first, and both dependents drop to their own commit. No push here since |
CI -- ebuild has been red on master since the 09-08 batch merge, and the first failing step (ruff) has hidden the ones behind it. Lint (ruff, all nine Test legs): - test_build_dir_resolution.py imported shutil twice (F811). - test_package_recipe.py lost its trailing newline (W292). - test_ci_gate.py had `import itertools` / `import re` two hundred lines down (E402) -- my own embeddedos-org#103, replayed onto a file that had moved. These three hunks are byte-identical to embeddedos-org#122's, so either PR merging first leaves the other clean. Type check and tests (never reached on master since 09-08): - ebuild/packages/index_sync.py calls PackageRecipe.to_dict(), which embeddedos-org#111 defined and embeddedos-org#112 -- merged five minutes later from a base without it -- deleted in its replay. mypy names it once; pytest fails nine test_index_sync cases with AttributeError. The method is restored verbatim from embeddedos-org#111 (cc90078): it emits the `package:`/`build:` keys parse_recipe() reads back, which an asdict() replacement would not. Vendored core drift: - embeddedos-org#109 (dba3d83) edited core/eos/docs/three-way-alignment.md, a vendored copy pinned to eos 5544c98, so drift went 44 -> 45 and the guard failed as designed. Reverted to the pinned content (blob 7f9c8c1, the same bytes as eos:docs/three-way-alignment.md at the pin). The alignment note belongs in ebuild's own docs or upstream in eos, not in the snapshot. OSSF Scorecard: - ossf/[email protected] pulls gcr.io/openssf/scorecard-action, and gcr.io now refuses the pull ("requires billing to be enabled"). v2.4.3 pulls from ghcr.io; eos already pins it and its Scorecard job is green. Not in this PR: EoSim Sanity's Windows/macOS legs install a wheel that has never been published; embeddedos-org#121 (srpatcha) already replaces that with the clone the other legs use. Verified locally: ruff clean, yamllint clean, mypy clean over 107 files, 680 passed / 1 skipped, scripts/check_vendor_drift.py 44/44 and 46/46.
…/3.11, yamllint on Windows Both surfaced on this branch's first CI run, once ruff let the job get past its first step. - ebuild/plugins/__init__.py: on Python 3.10 and 3.11 the stubs type entry_points() as the deprecated mapping, and its .get() wants an EntryPoints default, so mypy fails with arg-type. The line carried a '# type: ignore[attr-defined]' -- the wrong error code, so it suppressed nothing. Spelled out with a cast, byte-identical to embeddedos-org#122's hunk (54605f0). - .yamllint.yml: the Windows runners check out with core.autocrlf=true, so every YAML file arrives as CRLF and the default new-lines: unix rule rejected every line. The step was added on 09-03 and had never passed on that leg. new-lines: platform accepts the checkout's own convention.
…e checkout's line ending new-lines: platform was the wrong fix. The Windows runners' autocrlf turns LF files into CRLF -- except a file that already carries a stray CR, which git leaves alone, and auto-assign.yml had one on its last line. So under 'platform' Windows expected CRLF and got LF on that file's first line, and the leg was red again for the opposite reason. Pin *.yml and *.yaml to eol=lf so every OS lints the same bytes, keep yamllint's default unix rule, and drop the stray CR.
…h a round-trip test PackageRecipe.to_dict() was written before install_args existed and was never taught about it, so a recipe that went through index_sync came back from the cache with install_args empty while every other field survived. It also returned the recipe's own list objects, so a caller that appended to what it got back edited the recipe behind its back. install_args is now emitted after build_args, matching the order parse_recipe() reads them, and every list field is copied on the way out. index_sync's entry-to-recipe mapping carries install_args too; without that the field could not arrive from an index at all. The new round-trip test builds a recipe with every field set and asserts parse_recipe(safe_load(safe_dump(to_dict()))) equals it; against the previous to_dict() it fails on install_args. A second test checks the lists are copies, and test_index_sync gains a case that an index entry's install_args reaches the cached YAML.
actions/checkout, ossf/scorecard-action and codeql-action/upload-sarif were referenced by moving tags. A tag can be re-pointed; a commit cannot, and Scorecard itself flags unpinned actions. Each is now pinned to the commit its tag resolved to on 2026-09-14, with the tag kept in a trailing comment, the same shape linked-issue.yml already uses.
The changelog records what the master repair changed and why. CONTRIBUTING gains a note for Windows contributors: .gitattributes now pins YAML to LF, but the attribute governs future checkouts and commits, not files already in a working tree, so an existing clone needs one git add --renormalize . (or a fresh clone) before yamllint stops seeing CRLF.
…working tree git add --renormalize . re-applies the clean filter to the index; it never rewrites files, so a clone with CRLF YAML still has CRLF YAML afterwards (reproduced in a scratch clone with core.autocrlf=true: two CRs before, two after, status clean). What re-checks the files out is git rm --cached -r . && git reset --hard HEAD, or a fresh clone. CONTRIBUTING and the changelog now say that. The changelog entry also lists every file it describes and the plugins/__init__.py type-check fix it had left out, and the to_dict() comment no longer claims to follow parse_recipe()'s order.
The review at 43b0337 found the one-line assertion pinning install_args next to build_args and, six lines above it, recipe.py's docstring saying key order does not matter. Both are true: order is not a correctness property, because parse_recipe() reads every key by name, and it is a stability property, because index_sync writes the dict as cached YAML that humans diff. The assertion now says which of the two it is guarding and what to do when a reordering is deliberate, so a future failure reads as intentional rather than as a mystery. No behaviour change. ruff clean; pytest 683 passed, 1 skipped.
f4788f2 to
4054494
Compare
|
Rebased onto Verified at this head with CI's exact invocations: Dependents restacked: #144 → |
|
Status for the merger, 2026-09-17: green on all checks, rebased onto |
|
@srpatcha — re-approval request, with the facts that make it a two-minute read rather than a re-review. Your approval at The repair commit you approved got smaller, not different. Then What is new since your approval: 7 follow-up commits, +154/−8 across 9 files, each answering a finding from your automated review — What this unblocks: #146 and #148 are stacked on this head, plus your #121 and vivek3210's #141, whose remaining reds are inherited from master. Nothing further will be pushed to this branch. Merge order: this first. |
Problem
CI — ebuildhas been red on master since the 09-08 batch merge, and because the first failing step isruff, everything behind it (mypy, pytest) has not run on master since.Vendored core driftandOSSF Scorecardare red on the same commit for unrelated reasons.Verified against runs 34419838440 (
CI — ebuildon8b623d5, all nine Test legs atLint (ruff)), 34419838427 (drift) and 34419838445 (Scorecard), and reproduced locally on master before every fix below.What broke
Lint (ruff, 4 findings). A duplicate
import shutil(F811), a lost trailing newline (W292), and twoimports two hundred lines downtest_ci_gate.py(E402) — that last one is my own #103, replayed onto a file that had moved. These three hunks are byte-identical to #122's (blobse08444f,3c25922,b1d5e0f), so whichever merges first leaves the other clean.Type check + tests (never reached on master since 09-08).
ebuild/packages/index_sync.py:354callsPackageRecipe.to_dict(). #111 defined it; #112, merged five minutes later from a base without it, deleted it in its replay. mypy names it once; pytest fails ninetest_index_synccases withAttributeError. The method is restored verbatim from #111 (cc90078): it emits thepackage:/build:keys thatparse_recipe()reads back, which anasdict()replacement would not — the YAML it writes has to round-trip through the same parser. #119 and #124 both propose re-implementations; this is a restoration of what master already had, not a third design.Two more steps that had never passed, found on this PR's first run (run 34591499237), once ruff let the job past its first step:
ebuild/plugins/__init__.py:46— those interpreters' stubs typeentry_points()as the deprecated mapping whose.get()wants anEntryPointsdefault (arg-type). The line carried# type: ignore[attr-defined], the wrong error code, so it suppressed nothing. Spelled out with acast, byte-identical to ci: fix the lint findings that stop CI before any test runs #122's hunk (blob54605f0).core.autocrlf=true, so every YAML file arrives CRLF and the defaultnew-lines: unixrule rejected every line oftemplates.yamlandlayers/eni/build.yaml. The step was added 09-03 and has never passed on that leg. A first attempt (new-lines: platform) failed the other way:auto-assign.ymlcarried one stray CR, so autocrlf left it alone and Windows then expected CRLF on its first line. Fixed properly:.gitattributespins*.yml/*.yamltoeol=lfso every OS lints the same bytes, yamllint keeps its defaultunixrule, and the stray CR is gone.Vendored core drift. #109 (
dba3d83) editedcore/eos/docs/three-way-alignment.md, a vendored copy pinned to eos5544c98, so drift went 44 → 45 and the guard failed exactly ascore/UPSTREAM.yamlsays it should. Reverted to the pinned content — verified the result is blob7f9c8c1, the same bytes aseos:docs/three-way-alignment.mdat the pin. The alignment note itself is worth keeping; it belongs in ebuild's own docs or upstream in eos, not in the snapshot.OSSF Scorecard.
ossf/[email protected]pullsgcr.io/openssf/scorecard-action, and gcr.io now refuses with "This API method requires billing to be enabled".v2.4.3pulls from ghcr.io; eos already pins it and its Scorecard job is green.Test plan
Verified, run the way
ci.ymlruns them:Not in this PR
EoSim Sanity's Windows and macOS legs install a wheel that has never been published (run 34561966328); #121 already replaces that with the clone the other legs use.Review follow-ups (2026-09-14)
Rebased onto the governance commit; findings applied as separate commits on top:
to_dict()emitsinstall_args(finding 1) and hands back copies of its list fields;index_sync's entry mapping carriesinstall_argsthrough to the cached recipe.tests/ebuild/test_package_recipe.py::test_to_dict_round_trips_every_fieldassertsparse_recipe(yaml.safe_load(yaml.safe_dump(r.to_dict()))) == rwith every field populated (finding 5) — verified it fails withinstall_args == []when the field is removed fromto_dict()again — andtests/unit/test_index_sync.py::test_index_sync_caches_install_argsproves the field reaches the cached YAML. So the earlier text "restored verbatim from feat: implement package management system with recipe support, regist… #111" now reads: restored from feat: implement package management system with recipe support, regist… #111, then completed.origin/masterof all three repositories (eos PR "docs(alignment): replace the ✅ Aligned table with measured inventories"); the CHANGELOG entry here references it.scorecard.yml's three actions are pinned by commit SHA with a trailing version comment, followinglinked-issue.yml's precedent (checkout@11d5960a…v4,scorecard-action@4eaacf05…v2.4.3,codeql-action/upload-sarif@faaca9a8…v3; resolved from the tags on 2026-09-14).git add --renormalize .; that is wrong — it rewrites the index, never the working tree (verified in a scratch clone withcore.autocrlf=true: two CRs before, two after). The note now saysgit rm --cached -r . && git reset --hard HEAD(or re-clone), which does re-check the files out as LF.plugins/__init__.pymypy fix.Verified on the current head: ruff clean, yamllint clean, mypy clean,
pytest tests/683 passed / 1 skipped,check_vendor_drift.py44/44 and 46/46.Closing issue
Fixes #140