Skip to content

ci: add one job branch protection can require - #103

Open
Kartikey1306 wants to merge 2 commits into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate
Open

ci: add one job branch protection can require#103
Kartikey1306 wants to merge 2 commits into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

Addresses the part of #87 that is still open. @srpatcha's comment there is
exact:

required_status_checks is null. Nothing imports the package before the
merge lands, which is how a file that does not parse became master in the
first place — every command in the CLI broken, and no gate that would notice.
Making it required is a settings change, not work.

The settings change is the easy half. The missing half is a name worth pointing
it at, and this workflow does not currently produce one.

Why the existing checks cannot be required as they are

test is a 3×3 matrix. It reports as nine generated check names, one per
(python-version, os) leg. Requiring them means listing nine names that change
whenever the matrix does — and a required name that stops appearing does not
fail, it blocks: GitHub waits for a status that will never arrive, on every
pull request, until someone edits the protection rule.

release is skipped on every pull request (if: startsWith(github.ref, 'refs/tags/v')). A required check that is skipped has the same problem.

What this adds

One job, ci-gate, displayed as CI Gate — the single name to require:

  ci-gate:
    name: CI Gate
    needs: [test, build]
    if: always()

Three details that are the whole point of it:

  • if: always(). Without it the gate is skipped as soon as an earlier job
    fails, and a skipped required check never reports — a genuine failure would
    present as a pull request that hangs rather than one that goes red. That is a
    worse failure mode than no gate at all, because it looks like infrastructure
    flake.
  • Any non-success result fails it, skipped included. A job that did not
    run did not verify anything. Treating that as a pass is the fail-open shape
    this repo has been removing elsewhere.
  • release is excluded deliberately, and the test below enforces that the
    only jobs allowed outside the gate are tag-only ones — so the exclusion
    cannot quietly be used to drop a job out of coverage.

The test is the part that keeps working

tests/unit/test_ci_gate.py (6 tests) parses ci.yml and fails if a job is
added to the workflow without being wired into the gate. That is the way this
would rot: someone adds a job in six months, it is not in needs, and the
required check silently stops covering it.

Verified by mutation — each of these was applied to ci.yml and the suite
re-run:

mutation result
drop build from the gate's needs 2 failed
remove if: always() 1 failed
rename the gate's display name 1 failed
add a new job, forget to wire it in 2 failed
weaken the check to == "failure" 1 failed
delete the gate job entirely 6 failed
(restored) 6 passed

Verification

item result
pytest tests/ PASS — 567 passed
pytest tests/unit/test_ci_gate.py PASS — 6 passed
yaml.safe_load of the edited workflow PASS
mutation sweep above PASS — all six caught

Stacked on #102

master does not collect — 11 collection errors, 368 tests collected, suite
never runs
(pytest --collect-only on origin/master). #102 is the green tip,
so this branches from it. The stack fast-forwards, so merging this takes both.

What is still a maintainer action

This PR cannot set the branch protection rule — that needs admin rights. Once
merged, the action is: Settings → Branches → master → Require status checks →
add CI Gate.
One name, and it stays correct as the matrix changes.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

The gate was run against a real failure, not just reasoned about

Pushed a deliberately failing test to a branch on my fork so the whole workflow
would execute with one job red, and watched what the gate did.

Job results in that run:

  failure  Build & Test (Linux x86_64)      <- the deliberate failure
  success  Static Analysis (cppcheck + clang-tidy)
  skipped  Cross-compile ARM Cortex-M4      <- skipped, it needs: test
  skipped  Create GitHub Release            <- tag-only, correctly outside the gate
  failure  CI Gate                          <- reported red

And the gate's own log:

##[error]CI Gate failed. These jobs did not succeed:
  test: failure
  build-arm: skipped
##[error]Process completed with exit code 1.

Three things that confirms, which the workflow file alone does not:

  1. if: always() works. The gate ran at all despite an upstream failure. Had
    it been omitted, the gate would have been skipped — and a skipped required
    check never reports, so the pull request would have sat waiting for a status
    that never arrives instead of showing a red X.
  2. skipped is treated as a failure. build-arm was skipped and the gate
    named it. Had skipped counted as a pass, a job that verified nothing would
    have been reported as verified.
  3. release being outside the gate is correct. It was skipped and the gate
    did not care, which is exactly why it cannot be a required check itself.

The self-test branch has been deleted; nothing from it is in this PR.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

The matrix problem is worse than "nine names that change"

I checked what branch protection actually sees on this PR's head — the
check-run names, which is what a required check matches on:

$ gh api repos/embeddedos-org/ebuild/commits/<head>/check-runs --jq '.check_runs[].name' | sort | uniq -c

   3 Test (Python 3.12)
   3 Test (Python 3.11)
   3 Test (Python 3.10)
   ...
   1 CI Gate

Each name appears three times. The job's name: template interpolates
matrix.python-version but not matrix.os, so the ubuntu, macOS and Windows
legs all report under the identical name.

That makes them unusable as required checks, not merely awkward:

  • You cannot require the ubuntu leg without also requiring the macOS and Windows
    legs — there is no name that distinguishes them.
  • Required-check evaluation against a name with three distinct check runs behind
    it is ambiguous, so what you get is not what the settings screen appears to
    promise.
  • Adding a fourth OS silently changes what a single required name covers.

CI Gate is the only name in that list that is both unique and covers the whole
workflow.

Worth noting separately: the same name: template is arguably a bug on its own
— three checks called Test (Python 3.10) are indistinguishable in the PR
checks list, so a Windows-only failure reads as "one of the 3.10 jobs failed"
until you open it. Adding ${{ matrix.os }} to the template would fix the
display. I have deliberately not done that here, because it changes nine
check names at once, and if anyone has already pinned those names anywhere it
should be a separate, deliberate change rather than a side effect of adding a
gate.

@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#103 "ci: add one job branch protection can require"

head: 8f32b75 author: Kartikey1306 ci: pass (22 checks pass, assign and Create GitHub Release skipping, 0 fail — including CI Gate itself, 4s)

Verdict: The right fix for #87 and a good one — a single stable required-check name, with a test that keeps it honest as the workflow changes. I ran that test and mutation-tested it myself and it does what the body says. Two things need addressing: the "run against a real failure" comment presents a job list and gate log that this workflow cannot produce, and the gate covers one workflow file out of fifteen while the body describes the resulting maintainer action as complete.

Read this first, it is not what the bundle shows. gh pr diff reports 18 files and 996 additions because the branch was cut from #102 and the diff is against the merge base. #102 has since merged as e5d8052. Against the current master this PR is ahead 1, behind 1 and changes exactly two files — .github/workflows/ci.yml and tests/unit/test_ci_gate.py. The other sixteen are already on master and are not this PR's to answer for. I reviewed the two.

Findings

# Severity File:line Finding Recommended fix
1 High PR comment, 2026-09-01T15:42:32Z ("The gate was run against a real failure") The quoted gate log cannot have come from this workflow, so the two behaviours it is offered as proof of are unverified. The comment reports the gate's own output as:
test: failure / build-arm: skipped
The gate builds that list from ${{ toJSON(needs) }}, and needs: [test, build] — so the only keys jq can ever iterate are test and build. There is no build-arm in this workflow to appear there. The surrounding job list has the same problem: Build & Test (Linux x86_64), Static Analysis (cppcheck + clang-tidy) and Cross-compile ARM Cortex-M4 are not job names in this repository. git show origin/master:.github/workflows/ci.yml has three jobs — test (Test (Python ${{ matrix.python-version }})), build (Build Python Package) and release (Create GitHub Release) — and those four names are, verbatim, eBoot's CI job names. I am not disputing the design: if: always() and select(.value.result != "success") mean exactly what the comment says they mean, and I have no reason to think the conclusions are wrong. But the comment's own framing is "Three things that confirms, which the workflow file alone does not", and the run it rests on is a run of something else. Per the brief, an unsupported "verified" is itself the finding, and the two claims it backs — that the gate reports red rather than hanging, and that skipped fails it — are the whole argument for the design.
Re-run it here. Push a branch on the fork with one deliberately failing test, let this ci.yml execute, and post the actual gate log — it should name test: failure and build: skipped, which is the same demonstration with the right two job names. If the original run was in fact against another repository's workflow, say so; the semantics do carry across and it is a reasonable thing to have done, but it needs to be labelled rather than presented as this PR's evidence.
2 Medium .github/workflows/ci.yml:140, tests/unit/test_ci_gate.py:25 The gate covers one workflow file; the maintainer action in the body is described as if it covered CI. WORKFLOW = Path(__file__).resolve().parents[2] / ".github" / "workflows" / "ci.yml" — a single hardcoded file. master has fifteen workflow files. On this PR's own head, checks.txt lists 24 check names, and only four of them (Test (Python 3.10/3.11/3.12), Build Python Package) are inside the gate. The rest come from eosim-sanity.yml (eleven EoSim … jobs, three Cross-Platform …, Simulation Sanity Gate), codeql.yml (Analyze (Python), CodeQL) and auto-assign.yml (assign) — all of them outside CI Gate and none of them required after the settings change the body prescribes. The body's closing instruction is "Once merged, the action is: Settings → Branches → master → Require status checks → add CI Gate. One name, and it stays correct as the matrix changes." That is accurate about the matrix and misleading about coverage: it reads as the complete fix for #87 and it is the complete fix for ci.yml only. The same asymmetry applies to the rot-guard — test_ci_gate.py catches a new job added to ci.yml (verified below) and is blind to a new job added to any of the other fourteen. Nothing in the YAML needs to change; the gate is correctly scoped to its own workflow, and cross-workflow needs is not a thing GitHub offers. Fix the description: say plainly that CI Gate gates ci.yml, and list which other check names a maintainer should require alongside it — Simulation Sanity Gate and Analyze (Python) are the two that are already single, stable names and would cost nothing to add. If the intent is one gate for everything, that is a second gate job in eosim-sanity.yml and a follow-up PR, not a rewording.
3 Low .github/workflows/ci.yml (whole file) This PR and ebuild#104 both rewrite ci.yml's job block and will conflict. #104, from the same author and in this same review batch, is "ci: give each matrix leg its own check name" — which is precisely the change this PR's third comment says it deliberately left out ("Adding ${{ matrix.os }} to the template would fix the display. I have deliberately not done that here"). That reasoning is sound and the split is the right call. What is missing is that neither PR references the other, so whichever merges second gets a conflict in a file where a bad resolution silently changes what branch protection covers. Cross-link them, and state the intended order. This one should land first: it establishes a gate name that is stable regardless of the matrix names, which is what makes #104's nine renames safe to do afterwards.

Verified clean, executed rather than read, because the value of this PR is entirely in whether the guard actually guards. Extracted origin/master with git archive into /tmp/eb103, applied only this PR's two files:

  • tests/unit/test_ci_gate.py is 6/6 passing against the patched master tree.
  • The mutation that matters actually fails. I appended a lint job to ci.yml without adding it to the gate's needs and re-ran: 2 failed, 4 passedtest_gate_covers_every_job_that_runs_on_a_pull_request and test_jobs_left_out_of_the_gate_are_genuinely_tag_only, with the assertion pointing at _only_runs_on_tags({'name': 'Lint', ...}). Restoring the file returns it to 6 passed. That is the row of the body's mutation table that decides whether this PR is worth having, and it is real. I did not reproduce the other five rows.
  • The workflow parses and the wiring is what it claims. yaml.safe_load of the patched file gives jobs ['test', 'build', 'release', 'ci-gate'] with ci-gate.needs == ['test', 'build'] — so release, the tag-only job, is the only one outside the gate, which is what test_jobs_left_out_of_the_gate_are_genuinely_tag_only exists to pin.
  • CI Gate reports on this head. checks.txt line 3: CI Gate pass 4s. So the job is not hypothetical — it ran green here, which is the success path. Finding 1 is about the failure path only.
  • The runner pin is consistent, not new. runs-on: ubuntu-22.04 matches ci.yml:102 and ci.yml:123 already on master. Worth checking because ebuild#106 in this batch retires a runner — but that one is macos-13 in release.yml, so there is no interaction.
  • jq is present on GitHub-hosted Ubuntu images, so the gate step has no undeclared dependency.

Architecture conformance

Conforms. §21 Infrastructure — "governance, release automation and documentation". Nothing here is a runtime dependency of anything: ci.yml is CI configuration and tests/unit/test_ci_gate.py is a test that parses it, so §5.1's dependency direction is untouched and eBuild's "understands the complete graph but is never a runtime dependency" position is unaffected. tests/ is the correct home per .ai/architect.md's target layout. §21.1's split policy is not engaged — no repository is created or moved.

The design-level thing this PR serves is §28's evidence policy, and it serves it well: a required check is the mechanism that makes "tests pass" a fact about master rather than a claim about a branch.

No proposal appended. This is squarely covered by the existing entry in .ai/autoreview/proposals/2026-09.md, "The evidence policy is silent on checks that verify nothing" (§28, triggered by eAI#39, eAI#41, eBoot#81), whose proposed §28.2 — that a runner finding zero tests must fail, and that a check whose result is discarded is not a check — is the same principle this gate applies to a skipped job. This PR is a good implementation of that proposal rather than a new gap in the design, and §28 needs no amendment to accommodate it.

Proposed changes

  1. Re-run the failure-path demonstration against this workflow and replace the comment's log, or relabel it (finding 1). This is the only item I would hold the merge on, and it costs one push to a fork.
  2. Reword the "what is still a maintainer action" section to scope CI Gate to ci.yml and name the other checks worth requiring (finding 2). Documentation only.
  3. Cross-link #104 and state the merge order (finding 3).

All three are independent, and none of them touch the YAML. The workflow change itself I would take as it stands.

Not checked

  • I did not run the full suite. The body's 567 passed for pytest tests/ is uncorroborated; I ran tests/unit/test_ci_gate.py only, six tests. pytest here is a uv-managed CPython 3.12 without PyYAML, so it needed PYTHONPATH=/usr/lib/python3/dist-packages to import yaml at all — an environment quirk of this host, not of the repository, but it means my run is not the repository's supported invocation.
  • Five of the six mutation-table rows were not reproduced. I ran the "add a new job, forget to wire it in" row and confirmed 2 failed. The rows for dropping build from needs, removing if: always(), renaming the display name, weakening the comparison to == "failure", and deleting the gate entirely, I did not run — I read the six test functions and they look like they would cover them, which is not the same as having seen them fail.
  • Nothing was executed on GitHub Actions. Everything about runtime behaviour — that if: always() schedules the job after an upstream failure, that toJSON(needs) serialises skipped jobs with result: "skipped", that a required check that never reports blocks rather than fails — is GitHub Actions semantics I am asserting from documentation and from the file, not from a run. Finding 1 is the same gap seen from the other side, and my inability to close it locally is exactly why the author should.
  • The branch-protection claim was not verified. I did not query repos/embeddedos-org/ebuild/branches/master/protection, so "required_status_checks is null" is taken from the quoted comment on #87. If it has been set since, part of the premise has moved.
  • The test job's nine-check-runs claim was not verified. The author's third comment reports each Test (Python 3.x) name appearing three times in the check-runs API. gh pr checks collapses them to three rows, which is consistent but not confirmation. I did not call the API.
  • The sixteen files from #102 were not reviewed. They are on master already and the ledger shows no ebuild PR has ever been reviewed by this system, so #102's 900-odd lines — commands.py, footprint.py, dispatch.py, registry.py and their tests — went in unreviewed by this process. That is a gap in the backlog, not a defect in this PR, and it is worth someone's attention separately.
  • The local ebuild checkout is dirty and was skipped by the sync step (M TASKS.md, M ebuild/cli/integration.py, M tests/ebuild/test_integration_initramfs_security.py, ?? smart-sensor/), and sits on branch v90, not master. I read origin/master through git show and git archive and touched nothing in the working tree. The PR head is not fetched locally, so the ahead/behind figures come from gh api .../compare, not from a local range.
  • mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. No merge attempted. Conflicts with #104 on ci.yml (finding 3).

Automated architecture review of 8f32b751fe2f — 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.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Correction: the run log in my earlier comment is not from this repository

@srpatcha is right, and this needs saying plainly rather than quietly editing.

My comment of 2026-09-01 presented a gate log as evidence for this PR:

  failure  Build & Test (Linux x86_64)
  success  Static Analysis (cppcheck + clang-tidy)
  skipped  Cross-compile ARM Cortex-M4
  failure  CI Gate
...
  test: failure
  build-arm: skipped

Those are eBoot's job names. This workflow has three jobs — test, build, release — and needs: [test, build], so build-arm is a key jq could never produce here. I ran the fork experiment once, against eBoot, and posted the same comment on all three gate PRs (this one, eos#121, eBoot#90) without adjusting it. It is genuine evidence on eBoot#90 only.

The design conclusions still hold — if: always() and select(.value.result != "success") mean what I said they mean, and that reasoning is not repo-specific. But the comment claimed the stronger thing: that it had been observed here. It had not.

I am re-running it against this workflow and will post the real log, with test: failure and build: skipped, which is the same demonstration with the right two job names. Until then, treat the earlier comment as reasoning, not as a run.

Separately, on the coverage finding — you are right that the closing instruction oversells it. CI Gate gates ci.yml and nothing else. I will reword the body to name the other checks a maintainer needs to require alongside it, rather than implying one name is the whole fix.

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.
Kartikey1306 added a commit to Kartikey1306/ebuild that referenced this pull request Sep 3, 2026
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 to get the real log.

Verified:
  pytest tests/unit/test_ci_gate.py    8 passed
  pytest tests/                        all pass
  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.

Refs embeddedos-org#103, embeddedos-org#87
@Kartikey1306
Kartikey1306 force-pushed the ci/required-check-gate branch from 8f32b75 to 6c9fca9 Compare September 3, 2026 10:23
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
@Kartikey1306
Kartikey1306 force-pushed the ci/required-check-gate branch from 6c9fca9 to 06e5b9f Compare September 3, 2026 10:24
Kartikey1306 added a commit to Kartikey1306/ebuild that referenced this pull request Sep 3, 2026
…and let the path be exercised

Answers the review on embeddedos-org#106.

Finding 1 (Medium) -- `CIBW_ARCHS_MACOS: "x86_64 arm64"` was inert and the
reason given for dropping macos-13 rested on wheels that have never existed.
ebuild is pure Python: pyproject.toml declares setuptools.build_meta with no
ext-modules, no cmdclass and no setup.py, so cibuildwheel cannot complete on
any leg. All four fell through to `python -m build --wheel` and produced the
same ebuild-X.Y.Z-py3-none-any.whl, which download-artifact --merge-multiple
then collapsed back into one. macos-13's wheels were that same universal file.
Dropped the env line and corrected the comment.

Finding 2 (Medium) -- the `||` fallback was a fail-open build step, and this
PR's `shell: bash` is what makes it reachable on Windows. It does not merely
swallow a failure; it substitutes a different artefact and reports success,
and `skip-existing: true` on the publish step means the substitution would not
even collide with anything. Replaced with an explicit choice: detect whether
the project builds a C extension, then run cibuildwheel or `python -m build`
accordingly, with `set -euo pipefail` so a real failure fails. The step's name
now means what it says.

  Verified both branches of that detection: against this repo it reports
  has_ext = False and takes the `python -m build` path; with a setup.py
  present it reports True. So the day a C extension lands the step switches
  by itself, and a genuine compile failure is a failure.

Finding 4 (Low) -- added `workflow_dispatch`, with `pypi` and `release` gated
on `github.event_name == 'push'`. The workflow's only trigger was a version
tag, so the first execution of any change to it was a real release -- for a
path whose last 15 runs all failed. A maintainer can now run validate +
cibuildwheel + cross-compile on demand and watch the wheels land as artifacts
without spending a tag, and a manual run cannot publish.

Finding 3 (Low) is a PR-body correction and is handled there: the run history
is worse than the body said -- the last 15 runs are failure or cancelled, back
to v1.0.2 on 2026-04-28, and v1.0.2 failed in five minutes, which is neither
the macOS queue timeout nor the shell syntax error. The two causes fixed here
are the current ones, not the whole history, and the next tag is a test of
these two fixes rather than proof the release path is healthy.

Verified:
  yaml.safe_load of the result parses; jobs are
    ['validate', 'cibuildwheel', 'cross-compile', 'pypi', 'release']
  triggers are ['push', 'workflow_dispatch']
  pypi.if and release.if are both github.event_name == 'push'
  no `||` remains in the Build wheels step; `set -euo pipefail` is present
  pytest tests/                        1 failed, 565 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. test_ci_gate.py
    does not exist on this branch (it lives in embeddedos-org#103), so it was NOT RUN here.

  NOT RUN: the workflow itself. It is tag- and dispatch-only, so this PR's CI
  cannot execute it -- which is exactly what finding 4 is about. The
  workflow_dispatch trigger only becomes usable once this is on master.

Refs embeddedos-org#106
Kartikey1306 added a commit to Kartikey1306/ebuild that referenced this pull request Sep 3, 2026
…and let the path be exercised

Answers the review on embeddedos-org#106.

Finding 1 (Medium) -- `CIBW_ARCHS_MACOS: "x86_64 arm64"` was inert and the
reason given for dropping macos-13 rested on wheels that have never existed.
ebuild is pure Python: pyproject.toml declares setuptools.build_meta with no
ext-modules, no cmdclass and no setup.py, so cibuildwheel cannot complete on
any leg. All four fell through to `python -m build --wheel` and produced the
same ebuild-X.Y.Z-py3-none-any.whl, which download-artifact --merge-multiple
then collapsed back into one. macos-13's wheels were that same universal file.
Dropped the env line and corrected the comment.

Finding 2 (Medium) -- the `||` fallback was a fail-open build step, and this
PR's `shell: bash` is what makes it reachable on Windows. It does not merely
swallow a failure; it substitutes a different artefact and reports success,
and `skip-existing: true` on the publish step means the substitution would not
even collide with anything. Replaced with an explicit choice: detect whether
the project builds a C extension, then run cibuildwheel or `python -m build`
accordingly, with `set -euo pipefail` so a real failure fails. The step's name
now means what it says.

  Verified both branches of that detection: against this repo it reports
  has_ext = False and takes the `python -m build` path; with a setup.py
  present it reports True. So the day a C extension lands the step switches
  by itself, and a genuine compile failure is a failure.

Finding 4 (Low) -- added `workflow_dispatch`, with `pypi` and `release` gated
on `github.event_name == 'push'`. The workflow's only trigger was a version
tag, so the first execution of any change to it was a real release -- for a
path whose last 15 runs all failed. A maintainer can now run validate +
cibuildwheel + cross-compile on demand and watch the wheels land as artifacts
without spending a tag, and a manual run cannot publish.

Finding 3 (Low) is a PR-body correction and is handled there: the run history
is worse than the body said -- the last 15 runs are failure or cancelled, back
to v1.0.2 on 2026-04-28, and v1.0.2 failed in five minutes, which is neither
the macOS queue timeout nor the shell syntax error. The two causes fixed here
are the current ones, not the whole history, and the next tag is a test of
these two fixes rather than proof the release path is healthy.

Verified:
  yaml.safe_load of the result parses; jobs are
    ['validate', 'cibuildwheel', 'cross-compile', 'pypi', 'release']
  triggers are ['push', 'workflow_dispatch']
  pypi.if and release.if are both github.event_name == 'push'
  no `||` remains in the Build wheels step; `set -euo pipefail` is present
  pytest tests/                        1 failed, 557 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. test_ci_gate.py
    does not exist on this branch (it lives in embeddedos-org#103), so it was NOT RUN here.

  NOT RUN: the workflow itself. It is tag- and dispatch-only, so this PR's CI
  cannot execute it -- which is exactly what finding 4 is about. The
  workflow_dispatch trigger only becomes usable once this is on master.

Refs embeddedos-org#106

@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#103 "ci: add one job branch protection can require"

head: 06e5b9f author: Kartikey1306 ci: pass

Verdict: The gate is the right shape and the rot-guard genuinely works — I reproduced the "add a job, forget to wire it in" mutation and it fails two tests, and the workflow-accounting table added in the second commit correctly enumerates all seven pull-request workflows. Three things to fix: the guard has a one-clause bypass, NO_GATE drops CodeQL out of the required set, and the entry for simulation-test.yml understates a gate that cannot fail at all.

@srpatcha's two points on this PR — the fork run log being from eBoot, and the closing instruction overselling one name as the whole fix — are already made and already acknowledged. Not restating them.

Findings

# Severity File:line Finding Recommended fix
1 High .github/workflows/simulation-test.yml (Simulation Sanity Gate), described at tests/unit/test_ci_gate.py:56-59 The NO_GATE entry says this workflow "has a gate job whose body still fails open on part of its needs". It fails open on all of it. The failure check is commented out entirely and the step ends with an unconditional echo "✅ All simulation checks passed (skipped due to missing repo)" — a success message it never earned, printed regardless of what simulate and cross-platform returned. It is green on this PR's own head (checks.txt: Simulation Sanity Gate pass 4s), sitting on top of thirteen EoSim * and Cross-Platform (*) checks whose results it discards. That is .ai/reviewer.md's "a verification whose result is discarded", and it is a stronger version of the exact fail-open shape this PR exists to avoid. "tracked separately" carries no issue or PR reference, and no open PR in the repo touches that file. Not this PR's job to fix, and I have deliberately not opened one: uncommenting the check changes what CI blocks on and I cannot run GitHub Actions to prove it is safe across other PRs. Two things that are this PR's job: make the NO_GATE reason say what is actually true, and put a real issue number in it. An excuse table is only honest if its excuses are.
2 Medium tests/unit/test_ci_gate.py:96-99 _only_runs_on_tags() tests "refs/tags" in condition — a substring match against the whole if: expression. Any job that mentions refs/tags anywhere is treated as tag-only and silently exempted from gate coverage, including one that plainly runs on pull requests. Reproduced: adding a job with if: startsWith(github.ref, 'refs/tags/v') || github.event_name == 'pull_request' leaves all 8 tests passing, and that job is outside the gate. This is the one hole the guard exists to close. Assert the shape rather than search the text: require the condition to be exactly startsWith(github.ref, 'refs/tags/v') (the form release uses), and fail on any other if: on an ungated job. A job wanting a different exemption then has to change this test deliberately, which is the point.
3 Medium tests/unit/test_ci_gate.py:31-52 NO_GATE conflates two different verdicts: "must not be a required check" (auto-assign, book-build, vendor-drift, claude-code-review) and "must be required, just not through a gate job" — which is what the codeql.yml entry itself says: "already reports a single stable name, CodeQL, which should be required directly rather than wrapped in a gate". Because it lives in NO_GATE, it is outside REQUIRED_CHECKS, and REQUIRED_CHECKS's own docstring calls that mapping "the full required set … not one name". A maintainer following this table configures CI Gate and stops, leaving CodeQL unrequired — the static security analysis .github/STANDARDS.md lists under org-wide Security frameworks. Split the intent. Either move codeql.yml: "CodeQL" into REQUIRED_CHECKS (it satisfies test_gated_workflows_really_have_their_gate only if that test stops assuming the display name belongs to a gate job — codeql.yml's job displays as Analyze (Python) while the check run is CodeQL, so that test needs adjusting too), or add a third table, REQUIRED_DIRECTLY, and have the accounting test treat all three as accounted-for.
4 Low tests/unit/test_ci_gate.py:79 _pr_workflows() globs *.yml only. A workflow added as .yaml produces pull-request checks and is invisible to the accounting test — the one test whose job is to notice new workflows. All fifteen files are .yml today, so this is latent. glob("*.yml")list(glob("*.yml")) + list(glob("*.yaml")).
5 Low PR body, "tests/unit/test_ci_gate.py (6 tests)" and the verification table There are 8; the second commit added test_every_pull_request_workflow_is_accounted_for and test_gated_workflows_really_have_their_gate. Same for "6 passed" in the mutation table. Refresh the numbers.

What I verified

The rot guard works. Appending an ungated lint job to ci.yml:

FAILED tests/unit/test_ci_gate.py::test_gate_covers_every_job_that_runs_on_a_pull_request
FAILED tests/unit/test_ci_gate.py::test_jobs_left_out_of_the_gate_are_genuinely_tag_only
2 failed, 6 passed

Restored: 8 passed. That is the claim the whole PR rests on and it holds.

The gate covers everything in ci.yml that runs on a PR. The workflow has four jobs — test, build, release, ci-gate. needs: [test, build]; release is if: startsWith(github.ref, 'refs/tags/v'). Nothing is uncovered.

The workflow-accounting table is complete. I parsed all fifteen workflow files and listed which trigger on pull_request: auto-assign, book-build, ci, claude-code-review, codeql, simulation-test, vendor-drift — exactly the seven in REQUIRED_CHECKSNO_GATE, no more. I had expected to find the EoSim * and Cross-Platform (*) checks coming from a workflow the table missed; they come from simulation-test.yml, which is listed. The accounting is right.

ubuntu-22.04 on the gate is consistent, not a new risk. I checked this against #106, which removes a retired runner in the same repo: that PR drops macos-13, not ubuntu-22.04, and build and release in ci.yml already run on ubuntu-22.04. No conflict between the two PRs.

Finding 2, reproduced. Detailed above — 8 passed with a PR-running job outside the gate.

Architecture conformance

Master design §5.1 and §21: conforms. ebuild is Tier 1 – Foundation; CI workflow and its test are infrastructure inside the owning repository, which is where §21's Infrastructure row puts them. No runtime dependency is created — §5.1's "eBuild understands the complete graph but is not a runtime dependency" is untouched, since nothing here is importable by a target.

.ai/reviewer.md on weakened checks — "a || true on a build or test step, a verification whose result is discarded" — is the lens for finding 1. The gate this PR adds is the correct shape under that rule; the gate it documents in NO_GATE is a direct violation of it.

There is a design-level gap behind this whole PR, and it is the same one eBoot#94 runs into from the other side: the master design has no section on integration gates. §28 defines what evidence an artefact needs to claim a state, §36 defines go/no-go gates between product stages, and §23 defines the branch and release model — none of them say that the trunk must be verified, or by what. That is why "make it required is a settings change, not work" is true and also why nothing has made it. Proposal appended to .ai/autoreview/proposals/2026-09.md.

Proposed changes

  1. Correct the simulation-test.yml reason in NO_GATE to state that its gate never fails, and replace "tracked separately" with an issue number. Open the issue if it does not exist.
  2. Tighten _only_runs_on_tags() to an exact-form check (finding 2).
  3. Resolve the CodeQL classification (finding 3) — smallest version is a REQUIRED_DIRECTLY dict plus one line in the accounting test.
  4. Add *.yaml to the glob (finding 4) and refresh the body's counts (finding 5).

2–4 are each a couple of lines and covered by the suite already here.

Not checked

  • Nothing was run against GitHub Actions. Every claim about what the gate does at runtime — if: always() producing a red X rather than a hang, skipped failing the gate — is read off the YAML and the jq expression, not observed. The author has already withdrawn the fork run log as evidence for this repo, so there is currently no observed run of this workflow's gate. That gap is open.
  • pytest tests/ — "567 passed". I ran tests/unit/test_ci_gate.py only (8 passed, in an isolated interpreter with pytest and pyyaml). I could not run the full suite: uv cannot resolve this project's dev extras — requires-python = ">=3.8" against flake8>=6.0, which needs >=3.8.1. That is a pre-existing packaging inconsistency, not something this PR introduced, and not something I chased further.
  • The five other mutations in the body's table. I reproduced one (add an unwired job) and one of my own (the refs/tags bypass). The other five are plausible from reading the tests but I did not apply them.
  • Whether branch protection behaves as described for a name with three check runs behind it. That claim is from the author's second comment and I have no way to test it.
  • The local ebuild clone was left alone. The sync step reports it as dirty (4 files); I worked from a detached worktree off origin/master and the PR ref, and touched nothing in the user's checkout.

Automated architecture review of 06e5b9f07056 — 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.

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.

3 participants