fix(ci): gate the directory that is itself a gate - #5050
Conversation
A pull request touching only `structure/` ran no test or gate leg at all, so `tests/ci-workflows/structure-ssot.test.ts` never executed — and that test is the only place CI runs `bun run structure:check`. The aggregate `ci` check reported success over a change nothing had read. The cause is the `changes` job's `ci` path filter, which does not list `structure/**`. On #4999, a pure doc split, every expensive leg reported `skipped`: windows, macos, macos control, test 1/4 through 4/4, gates, npm-global, docker smoke, keyring, api usage, storage policy and docs site build. This adds a `structure` filter and a narrow `structure gate` job that runs the check alone, rather than adding `structure/**` to `ci`. Widening `ci` would also close the hole, and would start nine Windows shards and two macOS shards for a check that takes seconds — the cost this workflow's own comments argue against for `docs-site`. The gate is pull-request scope, like `docs-site-build`, because the push trigger's `paths:` is pinned to mirror the `ci` filter exactly. That costs nothing: `dev`, `main` and `preview` are protected to require a pull request. Closes #5001.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe workflow now detects pull requests that change ChangesStructure CI coverage
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant changes
participant structure-gate
participant ci
PullRequest->>changes: Evaluate structure/** filter
changes->>structure-gate: Produce structure=true
structure-gate->>structure-gate: Run bun run structure:check
structure-gate->>ci: Report success
ci->>ci: Require success for requested structure-gate
Merge Risk: ⚪ Minimal · up to Structure-only pull requests now run the intended validation without widening the full cross-platform matrix. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
리뷰 · 우선순위 77 / 80이 PR은 왜 오랫동안 안 보였는지도 현재 체크아웃과 맞습니다. 고치는 방식도 현재 새 테스트 라인 (PR) .github/workflows/ci.yml changes.outputs.structure - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2882a48c09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,74 @@ | |||
| import { expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
Add ci-structure-gate.test.ts to both scripts/test-layout/layout.json's explicit map and tests/fixtures/test-layout-expected.json. The current name happens to match the broad ci regex seed, so the layout checks tolerate it temporarily, but the repository requires every new test to be recorded in both authoritative inventories; leaving both entries absent makes the maintained test inventory incomplete.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
…t inventories (#5075) The regex seeds place a conventionally named file, so a regression test can sit in the tree, run in CI, and still be absent from the authoritative table. That is how the regression tests for #5050, #5051 and #5055 landed without ever entering scripts/test-layout/layout.json or tests/fixtures/test-layout-expected.json (#5059). The two inventories are two copies of one table and the membership oracle already compares them, so both sides get the same three entries. A new test names the three files so they cannot fall out again silently, and checks that each one actually sits in the directory its registration claims. No repository-wide explicit-registration policy is introduced here; the seeds keep carrying brand-new files as designed.
1 participant
Summary
structure/is the one directory whose entire purpose is a gate, and it was the one directory the gate never saw.bun run structure:checkreaches CI only throughtests/ci-workflows/structure-ssot.test.ts, which lives in the suite that thechangesjob'scipath filter decides whether to run — and that filter does not liststructure/**. So a pull request touching onlystructure/skipped every expensive leg and the aggregatecicheck reported success over a change nothing had verified. #4999, a pure doc split, is the live example: windows, macos, macos control,test 1/4through4/4, gates, npm-global, docker smoke, keyring, api usage, storage policy and docs site build all reportedskipped.It stayed invisible because the ownership rule in
structure/AGENTS.mdmeans astructure/change almost always arrives beside asrc/one, which thecifilter does match. Three pull requests hit thestructure/runtime.mdline budget earlier the same evening and all three were caught, precisely because they also touchedsrc/. Only a pure doc change exposes the gap, and it took a doc reaching its size budget to produce one.What this adds. A
structurefilter on thechangesjob, and astructure gatejob that runsbun run structure:checkdirectly when it fires.Why not widen
ci. That would close the hole too, and it would start nine Windows shards and two macOS shards on every prose edit — the cost this workflow's own comments already refuse fordocs-site. The narrow job takes seconds. It runs wheneverstructure/changes, including beside asrc/change where the suite covers it anyway; a duplicate of a seconds-long check is not worth a condition that could be wrong.Why pull-request scope. The push trigger's
paths:is pinned bytests/ci-workflows/ci-workflows.test.tsto equal thecifilter exactly, andstructure/**deliberately is not in that filter.docs-site-buildalready sits in the same position. It costs nothing here:dev,mainandprevieware all protected to require a pull request, so nostructure/change reaches an integration line without passing through one.Closes #5001.
Verification
This pull request verifies itself.
.github/workflows/ci.ymlis in both thecifilter and the newstructurefilter, so this run starts the full suite and the new gate.tests/ci-workflows/ci-structure-gate.test.tsis new. It reads the condition off the job rather than naming it, so renaming the job does not quietly pass: it asserts that exactly one job is selected byneeds.changes.outputs.structure == 'true'and runsbun run structure:check.It also pins the tradeoff —
structure/**must stay out of thecifilter, and the pushpaths:must keep mirroringci— so a future edit that closes this hole by wideningcifails here and gets read by a human instead of silently buying a matrix run per prose edit.The last case covers the aggregate.
ci.yml's own comment promises that adding a job without declaring it there fails the gate by name, which only holds if theexpected_forarm exists; a job missing from it reads asundeclared. The test asserts theneedsentry, theGATED_JOBSline, theexpected_forarm, and theCHANGES_STRUCTUREenv.Existing assertions checked and unaffected: the
ci.on.push.paths/ci-filter equality atci-workflows.test.ts, the aggregate's derived-needscheck (which reads the workflow's own job keys, so it accepts the new job only because it is inneeds), and the per-jobtimeout-minutesloop, which the new job satisfies at 10.Hosted CI on this branch is the check; no local suite was run.
Checklist
This touches
.github/workflows/, whichMAINTAINERS.mdputs behind security review. The new job adds no permission beyond the workflow defaultcontents: read, reads no secret, pinsactions/checkoutto the same SHA every other job uses withpersist-credentials: false, and uses the local./.github/actions/setup-project-bunrather than a third-party action. It runs one repository script and no head-controlled code beyond what every other job in this workflow already runs.Summary by CodeRabbit
CI Improvements
Tests