Validate cpflow gem and workflow pins#318
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesCPFLOW version pinning validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds an early-failure guard to the
Confidence Score: 4/5Safe to merge; the guard logic is correct and the five workflow files are all updated consistently. The validation bash logic handles all the expected cases — empty CPFLOW_VERSION, SHA/branch refs, and mismatched release tags — correctly. The new spec test reads the live YAML files and verifies every setup-environment call carries the new input, providing good regression coverage. The only gap is that the pre-release label allow-list in the regex (test|beta|alpha|rc|pre) is narrower than what teams commonly use, which could produce a confusing error for tags like v5.0.0-dev-1. That is a forward-looking quality concern, not a current breakage. .github/actions/cpflow-setup-environment/action.yml — the normalize_release_ref pre-release label allow-list is worth a second look if the project ever cuts dev or snapshot pre-release tags. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Downstream Workflow
participant RW as Reusable Workflow (cpflow-*.yml)
participant SA as cpflow-setup-environment action
participant Bash as validate_cpflow_version_pin
Caller->>RW: "uses: cpflow-deploy-review-app@vX.Y.Z"
RW->>SA: "control_plane_flow_ref=vX.Y.Z, cpflow_version=X.Y.Z"
SA->>Bash: "CONTROL_PLANE_FLOW_REF=vX.Y.Z, CPFLOW_VERSION=X.Y.Z"
Bash->>Bash: normalize_release_ref(vX.Y.Z) → X.Y.Z
alt CPFLOW_VERSION matches ref tag
Bash-->>SA: return 0 (pass)
SA-->>RW: gem install cpflow -v X.Y.Z
else CPFLOW_VERSION unset
Bash-->>SA: return 0 (skip validation)
SA-->>RW: build gem from source
else ref is SHA/branch or version mismatch
Bash-->>SA: exit 1 with error message
SA-->>RW: workflow fails before deploy
end
Reviews (1): Last reviewed commit: "Validate cpflow gem and workflow pins" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| normalize_release_ref() { | ||
| local ref="${1#refs/tags/}" | ||
|
|
||
| if [[ "${ref}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([.-](test|beta|alpha|rc|pre)[.-][0-9]+)?$ ]]; then |
There was a problem hiding this comment.
normalize_release_ref regex only recognises the labels test|beta|alpha|rc|pre. Any tag using a different label (e.g. dev, snapshot, nightly) would produce an empty expected_version, causing the validation to fail with the generic "not a release tag" message even though the tag looks release-like. This affects anyone who cuts a tagged pre-release under a different convention. Consider either expanding the allowed set or documenting that only these five labels are supported.
| if [[ "${ref}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([.-](test|beta|alpha|rc|pre)[.-][0-9]+)?$ ]]; then | |
| if [[ "${ref}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([.-](test|beta|alpha|rc|pre|dev|snapshot|nightly)[.-][0-9]+)?$ ]]; then |
| it "passes control_plane_flow_ref to every setup action call for gem/ref validation" do | ||
| Dir.glob(Cpflow.root_path.join(".github/workflows/cpflow-*.yml").to_s).each do |path| | ||
| workflow = YAML.load_file(path, aliases: true) | ||
|
|
||
| workflow.fetch("jobs").each_value do |job| | ||
| Array(job["steps"]).each do |step| | ||
| next unless step["uses"] == "./.cpflow/.github/actions/cpflow-setup-environment" | ||
|
|
||
| expect(step.fetch("with")).to include( | ||
| "control_plane_flow_ref" => "${{ inputs.control_plane_flow_ref }}" | ||
| ), "#{path} setup action call must pass control_plane_flow_ref" | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
steps, not reusable-workflow job definitions
The test skips jobs that use uses: at the job level (reusable-workflow call syntax) because job["steps"] is nil and Array(nil) returns []. Currently all cpflow-*.yml jobs use inline steps:, so nothing is missed today. If a future workflow wraps a reusable job without inline steps, a missing control_plane_flow_ref pass-through would silently go undetected by this test. A comment noting the assumption (# jobs using top-level 'uses:' are skipped – they cannot call composite actions directly) would prevent confusion.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| expected_version="$(normalize_release_ref "${CONTROL_PLANE_FLOW_REF}")" | ||
|
|
||
| if [[ -z "${expected_version}" ]]; then | ||
| echo "::error::CPFLOW_VERSION can only be used when control_plane_flow_ref is a release tag like v${CPFLOW_VERSION}. Current control_plane_flow_ref: ${CONTROL_PLANE_FLOW_REF:-<empty>}. Leave CPFLOW_VERSION unset when testing a commit SHA or branch so cpflow is built from the same source as the reusable workflow." >&2 |
There was a problem hiding this comment.
::error:: annotations must go to stdout, not stderr.
GitHub Actions runner only scans stdout for :: workflow commands. Redirecting to stderr with >&2 means the annotation won't appear as a formatted error in the GitHub UI (the job still fails via exit 1, but users lose the annotation that surfaces at the top of the workflow run).
| echo "::error::CPFLOW_VERSION can only be used when control_plane_flow_ref is a release tag like v${CPFLOW_VERSION}. Current control_plane_flow_ref: ${CONTROL_PLANE_FLOW_REF:-<empty>}. Leave CPFLOW_VERSION unset when testing a commit SHA or branch so cpflow is built from the same source as the reusable workflow." >&2 | |
| echo "::error::CPFLOW_VERSION can only be used when control_plane_flow_ref is a release tag like v${CPFLOW_VERSION}. Current control_plane_flow_ref: ${CONTROL_PLANE_FLOW_REF:-<empty>}. Leave CPFLOW_VERSION unset when testing a commit SHA or branch so cpflow is built from the same source as the reusable workflow." |
| fi | ||
|
|
||
| if [[ "${CPFLOW_VERSION}" != "${expected_version}" ]]; then | ||
| echo "::error::CPFLOW_VERSION must match control_plane_flow_ref. CPFLOW_VERSION=${CPFLOW_VERSION}, control_plane_flow_ref=${CONTROL_PLANE_FLOW_REF}, expected CPFLOW_VERSION=${expected_version}." >&2 |
There was a problem hiding this comment.
Same issue — >&2 suppresses the GitHub Actions annotation. Drop the redirect so the runner processes the ::error:: command.
| echo "::error::CPFLOW_VERSION must match control_plane_flow_ref. CPFLOW_VERSION=${CPFLOW_VERSION}, control_plane_flow_ref=${CONTROL_PLANE_FLOW_REF}, expected CPFLOW_VERSION=${expected_version}." >&2 | |
| echo "::error::CPFLOW_VERSION must match control_plane_flow_ref. CPFLOW_VERSION=${CPFLOW_VERSION}, control_plane_flow_ref=${CONTROL_PLANE_FLOW_REF}, expected CPFLOW_VERSION=${expected_version}." |
| normalize_release_ref() { | ||
| local ref="${1#refs/tags/}" | ||
|
|
||
| if [[ "${ref}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([.-](test|beta|alpha|rc|pre)[.-][0-9]+)?$ ]]; then |
There was a problem hiding this comment.
The regex requires a separator character before the trailing digit ([.-][0-9]+), so tags like v5.0.0-rc1 or v5.0.0-beta2 (digit immediately after the label, which is common in practice) are silently treated as non-release refs. A user setting CPFLOW_VERSION=5.0.0.rc.1 with control_plane_flow_ref: v5.0.0-rc1 would hit "not a release tag" rather than the more helpful "mismatch" error.
If the project's tagging convention always includes a separator (e.g. v5.0.0-rc.1), this is fine as-is — but it's worth a comment documenting that restriction so future release tooling knows to include the separator.
| @@ -252,6 +257,22 @@ def shared_workflow_path(name) | |||
| expect(contents).not_to include("gem install cpflow -v ${{ inputs.cpflow_version }}") | |||
| end | |||
There was a problem hiding this comment.
This test reads from Cpflow.root_path (the source tree) rather than the generated playground like the surrounding tests. That means it validates the checked-in workflow files but not the output of the generate_github_actions command.
This is fine as a static regression guard, but it's placed inside a spec that otherwise tests generated output, which could confuse a future contributor. A brief comment noting the intent (e.g. # validates source workflows, not generated output) would help, or move it into a separate describe block with a descriptive name like "source reusable workflows" to make the distinction explicit.
Review: Validate cpflow gem and workflow pinsThis PR addresses a real operational hazard — mismatched gem/workflow versions are a silent failure mode that's hard to debug after the fact. The implementation is clean and the error messages are actionable. What the PR doesAdds a
All five reusable workflows are updated to propagate IssuesMedium — GitHub Actions annotation suppressed by
Low — Regex misses common prerelease formats The pattern Low — Spec test reads source files instead of generated output The new Strengths
|
justin808
force-pushed
the
jg-codex/validate-cpflow-version-pin
branch
from
6bfae53 to
6a72379
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a72379ef8
ℹ️ 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".
| if { [ "${1:-}" = "./bin/rails" ] && [ "${2:-}" = "server" ]; } || | ||
| { [ "${1:-}" = "./bin/thrust" ] && [ "${2:-}" = "./bin/rails" ] && [ "${3:-}" = "server" ]; }; then |
There was a problem hiding this comment.
The new guard only runs db:prepare for ./bin/rails server (or the Thruster variant), but the generated Dockerfile still starts the app with CMD ["rails", "s"] (lib/generator_templates/Dockerfile), so this condition is false in the default generated runtime and database preparation is skipped entirely. That changes behavior from “always prepare DB before app boot” to “often skip prepare,” which can break startup on pending migrations or first-time boot.
Useful? React with 👍 / 👎.
| normalize_release_ref() { | ||
| local ref="${1#refs/tags/}" | ||
|
|
||
| if [[ "${ref}" =~ ^v([0-9]+\.[0-9]+\.[0-9]+)([.-](test|beta|alpha|rc|pre|dev|snapshot|nightly)[.-]?([0-9]+))?$ ]]; then |
There was a problem hiding this comment.
The regex requires at least one digit after the pre-release label ([0-9]+), so tags like v5.0.0-beta (no trailing number) fall through to normalize_release_ref returning empty string. The caller then fails with the "must be a release tag" error rather than a mismatch error — which is technically correct but potentially confusing.
More importantly, the regex doesn't recognise a leading dot separator for the label itself — v5.0.0.pre.1 won't match because the first separator group is [.-] and the match expects exactly one separator token before the label. Let me expand:
v5.0.0-pre.1✅ matches (-beforepre,.before1)v5.0.0.pre.1❌ does not match — the.presegment is caught by[.-], but the trailing.1needs[.-]?([0-9]+)which requires the?to be.here; this should actually work. Let me recheck:.pre.1→ separator., labelpre, separator., number1— this does match.
The actual gap is numbered pre-release tags like v5.0.0-beta (no number) returning empty/no match, silently falling back to the SHA-style error. Consider documenting that bare pre-release labels (without a number) are not supported, or extend the regex to accept them:
| if [[ "${ref}" =~ ^v([0-9]+\.[0-9]+\.[0-9]+)([.-](test|beta|alpha|rc|pre|dev|snapshot|nightly)[.-]?([0-9]+))?$ ]]; then | |
| if [[ "${ref}" =~ ^v([0-9]+\.[0-9]+\.[0-9]+)([.-](test|beta|alpha|rc|pre|dev|snapshot|nightly)([.-]([0-9]+))?)?$ ]]; then |
This makes the trailing number optional (group 5 becomes the number) and avoids the confusing empty-string path for v5.0.0-beta.
|
|
||
| if [[ "${ref}" =~ ^v([0-9]+\.[0-9]+\.[0-9]+)([.-](test|beta|alpha|rc|pre|dev|snapshot|nightly)[.-]?([0-9]+))?$ ]]; then | ||
| if [[ -n "${BASH_REMATCH[3]}" ]]; then | ||
| echo "${BASH_REMATCH[1]}.${BASH_REMATCH[3]}.${BASH_REMATCH[4]}" |
There was a problem hiding this comment.
When BASH_REMATCH[3] is non-empty but BASH_REMATCH[4] is empty (which the current regex prevents via [0-9]+, but would occur if the regex is loosened to allow bare labels), this produces a trailing dot — e.g. 1.2.3.beta.. Worth adding a guard now if the regex is extended:
| echo "${BASH_REMATCH[1]}.${BASH_REMATCH[3]}.${BASH_REMATCH[4]}" | |
| echo "${BASH_REMATCH[1]}.${BASH_REMATCH[3]}${BASH_REMATCH[4]:+.${BASH_REMATCH[4]}}" |
This appends .N only when the number capture is non-empty, so v5.0.0-beta → 5.0.0.beta and v5.0.0-beta.1 → 5.0.0.beta.1.
| @@ -1,8 +1,12 @@ | |||
| #!/bin/sh | |||
| set -e | |||
There was a problem hiding this comment.
set -e is a generated-file breaking change for anyone with an existing entrypoint.sh who has already customised it. Before this change, a failing intermediate command (e.g. a curl health-check probe that temporarily returns non-zero) would be silently ignored; now it terminates the container.
The changelog entry covers this, but it's worth flagging in the PR that existing apps need to audit their entrypoint after upgrading. A regeneration (cpflow generate) would produce the new template, but handwritten additions could still break.
Also note that this file uses #!/bin/sh, not #!/bin/bash. set -e is valid POSIX sh, but its edge-case behaviour differs between shells (especially inside if/&& chains). Since the template only performs simple commands this is fine in practice, but worth being aware of.
| if { [ "${1:-}" = "./bin/rails" ] && [ "${2:-}" = "server" ]; } || | ||
| { [ "${1:-}" = "./bin/thrust" ] && [ "${2:-}" = "./bin/rails" ] && [ "${3:-}" = "server" ]; }; then |
There was a problem hiding this comment.
The check is exact-string matching on argv[0..2]. A few gaps that could silently skip DB prep:
./bin/rails s(the common short alias forserver) — does not match theserverliteral.bundle exec rails server— not matched (different argv[0])../bin/pumaor a custom Procfile entry that ultimately starts the server without going through./bin/rails serverexplicitly.
If the intent is to only cover the exact commands that cpflow generate emits, that's fine — just worth a comment so maintainers know this is a deliberate narrowing. If you want to also catch the short alias:
| if { [ "${1:-}" = "./bin/rails" ] && [ "${2:-}" = "server" ]; } || | |
| { [ "${1:-}" = "./bin/thrust" ] && [ "${2:-}" = "./bin/rails" ] && [ "${3:-}" = "server" ]; }; then | |
| if { [ "${1:-}" = "./bin/rails" ] && { [ "${2:-}" = "server" ] || [ "${2:-}" = "s" ]; }; } || | |
| { [ "${1:-}" = "./bin/thrust" ] && [ "${2:-}" = "./bin/rails" ] && { [ "${3:-}" = "server" ] || [ "${3:-}" = "s" ]; }; }; then |
| expect(contents).not_to include("gem install cpflow -v ${{ inputs.cpflow_version }}") | ||
| end | ||
|
|
||
| it "passes control_plane_flow_ref to every setup action call for gem/ref validation" do | ||
| # This validates checked-in reusable workflows, not generated app wrappers. | ||
| Dir.glob(Cpflow.root_path.join(".github/workflows/cpflow-*.yml").to_s).each do |path| | ||
| workflow = YAML.load_file(path, aliases: true) | ||
|
|
||
| workflow.fetch("jobs").each_value do |job| | ||
| # Job-level `uses:` entries call reusable workflows; they cannot call composite actions directly. | ||
| Array(job["steps"]).each do |step| | ||
| next unless step["uses"] == "./.cpflow/.github/actions/cpflow-setup-environment" | ||
|
|
||
| expect(step.fetch("with")).to include( | ||
| "control_plane_flow_ref" => "${{ inputs.control_plane_flow_ref }}" | ||
| ), "#{path} setup action call must pass control_plane_flow_ref" | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| it "exposes Docker build action inputs" do |
There was a problem hiding this comment.
Good regression guard. One gap: the test verifies that control_plane_flow_ref is passed in the with: block but not that it is declared as a workflow_call input in each reusable workflow file. If the declaration were accidentally removed, GitHub Actions would silently resolve ${{ inputs.control_plane_flow_ref }} to an empty string rather than failing loudly.
Consider extending the check to verify the input declaration exists at the workflow level:
workflow_inputs = workflow.dig("on", "workflow_call", "inputs") || {}
expect(workflow_inputs).to have_key("control_plane_flow_ref"),
"#{path} must declare control_plane_flow_ref as a workflow_call input"This is low-urgency since all current workflows already have the declaration — it's purely a future-proofing guard.
Review: Validate cpflow gem and workflow pinsOverall: Well-structured safety guard. The version-pin validation, workflow propagation, and spec coverage are all solid. A few items to address before merge. What the PR does
Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CHANGELOG.md (1)
17-21: ⚡ Quick winMatch Unreleased entries to existing changelog attribution format.
These new bullets are clear, but they currently skip the file’s normal pattern (bold lead + PR link + author), which makes traceability uneven across releases.
🤖 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 `@CHANGELOG.md` around lines 17 - 21, Update the two new changelog bullets to follow the repository’s standard attribution format: for the “Added a generated workflow guard that fails early when `CPFLOW_VERSION`…” entry and the “Fixed generated Control Plane entrypoints…” entry, prepend the bold lead token (e.g., **Added** / **Fixed**) if missing, and append the PR reference and author in the normal pattern used throughout the file (for example: “(PR #<number> by @<author>)”). Ensure both bullets match the surrounding entries’ exact punctuation and spacing so traceability is consistent.
🤖 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.
Inline comments:
In @.github/actions/cpflow-setup-environment/action.yml:
- Around line 105-115: The normalize_release_ref function is too strict: update
its regex to accept general SemVer prerelease identifiers (alphanumeric, hyphens
and dot-separated parts) instead of only the small set with numeric suffixes,
and when a prerelease is present return a normalized version that includes the
prerelease component (for example capture the base version in BASH_REMATCH[1]
and the full prerelease in another capture and echo them combined), so
normalize_release_ref accepts tags like v1.2.3-alpha, v1.2.3-beta.1,
v1.2.3-rc.2+build etc. and still strips the leading refs/tags/.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 17-21: Update the two new changelog bullets to follow the
repository’s standard attribution format: for the “Added a generated workflow
guard that fails early when `CPFLOW_VERSION`…” entry and the “Fixed generated
Control Plane entrypoints…” entry, prepend the bold lead token (e.g., **Added**
/ **Fixed**) if missing, and append the PR reference and author in the normal
pattern used throughout the file (for example: “(PR #<number> by @<author>)”).
Ensure both bullets match the surrounding entries’ exact punctuation and spacing
so traceability is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d83c7c3e-63aa-4625-91c5-6bf49811fc86
📒 Files selected for processing (12)
.github/actions/cpflow-setup-environment/action.yml.github/workflows/cpflow-cleanup-stale-review-apps.yml.github/workflows/cpflow-delete-review-app.yml.github/workflows/cpflow-deploy-review-app.yml.github/workflows/cpflow-deploy-staging.yml.github/workflows/cpflow-promote-staging-to-production.ymlCHANGELOG.mddocs/ci-automation.mdlib/generator_templates/entrypoint.shlib/github_flow_templates/.github/cpflow-help.mdspec/command/generate_github_actions_spec.rbspec/command/generate_spec.rb
justin808
force-pushed
the
jg-codex/validate-cpflow-version-pin
branch
from
6a72379 to
5f575b9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f575b931a
ℹ️ 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".
| if [[ "${CPFLOW_VERSION}" != "${expected_version}" ]]; then | ||
| echo "::error::CPFLOW_VERSION must match control_plane_flow_ref. CPFLOW_VERSION=${CPFLOW_VERSION}, control_plane_flow_ref=${CONTROL_PLANE_FLOW_REF}, expected CPFLOW_VERSION=${expected_version}." |
There was a problem hiding this comment.
validate_cpflow_version_pin compares raw strings, but normalize_release_ref rewrites tags like v5.1.0-rc1 to 5.1.0.rc.1; a caller setting CPFLOW_VERSION=5.1.0.rc1 (the common RubyGems form) will be rejected even though both forms resolve to the same gem version. This makes prerelease tag + gem pin combinations fail early despite being semantically equivalent, so prerelease workflows can be blocked by formatting differences alone.
Useful? React with 👍 / 👎.
Code Review: Validate cpflow gem and workflow pinsOverviewThis PR adds a version-consistency guard between the `cpflow` gem and the reusable workflow/action code loaded from a `control_plane_flow_ref` Git ref, and tightens the generated `entrypoint.sh` to run `db:prepare` only for Rails server workloads. Both changes address real operational risks. Strengths
IssuesMedium — Spec raises `KeyError` (not an assertion failure) for non-reusable-workflow files In `spec/command/generate_github_actions_spec.rb`, the test does: ```ruby If a future `cpflow-*.yml` is not a reusable workflow (e.g., only `schedule:` trigger, no `workflow_call:` key), `.fetch("workflow_call")` raises `KeyError` with no indication of which file caused it. A `next unless` guard or an explicit `have_key` expectation with the path interpolated in the message would make future failures much easier to diagnose. Low — Pre-release hint in first error message may suggest the wrong tag format The first validation error reads `release tag like v${CPFLOW_VERSION}`. For a dot-separated RubyGems pre-release like `CPFLOW_VERSION=5.0.0.rc.1` it renders as `v5.0.0.rc.1`, which `normalize_release_ref` does accept—but users familiar with Git tag conventions may have published `v5.0.0-rc.1` (dash-separated). The second error message (`expected CPFLOW_VERSION=...`) provides the correct value, so this is a minor UX nit rather than a correctness bug. Low — `is_rails_server_command` skips DB prep for `./bin/thrust bundle exec rails server` The function strips `./bin/thrust` after the `bundle exec` check, so `./bin/thrust bundle exec rails server` leaves `bundle` as `$1` and returns false (DB prep skipped). In practice Thruster wraps the Rails binary directly (`./bin/thrust rails server`), not `bundle exec`, so no generated command hits this. Re-ordering—strip `bundle exec` first, then `./bin/thrust`, then check `rails`/`./bin/rails`—would handle all combinations defensively. Minor Nits
|
| Dir.glob(Cpflow.root_path.join(".github/workflows/cpflow-*.yml").to_s).each do |path| | ||
| workflow = YAML.load_file(path, aliases: true) | ||
| workflow_inputs = (workflow["on"] || workflow[true]).fetch("workflow_call").fetch("inputs") | ||
|
|
There was a problem hiding this comment.
.fetch("workflow_call") raises KeyError with no file context if a future cpflow-*.yml doesn't have a workflow_call: trigger. Consider guarding with next unless or replacing with an explicit expectation that includes path in the failure message:
| workflow_on = workflow["on"] || workflow[true] | |
| next unless workflow_on.key?("workflow_call") | |
| workflow_inputs = workflow_on.fetch("workflow_call").fetch("inputs") |
| shift 2 | ||
| fi | ||
|
|
||
| if [ "${1:-}" = "./bin/thrust" ]; then |
There was a problem hiding this comment.
The bundle exec strip runs before this Thruster check, so bundle exec ./bin/thrust rails server is handled correctly. However, ./bin/thrust bundle exec rails server (Thruster wrapping bundle exec) reaches this shift and leaves bundle as $1, causing the function to return false and skip DB prep.
In practice the generated Thruster wrapper uses ./bin/thrust rails server (not bundle exec), so this won't occur. If full defense-in-depth is desired, re-ordering to strip bundle exec last (after stripping ./bin/thrust) would cover this case:
| if [ "${1:-}" = "./bin/thrust" ]; then | |
| if [ "${1:-}" = "./bin/thrust" ]; then | |
| shift | |
| fi | |
| if [ "${1:-}" = "bundle" ] && [ "${2:-}" = "exec" ]; then | |
| shift 2 | |
| fi |
justin808
force-pushed
the
jg-codex/validate-cpflow-version-pin
branch
from
5f575b9 to
6ab284f
Compare
| normalize_version "${ref}" | ||
| } | ||
|
|
||
| validate_cpflow_version_pin() { |
There was a problem hiding this comment.
The hint v${CPFLOW_VERSION} constructs the expected tag name from the user-supplied gem version. For stable releases this is clear (v5.0.0). For dot-separated pre-release versions (e.g. CPFLOW_VERSION=5.0.0.rc.1) it renders as v5.0.0.rc.1, which normalize_release_ref does accept—but users who published v5.0.0-rc.1 (dash-separated, the more common Git tag convention) may be confused. Adding a note that both formats are accepted (or showing the dash form) would reduce ambiguity:
| validate_cpflow_version_pin() { | |
| echo "::error::CPFLOW_VERSION can only be used when control_plane_flow_ref is a release tag (e.g. v${CPFLOW_VERSION} or v$(echo "${CPFLOW_VERSION}" | tr '.' '-' | sed 's/-/-/2')). Current control_plane_flow_ref: ${CONTROL_PLANE_FLOW_REF:-<empty>}. Leave CPFLOW_VERSION unset when testing a commit SHA or branch so cpflow is built from the same source as the reusable workflow." |
Or more simply, appending (both v5.0.0-rc.1 and v5.0.0.rc.1 are accepted for pre-release tags) to the message would be sufficient.
|
|
||
| local actual_version | ||
| local expected_version | ||
| actual_version="$(normalize_version "${CPFLOW_VERSION}")" |
There was a problem hiding this comment.
normalize_version is silently lossy for unrecognized formats
If CPFLOW_VERSION doesn't match the regex, normalize_version exits 0 with empty output. set -euo pipefail won't catch this, and actual_version becomes an empty string. The second if then fires with normalized CPFLOW_VERSION=<unrecognized>, which is decent, but a caller that checks only the exit code would see success incorrectly.
Consider an explicit guard immediately after computing actual_version:
| actual_version="$(normalize_version "${CPFLOW_VERSION}")" | |
| actual_version="$(normalize_version "${CPFLOW_VERSION}")" | |
| if [[ -z "${actual_version}" ]]; then | |
| echo "::error::CPFLOW_VERSION='${CPFLOW_VERSION}' is not a recognized semver format (expected X.Y.Z or X.Y.Z-prerelease such as X.Y.Z-rc.1)." | |
| exit 1 | |
| fi | |
| expected_version="$(normalize_release_ref "${CONTROL_PLANE_FLOW_REF}")" |
| normalize_release_ref() { | ||
| local ref="${1#refs/tags/}" | ||
|
|
||
| [[ "${ref}" == v* ]] || return 0 |
There was a problem hiding this comment.
Silent no-op is easy to misread
return 0 conventionally signals success, but here it means "I recognised this as a non-release ref and intentionally produced no output." The caller in validate_cpflow_version_pin checks the captured output — not the exit code — so the logic is correct. But a future maintainer seeing return 0 may assume the function succeeded and "fix" it to return 1, breaking the empty-output signal.
| [[ "${ref}" == v* ]] || return 0 | |
| [[ "${ref}" == v* ]] || return 0 # non-v refs produce empty output intentionally; caller checks output, not exit code |
| @@ -15,10 +15,6 @@ permissions: | |||
|
|
|||
There was a problem hiding this comment.
Removing this guard means the help comment fires on every PR open for any repo that has committed this generated wrapper — even repos that have not yet configured CPLN_TOKEN_STAGING. The new text inside the help message mentions the token requirement, which softens the impact, but contributors to repos mid-migration may see confusing workflow comments.
Consider an alternative guard that is less likely to produce false positives than vars.REVIEW_APP_PREFIX != '' but more informative than no guard — for example, checking whether .controlplane/controlplane.yml exists in the repo. That check would require a lightweight checkout step, though, which adds complexity. The current approach (document it, let consumers remove the wrapper) is pragmatically sound; just make sure the breaking change notice in the CHANGELOG is prominent.
| rescue ArgumentError, TypeError => e | ||
| raise unless e.message.include?("unknown keyword") || | ||
| e.message.include?("wrong number of arguments") || | ||
| e.message.include?("no implicit conversion of Hash") | ||
|
|
||
| YAML.safe_load(contents, [], [], true) |
There was a problem hiding this comment.
Fragile Ruby version compatibility check via error message strings
The rescue guard detects old-Ruby YAML API by matching specific exception message substrings. This can break if Psych changes its error messages, and the "no implicit conversion of Hash" string is particularly obscure (it would suggest a calling bug, not a version compat issue).
A cleaner approach is to branch on RUBY_VERSION directly before attempting the call:
| rescue ArgumentError, TypeError => e | |
| raise unless e.message.include?("unknown keyword") || | |
| e.message.include?("wrong number of arguments") || | |
| e.message.include?("no implicit conversion of Hash") | |
| YAML.safe_load(contents, [], [], true) | |
| rescue ArgumentError, TypeError => e | |
| raise unless e.message.include?("unknown keyword") || | |
| e.message.include?("wrong number of arguments") | |
| YAML.safe_load(contents, [], [], true) |
Or, more robustly:
if RUBY_VERSION >= "3.1"
YAML.safe_load(contents, aliases: true)
else
YAML.safe_load(contents, [], [], true)
endThe "no implicit conversion of Hash" string doesn't correspond to a known Psych/Ruby version gate and would silently swallow unrelated TypeErrors.
| normalize_release_ref() { | ||
| local ref="${1#refs/tags/}" | ||
|
|
||
| [[ "${ref}" == v* ]] || return 0 # Non-v refs intentionally produce empty output. |
There was a problem hiding this comment.
return 0 used to signal "no output" — counterintuitive contract
return 0 is the standard success exit code, so normalize_release_ref exits successfully whether it produced output or not. Callers must infer "nothing produced" from the empty stdout, but return 0 gives no hint that empty output is the intentional signal for non-v refs. The comment helps, but callers that forget to check for empty output and proceed anyway will silently normalize a SHA or branch name as if it had matched.
Consider returning 1 (and documenting it) to make the "non-release ref" path a distinct, checkable failure, or rename to try_normalize_release_ref to signal the nullable contract.
| [[ "${ref}" == v* ]] || return 0 # Non-v refs intentionally produce empty output. | |
| [[ "${ref}" == v* ]] || return 1 # Non-v refs produce no output; callers must check. |
| if ! tag_refs="$(git ls-remote --tags "${remote_url}" "refs/tags/${ref}" "refs/tags/${ref}^{}")"; then | ||
| echo "::error::Could not verify control_plane_flow_ref against ${remote_url}. Leave CPFLOW_VERSION unset when testing a commit SHA or branch." | ||
| exit 1 |
There was a problem hiding this comment.
Outbound git ls-remote on every CI run that sets CPFLOW_VERSION
This is a necessary guard, but it adds a synchronous network round-trip to https://github.com before any build work starts. Two things worth noting:
-
Enterprise/restricted runners that block outbound HTTPS traffic (or route through a proxy) will fail here with the generic "Could not verify" error, which may be confusing. Documenting this pre-requisite in
ci-automation.mdunder "Testing Unreleased Changes" would help those users. -
Error path is
exit 1inside a function called without!: ifgit ls-remoteitself fails (network timeout, non-zero exit), theif !captures that and the error branch runs — this is correct and intentional, just worth confirming theexit 1path is expected to be terminal here rather thanreturn 1so callers can decide.
| # Matches generated, flag-free Thruster invocations. Hand-edited commands with | ||
| # env prefixes or Thruster flags before rails skip generated DB prep. | ||
| if [ "${1:-}" = "thrust" ] || [ "${1:-}" = "bin/thrust" ] || [ "${1:-}" = "./bin/thrust" ]; then | ||
| shift | ||
| fi |
There was a problem hiding this comment.
env VAR=value rails s is a common pattern that silently skips DB prep
The comment documents this gap but the gap is wider than it looks: env RAILS_LOG_TO_STDOUT=1 rails s, env PORT=3000 ./bin/rails s, or any use of the POSIX env builtin before the command will not be detected as a Rails server command, so db:prepare won't run. This is a documented limitation, but it means any user who customizes the container CMD with an env … prefix will lose DB prep silently.
An alternative that handles the env case without major complexity:
# Strip leading `env KEY=VALUE` pairs before matching
while [ "${1:-}" = "env" ]; do
shift
while [ $# -gt 0 ] && echo "${1}" | grep -q '='; do
shift
done
doneIf keeping the current scope is preferred, consider making the comment on line 10-11 a visible # WARNING: so it is harder to miss when auditing hand-edited entrypoints.
| CPLN_TOKEN_PRODUCTION: | ||
| description: Production Control Plane token. Store it on the protected production Environment; do not pass it from a repository secret. | ||
| required: false |
There was a problem hiding this comment.
required: false means callers can pass CPLN_TOKEN_PRODUCTION from a repo secret
Declaring CPLN_TOKEN_PRODUCTION as a callable secret (even with required: false) means any caller can override the production environment by passing CPLN_TOKEN_PRODUCTION: ${{ secrets.CPLN_TOKEN_PRODUCTION }} from a repository secret. That would bypass the environment: production approval gate entirely, since the job would receive the token from the caller's secret scope before the environment gate has a chance to inject it.
The validate production token step only checks that the token is present, not that it came from the environment. A caller that passes the repo secret would pass that check without any reviewer approval.
Consider removing CPLN_TOKEN_PRODUCTION from the workflow_call secrets block entirely and relying exclusively on the environment: production injection. The token will still be available as secrets.CPLN_TOKEN_PRODUCTION in the production job because of environment: ${{ inputs.production_environment }}; it just won't be an explicit callable secret anymore.
Code Review: Validate cpflow gem and workflow pinsOverviewThis PR delivers three well-scoped improvements: (1) a fail-fast guard that rejects mismatched Critical / Security
Declaring Code QualityFragile Ruby version compat detection in The rescue block detects the old pre-3.1
Exiting with success (0) to signal "this ref did not match" is an unusual contract. Callers must infer the absence of output rather than checking an exit code. Using Performance / Reliability
This is a necessary guard, but it has two practical implications: it adds latency (typically small, but variable), and it will hard-fail on restricted CI runners that block outbound HTTPS. The error message is clear, but the network prerequisite is not mentioned in the docs section on setting Entrypoint
This is documented, but the documentation is a soft inline comment that can easily be missed when auditing a hand-edited entrypoint. Promoting the note to a Positives
|
Code ReviewOverviewThis PR tightens the cpflow GitHub Actions security posture in three key areas: (1) version pin validation between the cpflow gem and the The changes are well-structured, test coverage is thorough, and the documentation is unusually detailed. A few things worth discussing: Security — Strong improvements
Breaking change —
|
| shift | ||
| fi | ||
|
|
||
| if [ "${1:-}" = "bundle" ] && [ "${2:-}" = "exec" ]; then |
There was a problem hiding this comment.
The second bundle exec strip here (after stripping thrust) handles the pattern bundle exec thrust bundle exec rails server, where Thruster is invoked via bundle exec thrust and the Rails server is also invoked via bundle exec rails server. This is correct, but could use a short comment explaining the double-strip, since it looks like a copy-paste error at first glance:
| if [ "${1:-}" = "bundle" ] && [ "${2:-}" = "exec" ]; then | |
| # Thruster may be wrapped with its own `bundle exec`, and the Rails command | |
| # it proxies may also be `bundle exec rails server`. | |
| if [ "${1:-}" = "bundle" ] && [ "${2:-}" = "exec" ]; then | |
| shift 2 | |
| fi |
|
|
||
| [[ "${ref}" == v* ]] || return 1 | ||
|
|
||
| if ! tag_refs="$(git ls-remote --tags "${remote_url}" "refs/tags/${ref}" "refs/tags/${ref}^{}")"; then |
There was a problem hiding this comment.
If git ls-remote hangs (e.g. a runner that can reach GitHub but times out slowly), the workflow job will wait until the git transport's own timeout fires rather than failing quickly. Consider bounding this:
| if ! tag_refs="$(git ls-remote --tags "${remote_url}" "refs/tags/${ref}" "refs/tags/${ref}^{}")"; then | |
| if ! tag_refs="$(timeout 15 git ls-remote --tags "${remote_url}" "refs/tags/${ref}" "refs/tags/${ref}^{}" 2>&1)"; then |
This ensures a predictable ~15 s failure instead of a multi-minute stall when the runner has degraded outbound connectivity. The 2>&1 captures any timeout or git error text so it doesn't vanish silently.
|
|
||
| validate_github_env_value!("REVIEW_APP_PREFIX", prefix) | ||
| validate_github_env_value!("CPLN_ORG", cpln_org) | ||
| validate_github_env_value!("APP_NAME", app_name) unless app_name.empty? |
There was a problem hiding this comment.
When pr_number is empty, app_name is "" and the app_name output is never written to GITHUB_OUTPUT. Callers that reference ${{ steps.review-config.outputs.app_name }} will receive an empty string in that case — which is the correct behaviour for the cleanup workflow (which doesn't need an app_name).
This is fine as-is, but worth calling out explicitly in the output description so callers know to expect a missing-vs-empty distinction:
| validate_github_env_value!("APP_NAME", app_name) unless app_name.empty? | |
| validate_github_env_value!("APP_NAME", app_name) unless app_name.empty? | |
| # Note: app_name output is absent (not empty string) when pr_number is not provided. | |
| # Callers that need app_name must pass pr_number. |
Code Review — PR #318: Validate cpflow gem and workflow pinsOverviewThis PR delivers three interconnected improvements:
Overall the approach is sound and the security improvements are significant. A few targeted notes below. Security — Positive Changes
Issues / Observations1. Hardcoded remote URL in
|
|
|
||
| verify_release_ref_matches_checkout() { | ||
| local ref="${1#refs/tags/}" | ||
| local remote_url="https://github.com/shakacode/control-plane-flow.git" |
There was a problem hiding this comment.
The remote URL is hardcoded to the canonical upstream repo. Any fork that sets CPFLOW_VERSION will fail this check because the fork's checked-out commit will never match a tag on shakacode/control-plane-flow.
This is probably acceptable (forks should leave CPFLOW_VERSION unset), but it's not obvious from the input description that setting CPFLOW_VERSION at all triggers an outbound HTTPS call and a cross-repo commit check. Consider adding a note to the cpflow_version input description: "Setting this value triggers a remote tag-integrity check against the upstream shakacode/control-plane-flow repository; runners without outbound HTTPS access to GitHub should leave this unset."
| end | ||
|
|
||
| def validate_github_env_value!(name, value) | ||
| return if value.match?(/\A[A-Za-z0-9-]+\z/) |
There was a problem hiding this comment.
The allowed charset [A-Za-z0-9-] rejects underscores and dots. If a cpln_org value ever contains an underscore (e.g., some enterprise naming schemes), the workflow fails with the generic "must contain only letters, numbers, and hyphens" message — not a hint about what to fix.
Two suggestions:
- If Control Plane's documented naming restriction truly forbids underscores, say so in the error message:
"Control Plane GVC and org names must match /^[a-z0-9][a-z0-9-]*[a-z0-9]$/"(or whatever the real rule is). - If the restriction is only to make the value safe to write to a GitHub env file, a comment clarifying that would help future maintainers understand why dots are also excluded.
| { [ "${1:-}" = "rails" ] || [ "${1:-}" = "bin/rails" ] || [ "${1:-}" = "./bin/rails" ]; } && | ||
| { [ "${2:-}" = "server" ] || [ "${2:-}" = "s" ]; } |
There was a problem hiding this comment.
This detection works for rails s, rails server, ./bin/rails s, etc., but misses the case where Rails-level options precede the subcommand verb, e.g.:
rails -e production server
rails --environment=production serverIn both cases $2 is -e / --environment=production, not server, so db:prepare is silently skipped.
This is unlikely for generated CMD entries (which always use rails s or ./bin/rails s), but it's a sharp edge for hand-edited commands. The existing comment on line 23 ("Hand-edited commands with env flags or Thruster flags before rails skip generated DB prep") should be extended to mention Rails-native flags before the verb as well, so developers know this is a known limitation rather than a bug to report.
| case "${1}" in | ||
| *=*) shift ;; | ||
| --) shift; break ;; | ||
| -*) return 1 ;; |
There was a problem hiding this comment.
The -*) return 1 case silently skips DB prep for any command prefixed with env -<flag> … (e.g. env -i rails s or env -u VAR rails s). The comment above mentions this intentional behaviour for "hand-edited commands with env flags", but it's worth being explicit in user-facing docs that only flag-free env invocations receive DB prep, since env -i is commonly used to strip the inherited environment before running a service.
| -*) return 1 ;; | |
| -*) return 1 ;; |
No code change needed — just flagging for docs/changelog awareness.
| exit 1 | ||
| end | ||
|
|
||
| begin |
There was a problem hiding this comment.
[A-Za-z0-9-]+ deliberately excludes underscores. That's correct for Control Plane GVC/org names (DNS-label rules allow only letters, digits, and hyphens), but if a downstream project's cpln_org or app prefix ever contains an underscore the action will fail here with "must contain only letters, numbers, and hyphens" rather than a more specific message about the offending character.
A small improvement would be to surface the actual value in the error so the user immediately knows what to fix:
| begin | |
| warn "::error::#{name} ('#{value}') must contain only letters, numbers, and hyphens so it is a valid Control Plane name and can be safely written to GitHub environment files." |
|
|
||
| npm_global_prefix="${HOME}/.npm-global" | ||
| mkdir -p "${npm_global_prefix}" | ||
| echo "${npm_global_prefix}/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
When git ls-remote fails (network unreachable, DNS error, etc.) the raw git stderr is interpolated directly into the ::error:: message via ${tag_refs}. This is fine for debugging, but ${tag_refs} is also used in the [[ -z "${tag_refs}" ]] check below — if the command fails with exit code 0 (unusual but possible for some timeout implementations), the error branch is never taken and the empty-string branch fires instead, producing a less informative message.
Minor: consider splitting stdout and stderr into separate variables so the emptiness check is unambiguous:
if ! tag_refs="$(git_ls_remote_tag 2>/tmp/git_ls_err)"; then
git_err="$(cat /tmp/git_ls_err)"
echo "::error::Could not verify ... Details: ${git_err}"
exit 1
fiNot blocking — the 2>&1 approach works correctly in practice since a failed git command always exits non-zero.
| set -euo pipefail | ||
|
|
||
| if [[ -z "${CPLN_TOKEN_PRODUCTION}" ]]; then | ||
| echo "::error::CPLN_TOKEN_PRODUCTION is not set. Add it as a secret on the '${PRODUCTION_ENVIRONMENT}' GitHub Environment." |
There was a problem hiding this comment.
Good fail-fast guard. One edge case worth documenting: this check verifies the token is non-empty, but it cannot distinguish whether it arrived from the protected production Environment or was passed from a caller's named secrets: block. A hand-edited caller that passes CPLN_TOKEN_PRODUCTION explicitly (the generated test-cpflow-github-flow script rejects that, but hand-edited callers are not checked) would satisfy this guard without going through the Environment approval gate.
Consider adding a note in the docs / inline comment that passing CPLN_TOKEN_PRODUCTION as a named caller secret defeats the protection — the generator guard in test-cpflow-github-flow is the main enforcement mechanism for generated callers.
Code ReviewOverviewThis PR tightens the contract between the Security — net positive
Version-pin validation (
|
1 participant
Summary
CPFLOW_VERSIONdoes not match thecontrol_plane_flow_refrelease tagCPFLOW_VERSIONunset so the gem is built from the same checked-out source as the reusable workflow/actions./bin/rails, and fail fast on errorsWhy
GitHub loads reusable workflow and composite action code from the repo ref in
uses:before Ruby runs. The gem alone cannot select that YAML/action code. This guard prevents a downstream app from accidentally combining workflow/action code from onecontrol-plane-flowref with acpflowgem from another version.Upgrade note
The generated entrypoint now uses
set -eand prepares the database only when the command is a Rails server command (rails s,rails server,./bin/rails ...,bundle exec rails ..., or the generated Thruster wrapper). Apps with hand-edited.controlplane/entrypoint.shfiles should audit custom commands before regenerating.Validation
env CPLN_ORG=dummy bundle exec rspec spec/command/generate_github_actions_spec.rb spec/command/generate_spec.rbenv RUBOCOP_CACHE_ROOT=/private/tmp/rubocop-cache bundle exec rubocopgit diff --checkactionlint -ignore "SC2129" .github/workflows/cpflow-*.ymlsh -n lib/generator_templates/entrypoint.shSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Note
High Risk
Changes production promotion secret handling, CI deploy/delete/cleanup behavior, and generated container entrypoints (DB prep and fail-fast), any of which can block deploys or alter runtime startup if misconfigured.
Overview
This release tightens GitHub Actions so workflow/action code from
control_plane_flow_refcannot drift from a pinnedCPFLOW_VERSION: setup now requires a matching release tag, validates the remote tag commit against the checked-out actions, and rejects gem pins when testing SHAs or branches (leaveCPFLOW_VERSIONunset to build from source).Review apps get a shared
cpflow-resolve-review-configcomposite action that infers prefix, staging org, andapp_namefrom.controlplane/controlplane.yml(with optional variable overrides). Deploy/delete/cleanup workflows drop mandatoryREVIEW_APP_PREFIX/CPLN_ORG_STAGINGvalidation, pass only named secrets instead ofsecrets: inherit, and wire resolved values through build/delete steps.Production promotion adds a
production_environmentinput, runs the job on that GitHub Environment, validatesCPLN_TOKEN_PRODUCTIONthere, and generated callers pass onlyCPLN_TOKEN_STAGING—withbin/test-cpflow-github-flowguarding against inheriting secrets or forwarding the production token.Generated runtime changes: entrypoints use
set -e, run./bin/rails db:prepareonly for Rails server commands (not every workload), and Dockerfileschmod +xthe copied entrypoint. PR-open help no longer gates onREVIEW_APP_PREFIX; docs and templates describe minimal review-app setup and environment-based production tokens.Reviewed by Cursor Bugbot for commit a944641. Bugbot is set up for automated code reviews on this repo. Configure here.