Move conformance off the CI hot path - #19
Conversation
📝 WalkthroughWalkthroughConformance monitoring now runs in a dedicated workflow with scheduled, manual, and labeled pull-request triggers. Aggregate CI no longer gates on conformance. Tests and documentation reflect the separated validation and publication paths. ChangesConformance monitoring separation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Trigger
participant ShardSelection
participant ConformanceMatrix
participant ArtifactStorage
Trigger->>ShardSelection: Resolve event and shard selection
ShardSelection->>ConformanceMatrix: Provide shard matrix
ConformanceMatrix->>ArtifactStorage: Upload results and screenshots
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/conformance.yml (1)
105-106: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass
matrix.shardthrough an environment variable instead of direct template interpolation.The static analysis tool flags this line for template injection.
matrix.shardvalues are constrained to digits 1-8 or the literalfull-excluded set in the current configuration, so the immediate risk is low. Interpolating${{ matrix.shard }}directly into therun:script string is still the pattern that this class of warning targets, and it removes the safety margin if the shard-selection logic changes later.Move the expression into an
env:block and reference it as a shell variable.🔒 Proposed fix
- name: Run standard conformance shard - run: pnpm conformance -- --shard=${{ matrix.shard }}/8 + env: + SHARD: ${{ matrix.shard }} + run: pnpm conformance -- --shard="$SHARD/8"Update the corresponding assertion in
scripts/conformance-workflow.test.mjs(line 87) to match the new command form.🤖 Prompt for 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. In @.github/workflows/conformance.yml around lines 105 - 106, Update the “Run standard conformance shard” workflow step to assign matrix.shard through its env block and reference that shell environment variable in the run command instead of interpolating the matrix expression directly. Also update the corresponding assertion in the conformance workflow test to expect the new command form.Source: Linters/SAST tools
scripts/conformance-workflow.test.mjs (1)
119-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared workflow-parsing test helpers into one module.
job,needs,scalarList,indentation, andescapeRegExpare duplicated verbatim betweenscripts/conformance-workflow.test.mjsandscripts/ci-workflow.test.mjs. The shared root cause is the absence of a common test-helper module for parsing GitHub Actions workflow YAML.
scripts/conformance-workflow.test.mjs#L119-L196: remove the local copies ofjob,needs,scalarList,indentation, andescapeRegExp, and import them from a new shared module (e.g.scripts/workflow-test-helpers.mjs).scripts/ci-workflow.test.mjs#L198-L276: remove the local copies ofjob,needs,scalarList,indentation, andescapeRegExp, keepmatrixIncludeslocal (or move it too if reused), and import the shared helpers from the same new module.🤖 Prompt for 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. In `@scripts/conformance-workflow.test.mjs` around lines 119 - 196, Create a shared workflow test-helper module exporting job, needs, scalarList, indentation, and escapeRegExp. In scripts/conformance-workflow.test.mjs lines 119-196, remove the local implementations and import the shared helpers; apply the same removal and import in scripts/ci-workflow.test.mjs lines 198-276, leaving matrixIncludes local unless it is also reused.
🤖 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.
Nitpick comments:
In @.github/workflows/conformance.yml:
- Around line 105-106: Update the “Run standard conformance shard” workflow step
to assign matrix.shard through its env block and reference that shell
environment variable in the run command instead of interpolating the matrix
expression directly. Also update the corresponding assertion in the conformance
workflow test to expect the new command form.
In `@scripts/conformance-workflow.test.mjs`:
- Around line 119-196: Create a shared workflow test-helper module exporting
job, needs, scalarList, indentation, and escapeRegExp. In
scripts/conformance-workflow.test.mjs lines 119-196, remove the local
implementations and import the shared helpers; apply the same removal and import
in scripts/ci-workflow.test.mjs lines 198-276, leaving matrixIncludes local
unless it is also reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be047564-7617-4621-8497-f0ec3667bd5e
📒 Files selected for processing (8)
.github/workflows/chart-library-benchmarks.yml.github/workflows/conformance.ymlAPI-FRICTION.mdCONTRIBUTING.mdREADME.mdbenchmarks/conformance/README.mdscripts/ci-workflow.test.mjsscripts/conformance-workflow.test.mjs
💤 Files with no reviewable changes (1)
- .github/workflows/chart-library-benchmarks.yml
1 participant
Summary
Validation
node --test scripts/ci-workflow.test.mjs scripts/conformance-workflow.test.mjs scripts/release-workflow.test.mjspnpm run validate(17 targets, 114 test files, 572 tests)Summary by CodeRabbit
New Features
Documentation
CI Improvements