Skip to content

Add reusable GitHub Actions flow generator#278

Draft
justin808 wants to merge 28 commits intomainfrom
add-github-flow-generator
Draft

Add reusable GitHub Actions flow generator#278
justin808 wants to merge 28 commits intomainfrom
add-github-flow-generator

Conversation

@justin808
Copy link
Copy Markdown
Member

@justin808 justin808 commented Apr 15, 2026

Summary

  • add cpflow generate-github-actions with reusable review-app, staging, promotion, and cleanup workflows
  • preserve executable bits for generated shell scripts and document the AI/demo-app rollout requirements
  • cover the new generator with command specs and keep command docs in sync

Verification

  • CPLN_ORG=test-org bundle exec rspec spec/command/generate_github_actions_spec.rb spec/command/generate_spec.rb
  • bundle exec rubocop lib/command/generate.rb lib/command/generate_github_actions.rb spec/command/generate_spec.rb spec/command/generate_github_actions_spec.rb
  • ruby -e 'require "yaml"; Dir["lib/github_flow_templates/.github/**/*.yml"].sort.each { |path| YAML.load_file(path, aliases: true); puts path }'\n- git diff --check

Note

Medium Risk
Introduces new CI workflow templates and changes generator output/CLI startup-check behavior, which can affect how users bootstrap and deploy projects. Core infrastructure interactions remain largely unchanged, but the breadth of scaffolding and new automation warrants careful review.

Overview
Adds a new reusable GitHub Actions pipeline scaffold via cpflow generate-github-actions, including composite actions and workflows for opt-in PR review apps (comment-driven), staging auto-deploys, manual staging→production promotion with health-check + rollback, and nightly stale review-app cleanup.

Introduces local-only helper commands for rollout (cpflow github-flow-readiness + cpflow ai-github-flow-prompt) and updates docs to emphasize a 3-step bootstrap flow and required GitHub secrets/vars, including optional SSH/extra docker build args for private dependencies.

Upgrades cpflow generate scaffolding: infers app prefix and Ruby version, injects detected frontend precompile hooks, generates a Node-capable Dockerfile with package-manager auto-detection, adds release_script.sh, and switches templates to persistent SQLite db/storage volumes when config/database.yml indicates SQLite. Also tweaks CLI startup behavior so commands can opt out of cpln/version checks, and allows copy-image-from-upstream to take its token from CPLN_UPSTREAM_TOKEN.

Adds extensive RSpec coverage for the new generators/readiness service and adjusts test harness/latest-image spec to be more resilient.

Reviewed by Cursor Bugbot for commit 2daf69b. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 15, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1be0793c-4bea-4595-9ed6-e2ebe2e41c61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-github-flow-generator

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

Code Review

This is a solid, well-structured addition. The namespaced cpflow-* file naming avoids collision with existing CI, the opt-in review app design (no deploy until /deploy-review-app is first issued) is the right default for cost control, and the tests cover the happy path and the already-exists guard cleanly. A few issues worth addressing before merge, in rough priority order:


Bugs / correctness

jq crash when GVC has no env vars (production promotion)
In cpflow-promote-staging-to-production.yml, .spec.env[].name throws a jq error if spec.env is null (a fresh GVC with no env vars). Use .spec.env // [] | .[].name to handle that safely.

Rollback only restores the first container
--set spec.containers[0].image patches a single container. An app with a Sidekiq or renderer sidecar in the same workload will have a split-image state after rollback. Consider capturing and restoring all container images, or at least noting this limitation in the docs.

Staging deploy fires on every branch push
branches: ["**"] causes the validate-branch job to spin up for every push to every branch in the repo, then immediately output is_deployable=false for 99% of them. Replace with a branch filter so GitHub skips the trigger entirely:

push:
  branches:
    - main
    - master
    - ${{ vars.STAGING_APP_BRANCH }}

(or simply list the concrete branch names and let users edit after generating, since vars.* is not valid in the on: filter block).


Security / safety

delete-app.sh guard is fragile
The grep -iqE '(production|staging)' check blocks deletion of any app whose name contains those words. An app named my-staging-review-pr-42 would be rejected. The guard should match exact suffixes or use the prefix variable (REVIEW_APP_PREFIX) as the allow-list instead of a deny-list on word fragments.

contains() vs == inconsistency in comment triggers
cpflow-deploy-review-app.yml uses contains(github.event.comment.body, '/deploy-review-app') but cpflow-delete-review-app.yml uses github.event.comment.body == '/delete-review-app'. contains() will match a comment like "please /deploy-review-app thanks", which may or may not be intentional. Either make both use exact match or document the deliberate difference.


Code quality

Duplicated make_shell_scripts_executable
The private method is identical in both Generator and GithubActionsGenerator. Extract it to a shared module (e.g. Concerns::ShellScriptPermissions) to avoid drift.

Hardcoded cpflow_version: "4.2.0" in setup action
Users who generate these files months from now will pin a stale version. Consider substituting the current gem version at generation time (e.g. via Cpflow::VERSION) into the template, or using a placeholder comment like # keep in sync with your Gemfile.lock.

No caching in cpflow-setup-environment
Every job installs Ruby + npm + the gem from scratch. Adding even a simple actions/cache keyed on the Ruby/gem versions would cut setup time meaningfully. At minimum, ruby/setup-ruby@v1 supports bundler-cache: true for bundled projects.

cpln profile create … || true silently swallows errors
The || true means any unexpected failure here (bad token, network issue) is silently ignored and the workflow continues to the cpln profile update step. Since profile update runs immediately after, the || true is only needed to swallow the "already exists" error. A tighter guard (cpln profile create … 2>&1 | grep -q "already exists" || true) or checking the exit code is safer.


Minor

  • cpflow-cleanup-stale-review-apps.yml has no concurrency group. Two simultaneous cleanup runs (e.g. scheduled + manual) could race.
  • The "AI Playbook" section in docs/ci-automation.md feels out of place in user-facing documentation. Consider moving it to a separate contributor/internal doc.
  • The health check accepts HTTP 301/302 as healthy — this is usually fine, but worth a comment explaining the intent (redirects mean the server is up and accepting traffic).

Inline comments are attached to the specific lines above.

Comment thread lib/command/generate_github_actions.rb Outdated
Comment thread lib/github_flow_templates/.github/workflows/cpflow-deploy-staging.yml Outdated
Comment thread lib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.yml Outdated
Comment thread lib/github_flow_templates/.github/actions/cpflow-setup-environment/action.yml Outdated
Comment thread lib/github_flow_templates/.github/actions/cpflow-setup-environment/action.yml Outdated
Comment thread lib/command/generate_github_actions.rb
Comment thread lib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.yml Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

Code Review: Add reusable GitHub Actions flow generator

Overall this is a well-structured addition that brings a Heroku-Flow-style CI pipeline to cpflow projects. The documentation is thorough, the generator guard (blocking re-runs when files exist) is correct, and the spec coverage is solid. A few issues need attention before merge.


🔴 Critical — Security

Anyone can trigger deployments via issue comments (public repos)

cpflow-deploy-review-app.yml and cpflow-delete-review-app.yml fire on issue_comment events and gate only on the comment body string (== '/deploy-review-app'). On a public repository any GitHub user — including bots and strangers — can post that comment on any PR and get your staging credentials used to build and deploy their branch code.

The fix is to check github.event.comment.author_association before proceeding:

if: |
  (github.event_name == 'issue_comment' &&
   github.event.issue.pull_request &&
   contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) &&
   github.event.comment.body == '/deploy-review-app') ||
  ...

This is the recommended pattern from GitHub's security lab for slash-command workflows. Without it, a malicious actor can exhaust your staging quota, exfiltrate CPLN_TOKEN_STAGING, or run arbitrary code inside the runner.


🟠 Important — Bugs

substitute_template_variables rewrites pre-existing .github/ files

GenerateGithubActions#call only checks whether any GENERATED_FILES path already exists. If none do, GithubActionsGenerator#copy_files runs, which calls substitute_template_variables(".github"). That method globs all files under .github/, including files the user already had before the generator ran. Any occurrence of __CPFLOW_VERSION__ in those pre-existing files will be silently overwritten.

The substitution should be scoped to only the files that were just generated.

Code duplication: make_shell_scripts_executable

The method is defined identically in both Command::Generator (generate.rb) and Command::GithubActionsGenerator (generate_github_actions.rb). It should live in a shared module (e.g. Command::GeneratorHelpers) and be included by both Thor groups.


🟡 Minor / Design

cpflow-deploy-staging.yml triggers on every branch push

on:
  push:
    branches: ["**"]

This enqueues the validate-branch job for every push to every branch. The validation is cheap but it creates noise in the Actions UI and uses runner time. Scoping the trigger to the actual staging branch(es) directly in the on: block is cleaner and avoids the fan-out. If the branch must be dynamic (read from a variable), the current two-job approach is the right workaround — but the comment should explain why.

Token interpolated directly into composite action shell

In cpflow-setup-environment/action.yml:

TOKEN="${{ inputs.token }}"

The token value is inlined into the generated script text. GitHub Actions masks it in logs, but the recommended pattern for secrets passed through composite action inputs is to expose them via env: on the run: step so they never appear in the script source:

env:
  TOKEN: ${{ inputs.token }}

and reference $TOKEN in the script body.

Hardcoded dependency versions will go stale

cpflow-setup-environment/action.yml pins ruby_version: "3.4.6" and cpln_cli_version: "3.3.1" as defaults. These will silently lag behind. Consider pointing users toward a .ruby-version file for the Ruby version, and document how to update the CLI version when a new release ships.

Health check rejects healthy apps that return 4xx at /

The promotion workflow accepts only HTTP 200/301/302 as healthy. Rails apps behind authentication commonly return 401 or 302 at /. 302 is included but 401/403 are not — any app that requires auth at root will always fail the health check and trigger rollback even after a successful deploy. Consider making the acceptable status codes configurable or widening the default set.

Rollback fires but does not await or verify success

The rollback step calls cpln workload update to restore container images but does not wait for the rollback rollout to finish or confirm the workload is ready. If the update itself fails, the step exits 0 (from set -euo pipefail perspective it will exit non-zero, which is correct), but there is no subsequent health check on the restored state.

Comment thread lib/command/generate_github_actions.rb
Comment thread lib/command/generate_github_actions.rb Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

Code Review – PR #278: Add reusable GitHub Actions flow generator

Overall this is a solid, well-structured addition. The namespaced cpflow-* file layout, opt-in review apps, and production-gated promotion are all good design choices. Tests cover the critical paths (generation, overwrite guard, chmod). A few issues worth addressing before merging:


Security

1. No commenter-permission check on issue_comment triggers

cpflow-deploy-review-app.yml, cpflow-delete-review-app.yml, and cpflow-help-command.yml fire on any /deploy-review-app, /delete-review-app, or /help comment. The only guard is github.event.issue.pull_request (confirms it's a PR thread), but there is no check that the commenter has write/collaborator access.

On a public repository this means any GitHub user can trigger a review-app deploy — consuming staging resources and potentially running release scripts against the staging org. Add an author_association guard:

github.event.comment.author_association == 'OWNER' ||
github.event.comment.author_association == 'MEMBER' ||
github.event.comment.author_association == 'COLLABORATOR'

2. Staging token passed as a CLI argument

In cpflow-promote-staging-to-production.yml:

cpflow copy-image-from-upstream ... -t "${{ secrets.CPLN_TOKEN_STAGING }}"

Arguments to CLI processes appear in /proc/<pid>/cmdline and are visible in ps aux output for the lifetime of the process. GitHub masks secrets in log output, but not in the process table. Prefer an env-var approach: set CPLN_UPSTREAM_TOKEN (or whatever the flag reads from) in the step's env: block.


Performance

3. Staging deploy fires on every branch push

cpflow-deploy-staging.yml uses push: branches: ["**"], which starts a validate-branch runner for every push to every branch in the repo. The actual deployment is correctly gated, but that first job wastes minutes for every feature-branch push. Filter at the trigger level instead, or at minimum document the cost in the generated file's comments.


Code Quality

4. Duplicate make_shell_scripts_executable method

Generator in generate.rb and GithubActionsGenerator in generate_github_actions.rb define an identical private method. Extract it to a shared module (e.g., Command::ShellScriptHelpers) and include it in both Thor group classes.

5. substitute_template_variables scans the entire .github directory

After generation the method walks Dir.glob(File.join(".github", "**/*")) — including any pre-existing user files that happened to be there before generate-github-actions ran. The next if updated_contents == contents guard prevents writes, but it's still unexpected behavior. Scope the glob to only the files this command generated (iterate GENERATED_FILES instead).

6. GENERATED_FILES list can drift from the template directory

GENERATED_FILES is a manually-maintained constant. When a new template is added to lib/github_flow_templates/, the list must be updated separately or the overwrite guard silently misses the new file. Consider deriving it dynamically:

GENERATED_FILES = Dir.glob(
  Cpflow.root_path.join("lib/github_flow_templates/.github/**/*")
).select { |f| File.file?(f) }
 .map { |f| f.delete_prefix(Cpflow.root_path.join("lib/github_flow_templates/").to_s) }
 .freeze

Minor / Nice-to-have

  • Health check accepts 301/302: Redirects indicate the process is alive but could mask a broken app that bounces all traffic to a login page. Consider following redirects (curl -L) and checking the final status, or only accepting 2xx.
  • Rollback scope: The rollback patches only spec.containers[].image. If the release script made other workload changes (env vars, resource limits), those are not reverted.
  • cpflow-review-app-help.yml always fires: Every new PR gets the help comment even when the repo has no secrets configured. A preflight check (same as the one in cpflow-deploy-review-app.yml) would avoid noise in repos that haven't finished setup.
  • Hardcoded default versions: ruby_version: "3.4.6" and cpln_cli_version: "3.3.1" are embedded in the generated action. Add a comment like # generated with cpflow X.Y.Z — update these as needed so adopters know they own them going forward.

Comment thread lib/github_flow_templates/.github/workflows/cpflow-deploy-staging.yml Outdated
Comment thread lib/command/generate_github_actions.rb Outdated
Comment thread lib/command/generate_github_actions.rb
@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

Code Review: Add reusable GitHub Actions flow generator

Overall this is a well-structured PR that brings meaningful CI/CD scaffolding to cpflow. The guard logic (opt-in review apps, prefix-check before deletion, env var diff before promotion) is solid. The rollback on failed production deploy is a particularly nice touch. A few issues worth addressing before merging.


Bugs / Correctness

cpflow-deploy-staging.yml triggers on every branch push
on: push: branches: ["**"] fires for every push across all branches — every PR commit, every feature branch, every tag-adjacent push. A cheap validate-branch job runs each time, but this creates workflow noise and wastes CI minutes. Better to filter at the event level (see inline comment).

Missing concurrency guard on cpflow-delete-review-app.yml
The deploy workflow has a per-PR concurrency group, but the delete workflow does not. If a PR closes while a /delete-review-app comment workflow is already running (or vice versa), two delete jobs can race against the same app. Adding a concurrency group on the delete workflow (matching the pattern used in the deploy workflow) fixes this.

existing_files computed twice
In GenerateGithubActions#call, existing_files is called once in the guard and again to build the warning string. This results in two filesystem scans of the same list and, more importantly, means the list of files shown in the warning could theoretically diverge from the list that triggered the check (e.g. if a file is created between the two calls). Memoize with @existing_files ||= in the private method.


Code Quality

make_shell_scripts_executable is duplicated
The identical private method appears in both Generator (generate.rb) and GithubActionsGenerator (generate_github_actions.rb). Extract it to a shared module (e.g. Command::GeneratorHelpers or a Concern) to avoid drift.

Release-phase detection is fragile

if cpflow config -a "${APP_NAME}" | grep -q "release_script:"; then

This grep matches on the raw text output of cpflow config, so a future formatting change to that command's output silently breaks the detection. Consider using a dedicated flag or a structured output format (-o json) to check for this attribute instead.

cpflow-setup-environment hardcodes Ruby 3.4.6
The default Ruby version in the composite action will drift out of sync with projects that pin a different version via .ruby-version or their Gemfile. The composite action could accept the version as an override (it already does via ruby_version input), but the documentation doesn't mention that users need to set it. At minimum, add a note in the docs; ideally, ruby/setup-ruby can read .ruby-version automatically when ruby-version is omitted — worth considering whether to drop the default entirely and let ruby/setup-ruby auto-detect.


Security

GitHub Actions not pinned to commit SHAs
Third-party actions used (actions/checkout@v4, actions/github-script@v7, ruby/setup-ruby@v1) reference mutable version tags. A tag can be force-pushed to point at a malicious commit. GitHub's own hardening guide and tools like step-security/harden-runner recommend pinning to exact SHAs (e.g. actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683). This is a common pragmatic trade-off, but worth flagging for security-sensitive users who may be deploying to production orgs.

issue_comment trigger + secrets access
The deploy workflow uses issue_comment to trigger on /deploy-review-app. This event runs in the context of the default branch, so it has access to secrets — which is intentional and correct here. However, the workflow then resolves the PR's headRefOid and checks out that commit to build a Docker image. A malicious PR author could embed a Dockerfile or build script that exfiltrates secrets during the Docker build. This is a known risk with any review-app workflow. It's worth documenting this assumption (trusted contributors only, or require branch protection + required reviews before review apps are triggered).


Performance / Robustness

Health-check window is 2 minutes (12 × 10 s)
For apps with significant cold-start times (Rails boot + asset precompile inside the container, database migrations), 2 minutes may not be enough. The window is configurable as env vars at the top of the promote workflow, but the defaults should probably be higher (e.g. 24 retries × 15 s = 6 minutes). Also consider whether 301/302 should count as "healthy" — a redirect loop or a maintenance-mode redirect would pass the check but doesn't prove the app is up.

build and deploy jobs in cpflow-deploy-staging.yml each check out the repo and set up the environment separately
The two jobs run on different runners, so the Docker image is built in build and then a fresh runner repeats setup in deploy. This is intentional for parallelism but means two full cpln/cpflow installs. No fix needed, just noting it's expected overhead with this job-split pattern.


Nits

  • cpflow-review-app-help.yml posts a comment on every PR open. In active repos this is noise on PRs that won't use review apps. Consider making this conditional (e.g. only if the required vars are set, or optionally gated by a label).
  • The cpflow-setup-environment action validates $TOKEN and $ORG in shell after they've been assigned from GitHub Actions expressions — since required: true inputs already enforce non-empty at the composite-action call site, these shell-level checks are redundant for callers that use required: true. They're harmless, but could be removed.
  • spec/command/generate_github_actions_spec.rb only covers the happy path and the no-overwrite guard. A test covering the version substitution (__CPFLOW_VERSION__ → actual version) would increase confidence in substitute_template_variables.

Comment thread lib/command/generate_github_actions.rb Outdated
@justin808
Copy link
Copy Markdown
Member Author

Review summary — how feedback was addressed

Thanks for the detailed reviews. The claude[bot] reviews fired against every commit, so a lot of the inline feedback in this PR points at code that has already been iterated on. Here's the mapping from reviewer themes to the commits that addressed them:

Fixed in 6f53828 (latest):

  • image_tag output in cpflow-build-docker-image — dropped (format was wrong, no consumer)
  • Promotion Ruby step ordering — Setup production environment now runs before Resolve production app workloads so the pinned Ruby is on PATH for YAML.load_file(..., aliases: true)
  • inferred_app_prefix extracted to RepoIntrospection.inferred_app_prefix(root); Generator and AiGithubFlowPrompt both delegate there
  • HEALTH_CHECK_ACCEPTED_STATUSES now honors vars.HEALTH_CHECK_ACCEPTED_STATUSES with "200 301 302" as default
  • Rollback loop is best-effort — single cpln failure no longer aborts remaining workloads; step aggregates failures and exits non-zero if any failed
  • substitute_template_variables uses gsub { } block form (no regex back-reference interpretation)
  • RELEASE_PHASE_FLAG converted from unquoted expansion to bash array across 3 workflows
  • http_get in github_flow_readiness_service warns to stderr when CPFLOW_DEBUG is set
  • Promotion rollback_state heredoc uses randomized delimiter (EOF_$(openssl rand -hex 8))

Fixed in earlier commits (46b6fdc, 9519b7b, e58af0f, 2890bf3):

  • make_shell_scripts_executable deduped into Command::GeneratorHelpers module
  • cpflow-deploy-staging.yml branches: ["**"]["main", "master"]
  • --ssh default--ssh=default (single valid arg)
  • SSH key handling: passed via env: block with explicit ::add-mask::
  • author_association check added to both cpflow-deploy-review-app.yml and cpflow-delete-review-app.yml
  • substitute_template_variables scoped to generated_files (no longer walks the whole .github/)
  • cpflow-setup-environment defaults: ruby_version now "" (auto-detect), cpflow_version substituted at generation time via __CPFLOW_VERSION__
  • cpln profile create || true replaced with targeted "already exists" swallow that surfaces real errors
  • cpflow copy-image-from-upstream uses CPLN_UPSTREAM_TOKEN env var (no more -t in /proc/<pid>/cmdline)
  • jq .spec.env[].name.spec.env // [] | .[].name
  • delete-app.sh allow-list prefix guard (not deny-list on word fragments)
  • contains() vs == inconsistency — both workflows use exact == match now
  • opened PR type added to trigger list; grep -qE '^[[:space:]]*release_script:' anchored to start-of-line
  • Dockerfile: pnpm@9.12.3, yarn@1.22.22 pinned; targeted COPY --from=node /usr/local/bin/node
  • npm scoped packages: name.gsub("/", "%2F") (keeps @ literal)
  • fetch_*_versions memoization uses @cache.key?(name) guard instead of ||= (caches nil results)
  • Health check window extended from 120s to 360s (covers Rails cold boot)
  • "Resolve production app workloads" gives explicit error message on missing app (replaces bare KeyError)

The unresolved inline threads pointed at themes that have all been addressed. I've replied + resolved the threads corresponding to the 11 items above; remaining stale threads are re-fires of the same feedback on earlier commits.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 5 total unresolved issues (including 3 from previous reviews).

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Rollback guard fires on failed capture with empty state
    • I updated both rollback step guards to also require steps.capture-current.outcome == 'success', preventing rollback execution when capture failed and emitted no output.
  • ✅ Fixed: SQLite path copies Postgres templates that are immediately overwritten
    • I changed base_template_files to return BASE_TEMPLATE_FILES - SQLITE_TEMPLATE_FILES for SQLite projects so overlapping files are not copied twice.

Create PR

Or push these changes by commenting:

@cursor push 346f23824c
Preview (346f23824c)
diff --git a/lib/command/generate.rb b/lib/command/generate.rb
--- a/lib/command/generate.rb
+++ b/lib/command/generate.rb
@@ -61,7 +61,9 @@
     end
 
     def base_template_files
-      sqlite_project? ? BASE_TEMPLATE_FILES : BASE_TEMPLATE_FILES + POSTGRES_TEMPLATE_FILES
+      return BASE_TEMPLATE_FILES - SQLITE_TEMPLATE_FILES if sqlite_project?
+
+      BASE_TEMPLATE_FILES + POSTGRES_TEMPLATE_FILES
     end
 
     def template_variables

diff --git a/lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml b/lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml
--- a/lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml
+++ b/lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml
@@ -261,7 +261,7 @@
           exit 1
 
       - name: Roll back on failure
-        if: failure() && steps.capture-current.outputs.rollback_state != '{}'
+        if: failure() && steps.capture-current.outcome == 'success' && steps.capture-current.outputs.rollback_state != '{}'
         env:
           ROLLBACK_STATE: ${{ steps.capture-current.outputs.rollback_state }}
           PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
@@ -296,7 +296,7 @@
           fi
 
       - name: Wait for rollback readiness
-        if: failure() && steps.capture-current.outputs.rollback_state != '{}'
+        if: failure() && steps.capture-current.outcome == 'success' && steps.capture-current.outputs.rollback_state != '{}'
         env:
           ROLLBACK_STATE: ${{ steps.capture-current.outputs.rollback_state }}
           PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}

You can send follow-ups to the cloud agent here.

Comment thread lib/command/generate.rb
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds a substantial GitHub Actions generator (cpflow generate-github-actions) delivering a Heroku-Flow-style deployment pipeline for Control Plane projects. The scope is broad: new commands (generate-github-actions, github-flow-readiness, ai-github-flow-prompt), an enhanced generate command with Rails introspection, reusable composite actions, seven workflow files, and updated docs. The overall design is solid — fork guards, author_association gating, opt-in review apps, and rollback on failed promotion are all good defaults.

Specific concerns below, roughly ordered by impact.


Security

1. Control Plane token exposed in process table (cpflow-setup-environment/action.yml)

cpln profile create default --token "$TOKEN" and cpln profile update default --token "$TOKEN" pass the token as a CLI argument. On Linux the token is visible in /proc/<pid>/cmdline and potentially in ACTIONS_STEP_DEBUG output. The PR already patched copy-image-from-upstream specifically to avoid this pattern (via CPLN_UPSTREAM_TOKEN). If cpln profile create/update accepts a CPLN_TOKEN env var, prefer that; otherwise write the token to a temp file and pass --token "$(cat /tmp/cpln-token)" with a tight umask.

2. ::add-mask:: is called after DOCKER_BUILD_SSH_KEY is already in the env (cpflow-build-docker-image/action.yml)

The comment in the code acknowledges that composite-action inputs are not auto-masked. The mask is applied on the first line of the script, so the window is tiny — but ACTIONS_STEP_DEBUG=true captures env vars before any script line runs. Document this limitation clearly, or move the SSH key handling to a separate action step that receives it only as a masked input.


Code Quality

3. GENERATED_FILES runs Dir.glob at class load time

Every cpflow invocation pays the cost of this glob at startup — even cpflow version. If the gem structure ever changes (e.g., the templates directory is missing in a partial install), the constant silently becomes [] and generate-github-actions becomes a no-op with no error. Defer evaluation or add an explicit guard that raises when the directory is missing.

4. Ruby version detection logic is duplicated across three files

Command::Generator, GithubFlowReadinessService, and RepoIntrospection all implement ruby_version_from_ruby_version_file, ruby_version_from_tool_versions, and ruby_version_from_gemfile. RepoIntrospection already hosts inferred_app_prefix and sqlite_database_in_production? as module-level methods. Adding RepoIntrospection.inferred_ruby_version(root) would eliminate this duplication.

5. Hardcoded package manager fallback versions in the generated Dockerfile

yarn@1.22.22 and pnpm@9.12.3 are pinned as fallbacks when packageManager is unset. These will drift silently; teams copy-paste the generated file and never update it. Either use corepack's latest stable resolution or add a comment flagging these need periodic updates — and consider surfacing them as a readiness check.


Minor Issues

6. package_json_parse_error? relies on a side-effectful call to parsed_package_json

The method body is just parsed_package_json; @package_json_parse_error. The dependency is invisible at the call site. Consolidate into a single memoized parse method that sets both the result and the error flag, then expose predicates that read those ivars directly.

7. load_gem_dependencies swallows all StandardError silently

The rescue block returns [] without any logging, so a corrupted Gemfile.lock silently skips the gem readiness checks. At minimum emit a warning under CPFLOW_DEBUG.

8. Health-check tuning constants are buried in the workflow

HEALTH_CHECK_RETRIES: 24 and HEALTH_CHECK_INTERVAL: 15 are YAML-level env vars that teams would need to find by reading the workflow file. The accepted-statuses variable already uses ${{ vars.HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }} — consider the same pattern for retries/interval so operators can tune them without editing the generated file.


Test Coverage

The spec suite is comprehensive — particularly the security-boundary test for Gemfile code execution and the YAML validity check over all generated files. A few gaps worth noting:

  • github_flow_readiness_service_spec.rb mocks all HTTP calls. No integration tests hit the real RubyGems/npm registries. That is fine for CI, but a periodic smoke test would catch registry API changes early.
  • No spec for the edge case where TEMPLATE_ROOT points to a missing path (empty GENERATED_FILES), causing a silent no-op.

Comment thread lib/github_flow_templates/.github/actions/cpflow-setup-environment/action.yml Outdated
Comment thread lib/github_flow_templates/.github/actions/cpflow-build-docker-image/action.yml Outdated
Comment thread lib/command/generate_github_actions.rb
Comment thread lib/core/github_flow_readiness_service.rb
Comment thread lib/core/github_flow_readiness_service.rb Outdated
Comment thread lib/generator_templates/Dockerfile
Address Cursor Bugbot review feedback on PR #278.

- Promote workflow: tighten rollback guard to also reject empty rollback_state.
  When the capture-current step fails before writing the heredoc output under
  `set -euo pipefail`, the output resolves to an empty string, which previously
  satisfied `!= '{}'` and triggered the rollback step on zero state. The loop
  iterated zero times so production was unharmed, but the guard's intent was
  violated. Require both non-empty and non-'{}'.
- Generator: subtract SQLITE_TEMPLATE_FILES from the base copy list when the
  project is SQLite-backed, so controlplane.yml, release_script.sh,
  templates/app.yml, and templates/rails.yml aren't copied twice (first from
  generator_templates/, then immediately overwritten with force:true from
  generator_templates_sqlite/). Eliminates confusing force-overwrite messages
  and duplicate entries in generated_paths.
- Update spec assertion to match the new rollback guard expression.
@justin808
Copy link
Copy Markdown
Member Author

Review addressed (commit 46c4871b804500ec9edbff53441f06375dea9a90)

Both outstanding Cursor Bugbot threads are now fixed and resolved.

1. Rollback guard fires on empty rollback_state (cpflow-promote-staging-to-production.yml:264,299)
Tightened the if: condition on both the rollback step and the rollback-readiness step to also reject the empty string, so a failed capture-current step no longer triggers rollback on unset state:

if: failure() && steps.capture-current.outputs.rollback_state != '' && steps.capture-current.outputs.rollback_state != '{}'

2. SQLite path double-copies templates (lib/command/generate.rb:63)
Changed base_template_files to subtract SQLITE_TEMPLATE_FILES from the base list for SQLite projects, so controlplane.yml, release_script.sh, templates/app.yml, and templates/rails.yml are no longer copied twice (once from Postgres-flavoured templates, then immediately overwritten). Only Dockerfile and entrypoint.sh are now copied from generator_templates/ for SQLite projects; the SQLite pass supplies the rest.

Tests: 27 examples, 0 failures in spec/command/generate_spec.rb and spec/command/generate_github_actions_spec.rb. Updated one assertion in the latter to match the new rollback guard expression.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 6 total unresolved issues (including 5 from previous reviews).

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: SQLite template missing maintenance_workload config key
    • Added maintenance_workload: maintenance to the SQLite common alias so generated SQLite apps have maintenance mode workload configured like the PostgreSQL template.

Create PR

Or push these changes by commenting:

@cursor push acef5afc22
Preview (acef5afc22)
diff --git a/lib/generator_templates_sqlite/controlplane.yml b/lib/generator_templates_sqlite/controlplane.yml
--- a/lib/generator_templates_sqlite/controlplane.yml
+++ b/lib/generator_templates_sqlite/controlplane.yml
@@ -21,6 +21,7 @@
       - rails
     additional_workloads: []
 
+    maintenance_workload: maintenance
     release_script: release_script.sh
 
     stale_app_image_deployed_days: 5

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 46c4871. Configure here.

Comment thread lib/generator_templates_sqlite/controlplane.yml
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review: PR #278 — Add reusable GitHub Actions flow generator

Overall assessment: This is a large, well-thought-out feature that adds significant value. The architecture is clean, the documentation is thorough, and the security-conscious design choices (TOCTOU-aware PR SHA resolution, author_association gating, SSH key masking, token-via-env-var) are commendable. Several issues are worth addressing before merging.


Security

pull_request_target + secret access in cpflow-deploy-review-app.yml

The deploy workflow uses pull_request (not pull_request_target) for the auto-deploy path — this is correct. The issue_comment path is gated by author_association. The fork check in "Validate review app deployment source" adds a third layer. This is a solid defense-in-depth approach.

One remaining concern: issue_comment events trigger against the default branch code, which is safe for secret access, but the subsequent actions/checkout@v4 ref: ${{ env.PR_SHA }} step then checks out untrusted code for the Docker build. If a trusted COLLABORATOR comments /deploy-review-app on a malicious fork PR... the fork check guards this (same_repo == false exits), but the "Validate review app deployment source" step only bails for pull_request events from forks — the issue_comment path falls through to exit 1 for fork PRs. This appears correct but is hard to audit at a glance. A comment explaining the fork+comment_event path explicitly would help maintainers.

Pinned ruby/setup-ruby@v1 without a SHA

The composite action uses uses: ruby/setup-ruby@v1. Supply-chain best practice is to pin third-party actions to a full commit SHA. This is a lower-risk concern for a generated template (users will own the generated files), but the generator ships a mutable reference.


Correctness / Bugs

requires_startup_checks? returns false for unrecognized commands

In lib/cpflow.rb, command_class_for_argv returns nil when the command is not found, and requires_startup_checks? returns false when command_class is nil. This means a typo like cpflow deploy-imag skips both the cpln version check and the cpflow version check before failing with Thor's "unknown command" error. It would be safer to default to true (run checks) when the command is unknown, since those checks are cheap and the status quo before this PR always ran them.

LEGACY_RUBY_VERSION = Gem::Version.new("3.0.0") allows EOL Ruby

Ruby 3.0.x reached end-of-life in March 2024. A repo pinned to Ruby 3.0.7 would pass the readiness check with :pass, even though it is an unsupported release line. The threshold should be at least 3.1.0.

cancel-in-progress: true on the deploy review-app workflow

Cancelling an in-flight cpflow deploy-image mid-run can leave the Control Plane app in an inconsistent state (image partially uploaded, workload deployment in progress). The delete workflow correctly uses cancel-in-progress: false. Consider the same for the deploy workflow, or at minimum document the risk.

Staging workflow fetches full git history without explanation

The build job in cpflow-deploy-staging.yml sets fetch-depth: 0. A full clone is unusual for a Docker build — if cpflow build-image doesn't actually need the full history, this wastes time on large repos. The reason should either be documented or the flag removed.


Performance

Sequential HTTP calls for gem/npm version checks

GithubFlowReadinessService checks each exact-pinned gem against rubygems.org sequentially with a 5-second read timeout each. A Gemfile with 50 exact-pinned gems means up to 250 seconds of serial HTTP calls in the worst case. Consider parallelising with threads, or at least documenting the expected runtime so users are not surprised.


Code Quality

Duplicated Ruby version inference logic

ruby_version_from_ruby_version_file, ruby_version_from_tool_versions, and ruby_version_from_gemfile are implemented independently in both Command::Generator and GithubFlowReadinessService, with only the parse_ruby_version leaf delegating to RepoIntrospection. The file-reading logic (3 methods × 2 classes) could be consolidated into RepoIntrospection so both callers share a single implementation.

Fragile error-pattern matching in delete-app.sh

The script infers "app not found" vs "real error" by matching substrings of cpflow exists output. The code itself acknowledges this fragility with a comment. Consider adding a structured exit-code or a --json flag to cpflow exists as a follow-up, so the shell script can rely on exit codes instead of text patterns.


Minor

  • lib/generator_templates/Dockerfile: npm install -g yarn@1.22.22 hardcodes a specific Yarn Classic version. Since Yarn Classic is in maintenance mode and the Dockerfile already detects packageManager, consider removing this fallback or making the version a build arg.
  • lib/core/github_flow_readiness_service.rb: The toolchain_version_result helper is clear, but the :warn status for "could not determine version" may surprise users who expect a definitive pass/fail. A note in the output that network or file errors produce :warn (not :fail) would set clearer expectations.
  • entrypoint.sh: The change from $@ to $* in the echo is correct (avoids issues with special chars in positional args appearing in the message), but exec "$@" on the next line is still correct and intentional — the $* change only affects the log message.

Comment thread lib/cpflow.rb
return false if version_flag?(argv)

command_class = command_class_for_argv(argv)
command_class ? command_class::REQUIRES_STARTUP_CHECKS : false
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When command_class_for_argv returns nil (unrecognized command name), this returns false, silently skipping startup checks before Thor reports "unknown command". Before this PR, startup checks always ran. Consider defaulting to true for unknown commands so a mistyped command still gets the version check:

Suggested change
command_class ? command_class::REQUIRES_STARTUP_CHECKS : false
command_class ? command_class::REQUIRES_STARTUP_CHECKS : true

keyword_init: true
)

LEGACY_RUBY_VERSION = Gem::Version.new("3.0.0")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby 3.0.x reached end-of-life in March 2024. A repo on Ruby 3.0.7 passes this check with :pass even though the release line is no longer supported. 3.1.0 is the oldest currently-supported line:

Suggested change
LEGACY_RUBY_VERSION = Gem::Version.new("3.0.0")
LEGACY_RUBY_VERSION = Gem::Version.new("3.1.0")


concurrency:
group: cpflow-review-app-${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}
cancel-in-progress: true
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancelling an in-flight cpflow deploy-image run mid-execution can leave the review app in a partially-deployed state (workload update in progress, rollout not yet settled). The delete workflow correctly uses cancel-in-progress: false for this reason. Consider the same here, or at minimum document the risk — a cancelled deploy is harder to diagnose than a failed one.

- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-depth: 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch-depth: 0 fetches the full git history. On large repos this meaningfully slows CI. If cpflow build-image only needs the current tree (not commit history), fetch-depth: 1 is sufficient. If there's a reason the full history is required (e.g., version tagging from git describe), a brief comment here would clarify the intent.

corepack prepare "$package_manager" --activate && \
(corepack yarn install --immutable || corepack yarn install --frozen-lockfile); \
else \
npm install -g yarn@1.22.22 && \
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hardcodes a specific Yarn Classic version. Yarn Classic is in maintenance mode (Yarn ≥ 4 is the actively-developed line), and the version will drift from what projects actually use. Since the Dockerfile already handles projects with a packageManager field via Corepack, the Yarn Classic fallback only applies to legacy projects. Consider making the version a build arg so users can override it without editing the generated file:

Suggested change
npm install -g yarn@1.22.22 && \
npm install -g "yarn@${YARN_CLASSIC_VERSION:-1.22.22}" && \

Or document it as a known limitation in the template comment.


def partition_dependencies(dependencies, availability_proc)
dependencies.each_with_object(unavailable: [], unknown: []) do |dependency, grouped_dependencies|
case availability_proc.call(dependency)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These registry checks are sequential — one HTTP round-trip per gem/package. For a Gemfile with many exact-pinned public gems, this can take minutes. Consider parallelising the lookups with threads, or capping the number checked (e.g., first 20 exact pins) and noting in the output that the rest were skipped.

…stency

Covers two must-fix correctness/security items and five optional
improvements surfaced by the Claude and Cursor review bots.

Must-fix
- cpflow-setup-environment action: stop passing the Control Plane token via
  `--token`, which would expose it in /proc/<pid>/cmdline and ps output.
  Renamed the step env from TOKEN to CPLN_TOKEN so cpln picks the token up
  from the environment automatically, matching the pattern already used
  across the rest of the generated workflows.
- generate_github_actions.rb: add an explicit `TEMPLATE_ROOT.directory?`
  guard before the Dir.glob that builds GENERATED_FILES. A broken gem
  install previously yielded an empty list and made `cpflow
  generate-github-actions` silently succeed with zero files; it now fails
  loudly at load time.

Optional
- cpflow-build-docker-image action: move SSH key handling into a dedicated
  "Prepare SSH agent" step that writes the key to ~/.ssh/cpflow_build_key
  via inline template interpolation, so DOCKER_BUILD_SSH_KEY is never
  present in the main build step's environment (where ACTIONS_STEP_DEBUG
  would otherwise dump it). The main build step now consumes the key file
  and cleans up on exit.
- github_flow_readiness_service.rb: document the side-effect memoization
  in `package_json_parse_error?` so future readers don't assume the ivar
  is set independently of `parsed_package_json`.
- github_flow_readiness_service.rb: surface malformed Gemfile.lock parse
  errors under CPFLOW_DEBUG so the silent rescue no longer hides diagnostic
  signal; default behavior (skip gem readiness check on parse error) is
  unchanged.
- generator_templates/Dockerfile: add a comment near the package-manager
  fallbacks noting that yarn@1.22.22 and pnpm@9.12.3 will go stale and that
  pinning `packageManager` in package.json avoids them entirely.
- generator_templates_sqlite/controlplane.yml: add `maintenance_workload:
  maintenance` to the common alias for parity with the Postgres template.
@justin808
Copy link
Copy Markdown
Member Author

Review addressed (commit 2daf69b6869cba64e5b513fab2568b47220ef186)

All 7 outstanding bot review threads are now fixed and resolved. Two were correctness/security must-fixes; five were optional improvements addressed inline rather than deferred.

Must-fix

1. Token exposed via --token in cpflow-setup-environment action — renamed step env from TOKEN to CPLN_TOKEN and dropped --token "$TOKEN" from both cpln profile create and cpln profile update. The token no longer appears in /proc/<pid>/cmdline or ps output; matches the existing CPLN_TOKEN env pattern used elsewhere in the generated workflows.

2. Silent failure if TEMPLATE_ROOT is missing — added raise "cpflow template directory not found: #{TEMPLATE_ROOT}" unless TEMPLATE_ROOT.directory? before the Dir.glob in generate_github_actions.rb. A broken install now fails loudly instead of generating zero files.

Optional improvements addressed

3. SSH key exposed in env with ACTIONS_STEP_DEBUG=true — split SSH setup into a dedicated Prepare SSH agent for Docker build step that writes the key to ~/.ssh/cpflow_build_key (chmod 600) via inline ${{ inputs.docker_build_ssh_key }} in a quoted heredoc. Main build step no longer has DOCKER_BUILD_SSH_KEY in env; it ssh-adds the file and wipes it on exit.

4. package_json_parse_error? side-effect memoization — documented the implicit dependency on parsed_package_json setting @package_json_parse_error so future readers don't miss it.

5. load_gem_dependencies silent rescue — added a CPFLOW_DEBUG-gated warn so malformed Gemfile.lock parse errors are discoverable. Default behavior (skip gem readiness check on parse error) unchanged.

6. Dockerfile yarn@1.22.22 / pnpm@9.12.3 fallbacks — added a comment near the install block noting that pinning packageManager in package.json takes the corepack path and avoids the fallbacks, and that the fallback versions should be reviewed periodically. (Deferred on the readiness-check version — happy to do that as a follow-up.)

7. SQLite controlplane.yml missing maintenance_workload — added maintenance_workload: maintenance to the common alias for parity with the Postgres template. (Behavior was actually identical today — maintenance_mode.rb:109 falls back to the literal "maintenance" — but making it explicit is clearer.)

Tests: 39 examples, 0 failures across generate_spec, generate_github_actions_spec, github_flow_readiness_service_spec, and github_flow_readiness_spec. RuboCop clean on the two Ruby files changed; all edited YAML still parses.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

This is a substantial, well-structured addition that brings a first-class Heroku Flow experience to Control Plane. The overall design is solid — the three-step bootstrap model is intuitive, the generated workflows handle the important edge cases (fork PR isolation, permission gating, rollback on failure), and the readiness service's decision to parse Gemfile.lock via Bundler::LockfileParser rather than eval_gemfile is a good security call. Here are the issues worth addressing before merge.


Security

SSH key heredoc uses a fixed delimiter (CPFLOW_SSH_KEY_EOF)

In cpflow-build-docker-image/action.yml, the SSH private key is written via a heredoc with a hardcoded terminator. If the key value ever contains a line consisting solely of CPFLOW_SSH_KEY_EOF, the heredoc terminates early and the written key file is silently truncated. The rollback_state block in the promotion workflow already uses openssl rand -hex 8 to generate a unique delimiter for exactly this reason — the same approach should be applied here.

CPLN_TOKEN is set at the workflow-level env block

In cpflow-deploy-review-app.yml and cpflow-delete-review-app.yml, CPLN_TOKEN: ${{ secrets.CPLN_TOKEN_STAGING }} is set at the top-level env. This makes the token available to every step in every job, including any third-party actions added later. Since cpflow and cpln pick up the token from the environment, the token should be scoped to only the steps that actually need it (primarily the cpflow-setup-environment composite action). Removing the workflow-level env entry and passing it only to the setup action input would tighten the blast radius.


Code Quality

Ruby version detection is duplicated

ruby_version_from_ruby_version_file, ruby_version_from_tool_versions, and ruby_version_from_gemfile appear in both GithubFlowReadinessService and the Generator Thor group, with the service additionally wrapping results in Gem::Version. Both classes already delegate parse_ruby_version_string and sqlite_database_in_production? to RepoIntrospection. The three detection methods should join them there so a future .tool-versions format change only needs a single fix.

shakapacker.yml precompile hook is parsed with a line-level regex

shakapacker_precompile_hook reads the entire file as a string and matches with /^\s*precompile_hook:\s*["']?(.+?)["']?\s*$/. This will misfire on commented-out keys (# precompile_hook: ...) and may silently misread multiline YAML values. Since the method already guards on File.file?("config/shakapacker.yml"), using YAML.safe_load with the result limited to String values would be more robust and only marginally more code.

REQUIRED_RAILS_PATHS doesn't include bin/rails

The Rails scaffold check verifies Gemfile, config/application.rb, and config.ru. Any Ruby app with a config.ru passes this gate. Adding bin/rails would make the check more precise and catch common cases like generator snapshots that have config.ru but are not runnable Rails apps.


Performance

Gem and npm availability checks run sequentially

fetch_versions_from_rubygems and fetch_versions_from_npm are called one package at a time with 5-second open_timeout and read_timeout per request. For a project with 20 exact-pinned public gems and 10 npm packages, the worst-case wall time is ~150 seconds before any result surfaces. Using Concurrent::Future (concurrent-ruby is already a transitive dependency via thor) or Ruby's built-in Thread to fan out the requests in parallel would keep the readiness check snappy. Since results are cached per name via @rubygems_versions / @npm_versions, the fan-out is straightforward.


Minor

  • Health check timing comment is slightly off. The env block comment says "~6 minute window" (24 × 15 s = 360 s) but each curl has its own --max-time 10, so worst-case wall time is 24 × (15 + 10) = 600 s (10 minutes). Worth updating the comment or decoupling the retry count from the curl timeout.

  • Exact-match comment trigger. /deploy-review-app and /delete-review-app use == '...' exact-string comparison. Any trailing whitespace or newline (common from mobile GitHub clients or copy-paste) silently drops the command. A startsWith or trim() approach in the if: expression, or a note in the generated /help output, would reduce user confusion.

  • cpflow-review-app-help.yml uses pull_request_target safely — no PR code is checked out and the job only calls actions/github-script, so this pattern is correct. Worth a brief inline comment in the generated file explaining why pull_request_target is used (to reach fork PRs) so future maintainers don't change it to pull_request without understanding the trade-off.

Comment on lines +59 to +61
cat > ~/.ssh/cpflow_build_key <<'CPFLOW_SSH_KEY_EOF'
${{ inputs.docker_build_ssh_key }}
CPFLOW_SSH_KEY_EOF
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heredoc uses a fixed terminator CPFLOW_SSH_KEY_EOF. If the SSH key value ever contains a line that is exactly that string, the heredoc closes early and the key file is silently truncated — the build would proceed with a malformed key.

The rollback-state block in the promotion workflow already guards against this with a randomized delimiter:

delim="EOF_$(openssl rand -hex 8)"

Apply the same pattern here:

Suggested change
cat > ~/.ssh/cpflow_build_key <<'CPFLOW_SSH_KEY_EOF'
${{ inputs.docker_build_ssh_key }}
CPFLOW_SSH_KEY_EOF
key_delim="CPFLOW_SSH_KEY_EOF_$(openssl rand -hex 8)"
eval "cat > ~/.ssh/cpflow_build_key <<\"${key_delim}\"
${{ inputs.docker_build_ssh_key }}
${key_delim}"
CPFLOW_SSH_KEY_EOF

Note: Because ${{ inputs.docker_build_ssh_key }} is expanded by GitHub Actions before bash runs, the key value is already a literal string at execution time. Using an eval-based heredoc with a randomized delimiter gives the same masking guarantee while eliminating the fixed-terminator risk.

env:
APP_NAME: ${{ vars.REVIEW_APP_PREFIX }}-${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}
CPLN_ORG: ${{ vars.CPLN_ORG_STAGING }}
CPLN_TOKEN: ${{ secrets.CPLN_TOKEN_STAGING }}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting CPLN_TOKEN at the workflow-level env block makes the staging token available to every step in every job — including any third-party actions that may be added later. cpflow and cpln both read CPLN_TOKEN from the environment, so the same behavior is achieved by scoping it to the cpflow-setup-environment composite action input, which already accepts token:.

Suggested change
CPLN_TOKEN: ${{ secrets.CPLN_TOKEN_STAGING }}
CPLN_ORG: ${{ vars.CPLN_ORG_STAGING }}

Then pass the secret only where it's consumed:

      - name: Setup environment
        uses: ./.github/actions/cpflow-setup-environment
        with:
          token: ${{ secrets.CPLN_TOKEN_STAGING }}
          org: ${{ vars.CPLN_ORG_STAGING }}

The same applies to cpflow-delete-review-app.yml.

Comment on lines +179 to +211
def ruby_version_from_ruby_version_file
file_path = root_path.join(".ruby-version")
return unless file_path.file?

parse_ruby_version(file_path.read)
end

def ruby_version_from_tool_versions
file_path = root_path.join(".tool-versions")
return unless file_path.file?

ruby_line = file_path.readlines(chomp: true).find { |line| line.match?(/^\s*ruby\s+/) }
return unless ruby_line

parse_ruby_version(ruby_line.sub(/^\s*ruby\s+/, ""))
end

def ruby_version_from_gemfile
file_path = root_path.join("Gemfile")
return unless file_path.file?

ruby_line = file_path.readlines(chomp: true).find { |line| line.match?(/^\s*ruby\s+/) }
return unless ruby_line

parse_ruby_version(ruby_line.sub(/^\s*ruby\s+/, ""))
end

def parse_ruby_version(source)
version = RepoIntrospection.parse_ruby_version_string(source)
return unless version

Gem::Version.new(version)
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three methods (ruby_version_from_ruby_version_file, ruby_version_from_tool_versions, ruby_version_from_gemfile) are nearly identical to the private methods of the same name in Command::Generator. Both classes already delegate parse_ruby_version_string and sqlite_database_in_production? to RepoIntrospection. Moving the detection trio there too — returning a raw version string — would make a single place to update when .tool-versions format or Gemfile syntax changes.

Suggested shape in RepoIntrospection:

def self.inferred_ruby_version_string(root)
  ruby_version_from_ruby_version_file(root) ||
    ruby_version_from_tool_versions(root) ||
    ruby_version_from_gemfile(root)
end

The service can then wrap the result in Gem::Version.new(...) and Generator can use it directly.

Comment thread lib/command/generate.rb
Comment on lines +140 to +145
def shakapacker_precompile_hook
return unless File.file?("config/shakapacker.yml")

config = File.read("config/shakapacker.yml")
match = config.match(/^\s*precompile_hook:\s*["']?(.+?)["']?\s*$/)
match && match[1]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex ^\s*precompile_hook: will match commented-out YAML lines (e.g. # precompile_hook: old_value), and the lazy quantifier (.+?) can misread folded/quoted multiline values. Since shakapacker.yml is a plain YAML file, parsing it properly costs very little:

Suggested change
def shakapacker_precompile_hook
return unless File.file?("config/shakapacker.yml")
config = File.read("config/shakapacker.yml")
match = config.match(/^\s*precompile_hook:\s*["']?(.+?)["']?\s*$/)
match && match[1]
def shakapacker_precompile_hook
return unless File.file?("config/shakapacker.yml")
config = YAML.safe_load(File.read("config/shakapacker.yml"))
hook = config.is_a?(Hash) && config["precompile_hook"]
hook.is_a?(String) && !hook.empty? ? hook : nil
rescue Psych::SyntaxError
nil
end

YAML.safe_load is already required in the file via repo_introspection and is safe here because the file is the developer's own project config.

Comment on lines +16 to +19
# Defaults give a ~6 minute window, which covers most Rails cold boots (asset precompile
# + db:migrate + workload readiness).
HEALTH_CHECK_RETRIES: 24
HEALTH_CHECK_INTERVAL: 15
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "~6 minute window" comment only accounts for sleep time (24 × 15 s = 360 s). Each iteration also runs curl --max-time 10, so the worst-case wall time is 24 × (15 + 10) = 600 s (~10 minutes). The comment should reflect the actual ceiling, or the curl timeout should be factored into the retry count calculation.

Suggested change
# Defaults give a ~6 minute window, which covers most Rails cold boots (asset precompile
# + db:migrate + workload readiness).
HEALTH_CHECK_RETRIES: 24
HEALTH_CHECK_INTERVAL: 15
# Defaults give a ~10 minute window (retries × (interval + curl max-time)), which
# covers most Rails cold boots (asset precompile + db:migrate + workload readiness).
HEALTH_CHECK_RETRIES: 24
HEALTH_CHECK_INTERVAL: 15

Comment on lines +285 to +291
def http_get(uri)
http = Net::HTTP.new(uri.host, uri.port)
http.use_ssl = true
http.open_timeout = 5
http.read_timeout = 5
http.get(uri.request_uri)
rescue StandardError => e
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gem and npm version checks run sequentially with up to 5 s per request. For a project with many exact-pinned direct dependencies the readiness command can take well over a minute before returning. The checks are fully independent and the results are cached per name, so they're a natural fit for parallel execution. concurrent-ruby ships as a transitive dependency (via Thor), but plain Thread also works:

def fetch_all_versions_in_parallel(packages, fetch_proc)
  packages.map { |pkg| Thread.new { fetch_proc.call(pkg) } }.each(&:join)
end

Alternatively, a note in the LONG_DESCRIPTION that the check makes outbound HTTP calls would set expectations without requiring a refactor now.

LEGACY_RUBY_VERSION = Gem::Version.new("3.0.0")
LEGACY_BUNDLER_VERSION = Gem::Version.new("2.0.0")
PUBLIC_RUBYGEMS_REMOTE = "https://rubygems.org"
REQUIRED_RAILS_PATHS = ["Gemfile", "config/application.rb", "config.ru"].freeze
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemfile, config/application.rb, and config.ru are present in many non-Rails Ruby apps (Rack apps, generator snapshots, tutorial repos). Adding bin/rails to the set would catch the common case of a repo that has the config directory but hasn't run rails new to completion:

Suggested change
REQUIRED_RAILS_PATHS = ["Gemfile", "config/application.rb", "config.ru"].freeze
REQUIRED_RAILS_PATHS = ["Gemfile", "bin/rails", "config/application.rb", "config.ru"].freeze

This is a tighter gate without being overly restrictive — every rails new output includes bin/rails.

name: Show Review App Commands on PR Open

on:
pull_request_target:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull_request_target is the right trigger here — it gives write permission to post a comment even when the PR is from a fork. This is safe because no PR code is checked out: the job only calls actions/github-script with a hardcoded message. Future maintainers may be tempted to switch this to pull_request (or add a checkout step); a short comment prevents that regression:

Suggested change
pull_request_target:
pull_request_target:
# pull_request_target is intentional: it has write permission to comment on fork PRs.
# No PR code is checked out here — keep it that way to preserve this safety guarantee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant