Skip to content

test(dispatch): cover configure no-op backends - #108

Merged
srpatcha merged 6 commits into
embeddedos-org:masterfrom
Tanisha1723:fix/dispatch-configure-syntax
Sep 8, 2026
Merged

test(dispatch): cover configure no-op backends#108
srpatcha merged 6 commits into
embeddedos-org:masterfrom
Tanisha1723:fix/dispatch-configure-syntax

Conversation

@Tanisha1723

@Tanisha1723 Tanisha1723 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Add focused regression coverage for eBuild configure backends that intentionally have no separate configure step.

Changes

  • Strengthened coverage for cargo, make, and kbuild configure backends.
  • Verify these backends do not invoke subprocess.run() during configuration.
  • Verify that rejecting ninja does not create the build directory as a side effect.
  • Consolidated the no-op assertion into the existing configure test coverage.

Testing

  • python -m pytest tests/ebuild/test_dispatch.py -v — 27 passed

This PR is test-only; no production behavior is changed.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py goes from 27 passed on origin/master to 30 passed with 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:13 is import subprocess, and _run_or_log() — the single helper every executing branch of configure() routes through — is defined in dispatch.py:112 and calls subprocess.run from that namespace. So mock_sub.run.assert_not_called() is a meaningful assertion rather than a vacuous one. Had _run_or_log lived in a sibling module, the test would have passed for the wrong reason.
  • The assertion tests the right property. cargo, make and kbuild reach pass # 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_path keeps it hermetic. BackendDispatcher(tmp_path, tmp_path / "build") means the self.build_dir.mkdir(parents=True, exist_ok=True) at dispatch.py:172 writes 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

  1. Decide what this PR is (finding 1): push the missing dispatch.py commit, 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.
  2. Remove the ninja claim, and consider adding the opposite assertion in TestUnknownBackend (finding 2).
  3. Rebase, get the workflow runs approved, and quote a real test tail; drop git diff --check (finding 3).
  4. 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 needed PYTHONPATH=/usr/lib/python3/dist-packages for yaml. The full suite was not run for this PR; it has a pre-existing failure on this host from a missing ninja module, unrelated.
  • I did not establish why the body describes a change that is not present. The createdAt/updatedAt gap 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 a dispatch.py commit exists somewhere, finding 1 becomes a mechanical problem rather than a substantive one.
  • ebuild/build/ninja_backend.py was not read. The observation about the ninja asymmetry rests on dispatch.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; whether ebuild configure in 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.py was recently rewritten by #102 (27 insertions, 63 deletions in 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 of configure() is a first reading rather than a re-reading.
  • mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. No merge attempted. tests/ebuild/test_dispatch.py is also touched by #103's and #104's stacked diffs, but those hunks are already on master via #102, so a conflict is unlikely.
  • The local ebuild checkout is dirty and was skipped by the sync step, and sits on branch v90. I read origin/master through git show and git 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.

@Tanisha1723 Tanisha1723 changed the title Fix configure backend dispatch test(dispatch): cover configure no-op backends Sep 3, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Fix the PR body — remove the dispatch.py/else-branch claim, remove ninja, replace the "25 passed" line with real current output. Nothing in the tree needs to change for this.
  2. 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()
  1. Close finding 5 by asserting the no-side-effect property on the rejection path (snippet in the table).
  2. If the intent really was to remove the dead else at dispatch.py:195-196, do that in a separate commit in this PR and say so — it is unreachable because :173 already rejects everything outside CONFIGURE_BACKENDS and the if/elif chain 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.txt is empty (0 bytes) and gh pr checks 108 reports "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 archive into a temp directory and ran pytest tests/ebuild/test_dispatch.py -q30 passed in 0.06s on 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 run tests/unit/test_dispatch.py or tests/unit/test_backend_dispatch.py, which also exist in this tree and may overlap further with the added case.
  • The local ebuild clone has a dirty working tree (4 files, reported by the sync step) and was left untouched; every statement above about master comes from origin/master and 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.

@Tanisha1723
Tanisha1723 force-pushed the fix/dispatch-configure-syntax branch from b9bd31b to 316070f Compare September 5, 2026 10:30

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -q30 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 316070f8 with git archive into /tmp/eb108h.pWadpX, pytest tests/ebuild/test_dispatch.py -q30 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, so assert_not_called() is a real assertion.
  • Nothing weakened. Diff is 16+ 0-; no skip, no xfail, no deleted assertion, no loosened lint.
  • Branch is current with master. git merge-base --is-ancestor origin/master 316070f8 succeeds; 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.

  1. Fold the duplicate (finding 2) — replace the existing test at :246 and delete TestConfigureBackends and 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()
  1. 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()
  1. 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.py and tests/unit/test_backend_dispatch.py exist in this tree and may overlap further with the added case; I did not run or read them.
  • uv run on the repo's own [dev] extra fails to resolve on this host — requires-python = ">=3.8" against flake8>=6.0, which needs >=3.8.1. Pre-existing on master, 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 b9bd31b7 back onto 39d1d2ad. 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. Whether ebuild configure in the CLI reaches these branches with these names is outside what I checked.
  • The local ebuild clone is dirty (4 files) and was skipped by the sync step. It was left untouched. I fetched pull/108/head into FETCH_HEAD only — 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 srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Resolved67498e1e is a real change (5 insertions(+), 18 deletions(-)), not another empty-tree commit.
2 Low Resolved in 67498e1eTestConfigureBackends 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 67498e1eassert 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 -q27 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.py was run. tests/unit/test_dispatch.py and tests/unit/test_backend_dispatch.py were not.
  • The local ebuild clone is dirty and was left untouched; the head was read via git fetch origin pull/108/head into FETCH_HEAD and extracted with git archive into /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.

@srpatcha
srpatcha enabled auto-merge (squash) September 8, 2026 00:37
@srpatcha
srpatcha disabled auto-merge September 8, 2026 00:37
@srpatcha
srpatcha enabled auto-merge (squash) September 8, 2026 00:38
@srpatcha
srpatcha disabled auto-merge September 8, 2026 00:41
@srpatcha
srpatcha merged commit b77a57c into embeddedos-org:master Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants