fix: fix(ci): install the project and its dev extra instead of a requirements.txt that does not exist - #82
fix: fix(ci): install the project and its dev extra instead of a requirements.txt that does not exist#82srpatcha wants to merge 1 commit into
Conversation
…irements.txt that does not exist Opened by the scheduled autoreview pipeline after review of open PRs. Reviewed against the EmbeddedOS Master Design v2.0. Files: .github/workflows/ci.yml
srpatcha
left a comment
There was a problem hiding this comment.
Review — eDB#82 "fix(ci): install the project and its dev extra instead of a requirements.txt that does not exist"
head: ef7e563 author: srpatcha ci: fail (6 of 9 matrix legs failed; 3 still queued — detail below)
Role boundary. This PR was opened by this same autoreview pipeline (branch
autofix/ci-install-project-deps, and the body says so)..ai/reviewer.md— "if you implemented it, you do not approve it" — applies. Nothing below is an approval or a merge verdict, andpost-review.shposts a plain comment only. Read this as a second pass over an automated change that no human has reviewed, with everything I could re-run independently re-run.
Verdict: The diagnosis and the one-line fix are correct and I reproduced both the fix and the failure it exposes. Conforms architecturally. Two things stop it short of doing the whole job: the Python 3.10 matrix leg still cannot install, for a reason this PR identified but declined to fix in the file it is already editing; and pytest-benchmark is installed unpinned outside pyproject.toml, which is the exact pattern the PR's own rationale argues against.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/ci.yml:17 |
The 3.10 matrix leg still fails at the install step and this PR cannot make it pass: pyproject.toml:11 declares requires-python = ">=3.11". Verified — job 100915594820 (Test (Python 3.10), ubuntu-22.04), step 4 Install dependencies = failure, everything after it skipped. The PR body names this but leaves it. The result is a workflow that stays red for two independent reasons, one of which is a one-line edit in the file already being changed. |
Resolve it in this PR rather than deferring: either drop "3.10" from the matrix at ci.yml:17, or lower requires-python. Which one is a maintainer policy call about the supported Python range — but shipping a CI fix that knowingly leaves a third of the matrix unable to install is not a neutral default. If it must be deferred, open the follow-up issue and link it here. |
| 2 | Low | .github/workflows/ci.yml:39-40 (new) |
pip install pytest-benchmark installs a test dependency unpinned and undeclared, immediately after a comment block arguing that dependencies belong in pyproject.toml so the declared floors take effect. pytest-benchmark gets no floor and is invisible to Dependabot and to the version-floor PRs the body cites. |
Add "pytest-benchmark>=4.0.0" to [project.optional-dependencies].dev in pyproject.toml:38-45 (or a separate bench extra) and reduce the step to the single pip install -e ".[dev]". Then the install step has one line and one source of truth, which is what the PR set out to achieve. |
| 3 | Low | .github/workflows/ci.yml:13 |
name: Test (Python ${{ matrix.python-version }}) omits matrix.os, so the 3×3 matrix at ci.yml:17-18 produces 9 jobs under 3 distinct check names. This is why the review bundle's checks.txt lists three Test (Python …) rows for nine legs, and why the two failing windows-2022 legs are invisible in it. It also makes branch-protection required-check names ambiguous. Pre-existing, not introduced here, but it directly obscures this PR's own effect. |
name: Test (Python ${{ matrix.python-version }} on ${{ matrix.os }}). Note this renames required checks, so branch protection must be updated in the same change. |
| 4 | Low | run 33838434123 |
The three macos-13 legs have been queued since 2026-09-04T04:52:50Z — about 45 minutes at the time of this review — with conclusion: null, so the run never reaches completed. macos-13 has been retired as a GitHub-hosted image. Consequence beyond a slow run: build has needs: test (ci.yml:67) and never starts, and gh run view --log-failed refuses with "run is still in progress", so nobody can read the failure logs. Pre-existing and outside this diff. |
Move the matrix to a supported macOS image (macos-14/macos-latest), and add a timeout-minutes to the test job so a runner that never picks up fails loudly instead of hanging the workflow. |
Correct, and worth stating explicitly: the PR refuses to touch .coveragerc:14 (fail_under = 100) or widen --cov. That is the right call — lowering a coverage gate inside a bot-authored PR to make a build go green is precisely the weakened-check pattern the review brief §4 treats as a finding. The "What this does NOT fix" section is accurate rather than optimistic, and the honesty about uv pip ≠ pip and about the untested matrix legs is the standard the brief asks for.
Architecture conformance
Conforms. CI-configuration change only, confined to Tier 3 eDB (master design §21). No source file, import, package dependency or manifest entry changes, so §5.1 dependency direction is untouched — nothing here can make a lower tier depend on a higher one. .github/STANDARDS.md is unaffected: no tag scheme, release-branch or compliance-assertion change. Making pyproject.toml's declared floors actually take effect in CI moves the repo toward the §28 evidence policy, since a version constraint no check consumes is an unevidenced claim.
Verification I ran
At the PR head (git archive ef7e563b | tar -x -C /tmp/edb-82-head), Python 3.12.14 on Linux:
uv venv --python 3.12 .venvthenuv pip install -e ".[dev]" pytest-benchmark→ rc 0. The install the PR proposes does resolve.- The workflow's own unit-test command,
python -m pytest tests/unit/ -q --tb=short --cov=. --cov-report=xml --cov-report=term-missing→ rc 1,23 passed in 0.41s,TOTAL 905 458 152 7 46.93%,FAIL Required test coverage of 100.0% not reached. Total coverage: 46.93%. The predicted post-fix failure reproduces exactly, including the 46.93% figure. - The coverage report body confirms the body's
--cov=.claim:tests/unit/test_unit_core.pyandtests/unit/test_unit_kv.pyappear as measured files despite.coveragerc:5-11listingomit = *tests*, so the omit list is not taking effect under--cov=..src/edb/exists (pyproject.toml:59-60,where = ["src"]), so the body's suggested--cov=src/edbis a real path. git show origin/master:requirements.txt→fatal: path 'requirements.txt' does not exist in 'origin/master';git ls-tree -r origin/master --name-only | grep -i requirements→ no matches. The premise holds.- CI step-level outcomes read from the API rather than from
checks.txt: 3.11 and 3.12 on ubuntu-22.04 now passInstall dependencies,Lint (ruff)andType check (mypy)— all three of which were skipped before this change because the install died first — and fail atRun unit tests. 3.10 on ubuntu-22.04 and all threewindows-2022legs fail. So the change does what it claims: ruff and mypy are running in CI for the first time on this workflow.
Not checked
- The failing step's log was never read.
gh run view --log-failedreturns "run is still in progress" because of the queuedmacos-13legs (finding #4). That the ubuntu 3.11/3.12 failure is the coverage gate is inferred from the local reproduction above plus the step-level outcome, not read from the CI log. It is consistent, but I did not confirm it from the job output. - The three
windows-2022failures were not diagnosed at all — same log blockage. They may fail at install or at the same coverage gate; I make no claim. Nothing in the PR body addresses them either. - The
macos-13legs have no conclusion. I am reporting an observed 45-minute queue and the fact that the image is retired; I did not establish that they will never run. - Python 3.10 and 3.11 were not exercised locally; only 3.12. Finding #1 rests on the CI step outcome plus
requires-python, which is sufficient for that specific claim. pipwas not used —uv pipwas, same as the PR body. The workflow usespip. They resolve the same metadata but are not the same resolver.- The
Run performance benchmarksandUpload coveragesteps were not run locally and were skipped in CI.
Automated architecture review of ef7e563be77e — 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.
What this fixes
.github/workflows/ci.ymlinstalls dependencies with:There is no
requirements.txtin this repository.git show origin/master:requirements.txtfails at any path. So the
Test (Python …)job cannot get past its install step, on anymatrix leg, and has not been able to for as long as the file has been missing. The most
recent
CI — eDBrun onmastercontains:The second line then installs five tools unconstrained and never installs the project or
its
[project.optional-dependencies].devextra — which is whereruffandmypyareactually declared. That has a knock-on effect: the version floors in
pyproject.tomlareread by nothing in CI, so the open dependency PRs that edit them (#71 mypy, #72
cryptography, #73 ruff) change a number no check consumes.
This replaces both lines with
pip install -e ".[dev]", pluspytest-benchmark, which thebenchmark step at
ci.yml:53-55needs and which is not part of thedevextra.Where this came from
The autoreview pass on #71, #72 and #73 (2026-09-03). All three reviews rate this High;
this is the fix for the finding they share.
What I ran to verify
In a clean worktree off
origin/masterwith the patch applied, Python 3.12.14 on Linux:Resolved and installed:
edb 3.0.1,pytest 9.1.1,pytest-cov 7.1.0,pytest-asyncio 1.4.0,httpx 0.28.1,ruff 0.16.6,mypy 2.3.1,cryptography 50.0.1,fastapi 0.141.1,pydantic 2.13.5,pytest-benchmark 5.3.0.Then the workflow's own test commands against that environment:
What this does NOT fix — please read before merging
The job will still be red after this change, for a different and now-visible reason.
The unit-test step exits 1 despite all 23 tests passing:
That comes from
.coveragerc:14,fail_under = 100, combined with--cov=.inci.yml:44-46measuring the whole tree includingtests/. I have deliberately not touchedeither. Lowering a coverage gate is a policy decision for the maintainers, and changing it
in a bot-authored PR would be weakening a check to make a build pass — which is exactly the
kind of change these reviews flag. Narrowing
--cov=.to--cov=src/edbwould be thesmaller, more defensible change of the two, but it is still yours to make.
So what this PR does is turn an uninformative failure ("cannot find a file that does not
exist") into a real one ("coverage gate not met"), and make the declared dependency floors
take effect. It is a prerequisite for #71/#72/#73 being meaningful, not a green build.
Also not verified here:
uv pip, not with thepipthe workflow uses;uvresolves the same package metadata, but they are not thesame program.
macos-13/windows-2022legs were not. Note thatpyproject.toml:11declaresrequires-python = ">=3.11"whileci.yml:17lists"3.10", so the 3.10 leg may nowfail at install where it previously failed at
requirements.txt— a pre-existinginconsistency this change makes visible. Dropping
"3.10"from the matrix, or loweringrequires-python, is the follow-up.🤖 Opened by the EmbeddedOS autoreview pass. Nobody reviewed this before it appeared; treat
it as a proposal, not a verified change.
Opened by the scheduled autoreview pipeline (model
claude-opus-5), branched fromorigin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.