No actionable comments were generated in the recent review. 🎉

Walkthrough

The CI test matrix gains a shard dimension to split RSpec runs into generators and unit shards; rspec invocation and artifact/test-result names are driven by the shard. Test helpers set Minitest’s at-exit flag to avoid its runner interfering with RSpec runs.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through CI with a twitchy nose,
I split the tests where the wild code grows,
Generators dig, units leap and run,
Together they finish the job much more fun. 🥕✨

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 help to get the list of available commands and usage tips.}

Greptile Summary

This PR splits the rspec-package-tests CI job into two parallel matrix shards (generators and unit) by adding a setup-gem-tests-matrix job, and uses an environment variable for SHARD to follow GitHub Actions security-hardening best practices. The implementation looks correct: the --exclude-pattern "**/generators/**" RSpec flag properly partitions the specs, artifact names are uniquified with the shard suffix, and the conditional logic for PR vs. main/full-ci matrix builds is sound.

Confidence Score: 5/5

Safe to merge — changes are isolated to CI configuration with no runtime code impact.

The only finding is a P2 style/consistency note about inline expressions in the matrix-setup step, which poses no real security risk given the values are trusted GitHub context outputs. All logic (shard routing, exclusion pattern, artifact naming, matrix dimensions) looks correct.

No files require special attention.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant GH as GitHub Actions
    participant DC as detect-changes
    participant SM as setup-gem-tests-matrix
    participant G as rspec-package-tests (shard: generators)
    participant U as rspec-package-tests (shard: unit)

    GH->>DC: trigger (push / PR / dispatch)
    DC-->>SM: docs_only, run_ruby_tests, has_full_ci_label

    alt main branch OR full-ci label
        SM-->>G: ruby 3.4 latest + ruby 3.2 minimum
        SM-->>U: ruby 3.4 latest + ruby 3.2 minimum
    else PR (latest only)
        SM-->>G: ruby 3.4 latest
        SM-->>U: ruby 3.4 latest
    end

    par Generators shard
        G->>G: bundle exec rspec spec/react_on_rails/generators
        G-->>GH: upload artifact (…-generators)
    and Unit shard
        U->>U: bundle exec rspec spec/react_on_rails --exclude-pattern **/generators/**
        U-->>GH: upload artifact (…-unit)
    end

_{Reviews (2): Last reviewed commit: "fix: use env var for matrix.shard to fol..." | Re-trigger Greptile}

Review: Split rspec-package-tests into parallel shards

Overall this is a clean, well-scoped CI-only change. The shard boundary (generators vs. everything else) is a sensible split — generators are the heaviest filesystem consumers — and the artifact naming update correctly prevents collisions.

What looks good

• fail-fast: false is already set, so a flaky generator spec won't kill the unit shard mid-run.
• Gem cache key deliberately excludes the shard (both shards share the same gems), which is correct and efficient.
• The --exclude-pattern "**/generators/**" glob is correct for RSpec; it will match spec/react_on_rails/generators/ and any nested paths.
• The 2×2 main matrix (2 Ruby versions × 2 shards) and 1×2 PR matrix are structured correctly.

Two items to address

1. Script injection pattern (inline comment on line 156) — ${{ matrix.shard }} is interpolated directly into a shell if condition. No real risk here since the values are workflow-controlled, but GitHub's hardening guide recommends routing through an env var instead. Easy fix.

2. Long JSON on lines 91/94 (inline comment) — minor readability nit; not blocking.

One implicit behaviour to be aware of

If matrix.shard ever takes an unexpected value (e.g., a typo), the else branch silently runs the unit suite instead of failing loudly. You could add an explicit guard:

elif [ "$SHARD" = "unit" ]; then
  bundle exec rspec spec/react_on_rails --exclude-pattern "**/generators/**"
else
  echo "Unknown shard: $SHARD" && exit 1
fi

Not required for this PR, but worth considering if/when more shards are added.

Review comments addressed

I (Claude Code) triaged all 6 review comments and addressed them as follows:

Fixed (1):

• Script injection hardening (@claude[bot]) — Moved ${{ matrix.shard }} to an env: block and switched [ ] → [[ ]] for bash consistency. Fixed in 29b9b3e.

Acknowledged with rationale (2):

• dev/pack_generator_spec.rb in unit shard (@greptile-apps[bot]) — Spec is lightweight; runtime split is balanced.
• Long matrix JSON (@claude[bot]) — Only 2–4 entries today; will revisit if matrix grows.

Skipped (3):

• CodeRabbit rate limit warning — bot status post
• Greptile summary + "Git commit step" observation — bot summary, harmless no-op
• Claude review summary — duplicates the inline comments above

All 3 review threads have been resolved.

Good optimization overall. Isolating heavy filesystem-I/O generator specs into their own shard is the right approach. No blockers, but a few notes:

Exclude pattern edge case: The pattern --exclude-pattern '**/generators/**' correctly partitions the current layout (all generator specs are under spec/react_on_rails/generators/). Worth noting: a future top-level file like generators_integration_spec.rb would NOT be excluded since the pattern is directory-based. This is fine for now but worth keeping in mind as the suite grows.

Bash comparison style (minor): [[ "$SHARD" = "generators" ]] uses single = — valid in [[ ]] but == is the idiomatic form for string comparison in bash.

Long single-line JSON (readability): The matrix JSON on lines 91 and 94 are very long one-liners. Consider a multi-line variable or heredoc approach before echoing to $GITHUB_OUTPUT to make the matrix easier to audit and extend.

Git Stuff step comment is misleading: The step runs for both shards when dependency-level == minimum and the comment says 'stop generators from complaining about uncommitted code' — but the commit is actually needed to clean up files modified by the convert script before any tests run, not just generators. Behavior is correct; just the comment that needs updating.

Review: Split rspec-package-tests into parallel generator/unit shards

Overall: Clean, well-motivated change. The shard split correctly targets the I/O-heavy generator specs and the artifact naming is updated consistently. A few observations below.

Correctness

• Shard coverage is complete. The generators shard runs spec/react_on_rails/generators directly; the unit shard uses --exclude-pattern '**/generators/**' which correctly excludes everything under that directory. No tests are double-counted or dropped.
• fail-fast: false is already set, so one failing shard does not cancel the other — good.

Minitest suppression (both spec_helpers)

The new Minitest.class_variable_set(:@@installed_at_exit, true) block is needed because rails/test_help registers a Minitest at_exit hook, and when RSpec exits with --exclude-pattern in ARGV, Minitest's hook tries to parse those args and errors. The existing Test::Unit.run = true guard was already handling the older Test::Unit layer; this extends it to Minitest 5+. The guard (class_variable_defined? check) makes it safe against future Minitest versions that remove the variable.

One question: react_on_rails_pro/spec_helper.rb is updated here, but gem-tests.yml only runs the open-source gem (cd react_on_rails). The pro spec_helper change is not exercised by this workflow. Is this a defensive fix bundled in, or will a separate pro CI PR follow?

Artifact path (pre-existing)

The log/test.log artifact path was there before this PR. Worth noting: cd react_on_rails only changes the shell working directory, not where actions/upload-artifact looks — it resolves paths relative to $GITHUB_WORKSPACE. If the log is written to react_on_rails/log/test.log, the artifact path should be react_on_rails/log/test.log. Pre-existing issue, not introduced here, but worth checking.

No blocking issues. The three inline comments are nits/notes — the implementation is sound and achieves the stated ~12 min reduction goal.