ci: give each matrix leg its own check name - #104
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confirmed on a real run
Against the same query on #103's head: Nine rows, every count
|
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#104 "ci: give each matrix leg its own check name"
head: 2350470 author: Kartikey1306 ci: pass (28 checks pass, assign and Create GitHub Release skipping, 0 fail)
Verdict: A one-line fix with a good reason and, unusually, evidence I could confirm without taking anyone's word for it — the bundle's own checks.txt shows all nine Test (Python X, os) names, each appearing exactly once. I ran the two new tests and reproduced the mutation the body leads with. One finding: the uniqueness guard models only one of the two ways GitHub expresses a matrix, and the form it misses is already used twice elsewhere in this repository.
Scope note. gh pr diff shows 18 files and 1076 additions because the branch is stacked on #103, which is stacked on #102, and #102 merged as e5d8052. Against current master this PR is ahead 2, behind 1 over the same two files as #103 — .github/workflows/ci.yml and tests/unit/test_ci_gate.py. Its own contribution is one name: template line plus two test functions; the rest is #103's gate, reviewed separately.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_ci_gate.py — _expanded_names() / _NOT_A_DIMENSION |
The guard is blind to an include:-only matrix, which is the same bug in a form the repository already uses twice. _expanded_names() builds the cartesian product of isinstance(v, list) entries and skips include/exclude. The comment justifying that is correct as far as it goes — they "shape a matrix but are not dimensions of it" — but the consequence is that they are then not modelled at all, and a matrix expressed entirely through include: has no list dimensions, so the function returns [job_name]: one name, however many legs the job really has. test_a_matrix_job_names_every_dimension_it_varies skips it for the same reason, since its dimensions list is also empty. I added a lint job to the patched ci.yml with a two-entry include: matrix and a static name: Lint, wired it into the gate, and ran the suite: 8 passed. Two check runs, both called Lint, and the test written to catch exactly that says nothing. This is not hypothetical — release.yml:67 and eosim-sanity.yml:83 are both matrix: include: with no list dimensions today, and #103's finding about the guard covering only ci.yml points at extending it to those files, at which point the blind spot becomes live. exclude: has the mirror problem and could produce a false positive by flagging a duplicate for a combination that never runs; ci.yml uses neither today (the two exclude hits there are pytest flags in comments), so that half is latent. |
Expand include: entries into the name set instead of ignoring them: for each mapping in include, substitute its keys into the template the same way, and add the result. Keep exclude out of the product by removing matching combinations rather than by ignoring the key. If that is more machinery than it is worth, the cheap alternative is honest failure: if a job has an include with more than one entry and no list dimensions, assert that its name: contains at least one ${{ matrix.… }} reference — that catches the Lint case without modelling GitHub's expansion rules. |
| 2 | Low | .github/workflows/ci.yml:97 |
The same defect this PR fixes is still present in the Codecov flag on the line below it. flags: ebuild-py${{ matrix.python-version }} interpolates matrix.python-version and not matrix.os — so after this change the nine legs report under nine distinct check names and three shared coverage flags, and the OS dimension is still lost in coverage exactly as it was in the checks list. It is the identical mistake in a sibling field four lines away, and the PR's own stated rule ("a matrix job names every dimension it varies") covers it. Low because coverage flags are not required checks and nothing is gated on them, and because merging three OS legs into one flag may well be what the maintainers want — but if so it should be a decision, not the leftover of the bug being fixed. |
Either flags: ebuild-py${{ matrix.python-version }}-${{ matrix.os }}, or a comment on that line saying the merge across OS legs is deliberate. Extending test_a_matrix_job_names_every_dimension_it_varies to cover flags: as well as name: would be over-reach; a comment is enough. |
| 3 | Low | PR body, "Why now specifically" | The "nothing refers to these names" grep is narrower than the claim it supports. grep -rn "Test (Python" ebuild/ --include='*.yml' --include='*.py' --include='*.md' covers three extensions inside one repository. It would not see a reference in .json, .toml, .sh or a workflow in embeddedos-org/.github, and it cannot see anything outside this repo — a dashboard, a status badge, or another repository's automation that names these checks. The conclusion is very probably right, and the timing argument ("cheap while nothing refers to them") is the correct instinct. It is the word nothing that is doing more work than the command supports. |
Either widen it (drop --include entirely) or soften the claim to what was actually checked. required_status_checks being null is the load-bearing half and that half is solid. |
Verified clean, executed, and worth recording because this PR's central claim is a claim about names that a reader cannot check by eye:
- The nine names are real and distinct, in this bundle.
checks.txton this head listsTest (Python 3.10, macos-latest),Test (Python 3.10, ubuntu-22.04),Test (Python 3.10, windows-2022)and the same for 3.11 and 3.12 — nine rows, each once. #103'schecks.txt, from the same repository four minutes earlier, lists three. So the before/after in the author's comment is corroborated by the run data in the bundle rather than resting on a quoted API call. That is worth saying explicitly because the equivalent comment on #103 did not survive the same test. - 8/8 pass against
origin/masterwith this PR's two files applied, out of tree in/tmp/eb104. - The headline mutation reproduces exactly. Reverting the template to
Test (Python ${{ matrix.python-version }})gives2 failed, 6 passed—test_every_check_name_is_uniqueandtest_a_matrix_job_names_every_dimension_it_varies, with the assertion readingassert not ['os']. That matches the body's table row for row, and it is the row that decides whether these are regression tests or restatements of the YAML. They are regression tests. - The rename does not disturb the gate.
needs:takes job ids, andtestis unchanged as an id;ci-gate.needsis still['test', 'build']in the patched file, andCI Gatepasses on this head in 3s. The body says this and it holds. - The matrix values are literals, so the new names are stable.
os: [ubuntu-22.04, macos-latest, windows-2022]—macos-latestappears in the check name as the literal string, not as whatever image it resolves to, so a runner rollover does not silently rename a required check. That is the property that makes these names safe to pin, and it is the whole point of the PR. ci.ymlhas no matrixinclude:orexclude:today, so finding 1 describes a hole in the guard, not a live defect in this workflow.
Architecture conformance
Conforms. §21 Infrastructure — "governance, release automation and documentation". No runtime dependency in either direction, so §5.1 is untouched and eBuild's position as something that "understands the complete graph but is never a runtime dependency" is unaffected. tests/unit/ is the right home per .ai/architect.md.
Worth naming the design principle this serves, because it is not obvious from a one-line rename: §28's evidence policy distinguishes Implemented from Validated, and Validated requires "verified on named configurations". Nine check runs sharing three names is the state in which the configuration cannot be named — a green Test (Python 3.10) did not say which of ubuntu, macOS and Windows it was green on. This change is what makes per-configuration evidence expressible at all, which is a larger contribution than the diff size suggests. §22's hardware support tiers make the same distinction for boards.
No proposal appended. §28's evidence model already requires named configurations; nothing in the master design is wrong or silent here, the repository was simply not meeting it. This is a repo-level fix, not a design gap.
Proposed changes
- Handle
include:in_expanded_names(), or add the cheap "a multi-entry include with a static name is a failure" assertion (finding 1). This is the one I would ask for before merge — not becauseci.ymlneeds it today, but because the guard's whole purpose is to still be right in six months, and #103's coverage argument points it straight at two files that use the form it cannot see. - Decide the Codecov flag question and either fix the line or comment it (finding 2).
- Soften or widen the grep claim (finding 3). Body text only.
- Merge order with #103: this branch contains #103, so merging this one alone lands both, and merging #103 first then this is also fine. What should not happen is #104 first and #103 separately afterwards — #103 would then be an empty or conflicting change. Say so in one of the two bodies.
Items 1–3 are independent.
Not checked
- The full suite was not run.
569 passedforpytest tests/is uncorroborated; I rantests/unit/test_ci_gate.pyonly, eight tests, and neededPYTHONPATH=/usr/lib/python3/dist-packagesto give this host'suv-managed pytest ayamlmodule — an environment quirk here, not a property of the repository, but it means my invocation is not the supported one. - Two of the three mutation-table rows were not reproduced. I ran "revert the template" and got the stated
2 failed of 8. "Add a third matrix dimension without naming it" and "removeosfrom the name, keep it in the matrix" I did not run. - Nothing was executed on GitHub Actions. I did not call the
check-runsAPI myself; the nine-distinct-names confirmation comes fromgh pr checksoutput captured in the bundle, which is the same information from a different endpoint. The3 Test (Python 3.10)counts quoted from #103's head are consistent with that PR'schecks.txtshowing three collapsed rows, but I did not verify the per-name run counts directly. - Branch protection was not queried.
required_status_checks is nullis taken from #87 via #103's body. If it has been set since, finding 3's premise moves and the timing argument weakens. - The blind spot in finding 1 was demonstrated on a synthetic job, not on
release.ymloreosim-sanity.yml. I confirmed those two files usematrix: include:by readingorigin/master, but the guard does not currently parse them, so I did not establish that either produces duplicate check names today — only that the form they use is the form the guard cannot model. - The sixteen files from #102 and the gate job from #103 were not re-reviewed here. #102 is on
masteralready and unreviewed by this system; #103 has its own report from this run. - The local
ebuildcheckout is dirty and was skipped by the sync step, and sits on branchv90. I readorigin/masterthroughgit showandgit archiveand modified nothing in the working tree; all patched trees are under/tmp. The PR head is not fetched locally, so ahead/behind comes fromgh api .../compare. mergeStateStatus: BLOCKED,mergeable: MERGEABLE,reviewDecision: REVIEW_REQUIRED. No merge attempted.
Automated architecture review of 2350470c4025 — 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.
embeddedos-org#87 and eos#92 both stay open on the same point: `required_status_checks` is null on master, so nothing builds the merge result before it becomes master. Marking a check required is a settings change, but there is no name here worth pointing it at. `test` is a 3x3 matrix, so it reports as nine generated names that change whenever the matrix does, and a required name that stops appearing blocks every pull request until someone edits the protection rule. `release` is skipped on every pull request, and a skipped required check never reports at all. `ci-gate` is one job, one stable name, that succeeds only if every other job that runs on a pull request succeeded. It carries `if: always()` -- without it the gate is skipped when an earlier job fails, and the pull request would wait for a status that never arrives rather than showing a red X. Any non-success result fails it, `skipped` included: a job that did not run did not verify anything. tests/unit/test_ci_gate.py fails if a job is added to the workflow without being wired into the gate, which is the way this would rot. Verified by mutation -- dropping a job from `needs`, removing `if: always()`, renaming the gate, adding an unwired job, weakening the result check to `== "failure"`, and deleting the gate are each caught. 567 tests pass.
Answers findings 1 and 2 from the review on embeddedos-org#103. Finding 2 (Medium) -- `CI Gate` gates ci.yml and nothing else, while the PR body's closing instruction ("Settings -> Branches -> master -> Require status checks -> add `CI Gate`. One name") read as the complete fix for embeddedos-org#87. It is the complete fix for ci.yml. Six other workflows produce pull-request checks here, and the rot-guard was hardcoded to ci.yml so it could not see any of them: a job added to codeql.yml or simulation-test.yml was uncovered and nothing failed. Adds two tests over every workflow with a `pull_request` trigger: - test_every_pull_request_workflow_is_accounted_for -- each must be in REQUIRED_CHECKS (gated, with the display name a maintainer requires) or in NO_GATE with a reason about the workflow itself. "It has no gate yet" is explicitly not a reason, since that is the gap being made visible. - test_gated_workflows_really_have_their_gate -- every name in REQUIRED_CHECKS resolves to a job that exists and displays under that name, so the required set cannot point at a status that never arrives. REQUIRED_CHECKS is the answer to "what does a maintainer actually require", which one name was never going to be. The six NO_GATE entries carry their reasons; simulation-test.yml's says plainly that its gate still fails open on part of its `needs`, which is why requiring it today would assert more than it checks. Finding 1 (High), the run log, is answered on the thread: the log in my 2026-09-01 comment is from eBoot, not from this repository -- those are eBoot's job names and `build-arm` is a key `jq` could never produce here. I posted the same comment on three gate PRs without adjusting it. Correction posted; re-running against this workflow for the real log. Verified: pytest tests/unit/test_ci_gate.py 8 passed pytest tests/ 1 failed, 565 passed, 3 skipped discrimination: dropping an ungated pull_request workflow into .github/workflows/ gives AssertionError: these workflows produce pull-request checks but are neither gated nor excused: ['zz-probe.yml'] so the guard catches the class it was written for. The one failure is PRE-EXISTING and not caused by this change -- confirmed by checking out origin/master and reproducing it there: test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary No module named ninja It is the `[sys.executable, "-m", "ninja"]` invocation @srpatcha raised on embeddedos-org#66: the build ignores a perfectly good `ninja` on PATH and fails when the PyPI wheel is absent. CI installs that wheel, so CI does not see it. Their offer to open the `shutil.which("ninja")` follow-up is still open and still worth taking. Refs embeddedos-org#103, embeddedos-org#87, embeddedos-org#66
The test job's `name:` interpolated the Python version but not the OS, so all three legs of each version reported under one name. Measured on embeddedos-org#103's head: $ gh api repos/embeddedos-org/ebuild/commits/<sha>/check-runs \ --jq '.check_runs[].name' | sort | uniq -c 3 Test (Python 3.12) 3 Test (Python 3.11) 3 Test (Python 3.10) Nine check runs, three names. Two consequences, one for people and one for branch protection: A Windows-only failure is indistinguishable from the ubuntu and macOS legs in the checks list -- all three rows read "Test (Python 3.10)" and you have to open them to find which failed. That is not hypothetical here: the Windows leg did not run at all until `shell: bash` was added, and once it did it surfaced failures the other two legs never saw. And a required status check names a check. With three check runs behind one name, a rule naming it cannot say which leg it means. Adding `${{ matrix.os }}` makes all nine names distinct. Doing it now, before embeddedos-org#103's `CI Gate` gives anyone a reason to start pinning check names: `required_status_checks` is null today, so nothing anywhere refers to these names -- `grep` finds the template itself and nothing else. Renaming nine checks is cheap exactly while no rule depends on them, and expensive afterwards. Two tests added to tests/unit/test_ci_gate.py: check names must be unique across the whole workflow, and a matrix job must name every dimension it varies. Verified by mutation -- reverting the template to its previous form, adding a third matrix dimension without naming it, and removing `os` from the name while leaving it in the matrix are each caught (2 failed of 8 in every case). 569 tests pass.
…ns in the flag too Answers the review on embeddedos-org#104. Finding 1 (Medium) -- _expanded_names() built the cartesian product of list dimensions and skipped `include`/`exclude` entirely. The comment justifying that was right as far as it went, but the consequence was that a matrix expressed *entirely* through `include:` has no list dimensions, so the function returned one name for however many legs the job really has. The reviewer's probe -- a `lint` job with a two-entry `include:` and a static `name: Lint` -- passed the whole suite, which is the exact defect this PR exists to catch, in the form the guard could not see. Not hypothetical: release.yml:67 and eosim-sanity.yml:83 are both `matrix: include:` with no list dimensions today, so the blind spot goes live as soon as the guard is pointed at them -- which embeddedos-org#103's REQUIRED_CHECKS table now makes a natural next step. `include` entries that refine an existing combination update it; entries that introduce new values add a leg. `exclude` removes combinations rather than being ignored, which also closes the mirror problem the finding notes -- a combination that never runs could otherwise be reported as a duplicate. Finding 2 (Low) -- the Codecov flag four lines below the name template had the identical defect: `ebuild-py${{ matrix.python-version }}` interpolates the Python version and not the OS, so after this PR nine legs reported under nine check names and three coverage flags. Fixed rather than commented, since the PR's own stated rule ("a matrix job names every dimension it varies") covers it and merging three OS legs into one flag was the leftover of the bug, not a decision. Finding 3 (Low) is a PR-body correction and is handled there: the "nothing refers to these names" grep covered three extensions in one repository and cannot support the word *nothing*. `required_status_checks` being null is the load-bearing half, and that half is solid. Verified: pytest tests/unit/test_ci_gate.py 10 passed pytest tests/ 1 failed, 567 passed, 3 skipped discrimination: with an include:-only `lint` job wired into the gate, AssertionError: these check names are produced more than once: ['Lint'] from test_every_check_name_is_unique. Before this commit the same probe passed. The one failure is PRE-EXISTING on origin/master and unrelated -- the `python -m ninja` invocation from embeddedos-org#66's open follow-up; see the note on embeddedos-org#103's commit. Refs embeddedos-org#104
2350470 to
9382536
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#104 "ci: give each matrix leg its own check name"
head: 9382536 author: Kartikey1306 ci: pass
Verdict: All three findings from the review of 2350470c are closed, and I verified the important one by rebuilding the exact case that exposed it. The change itself is right and the tests are regression tests rather than restatements of the YAML. Two small things left, both about reach rather than correctness.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | tests/unit/test_ci_gate.py:262, :280 |
A matrix job with no name: key is reported as a name collision that does not exist. _expanded_names() falls back to job.get("name", job_id) and substitutes nothing into it, so an unnamed two-leg matrix yields ["smoke", "smoke"]. GitHub auto-generates distinct names for exactly this case — smoke (ubuntu-22.04), smoke (macos-latest) — so the legs do not share a check name. Reproduced: adding an unnamed os-matrix job gives 2 failed, with test_a_matrix_job_names_every_dimension_it_varies asserting not ['os'] and the other reporting a duplicate. Pushing contributors toward explicit names is a defensible outcome; asserting a collision that GitHub would not produce is not, and the message sends someone looking for a bug that is not there. |
Model the default: when a job has no name:, expand to f"{job_id} ({', '.join(str(v) for v in combo.values())})", which is unique by construction. If that is more fidelity than it is worth, keep the failure but change the message to "give this job an explicit name:" and stop claiming the legs collide. |
| 2 | Low | .github/workflows/ci.yml:22 (scope, not the line) |
The rule this PR establishes — a matrix job names every dimension it varies — is enforced only in ebuild, while the workflow it fixes is copied across the org. The identical defect is live in four sibling repos today: eBrowser/.github/workflows/ci.yml:13, eCAD-Hardware-Products/…:13, eDB/…:13, eosllm/…:13, each name: Test (Python ${{ matrix.python-version }}) over the same 3×3 python-version × os matrix — three check runs per name, nine runs, three names, in every one of them. Fixing one repo and leaving four is how the org got five copies of the same workflow in the first place. |
Out of scope for this PR and it should not grow to cover them. Worth one line in the body pointing at the four, or an issue in embeddedos-org/.github: this belongs in the shared workflow template, alongside sync-release-branch.yml and check-canon.yml, which .github/STANDARDS.md already lists as org-inherited. |
Prior findings, all three closed
Finding 1 (Medium, the include:-only blind spot) — fixed, and I rebuilt the case that found it. _expanded_names() now expands include: entries into the name set and removes exclude: combinations from the product rather than ignoring both. Re-running the exact construction from the previous review — a lint job whose matrix is two include: entries with a static name: Lint, wired into the gate:
E assert not ['Lint']
FAILED tests/unit/test_ci_gate.py::test_every_check_name_is_unique
1 failed, 9 passed
Previously that was 8 passed. The blind spot is closed, and it closed in the form that matters — release.yml:67 and eosim-sanity.yml:83 both use matrix: include: with no list dimensions, so the guard is now correct for the files it will be pointed at next.
Finding 2 (Low, the Codecov flag) — fixed. flags: ebuild-py${{ matrix.python-version }}-${{ matrix.os }} at ci.yml:104, with a comment giving the reason. The resulting flag names (ebuild-py3.10-ubuntu-22.04 and the rest) are within Codecov's character set and length limit.
Finding 3 (Low, the narrow grep) — resolved, by running the wider one. I dropped the --include filters and searched the whole working root rather than one repo:
$ grep -rn "Test (Python" --exclude-dir=.git .
eBrowser/.github/workflows/ci.yml:13
eCAD-Hardware-Products/.github/workflows/ci.yml:13
eDB/.github/workflows/ci.yml:13
eosllm/.github/workflows/ci.yml:13
ebuild/.github/workflows/ci.yml:15
Five hits, all of them the name: template itself — no consumer of these names anywhere: no dashboard config, no badge, no other repo's automation. So the body's "nothing refers to them" conclusion is correct, and the timing argument ("cheap while nothing refers to them, expensive afterwards") holds. Note that the same search is what produced finding 2 above.
Other checks
uv run --no-project --with pytest --with pyyaml pytest tests/unit/test_ci_gate.py → 10 passed at this head. The body says 8; the second commit added two. Same stale-count nit as #103, not worth a row of its own.
checks.txt on this head carries the nine distinct names — Test (Python 3.10, macos-latest), Test (Python 3.10, ubuntu-22.04), Test (Python 3.10, windows-2022) and so on, each exactly once — against three on #103's head from the same repo minutes earlier. The before/after rests on run data in this bundle, not only on the author's quoted API call, which matters here because the equivalent comment on #103 did not survive that test.
The gate is undisturbed: needs: takes job ids, test is unchanged as an id, and CI Gate is green on this head.
Architecture conformance
Master design §5.1 and §21: conforms. ebuild is Tier 1 – Foundation; a workflow and its unit test are infrastructure inside the owning repo, no runtime dependency created, nothing moves tier.
§21.1's split policy is the lens for finding 2, in its "otherwise keep the subsystem inside the owning repository" direction: five repos maintaining five copies of one workflow by hand is the duplication the policy exists to discourage, and .github/STANDARDS.md already establishes embeddedos-org/.github as the home for org-inherited workflows. The master design itself says nothing about shared CI templates — §21 lists ".github, website, docs, CI templates" as an Infrastructure tier and stops there — which is the same silence on integration machinery that #103 and eBoot#94 both run into. Covered by the proposal in .ai/autoreview/proposals/2026-09.md.
Proposed changes
- Handle the unnamed-matrix-job default in
_expanded_names(), or reword the assertion (finding 1). - One line in the body, or an issue against
embeddedos-org/.github, naming the four sibling repos (finding 2). - Refresh "8 passed" to 10 in the body.
None of these should hold the PR.
Not checked
- Nothing was run against GitHub Actions. The nine distinct names are corroborated by
checks.txtin this bundle, which is real run data, but I did not query the API myself and did not observe branch protection resolving any of these names. pytest tests/— "569 passed". I rantests/unit/test_ci_gate.pyonly. The full suite would not resolve here:uvfails on this project's dev extras,requires-python = ">=3.8"againstflake8>=6.0which needs>=3.8.1. Pre-existing, not introduced by this PR, and not chased further.- Three of the four mutations in the body's table. I reproduced the
include:-only case and the unnamed-job case (my own). The template-revert row is corroborated by the previous review, which ran it. - Whether the four sibling repos'
macos-13legs run at all. I noticed while checking finding 2 that all four pinmacos-13in the same matrix, which #106 states is retired and never assigned a runner. I did not investigate; it belongs with #106 and I have raised it there. - The local
ebuildclone was left alone — reported dirty by the sync step. All work was in detached worktrees under/tmp.
Automated architecture review of 9382536f73aa — 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.
… it prevents Answers finding 1 from the second review on embeddedos-org#104. A matrix job with no `name:` was reported as a check-name collision that cannot happen. `_expanded_names()` fell back to `job.get("name", job_id)` and substituted nothing into it, so an unnamed two-leg matrix yielded `["smoke", "smoke"]`. GitHub generates distinct names for exactly this case -- `smoke (ubuntu-22.04)`, `smoke (macos-latest)` -- so the legs never share a name. Asserting a duplicate GitHub prevents sends the reader looking for a bug that is not there, which is worse than not checking. `_expanded_names()` now takes `has_explicit_name` and, when there is none, expands to the default form with the matrix values in it. `test_a_matrix_job_names_every_dimension_it_varies` skips an unnamed job for the same reason: the rule is about a `name:` that varies less than its matrix does, and no `name:` at all already satisfies it. Reproduced the reviewer's probe. An unnamed two-leg `os` matrix wired into the gate: before this commit 2 failed (test_every_check_name_is_unique and test_a_matrix_job_names_every_dimension) after 10 passed And the guard still catches the real thing -- reverting the `name:` template on `test` gives `2 failed` with `assert not ['os']`, unchanged. Finding 2 (Low) is scope rather than a code change: this rule is enforced in ebuild only, while eos and eBoot have the same matrix jobs. Carrying test_ci_gate.py across is worth doing and belongs with the gate PRs there (eos#121, eBoot#90) rather than here, where it would be a third copy landing before the first two have merged. Noted in the PR body. Verified: pytest tests/unit/test_ci_gate.py 10 passed pytest tests/ 1 failed, 567 passed, 3 skipped The one failure is PRE-EXISTING on origin/master and unrelated -- the `python -m ninja` invocation from embeddedos-org#66's open follow-up. Refs embeddedos-org#104
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#104 "ci: give each matrix leg its own check name"
head: 6060505 author: Kartikey1306 ci: pass (29 checks; only Create GitHub Release skipping, which is tag-only)
Verdict: Correct, well-timed, and the claims check out — I reproduced the measurement independently rather than taking the comment's word for it. gh api repos/embeddedos-org/ebuild/commits/60605054/check-runs at this head returns 29 check runs and 29 distinct names, including the nine Test (Python X, os) rows. needs: does refer to job ids (ci.yml:171), so the rename leaves CI Gate alone. The guard is genuinely a regression test and not a restatement of the YAML — it models exclude, include, and GitHub's auto-generated name for an unnamed matrix job, which is the detail that usually makes this kind of test report collisions that cannot happen.
The gap is scope. tests/unit/test_ci_gate.py:20 pins WORKFLOW = ci.yml, and the repo has one remaining instance of exactly this bug in a file the guard cannot see.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/nightly.yml:16–21 |
full-test-suite is name: Full Test Suite over python-version: ["3.10", "3.11", "3.12"] — three check runs, one name. The identical defect this PR fixes, in the same repository, and test_every_check_name_is_unique cannot reach it because the fixture reads ci.yml only. I expanded every matrix in all 15 workflows using this PR's own _all_check_names helper: nightly.yml's full-test-suite is the only remaining offender in the repo (release.yml cibuildwheel 5 legs, eosim-sanity.yml install-validate 9 legs and nested-simulation 7 legs, nightly.yml board-matrix 9 legs all name every dimension they vary). The PR's own argument — "renaming is cheap exactly while nothing refers to them" — applies here and this is the last chance to take it for free. |
One line: name: Full Test Suite (Python ${{ matrix.python-version }}). Then point the guard at every workflow, not one — see finding 3. |
| 2 | Low | tests/unit/test_ci_gate.py, docstring of test_a_matrix_job_names_every_dimension_it_varies (~:240) |
"release.yml and eosim-sanity.yml both use that form today, so the blind spot goes live the moment this guard is pointed at them." Neither does. Verified by expansion (see finding 1): both name every varying dimension, unnamed_dims=[] for all four of their matrix jobs. The one file that does use that form is nightly.yml, which the docstring does not mention. So the comment sends the next reader to two clean files and away from the real one. |
Correct the docstring to name nightly.yml, or delete the claim. If finding 1 is fixed in this PR, say the repo is clean and the guard is there to keep it that way. |
| 3 | Low | tests/unit/test_ci_gate.py:20, _all_check_names |
Uniqueness is checked within one workflow file, but branch protection matches check-run names repo-wide — a name in ci.yml colliding with one in book-build.yml or simulation-test.yml would be just as unrequireable and the guard would pass. I checked: among the 7 PR-triggered workflows at this head there are no cross-workflow duplicates, so nothing is masked today. The guard just cannot tell. |
Build the name set over every pull_request-triggered workflow (the file already has test_every_pull_request_workflow_is_accounted_for, so the enumeration exists) and assert uniqueness across the union. That makes finding 1 fail on its own rather than needing to be noticed. |
| 4 | Low | .github/workflows/ci.yml:100–103 |
The codecov flags: change (ebuild-py${{ matrix.python-version }} → …-${{ matrix.os }}) is not in the PR body, whose "The fix" section shows only the name: line — in a body that is otherwise exact about what it changes. The new names are valid (≤45 chars, allowed charset: ebuild-py3.10-ubuntu-22.04 is 26), and the reasoning is the same as the job rename, so this is a disclosure point rather than a defect. Its one real effect: codecov.yml has no flag_management or carryforward and comment.layout includes flags, so PR comments will list 9 new flags with the previous 3 absent until history rolls over. |
Add it to the body's "The fix" section with that one-line consequence. |
Architecture conformance
Conforms. ebuild is Tier 1 Foundation (§21) and this touches only .github/ and tests/, so no dependency direction is engaged (§5.1) and §21.1 is not in play. It serves §9.2's SDK design rules indirectly — a check name is the machine-readable surface CI and branch protection consume, and nine runs behind three names is not machine-readable. §23's release model and .github/STANDARDS.md's "a claim without evidence is aspirational, not asserted" are the relevant frame for why a requireable check name matters at all.
No architecture proposal from this PR's specifics. The broader pattern it sits in — required_status_checks being null (#87) across an org whose default branches can and do ship uncompilable — is a gap in the master design rather than in this PR, and I have appended it to .ai/autoreview/proposals/2026-09.md rather than raising it here.
Proposed changes
name: Full Test Suite (Python ${{ matrix.python-version }})innightly.yml.- Expand
_all_check_namesover allpull_request-triggered workflows and assert uniqueness across the union. - Fix the docstring to name
nightly.yml. - Mention the codecov
flags:change in the body.
Not checked
pytest tests/(569 passed) andpytest tests/unit/test_ci_gate.py(8 passed): NOT RUN. No pytest on this host. I re-executed the module's helper functions directly to do the expansions above, which exercises_expanded_names/_all_check_namesbut not the assertions or the fixtures. The 569 figure is the author's;checks.txtshows the nineTestlegs green at this head, which is independent evidence for the suite passing on CI.- The mutation table in the body: NOT REPRODUCED. I did not revert the template, add a fourth dimension, or drop
osfrom the name to confirm "2 failed of 8" each time. That table is the strongest claim in the PR and it is the one I have not verified. ebuild's working tree was left untouched. The sync step reported it dirty (4 files) and skipped it, so everything above comes from agh api tarballsnapshot at60605054and from livegh apiqueries — nothing was read from or written to the local clone.- Codecov's actual behaviour on the flag rename: not verified. I read
codecov.ymland the flag-name constraints; I did not check what Codecov reports after the change. - The
ci-gatejob,needs: [test, build], thereleaseexclusion andif: always()are #103's, and are reviewed there — I only confirmed that the rename does not disturb them.
Automated architecture review of 60605054ed2c — 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.
3 participants
Follow-on to #103, and the reason it exists came out of testing that PR.
The measurement
Checking what branch protection would actually see on #103's head — the
check-run names, which is what a required rule matches on:
Nine check runs, three names. The job's
name:interpolatesmatrix.python-versionbut notmatrix.os, so the ubuntu, macOS and Windowslegs of each version all report under one name.
Why it matters, twice
For anyone reading a pull request. Three rows in the checks list say
Test (Python 3.10). A Windows-only failure is indistinguishable from theother two until you open the run. That is not a hypothetical shape of failure
in this repo — the Windows leg did not execute at all until
shell: bashwasadded, and the moment it did it surfaced failures the other legs never saw
(
l.split(":")[0]on abuild.ninjaline taking apart the drive letter, and agzip-timestamp flake in a fixture). Those are precisely the failures you want to
identify at a glance.
For branch protection. A required check names a check. With three check runs
behind one name, a rule naming it cannot say which leg it means.
The fix
Nine names, nine distinct:
Why now specifically
Renaming nine checks is cheap exactly while nothing refers to them, and
expensive afterwards.
The template itself and nothing else. And
required_status_checksisnulltoday (#87), so no protection rule names them either. #103 is about to give
people a reason to start pinning check names — better to have the names right
before that, not after.
Tests
Two added to
tests/unit/test_ci_gate.py:test_every_check_name_is_unique— expands each job's name templateacross its matrix and fails on any duplicate.
test_a_matrix_job_names_every_dimension_it_varies— the rule behind it,stated where someone adding a dimension will read it.
Verified by mutation:
osfrom the name, keep it in the matrixThe first row is the one that matters: these are regression tests for this
change, not restatements of it.
Verification
pytest tests/pytest tests/unit/test_ci_gate.pyCI Gatejob unchangedneeds: [test, build],if: always()needs:refers to job ids, so renaming the display name does not affect thegate.
Stacked on #103
Same file, and the argument for this change is #103's evidence. The stack
fast-forwards, so merging this takes both.