Fix: one vocabulary for CI change detection, and one fail-open guard - #1589
Merged
Merged
Conversation
📝 WalkthroughWalkthroughCI change detection now classifies non-code changes through a shared ChangesCI Gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant detect_changes
participant packaging_matrix
participant unit_tests
detect_changes->>packaging_matrix: Publish non_code_only
detect_changes->>unit_tests: Publish non_code_only
packaging_matrix-->>detect_changes: Skip when non_code_only=true
unit_tests-->>detect_changes: Skip when non_code_only=true
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/ci-change-detection.md:
- Around line 3-8: Update the invariant wording in the CI change-detection
documentation to apply only to downstream PR jobs, explicitly exempting the
intentionally ungated pre-commit job and the detect-changes job itself. Keep the
existing guidance about gates and the later exceptions consistent with
docs/ci.md.
In @.github/workflows/ci.yml:
- Around line 756-763: Resolve the empty FILES behavior consistently: update the
empty-diff handling around the non_code_only calculation in
.github/workflows/ci.yml (lines 756-763) so arch-gated scene tests run by
setting both arch outputs true, or explicitly preserve/document that only UT and
packaging jobs run. If the latter behavior remains, update the full-matrix claim
in docs/ci.md (lines 134-136); otherwise no direct documentation change is
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dcf60ff3-a7a9-451f-ad46-588b219cbec0
📒 Files selected for processing (4)
.claude/rules/ci-change-detection.md.github/workflows/ci.ymlCLAUDE.mddocs/ci.md
4 tasks
ChaoWao
force-pushed
the
ci/one-non-code-vocabulary
branch
from
July 30, 2026 02:01
16b2240 to
f3cf009
Compare
ChaoWao
changed the title
`detect-changes` carried two definitions of "changing this cannot change what the code does". `NON_CODE` covered `docs/ .docs/ .claude/ .gitignore .pre-commit-config.yaml *.md`; a separate `docs_only` meant only "every file ends in .md". The arch-specific jobs gated on the wide one and the **self-hosted hardware unit tests gated on the narrow one**, so a PR touching just `.gitignore` skipped every scene test and still booked `ut-a2a3` and `ut-a5` on real silicon. `NON_CODE` also did not know about docs tooling living outside `docs/`. `mkdocs.yml` is pure docs config at the repo root, and it was the one file in PR hw-native-sys#1581 (six `docs/*.md` plus `mkdocs.yml`) that survived every filter — that PR ran the entire matrix, including `st-onboard-a2a3`, `st-onboard-a5`, `ut-a2a3` and `ut-a5`, and a pre-existing flake in the a5 L3 close path then reddened it. `docs/ci.md` documented the narrow `docs_only` as deliberate, on the grounds that "a change to `.pre-commit-config.yaml` or a `.claude/` script can still change what CI does". Neither holds. `pre-commit` is ungated and every gated job declares `needs: [detect-changes, pre-commit]`, so that config's only consumer always runs — `ut` and friends do not read it. And no workflow invokes anything under `.claude/`: `grep -rn '\.claude' .github/workflows/` finds only comments. The three scripts there are local/agent tooling. So there is now one set. `NON_CODE` gains `mkdocs.yml` and `.github/workflows/docs.yml` — the latter because `docs.yml` runs unconditionally on every PR and is therefore its own gate. `ci.yml` stays out deliberately: a change to the gates must run everything, including whatever it just switched off. `docs_only` becomes `non_code_only`, defined as "no changed file falls outside NON_CODE", and all four consumers move with it; the old name would have been a lie once the set widened past markdown. The empty-diff case is also unified. The emptiness test used to live inside the `non_code_only` branch alone, which left that flag false — UT and packaging ran — while both arch flags independently came out false too, skipping every scene test. Two axes failing in opposite directions on the same input, under a doc claiming it "runs the full matrix". A single guard now short-circuits before any pattern runs: attribution is impossible, so `non_code_only=false`, both arch flags `true`, return. The arch outputs otherwise need no change — they already subtract `NON_CODE` before deciding, so they inherit the widening. Replayed the pattern against twelve file lists. Newly skipping: docs-only, `.gitignore`-only, `.claude/`-only, `docs/_hooks/*.py`, `workflows/docs.yml`-only. Still running everything: `ci.yml`, `src/common/`, `python/simpler/`, and an empty diff. Still single-sided: `src/a5/` only, `examples/a2a3/` only, and `docs/` mixed with `src/a5/`. `docs/ci.md` moves with the code: principle 6, the job matrix, the flag list, and the two paragraphs whose rationale no longer describes the design. `.claude/rules/ci-change-detection.md` records the invariants rather than the patterns: one vocabulary, membership by effect rather than path, never gate an expensive job more loosely than a cheap one, why the two axes are layered, fail open once at the top, why a schedule trigger needs its own workflow, and that a gating bug surfaces as a green check — so it has to be verified by looking at what ran, not by reading the regex. It also names the two jobs that are deliberately ungated, `detect-changes` and `pre-commit`, rather than claiming every job is gated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7 tasks
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. Fault injection therefore always runs on a device that has never provisioned — which is what actually addresses the interaction, rather than merely quarantining it. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: in-session ordering separates the cases, but a fault on a device that has already provisioned still costs minutes, so they must not share a device. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once hw-native-sys#1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once hw-native-sys#1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Jul 31, 2026
…#1609) The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (#1425, contained in #1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once #1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until #1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in #1601 and `detect-changes` reworked in #1589 / #1590 / #1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PR #1581 changed six
docs/*.mdfiles andmkdocs.yml. It ran the entire matrix:Then a pre-existing flake in the a5 L3 close path (
TimeoutError: child process(es) [...] did not exit within the close budget) reddened the PR. My earlier pure-.mdPRs skipped all of it correctly — the guard works; the allow-list was missing one file.Two defects
1. Two definitions of "cannot change what the code does", and the hardware jobs used the weaker one.
NON_CODEdocs/ .docs/ .claude/ .gitignore .pre-commit-config.yaml *.mddocs_only.mdArch jobs gated on the wide set;
ut,packaging-matrix,ut-a2a3,ut-a5gated on the narrow one. A.gitignore-only PR therefore skipped every scene test and still booked two self-hosted hardware pools.2.
NON_CODEdid not know about docs tooling outsidedocs/.mkdocs.ymlis docs config at the repo root; it is the file that survived every filter in #1581.The documented rationale no longer holds
docs/ci.mdsaid the narrowdocs_onlywas deliberate because "a change to.pre-commit-config.yamlor a.claude/script can still change what CI does". Checked both:pre-commithas noif:and every gated job declaresneeds: [detect-changes, pre-commit], so that config's only consumer always runs.ut/ut-a2a3/ut-a5never read it.grep -rn '\.claude' .github/workflows/finds only comments. No workflow invokes anything under.claude/; the three scripts there are local/agent tooling.So the split is collapsed rather than preserved, and
docs/ci.mdis corrected in the same commit.Change
NON_CODEgainsmkdocs.ymland.github/workflows/docs.yml— the latter becausedocs.ymlis unconditional on every PR and is its own gate.ci.ymlstays out, deliberately. A change to the gates must run everything, including whatever it just switched off.docs_only→non_code_only, meaning "no changed file falls outsideNON_CODE". All four consumers move with it; keeping the old name would be a lie once the set widened past markdown.NON_CODE, so they inherit the widening.New rule
.claude/rules/ci-change-detection.mdrecords the invariants, not the patterns: one vocabulary; membership by a file's effect, not its path; never gate an expensive job more loosely than a cheap one; why the two axes are layered (the arch gate already implies the non-code gate); why ascheduletrigger needs its own workflow; and that a gating bug surfaces as a green check, so it must be verified by looking at what ran rather than by reading the regex. Indexed inCLAUDE.md.Testing
Replayed the pattern against twelve representative file lists:
non_code_onlya2a3a5docs/*.md+mkdocs.yml).gitignoreonly.claude/settings.jsononlydocs/_hooks/repo_links.py.github/workflows/docs.ymlonly.md.github/workflows/ci.ymlsrc/common/log/host_log.cpppython/simpler/worker.pysrc/a5/...onlyexamples/a2a3/...onlydocs/testing.md+src/a5/...yaml.safe_loadonci.ymlparsesmkdocs build --strict— exit 0markdownlint-cli2— 0 errors on the new rule,CLAUDE.md,docs/ci.mdgrep -rIn docs_onlyacrossdocs/ .github/ .claude/— only the historical mention inside the new ruleNote for reviewers: this changes shared CI gating. Per the rule's own §6, the check to run on this PR is what ran — it touches
ci.yml, sonon_code_onlyisfalseand the full matrix should fire here, which is the intended behaviour.