Add Babel 8 preset smoke

[justin808]

Summary

• add an opt-in Babel 8 smoke test under test/packages/ that installs exact Babel 8 plus babel-loader 10 versions into a temp npm app
• compile a fixture through Webpack using Shakapacker's built Babel preset copied into a temp consumer node_modules/shakapacker layout
• add a dedicated Babel 8 smoke workflow on Node 22 with narrow path filters plus workflow_dispatch, without changing normal Babel 7 dev pins
• fix existing node workflow actionlint findings that would block workflow-change PRs

Closes #1191

Local validation

• yarn install --frozen-lockfile --non-interactive --prefer-offline
• yarn build
• BABEL_ENV=production RUN_BABEL8_SMOKE=1 node node_modules/jest/bin/jest.js test/packages/babel8-smoke.test.js --runInBand --no-cache
• node node_modules/eslint/bin/eslint.js test/packages/babel8-smoke.test.js
• node node_modules/prettier/bin/prettier.cjs --check .github/workflows/babel8-smoke.yml .github/workflows/node.yml test/packages/babel8-smoke.test.js
• actionlint .github/workflows/babel8-smoke.yml .github/workflows/node.yml
• git diff --check origin/main...HEAD

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0fab111d-c433-47c4-9994-0c2c66a496d4

📥 Commits

Reviewing files that changed from the base of the PR and between 0007880 and 21c029f.

📒 Files selected for processing (4)

• .agents/bin/README.md
• .github/workflows/babel8-smoke.yml
• .github/workflows/node.yml
• test/packages/babel8-smoke.test.js

Walkthrough

Adds an opt-in Babel 8 smoke test that builds a sample bundle through Shakapacker’s preset, and wires a new CI job to run it. The workflow also includes a curl handling refactor and a d.ts glob path correction.

Changes

Babel 8 smoke test and CI wiring

Layer / File(s)	Summary
Smoke test gating and helpers test/package/babel8-smoke.test.js	Adds opt-in gating, environment/file precondition checks, subprocess execution helpers, installed-version readers, temp package copying, and Webpack invocation helpers.
Smoke test suite and assertion test/package/babel8-smoke.test.js	Creates the temp workspace lifecycle and runs the Babel 8 Webpack smoke build, asserting exact package versions and that the bundle output contains 42.
CI job and workflow fixes .github/workflows/node.yml	Adds the babel8-smoke job on Node 22, and updates the Actionlint curl handling plus the d.ts verification glob.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI as GitHub Actions
  participant Jest as babel8-smoke.test.js
  participant npm as npm
  participant Webpack as webpack
  participant Preset as Shakapacker Babel preset

  CI->>Jest: run with RUN_BABEL8_SMOKE=1
  Jest->>npm: install Babel 8 and babel-loader packages
  Jest->>Webpack: build sample entry
  Webpack->>Preset: resolve and apply preset
  Preset-->>Webpack: transformed code
  Webpack-->>Jest: bundle output
  Jest-->>CI: pass/fail result

Loading

Possibly related PRs

• shakacode/shakapacker#1187: Introduced the Babel 8 preset and loader compatibility that this automated smoke test exercises in CI.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR adds an automated Babel 8 smoke in CI that runs a real transform with Babel 8 and babel-loader 10 while keeping normal Babel 7 pins unchanged.
Out of Scope Changes check	✅ Passed	The workflow lint fix and glob tweak support the CI smoke setup and do not appear unrelated to the Babel 8 smoke work.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding a Babel 8 smoke test and CI job.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/1191-babel8-smoke

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR adds a Babel 8 smoke path without changing the normal Babel 7 development pins. The main changes are:

• A new Node 24 workflow job for the Babel 8 smoke.
• A temp npm app test that installs Babel 8 and babel-loader 10.
• A webpack compile check using Shakapacker's built Babel preset.
• Small workflow fixes for actionlint and declaration-file type checking.

Confidence Score: 5/5

The changed flow looks mergeable after a small cleanup to the smoke test shutdown path.

• The workflow additions are contained to CI.
• The smoke test correctly isolates Babel 8 and babel-loader 10 in a temp app.
• Webpack close errors can be swallowed, which can hide a test-harness failure.

test/package/babel8-smoke.test.js

Important Files Changed

Filename	Overview
.github/workflows/node.yml	Adds the Node 24 Babel 8 smoke job and fixes existing workflow lint/type-check command issues.
test/package/babel8-smoke.test.js	Adds the opt-in Babel 8 webpack smoke test, with one cleanup issue in the webpack close handling.

_{Reviews (1): Last reviewed commit: "Add Babel 8 preset smoke" | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 712cbc7b9f

ℹ️ 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".

[claude]

Review summary

Reviewed the workflow change and the new test/package/babel8-smoke.test.js.

Main finding (inline comment on the test file): The temp app's Babel 8 packages are installed into os.tmpdir(), but presets: [presetPath] points at this repo's own package/babel/preset.js, whose returned config includes bare string names ("@babel/preset-env", "@babel/plugin-transform-runtime"). Babel resolves those relative to the directory of the file that produced them — i.e. this repo's package/babel/, not the temp workRoot. Since those two packages are only optional peerDependencies here (absent from yarn.lock), that resolution walk-up never reaches the repo root's node_modules or the temp dir's node_modules, so this could fail with Cannot find module '@babel/preset-env' on a clean runner rather than actually validating Babel 8 compatibility. Worth double-checking this passes on a genuinely clean CI runner (not just a dev machine that might have stray global Babel installs on the resolution path).

Smaller/non-blocking notes:

• The new babel8-smoke job triggers on the same broad path filters as the rest of node.yml (any **.js/**.ts/package.json change), so it adds a network npm install + full yarn build to most PRs, not just Babel-related ones. Given it's explicitly framed as an opt-in smoke test, it might be worth scoping this job's trigger more narrowly (e.g. only when package/babel/** or the workflow file itself changes) to keep routine PRs faster.
• Installing @babel/core@^8, @babel/preset-env@^8, etc. via unpinned ^8/^10 ranges means a new Babel 8 patch/minor release could break this job independent of any change in this repo. Consider pinning exact versions for reproducibility, bumping deliberately.

Workflow fixes (.github/workflows/node.yml): The curl/$? → if response=$(...); then change and the *.d.ts → ./*.d.ts change are both correct, standard shellcheck-style hardening with no behavior change.

[justin808]

Address-review follow-up summary:

• runWebpack now waits for compiler.close and surfaces close-time errors before resolving the smoke.
• The smoke now copies the built Shakapacker preset/helpers into workRoot/node_modules/shakapacker and resolves the preset from the temp app, matching the consumer package layout used for Babel package resolution.

Focused verification after the latest patch:

• RUN_BABEL8_SMOKE=1 node node_modules/jest/bin/jest.js test/package/babel8-smoke.test.js --runInBand
• node node_modules/eslint/bin/eslint.js test/package/babel8-smoke.test.js
• node node_modules/prettier/bin/prettier.cjs --check test/package/babel8-smoke.test.js
• git diff --check origin/main...HEAD

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e90f8db20b

ℹ️ 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".

[claude]

Review summary

Reviewed the actionlint fixes and the new Babel 8 smoke test/job. Two inline comments posted with details:

1. Test-coverage gap (main finding): the new smoke test likely never exercises the Babel‑8‑specific presetEnvOptions branch in package/babel/preset.ts. Jest sets NODE_ENV=test by default, which routes the preset through its isTestEnv branch (targets: { node: "current" }) instead of the isProductionEnv || isDevelopmentEnv branch where the Babel‑8-specific presetEnvOptions logic lives. As written, the test would still pass even if that branch were broken for Babel 8. Suggest explicitly setting envName: "production" (or "development") in the babel-loader options used by the test.
2. CI cost/reliability nit: the new babel8-smoke job runs on every push to main (no path filter on push, unlike pull_request) and does an unpinned npm install of floating ^8/^10 Babel/babel-loader ranges with no lockfile — this could turn into an unrelated, flaky failure when a new Babel 8 patch lands, and adds real time to every merge since there's no cache for that npm install.

Other things checked and looking good:

• The if [ $? -eq 0 ] → if response=$(curl ...); then fix in node.yml is a legitimate correctness fix — under the default bash -e for run: steps, the previous form would abort the whole step on a failed curl before the if check ever ran.
• The *.d.ts → ./*.d.ts change is a reasonable shellcheck/actionlint (SC2035) fix.
• babel-loader@10.1.1 and webpack@5.106.2 are already devDependencies, so the versions installed in the temp app for the smoke test are compatible with the compiler used to run it.
• Test cleanup (afterAll removing the temp dir), opt-in gating (RUN_BABEL8_SMOKE=1 + test.todo skip), and error handling in the run()/runWebpack() helpers all look solid.

[claude]

Review summary

Overall this is a clean, well-isolated addition: the Babel 8 smoke test is opt-in (RUN_BABEL8_SMOKE=1), auto-skips via test.todo when not opted in or when npm/the built package aren't available, and runs in a fully isolated temp dir via npm install so it doesn't touch the repo's Yarn lockfile or pollute the normal Babel 7 dev pins. The dedicated babel8-smoke CI job on Node 24 is well-scoped and won't affect the existing test matrix (confirmed shouldRun is gated on the opt-in env var, so the file is a safe no-op under plain yarn test). The node.yml actionlint/shellcheck fixes (if response=$(...); then instead of checking $? afterward, and ./*.d.ts instead of a bare leading-* glob) are correct, low-risk cleanups.

One organizational note (left as an inline comment): the new test lives in test/package/ (singular), but the existing near-identical pattern for opt-in "install into a temp app and smoke test the built package" tests (install-smoke.test.js) lives in test/packages/ (plural). Moving babel8-smoke.test.js there (and updating the yarn jest test/package/babel8-smoke.test.js path in the workflow) would keep the convention consistent.

No security or correctness issues found — the smoke test only shells out to fixed, hardcoded package specs (no user-controlled input reaches spawnSync), and cleans up its temp directory in afterAll.

[justin808]

CI follow-up: the hosted Babel 8 smoke failed twice on the dedicated Node 24 runner with a Webpack parser error (Unexpected token export) that I could not reproduce locally on the exact PR merge ref with Node 24 and npm 11. I moved the smoke job to Node 22.x, which is already in this repo’s Node CI support matrix, while keeping exact Babel 8/babel-loader pins and envName: "production". Focused local verification after this change passed: BABEL_ENV=production RUN_BABEL8_SMOKE=1 node node_modules/jest/bin/jest.js test/package/babel8-smoke.test.js --runInBand --no-cache, eslint, prettier, actionlint, and git diff --check.

[coderabbitai]

🧹 Nitpick comments (1)

.github/workflows/node.yml (1)

171-196: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider adding an explicit permissions: block to the new job.

Static analysis flags the file for relying on default (broad) GITHUB_TOKEN permissions. The new babel8-smoke job only checks out code, installs deps, builds, and runs Jest — it needs no write access. Since this job doesn't use secrets.GITHUB_TOKEN for anything, scoping it to read-only (or permissions: {}) reduces blast radius if a compromised dependency runs during npm install/yarn build/Jest.

🔒 Suggested addition

   babel8-smoke:
     name: Babel 8 smoke
     runs-on: ubuntu-latest
+    permissions:
+      contents: read

This mirrors a pre-existing gap already flagged by zizmor on the type-check-dts-without-webpack job (Lines 111-143), which also has no permissions: block.

🤖 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/node.yml around lines 171 - 196, The new babel8-smoke job
is relying on the default broad GITHUB_TOKEN scope. Add an explicit permissions
block to this job in the node workflow so it runs with read-only access or no
token permissions at all, since it only uses actions/checkout, setup-node, yarn
install/build, and the Jest smoke test. Keep the change localized to the
babel8-smoke job so the workflow’s other jobs are unaffected.

Source: Linters/SAST tools

🤖 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/node.yml:
- Around line 171-196: The new babel8-smoke job is relying on the default broad
GITHUB_TOKEN scope. Add an explicit permissions block to this job in the node
workflow so it runs with read-only access or no token permissions at all, since
it only uses actions/checkout, setup-node, yarn install/build, and the Jest
smoke test. Keep the change localized to the babel8-smoke job so the workflow’s
other jobs are unaffected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c34f4eac-d3c2-4ec4-9c84-0dfcdecd7163

📥 Commits

Reviewing files that changed from the base of the PR and between 712cbc7 and 0007880.

📒 Files selected for processing (2)

• .github/workflows/node.yml
• test/package/babel8-smoke.test.js

[claude]

Review summary

Overall this is a well-scoped, self-contained addition: an opt-in Babel 8 smoke test that installs Babel 8 + babel-loader 10 into a temp npm project and compiles a fixture through Shakapacker's built Babel preset, plus a dedicated CI job for it. I traced the preset's dependency chain (preset.ts → utils/helpers.ts → utils/errorHelpers.ts → utils/errorCodes.ts) against the builtPackageFiles list in the test and it's complete — no missing runtime deps. The synthetic package.json/subpath require also correctly mirrors the real exports map and the documented require('shakapacker/package/babel/preset.js') usage pattern in docs/customizing_babel_config.md. The unrelated actionlint/shellcheck fixes (if response=$(...) and ./*.d.ts) are correct, behavior-preserving cleanups.

Left two inline notes:

1. Reliability of a required check: the smoke pins exact patch versions of very-new Babel 8 / babel-loader 10 packages and fetches them from the live npm registry on every JS/TS-touching PR. If any of those exact versions is ever unpublished/deprecated upstream, this job fails on unrelated PRs. Consider a caret range, continue-on-error, or at least documenting the expectation that these pins need periodic bumping.
2. Minor inconsistency: the PR description says the new job "runs the smoke with Node 24," but the workflow pins Node 22.x. Worth reconciling one way or the other.

No security concerns — the installed package list is a fixed, hardcoded array (no interpolation of untrusted input into npm install/shell commands), and the temp directories are created via mkdtempSync and cleaned up in afterAll.

[claude]

Review summary

Overall this is a well-built, well-tested addition — the set -e/curl fix and the ./*.d.ts glob fix are both legitimate corrections (the old if [ $? -eq 0 ] pattern was actually dead code under GitHub Actions' default bash -e, since a failing response=$(curl ...) assignment would exit the step before that check ever ran). The smoke test itself correctly exercises Babel 8 + babel-loader 10 through the real, built Shakapacker preset, and the dependency chain hardcoded in the test currently matches preset.ts's actual imports.

Left two inline notes worth considering before merge:

1. The new babel8-smoke CI job isn't actually opt-in in practice — it hardcodes RUN_BABEL8_SMOKE: "1", so it runs on every PR that touches the paths this workflow already watches (basically all JS/TS PRs), unlike the sibling install-smoke.test.js, which the repo deliberately never wires into CI (RUN_INSTALL_SMOKE is never set anywhere in .github/workflows/). That means routine PRs now depend on the npm registry and on exact pinned Babel 8 pre-release versions staying installable.
2. The builtPackageFiles list in the test is a hand-maintained mirror of preset.ts's transitive requires. It's correct today, but a future edit to preset.ts/helpers.ts that adds a new import would silently go stale and produce a confusing MODULE_NOT_FOUND instead of a clear signal. The sibling install-smoke.test.js sidesteps this by npm pack-ing the real built package instead of curating a file list.

No security issues found (no secrets touched, persist-credentials: false kept, standard public-registry install pattern already used elsewhere in the test suite).

[justin808]

CI follow-up: the hosted smoke failure remained runner-only after moving to Node 22, still at Webpack parsing transformed output. I simplified the fixture source to avoid syntax transforms and keep this smoke focused on the target contract: Babel 8 packages are installed at exact versions, the built Shakapacker preset is loaded from a temp consumer node_modules/shakapacker layout, envName: "production" exercises the production/development preset branch, and webpack compiles through babel-loader@10.1.1. Focused local validation passed after the change.

[claude]

Review summary

Overall this is a well-scoped, opt-in smoke test (mirrors the existing install-smoke.test.js pattern: skips cleanly via test.todo unless RUN_BABEL8_SMOKE=1 + built + npm present, so it's a no-op in the normal yarn test job/matrix). The node.yml diff is a straightforward actionlint/shellcheck cleanup ($?-after-assignment → if cmd; then, and quoting the glob as ./*.d.ts) with no behavior change. New workflow is properly scoped with narrow paths: filters, workflow_dispatch, and a concurrency group.

Left one inline comment on test/packages/babel8-smoke.test.js:

• Test-fidelity gap: installBuiltShakapackerPackage hand-writes a minimal synthetic package.json for the fake node_modules/shakapacker (no exports field), instead of using npm pack against repoRoot the way the sibling install-smoke.test.js does (packTarball/coreTarball). Because the real repo package.json has an exports map (including an entry specifically for ./package/babel/preset.js), and Node applies no subpath restriction when exports is absent, this smoke test would keep passing even if that exports entry were later broken/removed — silently missing the exact regression it's meant to guard against for consumers using the documented preset path.

Minor/non-blocking notes (not filed inline):

• The exact pinned versions (@babel/core@8.0.1, babel-loader@10.1.1, etc.) will need periodic bumping as Babel 8 patches land, and could fail outright if a pinned version is ever unpublished — acceptable for an opt-in smoke test, just a maintenance note.

[claude]

Review: Add Babel 8 preset smoke

Reviewed the diff via gh pr diff 1214 (3 files, +254/-3).

Summary

• Adds an opt-in (RUN_BABEL8_SMOKE=1) smoke test that installs exact Babel 8 + babel-loader@10.1.1 versions into a scratch npm app and runs a real Webpack compile through Shakapacker's built Babel preset (package/babel/preset.js).
• Adds a dedicated babel8-smoke.yml workflow (Node 22, narrow paths filters, workflow_dispatch) that doesn't touch the normal Babel 7 dev pins.
• Fixes two pre-existing actionlint/shellcheck findings in node.yml ($?-after-assignment pattern → if response=$(...); then, and unquoted *.d.ts glob → ./*.d.ts).

What's good

• Real integration coverage is a nice complement to the existing mocked unit tests in test/package/rules/babel.test.js (which only mock a fake Babel API version/packageMajorVersion); this test actually installs Babel 8 packages and runs a real compile.
• Exact package pins (@babel/core@8.0.1, etc.) rather than ranges — avoids drifting silently onto a broken future Babel 8 patch.
• Auto-skips cleanly via test.todo when not opted in / core not built / npm missing, following the same pattern as the existing install-smoke.test.js.
• persist-credentials: false, concurrency.cancel-in-progress, and narrow paths filters on the new workflow are all good practice and match repo conventions.
• The node.yml fixes are correct, minimal, and match the described actionlint findings.

Suggestions (non-blocking)

1. Duplicated helper — toolVersion() in test/packages/babel8-smoke.test.js is copy-pasted verbatim from test/packages/install-smoke.test.js. Worth extracting to a shared helper (e.g. test/packages/smokeHelpers.js) now that a second opt-in smoke test needs it, to avoid a third copy next time.
2. Unbounded npm install in CI — run("npm", ["install", ...]) (and the underlying spawnSync calls) have no timeout, and the new workflow job has no timeout-minutes. If the registry install stalls (network flake), the job can run for a long time before GitHub's default job timeout kicks in. Not new to this PR's conventions (no existing workflow sets timeout-minutes either), but worth considering given this job does a real network install rather than using the yarn cache.
3. Minor style nit — let presetPath is declared at the describe scope but only ever assigned/used inside the single test(...) body; could just be a local const there.

Nothing here blocks merging — the core coverage gap being closed (real compile through Babel 8 + babel-loader 10 with the built preset) looks well-targeted and matches the PR description and issue #1191.

[justin808]

CI follow-up: the hosted failure was Jest parsing Babel 8 ESM while Webpack/babel-loader ran inside the Jest runtime. The smoke now keeps the Webpack + babel-loader coverage, but writes and runs a small child-process Node script from the temp app. That runs outside Jest’s CommonJS module wrapper, still uses the temp consumer node_modules/shakapacker layout, babel-loader@10.1.1, and envName: "production". Focused local smoke, eslint, prettier, actionlint, and diff checks pass.

[claude]

Review

Went through all three changed files (babel8-smoke.yml, node.yml, babel8-smoke.test.js). Overall this is a well-isolated, opt-in addition — it follows the existing install-smoke.test.js pattern closely (env-var opt-in, test.todo skip with a reason, temp-dir isolation, cleanup in afterAll), doesn't touch the Babel 7 dev pins, and the new workflow's path filters look correctly scoped to the babel preset/rules/utils surface.

Things I verified and are correct:

• preset.ts's isBabel8 branch already special-cases Babel 8 (skips core-js/useBuiltIns and passes @babel/plugin-transform-runtime without the helpers: false option), so the smoke test doesn't need core-js installed in the temp app — this is consistent with existing logic, not new.
• The generated webpack runner correctly resolves babel-loader/@babel/* from the temp app's node_modules (via createRequire) while reusing the repo's own webpack, so there's no cross-contamination between the Babel 7 repo deps and the Babel 8 temp install.
• The node.yml diff is a faithful behavior-preserving shellcheck/actionlint cleanup ($? → if response=$(...), *.d.ts → ./*.d.ts), no functional change.

One minor nit left as an inline comment: toolVersion() in babel8-smoke.test.js is a verbatim duplicate of the same helper in install-smoke.test.js — worth extracting to a shared test util given jest.config.js already reserves an __utils__ convention, but not blocking.

Low-severity observation (not requesting a change, just flagging): the dedicated babel8-smoke.yml workflow is the only thing that ever sets RUN_BABEL8_SMOKE=1, and if any precondition silently fails to hold (e.g. yarn build ever stops emitting package/babel/preset.js at that exact path, or npm were somehow missing from the runner), the test degrades to test.todo rather than a hard failure — so the workflow would stay green without ever having exercised the Babel 8 compile. This mirrors the existing install-smoke.test.js design already accepted in this repo, so it's a pre-existing risk profile rather than something new, but worth keeping in mind since this workflow's entire purpose is that one check.

No security or correctness issues found in the new workflow YAML (persist-credentials: false, scoped path filters, no secrets used) or in the test's use of spawnSync/array args (no shell injection surface).

[claude]

Review: Add Babel 8 preset smoke

Overview

Adds an opt-in Babel 8 compatibility smoke test (test/packages/babel8-smoke.test.js) that installs exact Babel 8 + babel-loader 10 packages into a temp app, copies the built Shakapacker package/ output into a fake node_modules/shakapacker, and runs a real Webpack compile through babel-loader using Shakapacker's built preset. A companion babel8-smoke.yml workflow runs this on Node 22 with narrow path filters + workflow_dispatch, and node.yml gets two small actionlint/shellcheck fixes unrelated to the smoke test itself.

This closes #1191 and builds on the Babel 8 preset support already merged in #1187 — this PR is pure test/CI infrastructure, so per project convention it correctly carries no CHANGELOG entry.

Strengths

• Closely mirrors the existing test/packages/install-smoke.test.js convention (opt-in env var, coreIsBuilt/tool-detection auto-skip via test.todo, temp dir cleanup in afterAll), so it's consistent with established repo patterns rather than inventing a new one.
• No shell interpolation risk: spawnSync is always called with an argument array (no shell: true), including in the generated run-webpack-smoke.cjs runner, where all embedded paths go through JSON.stringify rather than string concatenation.
• Path-filtered workflow keeps the extra Node 22 / npm-install job from running on unrelated PRs, and concurrency + timeout-minutes: 10 are sensible guardrails.
• The two node.yml changes (if response=$(curl ...); then instead of checking $? separately, and ./*.d.ts instead of a bare glob) are correct, minimal actionlint/shellcheck fixes with no behavior change.

Suggestions / things to watch

1. Exact version pins will go stale silently. babel8SmokePackages hardcodes exact versions (@babel/core@8.0.1, @babel/preset-env@8.0.2, babel-loader@10.1.1, etc.) inside the test file rather than in package.json/devDependencies. Since these live in a JS array literal, Dependabot/Renovate won't track or bump them, so the smoke test will keep validating against the same fixed snapshot indefinitely unless someone remembers to bump it by hand. Worth a comment near the array (or a tracking issue) noting these need periodic manual review, similar to how install-smoke.test.js documents its version-sensitivity in its file header.
2. Coverage gap vs. the workflow's path filters. babel8-smoke.yml triggers on changes to package/rules/babel.ts, but the test never exercises that file — it calls babel-loader directly with presets: [presetPath] rather than going through loaderMatches/validateBabelLoaderCompatibility() in package/rules/babel.ts. That guard (which throws if Babel 8 + babel-loader < 10 are paired) is exactly the kind of regression this smoke suite seems intended to catch, but a change that broke it wouldn't fail this test. Consider either adding a small assertion that exercises the rule directly, or documenting the intentional scope (preset only, not the loader rule).
3. Minor: no explicit permissions: block on the new workflow. Not a regression (existing workflows like node.yml also omit it), but since this is a brand-new workflow file that only needs read access, adding permissions: contents: read at the job level would be a small defense-in-depth improvement.

Risks

• If the pinned Babel 8 packages are pre-release/beta versions that get renamed or unpublished upstream, this workflow will start failing on every relevant PR until someone updates the pins — worth confirming shortly after merge that the exact versions resolve cleanly in CI (the sandboxed review environment here couldn't reach the npm registry to verify).

Overall this is a well-scoped, low-risk addition that follows existing conventions closely. The suggestions above are minor and non-blocking.

[claude]

Review: Add Babel 8 preset smoke

Overview

Adds an opt-in smoke test (test/packages/babel8-smoke.test.js) that installs exact Babel 8 + babel-loader@10.1.1 versions into a temp npm app and compiles a trivial fixture through Webpack using Shakapacker's built Babel preset, plus a dedicated babel8-smoke.yml GitHub Actions workflow (Node 22, narrow path filters, workflow_dispatch), and two small actionlint/shellcheck fixes in node.yml. Closes #1191.

Code quality

• The opt-in gating (RUN_BABEL8_SMOKE=1, coreIsBuilt, hasNpm) is clean and fails safe via test.todo(...) rather than silently skipping or failing when preconditions aren't met — good for keeping the default yarn test run green while still surfacing that the smoke exists.
• Good separation: the workflow installs host tooling with yarn --frozen-lockfile (webpack, jest, ts) while the test itself uses a plain npm install in a temp dir for the exact Babel 8 trio — avoids polluting yarn.lock with prerelease/major-bump pins.
• installBuiltShakapackerPackage correctly mirrors the real published package shape (main/exports/files from the root package.json, copying the built package/ dir), so the smoke exercises the same resolution path a real consumer would hit.
• The node.yml fixes (if response=$(curl -sf ...) instead of checking $? separately, and ./*.d.ts instead of a bare glob) are correct, minimal SC2181/glob-safety fixes consistent with the stated goal of unblocking actionlint on workflow-change PRs.

Suggestions

• The fixture compiled in the smoke (var answer = 42; console.log(answer);) contains no syntax that actually requires transformation (no arrow functions, classes, optional chaining, etc.), so the test mainly verifies that the preset/plugins load and run under Babel 8's API, not that transforms still work correctly. Consider adding at least one modern-syntax construct so a regression in @babel/preset-env option handling would actually change the output, not just fail to error out (see inline comment).
• The babel8-smoke.yml path filters include package/rules/babel.ts, but the smoke test calls the built preset directly (presets: [presetPath]) and never goes through package/rules/babel.ts's validateBabelLoaderCompatibility logic — so changes to that file trigger this workflow without actually being exercised by it (harmless, just a bit misleading; see inline comment).
• .agents/bin/README.md is a whitespace-only table-formatting change unrelated to the Babel 8 smoke — per this repo's convention of keeping PRs focused, consider splitting it into its own commit/PR unless it was an unavoidable side effect of running the formatter over the diff.

Risks / considerations

• Exact pins (@babel/core@8.0.1, @babel/preset-env@8.0.2, @babel/plugin-transform-runtime@8.0.1, @babel/runtime@8.0.0, babel-loader@10.1.1) will need periodic bumping as Babel 8 stabilizes; the in-file comment calling this out during "Babel compatibility reviews" is a reasonable mitigation, but there's no automated reminder (e.g., Dependabot/renovate ignoring these, or a scheduled check) — low risk given this is opt-in and narrowly path-filtered, so it fails loudly rather than silently.
• The workflow performs npm install of packages from the public registry on pull_request (including forked PRs, since it's opt-in via path filters and doesn't touch secrets) — acceptable since versions are pinned to specific official @babel/* releases and no secrets are exposed to the job.

Overall this is a well-scoped, low-risk addition that keeps Babel 7 as the default dev pin while giving early signal on Babel 8 compatibility. No blocking issues found.

[claude]

Review summary

Overall this is a well-structured, self-contained addition — the opt-in gating (RUN_BABEL8_SMOKE=1 + built-package check + npm presence check), temp-workspace isolation, and cleanup in afterAll all follow the existing pattern from test/packages/install-smoke.test.js. The node.yml actionlint fixes (if response=$(...) instead of checking $? separately, and ./*.d.ts instead of a bare glob) are correct, minimal shellcheck-driven fixes. The new babel8-smoke.yml workflow is scoped with sensible path filters, concurrency, and a timeout-minutes, and correctly uses pull_request (not pull_request_target), so it won't expose secrets to fork PRs.

I verified the core logic being smoke-tested: package/babel/preset.ts's isBabel8 branching (different presetEnvOptions, and unconditional @babel/plugin-transform-runtime instead of { helpers: false }) is exactly the code this new test exercises, and the babel-loader version guard it explicitly defers to (test/package/rules/babel.test.js) does already cover that case.

One coverage gap worth a look — left as an inline comment: the compiled fixture (var answer = 42; console.log(answer);) is plain ES5, so @babel/preset-env has nothing to downlevel and @babel/plugin-transform-runtime never injects a helper. The test currently confirms the Babel 8 preset/plugin config loads and resolves without throwing, but doesn't exercise the actual transform path that's most likely to regress between Babel 7 and 8. Adding one modern-syntax construct (arrow function/class/async function) to the fixture would make this smoke test meaningfully stronger.

No security or performance concerns beyond the expected cost of the new opt-in CI job (which only runs on relevant path changes, as intended).

[claude]

Review summary

Reviewed the Babel 8 smoke test, the new dedicated workflow, and the incidental Node workflow/actionlint fixes.

Overall: this is a low-risk, test/CI-only change that closely follows the existing test/packages/install-smoke.test.js opt-in-smoke-test convention (env-var gate, auto-skip via test.todo when not opted in, temp-dir install + cleanup). The mechanism — building the real preset, copying it into a fake node_modules/shakapacker, then installing exact Babel 8 + babel-loader@10 versions into a throwaway app and driving webpack directly — is a sound way to smoke-test the published preset path without touching the repo's own Babel 7 dev pins.

Things I checked and are fine:

• The smoke workflow is scoped with narrow paths: filters and workflow_dispatch, doesn't touch secrets, uses persist-credentials: false, and isn't part of required checks, so a stale pinned Babel 8 pre-release version (8.0.1/8.0.2) failing later only breaks this one opt-in job, not the main suite.
• package.json's exports/main/files are copied correctly into the fake package so appRequire.resolve("shakapacker/package/babel/preset.js") resolves to the built output.
• The generated webpack runner script safely JSON-stringifies all interpolated paths (no injection risk), and correctly propagates compiler errors/exit codes back to the parent spawnSync check.
• The node.yml diff is a straightforward actionlint/shellcheck style fix (if response=$(...) instead of $? after the fact) plus a ./*.d.ts glob fix — no behavior change.
• .agents/bin/README.md is a pure formatting diff.

One minor nit left as an inline comment: the run() helper in babel8-smoke.test.js discards spawnSync's error field, so a "tool not found" failure mode (as opposed to "compile failed") produces a less useful error message. Not blocking.

No security or performance concerns found; no CHANGELOG entry needed since this is test/CI tooling only, consistent with CLAUDE.md guidance.