test(dispatch): cover configure no-op backends - #108
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#108 "Fix configure backend dispatch"
head: 39d1d2a author: Tanisha1723 ci: none reported (checks.txt is 0 bytes)
Verdict: The three tests this adds are correct, correctly targeted and cover something that was uncovered. But the change the title and body describe is not in the PR: changedFiles is 1 and it is a test file, ebuild/build/dispatch.py is untouched, and the else branch the body says was removed is still on master. One of the four backends the body claims to have verified would have failed the very test being added, and the reported test count matches neither master nor this branch.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | — (PR body vs. diff) | The described code change is absent. The title is "Fix configure backend dispatch" and the body's first line is "Fix BackendDispatcher.configure() so that supported build backends without dedicated configure steps are handled as intentional no-ops instead of falling through to invalid control flow", with "Removed the unreachable/invalid else branch in BackendDispatcher.configure()" listed under Changes. pr.json reports changedFiles: 1, additions: 12, deletions: 0, and files.txt is a single line: tests/ebuild/test_dispatch.py. I applied the patch to origin/master and grep -n "raise _unknown_backend" ebuild/build/dispatch.py still returns line 196 — the else is exactly where it was. So this PR fixes nothing; it adds tests to unchanged code. Worth adding, in fairness to the author, that the intended change would not have been a fix either: line 173 already rejects anything outside CONFIGURE_BACKENDS before the mkdir, and CONFIGURE_BACKENDS (line 50) is precisely the five backends the if/elif chain enumerates, so line 196 is unreachable defensive code, not "invalid control flow". Removing it is a tidy-up worth doing; it does not change behaviour, and the comment above line 173 already records why the upfront guard was put there. The createdAt / updatedAt gap (17:40:59Z → 18:55:02Z) suggests a force-push that dropped a commit. |
Either push the dispatch.py commit so the title is true, or retitle to test(dispatch): cover configure no-op backends and rewrite the body to describe what is here. The second is the smaller and, given that no behaviour changes either way, probably the right one. |
| 2 | Medium | tests/ebuild/test_dispatch.py:70 |
ninja is listed in the body as covered and verified, and it is neither in the test nor a valid input. The Changes section enumerates "supported backends that do not require a configure step: cargo, make, kbuild, ninja" and adds "Verified that these backends do not invoke subprocess.run() during configuration." The @pytest.mark.parametrize list is ["cargo", "make", "kbuild"] — three, not four. Had ninja been included it would have failed, not passed: CONFIGURE_BACKENDS at dispatch.py:50 is {"cmake", "meson", "cargo", "make", "kbuild"}, so configure("ninja") raises at line 174 before reaching any no-op. I ran it with ninja added: 1 failed, 30 passed, with UnknownBackendError: Unknown build backend 'ninja'. BackendDispatcher can configure: cargo, cmake, kbuild, make, meson. ebuild's own ninja backend is invoked directly rather than through BackendDispatcher, and requires 'targets' in build.yaml. The existing error message already states the correct model, so the claim contradicts something the code says out loud. |
Drop ninja from the body. If the intent was to cover it, the right test is the opposite assertion — that configure("ninja") raises UnknownBackendError — and it belongs in TestUnknownBackend below, where ninja is the most interesting case precisely because it is a supported build backend that this dispatcher deliberately does not configure. |
| 3 | Medium | — (checks.txt is empty; PR body "Testing") |
No CI ran, the quoted test count does not match this repository, and a whitespace check is offered as verification of a behavioural change. checks.txt is 0 bytes while ebuild#103 and #104 in this batch carry 24 and 30 checks. The body reports python -m pytest tests/ebuild/test_dispatch.py -v → "25 passed in 0.22s". On origin/master that file is 27 passed; with this patch applied it is 30 passed (27 + 3 parametrised cases). 25 matches neither, so the run was against some other tree — plausibly one that predates recent additions to the file, which fits a branch that was not rebased. The second verification listed is git diff --check → "No whitespace errors reported", which checks trailing whitespace and conflict markers and says nothing about the change. Under the brief's rule that an unsupported "verified" is itself the finding, both lines are it. |
A maintainer approves the workflow runs. Rebase on master, re-run, and quote the real tail. Drop git diff --check from the Testing section — it is not evidence of anything the PR claims. |
| 4 | Low | tests/ebuild/test_dispatch.py:66-68 |
The new class is inserted under a section banner that belongs to the class below it. # ── BackendDispatcher — unknown backend ───── now sits immediately above TestConfigureBackends, with TestUnknownBackend — the class the banner names — following it. A reader scanning the file by banner will attribute the wrong tests to the wrong section, and the file uses these banners consistently enough that they are load-bearing for navigation. |
Move TestConfigureBackends above the banner and give it its own — # ── BackendDispatcher — configure ───── — matching the surrounding style. |
Verified clean, and worth recording because the tests themselves are the good part of this PR:
- The three new cases pass and are real coverage.
pytest tests/ebuild/test_dispatch.pygoes from27 passedonorigin/masterto30 passedwith this patch, out of tree in/tmp/eb108. Nothing in the file previously asserted that the no-op backends are no-ops. - The mock target is correct, which is the part that could easily have been wrong.
@patch("ebuild.build.dispatch.subprocess")only proves anything if the subprocess call is reached through that module's own name.dispatch.py:13isimport subprocess, and_run_or_log()— the single helper every executing branch ofconfigure()routes through — is defined indispatch.py:112and callssubprocess.runfrom that namespace. Somock_sub.run.assert_not_called()is a meaningful assertion rather than a vacuous one. Had_run_or_loglived in a sibling module, the test would have passed for the wrong reason. - The assertion tests the right property.
cargo,makeandkbuildreachpass # These backends have no separate configure step, so "did not shell out" is exactly the behaviour worth pinning: if someone later gives one of them a configure command, this test is what will notice. tmp_pathkeeps it hermetic.BackendDispatcher(tmp_path, tmp_path / "build")means theself.build_dir.mkdir(parents=True, exist_ok=True)atdispatch.py:172writes into pytest's temp directory rather than the repository.
Architecture conformance
Conforms. §21 Tier 1 — Foundation (ebuild). tests/ebuild/ is the right home per .ai/architect.md; nothing crosses a tier, nothing is a runtime dependency, and §5.1 is untouched. ebuild/build/dispatch.py is host-side build orchestration, which is §9.1's "eBuild engine — Toolchains / Packages / Targets → Dependency Graph → Configure / Build", so configure() sits directly on the design's central developer path.
One observation rather than a finding, because it is not this PR's to answer: ALL_BACKENDS (line 26) and CLEAN_BACKENDS (line 54) include ninja while CONFIGURE_BACKENDS (line 50) and BUILD_BACKENDS (line 52) do not, because ebuild's own ninja backend is driven through ebuild/build/ninja_backend.py rather than through the dispatcher. That asymmetry is deliberate and the error message explains it, which is better than most such splits manage — it is also exactly what finding 2's author appears to have missed, and a test asserting the asymmetry would document it where the next person will look.
No proposal appended. Nothing in the master design is wrong or silent here — §9.1 and §9.2 already describe the eBuild engine's structure, and this is ordinary repository work against it.
Proposed changes
- Decide what this PR is (finding 1): push the missing
dispatch.pycommit, or retitle and rewrite the body as a test-only change. Nothing else should be settled before this, because the answer changes what the rest of the review is about. - Remove the
ninjaclaim, and consider adding the opposite assertion inTestUnknownBackend(finding 2). - Rebase, get the workflow runs approved, and quote a real test tail; drop
git diff --check(finding 3). - Move the class above the section banner (finding 4).
Items 2–4 are independent and small. The three tests themselves I would take as they are.
Not checked
- Nothing ran in CI, so nothing here is independently corroborated. All counts are from this host, using a
uv-managed pytest that neededPYTHONPATH=/usr/lib/python3/dist-packagesforyaml. The full suite was not run for this PR; it has a pre-existing failure on this host from a missingninjamodule, unrelated. - I did not establish why the body describes a change that is not present. The
createdAt/updatedAtgap is consistent with a dropped commit, but I did not fetch the branch's commit list or reflog and the explanation is a hypothesis, not a finding. If adispatch.pycommit exists somewhere, finding 1 becomes a mechanical problem rather than a substantive one. ebuild/build/ninja_backend.pywas not read. The observation about theninjaasymmetry rests ondispatch.py's constant sets and its error message, plus the module's existence. Whether ninja's own path has an equivalent configure step, and whether it is tested, I do not know.- No caller of
configure()was examined. The three new tests exercise the dispatcher directly; whetherebuild configurein the CLI reaches these branches with these backend names, and whether the no-op is the right behaviour at that level, is outside what I checked. dispatch.pywas recently rewritten by #102 (27 insertions, 63 deletionsin that area) and #102 is unreviewed by this system — the ledger has no ebuild entries before this run. So the code this PR tests went in without review, and my reading ofconfigure()is a first reading rather than a re-reading.mergeStateStatus: BLOCKED,mergeable: MERGEABLE,reviewDecision: REVIEW_REQUIRED. No merge attempted.tests/ebuild/test_dispatch.pyis also touched by #103's and #104's stacked diffs, but those hunks are already onmastervia #102, so a conflict is unlikely.- The local
ebuildcheckout is dirty and was skipped by the sync step, and sits on branchv90. I readorigin/masterthroughgit showandgit archive; the working tree was not touched and all patched trees are under/tmp.
Automated architecture review of 39d1d2adc398 — 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#108 "test(dispatch): cover configure no-op backends"
head: b9bd31b author: Tanisha1723 ci: pending
Verdict: Test-only change (16 added lines, one file) that adds a parametrised no-op assertion for configure(). The tests pass and the change is architecturally harmless, but the PR body describes work that is not in the diff and reports a result that does not match the file, and the coverage it adds is almost entirely already present on master.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | (PR body) | Body claims "Removed the unreachable/invalid else branch in BackendDispatcher.configure()". The diff touches only tests/ebuild/test_dispatch.py; ebuild/build/dispatch.py is unchanged and the else: raise branch is still present at dispatch.py:195-196 on this head. Verified: git diff --stat origin/master...<head> → 1 file changed, 16 insertions(+). |
Either include the dispatch.py change or remove the claim from the body. Do not describe unmade edits. |
| 2 | Medium | (PR body) | Body lists ninja among "supported backends that do not require a configure step" and says it "Verified that these backends do not invoke subprocess.run()". ninja is deliberately excluded from CONFIGURE_BACKENDS (ebuild/build/dispatch.py:50, with the reason in the comment at :46-49), so configure("ninja") raises UnknownBackendError — an assertion that it is a silent no-op would fail, and master already has test_configure_ninja_raises_instead_of_silently_passing (tests/ebuild/test_dispatch.py:214) asserting the opposite. The added parametrize list correctly contains only cargo, make, kbuild; the body is wrong, not the test. |
Drop ninja from the body's list. |
| 3 | Low | (PR body) | Body reports pytest tests/ebuild/test_dispatch.py -v → "25 passed". Running that file at this head collects 30 tests, not 25 (30 passed in 0.06s). The stated evidence does not correspond to the committed tree. |
Re-run and paste current output. |
| 4 | Low | tests/ebuild/test_dispatch.py:69-79 | Duplicate coverage. test_no_configure_step_backends_stay_noops already exists on origin/master at tests/ebuild/test_dispatch.py:230 and already loops ("cargo", "make", "kbuild") through configure() asserting no raise. The only new information in TestConfigureBackends is the mock_sub.run.assert_not_called() assertion. |
Add the @patch("ebuild.build.dispatch.subprocess") + assert_not_called() assertion to the existing test at :230 and delete the new class, rather than carrying two tests with the same name-in-spirit and the same parameter list. |
| 5 | Low | tests/ebuild/test_dispatch.py:214-222 | Coverage gap the PR was well placed to close and did not. dispatch.py:169-177 documents a specific past regression: the pre-guard code raised only from the else branch, after self.build_dir.mkdir(...), "left a stray build directory behind for a backend this step never handles". The guard at :173 now runs before the mkdir at :177, but no test asserts the side-effect-free property — test_configure_ninja_raises_instead_of_silently_passing only asserts that it raises. The mkdir could be moved back above the guard without any test failing. |
Extend the ninja test: d = BackendDispatcher(tmp_path, tmp_path / "build"); with pytest.raises(UnknownBackendError): d.configure("ninja"); then assert not (tmp_path / "build").exists(). |
| 6 | Low | (CI) | No checks reported on fix/dispatch-configure-syntax; statusCheckRollup is empty and mergeStateStatus is BLOCKED. ci.yml does declare pull_request: branches: [master, main], so the trigger is correct — this is a fork PR (isCrossRepository: true, head owner Tanisha1723) whose workflow runs are awaiting maintainer approval. Not the author's defect, but the PR carries no CI evidence at all right now. |
Maintainer: approve the workflow run so the test matrix actually gates this. |
Architecture conformance
Conforms. Master design §21 places ebuild in Tier 1 — Foundation, and §5.1 requires that eBuild "understands the complete graph but is not a runtime dependency". This diff adds a test inside tests/ebuild/, introduces no import, no target_link_libraries entry and no manifest dependency, and touches nothing outside the repo's own test tree — so no dependency direction is affected and no tier boundary is crossed. §9.2's "integrated test" rule is served, weakly, by the added case.
No weakened check: nothing is skipped, xfailed or deleted; the diff is purely additive (16+ 0-).
Proposed changes
Smallest sequence that keeps things working:
- Fix the PR body — remove the
dispatch.py/else-branch claim, removeninja, replace the "25 passed" line with real current output. Nothing in the tree needs to change for this. - Fold the new assertion into the existing test and drop the duplicate class:
# tests/ebuild/test_dispatch.py — replace the existing test at :230
@patch("ebuild.build.dispatch.subprocess")
def test_no_configure_step_backends_stay_noops(self, mock_sub, tmp_path):
"""cargo/make/kbuild are accepted-and-skipped, not errors."""
d = BackendDispatcher(tmp_path, tmp_path / "build")
for backend in ("cargo", "make", "kbuild"):
d.configure(backend) # must not raise
mock_sub.run.assert_not_called()- Close finding 5 by asserting the no-side-effect property on the rejection path (snippet in the table).
- If the intent really was to remove the dead
elseatdispatch.py:195-196, do that in a separate commit in this PR and say so — it is unreachable because:173already rejects everything outsideCONFIGURE_BACKENDSand theif/elifchain covers all five members of that set. It is dead code, not a defect, so it is not urgent.
Not checked
- CI. Nothing ran.
checks.txtis empty (0 bytes) andgh pr checks 108reports "no checks reported on the 'fix/dispatch-configure-syntax' branch". I did not trigger a run. - Test result is mine, not CI's, and on one interpreter only. I extracted the head tree with
git archiveinto a temp directory and ranpytest tests/ebuild/test_dispatch.py -q→30 passed in 0.06son Python 3.14.4. The repo's matrix targets 3.10/3.11/3.12 on ubuntu-22.04, macos-latest and windows-2022; none of those combinations were exercised. - The rest of the suite. I ran only
tests/ebuild/test_dispatch.py. I did not runtests/unit/test_dispatch.pyortests/unit/test_backend_dispatch.py, which also exist in this tree and may overlap further with the added case. - The local
ebuildclone has a dirty working tree (4 files, reported by the sync step) and was left untouched; every statement above aboutmastercomes fromorigin/masterand every statement about the PR from the fetched head object, not from the working tree.
Automated architecture review of b9bd31b76331 — 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.
b9bd31b to
316070f
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#108 "test(dispatch): cover configure no-op backends"
head: 316070f author: Tanisha1723 ci: none reported (checks.txt is 0 bytes; gh pr checks 108 → "no checks reported on the 'fix/dispatch-configure-syntax' branch")
Verdict: Follow-up. The three body-accuracy findings from the last review are resolved — but by editing the PR body, not by changing code. The tree at this head is byte-identical to the previously reviewed head, so the two substantive test findings (4 and 5) are untouched and remain open. Nothing new to review on its own merits, because nothing new was written.
Status of the previous review (ebuild-108-b9bd31b.md)
| Prev # | Sev | Status | Evidence |
|---|---|---|---|
| 1 | Medium | Resolved | Body no longer claims the else branch was removed. Current body: "This PR is test-only; no production behavior is changed." Consistent with the diff (1 file changed, 16 insertions(+)). |
| 2 | Medium | Resolved | Body now reads "ninja is intentionally not included because BackendDispatcher.configure() does not support configuring eBuild's own ninja backend" — which is the correct reason, and matches the comment at ebuild/build/dispatch.py:46-49. |
| 3 | Low | Resolved | Body now reports "30 passed". Measured at this head: pytest tests/ebuild/test_dispatch.py -q → 30 passed in 0.07s. The number is now real. |
| 4 | Low | Open — untouched | test_no_configure_step_backends_stay_noops is still at tests/ebuild/test_dispatch.py:246 on this head, still looping ("cargo", "make", "kbuild") through configure(). The new TestConfigureBackends at :69 parametrises the same three backends over the same call. The only new information remains the assert_not_called(). |
| 5 | Low | Open — untouched | No test asserts the no-side-effect property of the rejection path. test_configure_ninja_raises_instead_of_silently_passing (:214) still only asserts that it raises. dispatch.py:169-172 documents the exact regression this would pin ("left a stray build directory behind"), and the mkdir could still be moved back above the guard at :173 without any test failing. |
| 6 | Low | Open — maintainer action | Still zero checks. isCrossRepository: true, mergeStateStatus: BLOCKED, reviewDecision: REVIEW_REQUIRED. Fork workflow runs are still awaiting approval. |
Findings 1 and 4 of the first review (ebuild-108-39d1d2ad.md) — wrong title, misplaced section banner — were already resolved at b9bd31b7 and are not restated.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | (branch history) | The new commit changes nothing. 316070f8 "test(dispatch): organize configure backend coverage" has parent 39d1d2ad — the first reviewed head — so the branch was force-pushed over b9bd31b7 rather than built on it. Its tree is identical: git rev-parse 316070f8^{tree} and git rev-parse b9bd31b7^{tree} both give 3e1620a3, and git diff --stat b9bd31b7..316070f8 is empty. The commit subject says "organize configure backend coverage", but the organising (the # ── BackendDispatcher — configure ── banner) was already in b9bd31b7. A reviewer reading the commit log is told work happened that did not. |
Nothing to fix in the tree. Worth knowing that this PR has now been through three heads without the content changing since the second — the open items below are what would actually move it. |
| 2 | Low | tests/ebuild/test_dispatch.py:69-79 vs :246-250 | Carried from prev #4. Two tests, same three backends, same call, different classes and 177 lines apart. The second reader to touch this file will not know which one to extend. | Fold the assertion into the existing test and delete the new class — diff in Proposed changes. |
| 3 | Low | tests/ebuild/test_dispatch.py:214-222 | Carried from prev #5. The guard-before-mkdir ordering that dispatch.py:169-172 was written to protect is still unpinned by any assertion. |
assert not (tmp_path / "build").exists() after the pytest.raises block — diff in Proposed changes. |
Blocked / stale: stale. Two open findings, and the only commit since the last review has an empty diff. What unblocks it: a maintainer approving the fork's workflow runs (finding 6 above — no CI evidence exists at all and none can be produced by the author), plus either folding the duplicate or an explicit decision to keep both.
Verified clean at this head, unchanged from the last review and re-confirmed rather than assumed:
- 30 tests pass. Extracted
316070f8withgit archiveinto/tmp/eb108h.pWadpX,pytest tests/ebuild/test_dispatch.py -q→30 passed in 0.07s(Python 3.12, uv venv, pytest + pyyaml). - The mock target is still correct.
@patch("ebuild.build.dispatch.subprocess")binds the name that_run_or_log()actually resolves through, soassert_not_called()is a real assertion. - Nothing weakened. Diff is
16+ 0-; no skip, noxfail, no deleted assertion, no loosened lint. - Branch is current with
master.git merge-base --is-ancestor origin/master 316070f8succeeds; no rebase needed,mergeable: MERGEABLE.
Architecture conformance
Conforms. §21 places ebuild in Tier 1 — Foundation; §5.1 requires eBuild to understand the whole graph without being a runtime dependency. This diff adds a test class inside tests/ebuild/ and introduces no import, link line or manifest entry, so no dependency direction is affected and no tier is crossed. BackendDispatcher.configure() sits on §9.1's "Dependency Graph → Configure / Build" path, and §9.2's "integrated test" rule is served — weakly, given finding 2.
No proposal appended. §9.1 and §9.2 already describe this part of the engine correctly; nothing here shows the master design to be wrong, stale or silent.
Proposed changes
Neither of these is urgent; both are one edit each.
- Fold the duplicate (finding 2) — replace the existing test at
:246and deleteTestConfigureBackendsand its banner:
@patch("ebuild.build.dispatch.subprocess")
def test_no_configure_step_backends_stay_noops(self, mock_sub, tmp_path):
"""cargo/make/kbuild are accepted-and-skipped, not errors."""
d = BackendDispatcher(tmp_path, tmp_path / "build")
for backend in ("cargo", "make", "kbuild"):
d.configure(backend) # must not raise
mock_sub.run.assert_not_called()- Pin the no-side-effect property (finding 3) — extend the ninja test at
:214:
d = BackendDispatcher(tmp_path, tmp_path / "build")
with pytest.raises(UnknownBackendError, match="ninja"):
d.configure("ninja")
assert not (tmp_path / "build").exists()- Maintainer: approve the workflow run. Until that happens this PR has no CI evidence whatsoever, and neither the author nor this review can supply it.
Not checked
- CI. Nothing ran, and nothing here is independently corroborated. Every test result above is from this host, on one interpreter (3.12). The repo's matrix is 3.10/3.11/3.12 across ubuntu-22.04, macos-latest and windows-2022 — none of those nine combinations were exercised.
- The rest of the suite. I ran only
tests/ebuild/test_dispatch.py.tests/unit/test_dispatch.pyandtests/unit/test_backend_dispatch.pyexist in this tree and may overlap further with the added case; I did not run or read them. uv runon the repo's own[dev]extra fails to resolve on this host —requires-python = ">=3.8"againstflake8>=6.0, which needs>=3.8.1. Pre-existing onmaster, unrelated to this PR, and I did not check whether CI hits it (CI pins concrete Python versions, so probably not). Noted only so the venv I built by hand is not mistaken for the project's own environment.- Why the branch was force-pushed over
b9bd31b7back onto39d1d2ad. I read the commit graph, not the reflog or the author's intent. Finding 1 states what the objects show and nothing more. - No caller of
configure()was examined. Whetherebuild configurein the CLI reaches these branches with these names is outside what I checked. - The local
ebuildclone is dirty (4 files) and was skipped by the sync step. It was left untouched. I fetchedpull/108/headintoFETCH_HEADonly — no branch, no checkout, no reset — and every tree I ran against lives under/tmp.
Automated architecture review of 316070f8ecf1 — 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#108 "test(dispatch): cover configure no-op backends"
head: 67498e1 author: Tanisha1723 ci: none reported (fork PR; workflow runs still awaiting maintainer approval)
Verdict: All prior findings resolved, and the new commit is clean. Nothing open from this pipeline.
Status of the previous review (ebuild-108-316070f8.md)
| Prev # | Sev | Status |
|---|---|---|
| 1 | Low | Resolved — 67498e1e is a real change (5 insertions(+), 18 deletions(-)), not another empty-tree commit. |
| 2 | Low | Resolved in 67498e1e — TestConfigureBackends deleted; the @patch(...) + mock_sub.run.assert_not_called() assertion folded into the existing test_no_configure_step_backends_stay_noops (tests/ebuild/test_dispatch.py:232). One test for the property, not two. |
| 3 | Low | Resolved in 67498e1e — assert not (tmp_path / "build").exists() added to test_configure_ninja_raises_instead_of_silently_passing (:207). The guard-before-mkdir ordering that dispatch.py:169-172 was written to protect is now pinned; moving the mkdir back above the guard would fail this test. |
6 (from b9bd31b) |
Low | Still open, maintainer action — no checks on the branch. Not the author's to fix. |
Verified at this head: pytest tests/ebuild/test_dispatch.py -q → 27 passed in 0.06s, out of tree in /tmp (Python 3.12, uv venv). 27 rather than 30 is the expected result of collapsing three parametrised duplicates into one loop while adding one assertion — coverage is unchanged plus the new side-effect check. The body's "27 passed" matches what I measured, and the git diff --check line is gone.
Architecture conformance
Conforms, unchanged. §21 Tier 1 — Foundation; test-only, no import, link line or manifest entry, no tier crossed, §5.1 untouched.
Proposed changes
None. Merge when CI is approved and green.
Not checked
- CI. Nothing ran. All results above are from this host on Python 3.12 only; the repo's matrix (3.10/3.11/3.12 × ubuntu-22.04/macos-latest/windows-2022) was not exercised. The PR still carries no CI evidence, and cannot until a maintainer approves the fork's workflow runs.
- Only
tests/ebuild/test_dispatch.pywas run.tests/unit/test_dispatch.pyandtests/unit/test_backend_dispatch.pywere not. - The local
ebuildclone is dirty and was left untouched; the head was read viagit fetch origin pull/108/headintoFETCH_HEADand extracted withgit archiveinto/tmp. No branch, checkout or reset.
Automated architecture review of 67498e1e126b — 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
Add focused regression coverage for eBuild configure backends that intentionally have no separate configure step.
Changes
cargo,make, andkbuildconfigure backends.subprocess.run()during configuration.ninjadoes not create the build directory as a side effect.Testing
python -m pytest tests/ebuild/test_dispatch.py -v— 27 passedThis PR is test-only; no production behavior is changed.