style: integrate yamllint, move ruff config to pyproject.toml, fix linter issues - #113
style: integrate yamllint, move ruff config to pyproject.toml, fix linter issues#113hanggrian wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent with the stated goal of making lint/type checks gating in CI, with only a minor maintainability improvement suggested in one typing annotation.
Pull request overview
This PR tightens CI linting by adding yamllint and ensuring ruff/mypy failures no longer get ignored, while consolidating Ruff configuration into pyproject.toml and applying repo-wide lint/type-driven cleanups.
Changes:
- Enforce lint/type checks in CI: run
ruff check .,yamllint ., andmypywithoutcontinue-on-errorfor these steps. - Move Ruff configuration into
pyproject.toml(select/ignore rules + excludes). - Apply YAML/Python formatting and typing fixes to satisfy the updated lint/type tooling.
Status / Verification (per repo guidelines)
- Status: Review complete; 1 maintainability suggestion raised.
- Current mode: Reviewer
- Completed work: Inspected CI workflow changes, new lint configs, and type/lint cleanups; validated removed imports/usages via code search; left one targeted maintainability comment.
- Files changed: See “Reviewed Changes” table below.
- Verification status: NOT RUN (no CI/test execution performed in this review environment).
- Remaining work: Consider addressing the stored comment in
ebuild/cli/integration.py. - Known risks: CI strictness may surface additional pre-existing lint/type issues in paths not covered by this diff in future changes.
- Assumptions: Repo targets a Python version compatible with the
importlib.metadata.entry_points()compatibility logic and Ruff’spyproject.tomlconfiguration format. - Recommended next step: Apply the small typing/maintainability adjustment suggested in the PR comment, then merge.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds yamllint to CI and removes continue-on-error for ruff/mypy, making these steps gating. |
.github/workflows/video-build.yml |
Removes trailing whitespace/blank line to satisfy YAML linting. |
.yamllint.yml |
Introduces yamllint configuration and ignore list for consistent YAML linting. |
codecov.yml |
Normalizes YAML truthy values (yes/no → true/false) for yamllint compatibility. |
hardware/board/eradar360.yaml |
Adjusts indentation/alignment to satisfy YAML lint rules. |
recipes/freertos.yaml |
Fixes indentation/formatting for YAML lint compliance. |
pyproject.toml |
Adds [tool.ruff] configuration previously provided via CLI flags/other config. |
ebuild/__main__.py |
Formatting-only change (indent/whitespace) to satisfy linters. |
ebuild/build/dispatch.py |
Removes unused typing import(s) after lint cleanup. |
ebuild/build/ninja_backend.py |
Removes unused re import after lint cleanup. |
ebuild/cli/__init__.py |
Formatting-only change (indent/whitespace) to satisfy linters. |
ebuild/cli/integration.py |
Tightens typing annotations for collections used in integration helpers (incl. initramfs builder). |
ebuild/deliverable_packager.py |
Adds mypy suppression for conditional import redefinition in fallback import path. |
ebuild/deps/__init__.py |
Adds explicit typing for DEFAULT_CONFIG to satisfy type checking. |
ebuild/eos_ai/__init__.py |
Adds type-ignore annotations for conditional imports that may resolve to None. |
ebuild/plugins/__init__.py |
Adds mypy suppression and clarifying comment for entry_points() API differences across Python versions. |
ebuild/system/doctor.py |
Removes unused typing import(s) after lint cleanup. |
tests/ebuild/test_dispatch.py |
Removes unused imports after lint cleanup. |
tests/ebuild/test_eos_ai.py |
Formatting-only change (indent/whitespace) to satisfy linters. |
tests/ebuild/test_integration_initramfs_security.py |
Removes unused subprocess import after lint cleanup. |
tests/unit/test_cad_pipeline.py |
Removes unused pytest import after lint cleanup. |
tests/unit/test_doctor.py |
Removes unused imports after lint cleanup. |
tests/unit/test_empty_test_run_is_not_a_pass.py |
Removes unused pytest import after lint cleanup. |
tests/unit/test_package_efw.py |
Removes unused import (verify) after lint cleanup. |
tools/cad_pipeline.py |
Removes unused typing import(s) after lint cleanup. |
Review details
Suppressed comments (1)
ebuild/cli/integration.py:301
hardlink_inodesis typed with anOptional[...]key solely to allow.get(None), butNoneis never stored as a key. This makes the mapping less precise than needed and can hide mistakes (e.g., accidentally persisting theNonesentinel). Prefer keeping the dict keyed by the actual hardlink tuple and only calling.getwhenhardlink_keyis notNone.
hardlink_inodes: Dict[Optional[Tuple[int, int]], int] = {}
hardlink_data_written = set()
next_inode = 1
with gzip.GzipFile(filename=str(initramfs), mode="wb", mtime=0) as out:
- Files reviewed: 20/25 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#113 "style: integrate yamllint, move ruff config to pyproject.toml, fix linter issues"
head: 5eb8ba0 author: hanggrian ci: pending (no checks reported)
Verdict: Net positive and worth landing. Removing continue-on-error from the ruff and mypy steps is a genuine strengthening — the file's own comment records that the type check "was doing no work at all" — and adding a yamllint gate is new coverage. But the green ruff gate is achieved partly by widening the ignore list from one rule to seven, which suppresses 239 current violations rather than fixing 7 real ones plus 130 auto-fixable ones. One of the newly suppressed violations is a live defect in a linker-script generator.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | pyproject.toml:43-51 | Lint ignore list widened from 1 rule to 7 in the same change that makes ruff blocking. master's CI ran ruff check . --select=E,F,W --ignore=E501; this head keeps select = ["E","F","W"] but adds E731, E741, F403, F405, F541, F841. Verified against this head: those six rules currently flag 239 violations (F541 130, F405 102, E741 4, E731 1, F403 1, F841 1). Of those, 130 F541 are auto-fixable (ruff check . --select=F541 --fix, reported by ruff as [*] 130 fixable), and only 7 are non-cosmetic. Per .github/STANDARDS.md and the brief's weakened-check rule, a loosened lint is a finding regardless of the reason. |
Do not blanket-ignore. Three targeted steps: (a) run ruff check . --select=F541 --fix and drop F541 from ignore — 130 of the 239 disappear with one command; (b) scope the star-import rules to where they are actually required rather than repo-wide (see finding 2); (c) fix the 6 remaining (1 F841, 4 E741, 1 E731 — all listed below) and drop E731, E741, F841 from ignore too. |
| 2 | Medium | pyproject.toml:47-48 | F403/F405 are ignored repo-wide to accommodate one directory. The inline comment is accurate about the cause — "manim requires it in promo/" — and I confirmed it: all 103 F403/F405 violations are under promo/, zero elsewhere. But the suppression is global, so a future from x import * anywhere in the shipped ebuild/ package will not be flagged. |
Use a scoped exemption instead of a global ignore:[tool.ruff.lint.per-file-ignores]"promo/*" = ["F403", "F405"]and remove F403/F405 from the global ignore list. Same effect on CI, no loss of coverage over ebuild/. |
| 3 | Medium | tools/cad_pipeline.py:194 | The single F841 being suppressed is a live defect, in a linker-script generator. heap = find("HEAP") is assigned and never read — grep -n "heap|HEAP" tools/cad_pipeline.py returns exactly one line, the assignment. The surrounding function builds a linker script from board memory regions and validates flash and ram at :198 (raise ValueError("Board must define at least FLASH and RAM...")) but does nothing with heap. A board YAML that declares a HEAP region therefore has it silently dropped from the generated script. This is pre-existing, not introduced here — but this PR edits tools/cad_pipeline.py (the typing import at :25) and permanently suppresses the rule that surfaces it. |
Out of scope to fix here, but do not bury it: keep F841 enabled and add # noqa: F841 with a TODO on :194, or open an issue and reference it. Either way the rule should stay on for the other ~40 modules. |
| 4 | Medium | .github/workflows/ci.yml:51-62 | Three steps flip from advisory to blocking on a branch where CI has never run. continue-on-error: true is removed from ruff and mypy, and yamllint is added with no continue-on-error. checks.txt is empty and statusCheckRollup is empty — no workflow has executed on this head. I verified ruff check . passes clean under the new config, but yamllint . and the new mypy --exclude '^(layers|core|promo)/' invocation are unverified (neither tool is installed in my environment). If either fails, this merges a red gate onto master for every subsequent PR. |
Maintainer: approve the fork workflow run and confirm all three steps are green before merging. This is the one thing that must not be taken on trust, because the whole point of the PR is that these steps now block. |
| 5 | Low | .github/workflows/ci.yml:62 | mypy's exclude widens from ^layers/ to ^(layers|core|promo)/. For core/ this is defensible and I am not objecting: it is a vendored upstream mirror (core/UPSTREAM.yaml, core/eos, core/eboot, 518 files, only 4 Python), and linting a vendored mirror is noise. promo/ (2 manim scripts) is a convenience carve-out. Note the change is not a net loss — mypy's result was previously discarded by continue-on-error, so it gated nothing at all before. |
State the reason in the workflow comment, as the existing ^layers/ carve-out already does, so the next person does not have to reconstruct it. |
| 6 | Low | pyproject.toml:35-39, .yamllint.yml:3-5, .github/workflows/ci.yml:62 | The three linters now have three different exclude sets, and nothing keeps them in step. ruff excludes .venv, core, layers/eosuite; yamllint ignores .venv/, core/; mypy excludes layers, core, promo. So layers/ is fully excluded from mypy, only layers/eosuite from ruff, and not at all from yamllint; promo/ is excluded only from mypy. |
Pick one intended scope and make the three configs agree, or add a one-line comment in each naming what it deliberately differs on. Not urgent, but this is the drift .github/STANDARDS.md warns about for duplicated lists. |
| 7 | Low | (PR body) | Unsupported and self-contradictory test claims. Every box under Testing is unchecked — including "Unit tests pass" — while the Pre-Submission Checklist checks "[x] All existing tests pass". No command or output is given for either. "[x] Code compiles without warnings (-Wall -Wextra -Werror for C)" is also checked, but the diff contains no C. The only evidence offered is a screenshot. Per the brief, an unsupported "tests pass" assertion is itself the finding. Separately, the Changes list says tests/unit/cad_pipeline.py; the file actually changed is tools/cad_pipeline.py. |
Paste the actual pytest, ruff, yamllint and mypy invocations and their tails, or uncheck the boxes. Uncheck the C-compiler line. Fix the path. |
Architecture conformance
Conforms. Master design §21 places ebuild in Tier 1 — Foundation and lists .github/CI templates under Infrastructure; this change touches only lint configuration, CI workflow, and cosmetic Python/YAML cleanups. No #include, import, link line or manifest dependency changes direction — the only Python import edits are removals of now-unused typing and stdlib names (Set, re, Optional, subprocess, pytest) plus additions of Any/Tuple used immediately in new annotations at ebuild/cli/integration.py:143,163,297. Nothing points up a tier. §21.1 is not in play.
.github/STANDARDS.md does not prescribe a lint rule set, so moving ruff config into pyproject.toml is a free choice and a reasonable one — it makes local ruff check . match CI, which is what §9.2's "one source of truth for CLI, VS Code and EoStudio" is getting at.
No test was disabled and no assertion was removed. I checked each test-file change: all seven are removals of unused imports only (ALL_BACKENDS and Path in tests/ebuild/test_dispatch.py, pytest in test_cad_pipeline.py/test_doctor.py/test_empty_test_run_is_not_a_pass.py, run_all in test_doctor.py, verify in test_package_efw.py, subprocess in test_integration_initramfs_security.py). No skip, xfail, or deleted assertion anywhere in the diff.
The recipes/freertos.yaml change (11+/11-) is a CRLF→LF line-ending normalisation, not a content change: file reports ASCII text, with CRLF line terminators on origin/master and ASCII text at this head. The checksum: sha256:eebd58aa... and url: lines are byte-identical. The hardware/board/eradar360.yaml change is comment realignment only — every address literal is unchanged.
One pre-existing issue this PR is well placed to notice but does not touch, flagged only so it is not lost: .github/workflows/ci.yml:84 still passes --cov-fail-under=0, a coverage gate that cannot fail, and :88 keeps continue-on-error: true on the benchmark step. Both are outside this diff and I am not asking for them here.
Proposed changes
Smallest sequence that keeps CI green while not losing coverage:
- Auto-fix the cosmetic bulk and drop the rule — removes 130 of 239 violations with no manual edits:
ruff check . --select=F541 --fix
- Scope the manim exemption instead of globalising it, in
pyproject.toml:
[tool.ruff.lint]
select = ["E", "F", "W"]
ignore = ["E501"] # line too long
[tool.ruff.lint.per-file-ignores]
"promo/*" = ["F403", "F405"] # manim's `from manim import *` is required-
Fix the six remaining violations — all of them, exhaustively:
tools/cad_pipeline.py:194—heapunused (finding 3);# noqa: F841+TODOif the HEAP-region fix is deferred.tests/unit/test_add_and_summary.py:171,172,173andtests/unit/test_golden_path_commands.py:165— rename the loop variablel.scripts/check_vendor_drift.py:76— convert the assigned lambda to adef.
Then
ignoreneeds onlyE501, and no rule class is silently off. -
Before merge, confirm
yamllint .andmypy . --ignore-missing-imports --no-strict-optional --exclude '^(layers|core|promo)/'both exit 0 in CI (finding 4). -
Fix the PR body (finding 7).
Not checked
- CI. Nothing ran.
checks.txtis 0 bytes;gh pr viewreports an emptystatusCheckRollupandmergeStateStatus: BLOCKED. This is a fork PR (isCrossRepository: true, head ownerhanggrian, head branchmaster) awaiting workflow approval. I did not trigger a run. yamllint .— not run, tool not installed in my environment. I therefore cannot say whether the new blocking yamllint step passes, or whether.yamllint.yml'signorelist (.venv/,core/) is sufficient for the 65 YAML files undercore/plus everything underlayers/andpromo/, which it does not ignore. This is the single largest unverified risk in the PR.mypy— not run, tool not installed. The widened--exclude '^(layers|core|promo)/'and the three new# type: ignore[...]comments (ebuild/deliverable_packager.py:29,ebuild/eos_ai/__init__.py:29-31,ebuild/plugins/__init__.py:44) are unverified; I confirmed only that each carries an explanatory comment and a specific error code rather than a bare# type: ignore.pytest— not run for this PR. I verified by reading the diff that no assertion or test was removed, but I did not execute the suite;clickandpytest-covare absent from my environment.- Python/OS matrix. My
ruff check .run was on Linux with a single ruff build. The repo matrix is 3.10/3.11/3.12 × ubuntu-22.04/macos-latest/windows-2022; none of those nine combinations were exercised. - The screenshot in the PR body I did not open or verify; it is not usable as evidence in a written review.
- The local
ebuildclone has a dirty working tree (4 files, reported by the sync step) and was left untouched. Allmasterstatements come fromorigin/masterand all head statements from the fetched head object, extracted withgit archiveinto a temp directory.
Automated architecture review of 5eb8ba06cf6d — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Summary
This PR integrates yamllint into CI to enforce consistency in YAML files. It disables
continue-on-error, forcing the job to exit if there is any issue with yamllint, ruff or mypy.Type of Change
Changes
ebuild/__main__.pyebuild/build/dispatch.pyebuild/build/ninja_backend.pyebuild/cli/__init__.pyebuild/cli/integration.pyebuild/deliverable_packager.pyebuild/deps/__init__.pyebuild/eos_ai/__init__.pyebuild/plugins/__init__.pyebuild/system/doctor.pytests/ebuild/test_dispatch.pytests/ebuild/test_eos_ai.pytests/ebuild/test_integration_initramfs_security.pytests/unit/test_cad_pipeline.pytests/unit/test_doctor.pytests/unit/test_empty_test_run_is_not_a_pass.pytests/unit/test_package_efw.pytests/unit/cad_pipeline.py.github/workflows/ci.yml.github/workflows/video-build.yml.yamllint.ymlcodecov.ymlhardware/board/eradar360.yamlrecipes/freertos.yamlpyproject.tomlTesting
Pre-Submission Checklist
Related IssuesScreenshots / Logs
Additional Notes