[codex] Apply generated cpflow GitHub Actions flow#732
[codex] Apply generated cpflow GitHub Actions flow#732
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Upstream token not passed to copy-image command
- Added the required -t upstream token argument to the cpflow copy-image-from-upstream command while keeping the token sourced from secrets via the environment.
Or push these changes by commenting:
@cursor push 3606e16506
Preview (3606e16506)
diff --git a/.github/workflows/cpflow-promote-staging-to-production.yml b/.github/workflows/cpflow-promote-staging-to-production.yml
--- a/.github/workflows/cpflow-promote-staging-to-production.yml
+++ b/.github/workflows/cpflow-promote-staging-to-production.yml
@@ -203,14 +203,13 @@
- name: Copy image from staging
env:
- # Pass the upstream token via env rather than `-t` so it doesn't appear in /proc/<pid>/cmdline.
CPLN_UPSTREAM_TOKEN: ${{ secrets.CPLN_TOKEN_STAGING }}
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
shell: bash
run: |
set -euo pipefail
- cpflow copy-image-from-upstream -a "${PRODUCTION_APP_NAME}" --org "${CPLN_ORG_PRODUCTION}"
+ cpflow copy-image-from-upstream -a "${PRODUCTION_APP_NAME}" -t "${CPLN_UPSTREAM_TOKEN}" --org "${CPLN_ORG_PRODUCTION}"
- name: Deploy image to production
env:You can send follow-ups to the cloud agent here.
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| cpflow copy-image-from-upstream -a "${PRODUCTION_APP_NAME}" --org "${CPLN_ORG_PRODUCTION}" |
There was a problem hiding this comment.
Upstream token not passed to copy-image command
High Severity
The copy-image-from-upstream step sets CPLN_UPSTREAM_TOKEN as an environment variable but never passes the upstream token to the cpflow command. The cpflow documentation states this command requires --upstream-token or -t flag — there is no documented support for a CPLN_UPSTREAM_TOKEN env var in cpflow 4.2.0 (the version installed by cpflow-setup-environment). The old workflow correctly used -t ${{ secrets.CPLN_TOKEN_STAGING }}. Without the flag, the command cannot authenticate with the staging org and production promotion will fail at the image copy step.
Reviewed by Cursor Bugbot for commit eb1dbb2. Configure here.
There was a problem hiding this comment.
Leaving this unchanged. cpflow copy-image-from-upstream in cpflow 4.2.0 supports CPLN_UPSTREAM_TOKEN as an env var, so passing -t here would reintroduce the token-in-process-argv exposure this generated flow is intentionally avoiding. Reference: https://github.com/shakacode/control-plane-flow/blob/add-github-flow-generator/lib/command/copy_image_from_upstream.rb#L34-L36
Code Review: cpflow GitHub Actions MigrationThis is a solid migration from hand-written to generated Security — meaningful fixes
Bugs fixed
Potential issues
Nits
Overall this is well-structured with meaningful security and reliability improvements. The one item I'd prioritize is the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if cpflow exists -a "${APP_NAME}" --org "${CPLN_ORG}"; then | ||
| echo "exists=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "exists=false" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
This if/else around cpflow exists conflates two different failure modes: "app does not exist" (expected non-zero) and "API/auth error" (unexpected non-zero). Both land in the else branch and set exists=false.
Compare delete-app.sh in this same PR, which carefully pattern-matches the output ("Double check your org", "Unknown API token format", "ERROR", etc.) to distinguish a real failure from a genuine not-found and surface it as a hard error.
If there's a transient auth issue when a PR is pushed, this step sets exists=false and the pull_request path silently writes a step-summary saying the app doesn't exist yet — masking the real problem until the deploy is manually retried.
Suggested approach: mirror delete-app.sh's pattern — capture the output, check exit code, inspect the text for known error tokens, and exit non-zero on a real failure rather than treating it as "not found".
There was a problem hiding this comment.
Fixed in b9095c6 by capturing cpflow exists output and failing hard on auth/API error tokens instead of treating every non-zero exit as app-not-found.
|
|
||
| if [[ -f "${HOME}/.ssh/cpflow_build_key" ]]; then | ||
| eval "$(ssh-agent -s)" | ||
| trap 'ssh-agent -k >/dev/null; rm -f "${HOME}/.ssh/cpflow_build_key"' EXIT |
There was a problem hiding this comment.
The EXIT trap kills the SSH agent and removes cpflow_build_key, but the ~/.ssh/known_hosts file written in the previous step is not cleaned up here. On ephemeral GitHub-hosted runners this is harmless (the VM is discarded), but if this action is ever adopted on self-hosted runners with a persistent ~/.ssh, the custom known_hosts entries would accumulate across runs.
Consider adding rm -f "${HOME}/.ssh/known_hosts" to the trap, or using a temp file under $RUNNER_TEMP instead of ~/.ssh for the known_hosts.
| default: "3.3.1" | ||
| cpflow_version: | ||
| description: cpflow gem version | ||
| required: false |
There was a problem hiding this comment.
The CLI version is pinned here as a default, which is good for reproducibility. Worth noting: since sudo npm install -g fetches from the npm registry at runtime, a compromised release of @controlplane/cli@3.3.1 would silently affect all runs. Consider adding a SHA/integrity check (e.g. via npm install --integrity) or at minimum locking to a digest-pinned download, especially since this step runs in workflows that access staging/production tokens.
|
|
||
| current_image="${selected_image:-${first_image}}" | ||
| current_version="${selected_version:-${first_version}}" | ||
|
|
There was a problem hiding this comment.
The staging env-var parity check only compares names, not values. It will catch a missing DATABASE_URL in production, but it won't catch a case where production has the right key names but wrong or stale values (e.g. a staging secret was rotated but the production counterpart wasn't updated). This is expected behaviour for a name-only check — just worth documenting explicitly so it's clear what the check does and doesn't guarantee.
There was a problem hiding this comment.
Documented in b9095c6: the production promotion workflow checks that production has the same environment variable names as staging, but it does not compare secret values.
|
Code Review: Replace hand-written CI/CD Actions with generated cpflow- flow* Well-considered overhaul that hardens security, safety, and operational reliability. No blocking issues found. Security improvements Token no longer passed via --token flag — old setup-environment exposed it in /proc/cmdline and ps; now uses CPLN_TOKEN env var only. SSH key scoped to a dedicated step so ACTIONS_STEP_DEBUG cannot leak it from the build step. Delete command now gated on author_association (old workflow had no permission check). Fork PR protection blocks syncs before any secret is consumed. Old contains() command matching replaced with exact match. Safety improvements cancel-in-progress changed from true to false everywhere — old staging deploy risked partial GVC deployments mid-rollout. Prefix-based delete guard replaces the old substring check for production/staging names. Production rollback logic added with per-workload state capture and restore — old workflow had no rollback at all. Health checks now configurable (HEALTH_CHECK_RETRIES, HEALTH_CHECK_INTERVAL, HEALTH_CHECK_ACCEPTED_STATUSES). Issue 1: Fork-PR /deploy-review-app comment silently fails with no PR feedback When a collaborator comments /deploy-review-app on a fork PR, the job condition passes but Validate review app deployment source exits 1 with only a runner log message. No PR comment explains the rejection. Suggest posting an issues.createComment before exit 1, similar to how the unconfigured-secrets path writes to GITHUB_STEP_SUMMARY. Issue 2: cpflow exists error detection is string-matching fragile delete-app.sh uses case matching over stderr to distinguish app-not-found from auth/network errors. The inline TODO acknowledges this. If cpflow changes an error message, the pattern will not match and a real failure will be silently treated as app not found, causing delete to return success vacuously. The TODO to switch to a structured exit code is the right long-term fix. Issue 3: gem install / npm install without integrity pinning cpflow-setup-environment/action.yml installs @controlplane/cli@3.3.1 and cpflow 4.2.0 without hash verification. A re-tagged or compromised release would install silently. For production-adjacent CI, consider verifying SHA256 after install or using lockfiles with integrity entries. Issue 4: Nightly cleanup has no failure notification cpflow-cleanup-stale-review-apps.yml runs on cron with no failure notification. Silent failures accumulate stale apps over days. Consider a step gated on if: failure() that posts a GitHub issue or annotation. Minor observations APP_NAME / PR_NUMBER double-assignment is intentional (workflow-level env needed early for concurrency.group, step overwrites with resolved values) but a short comment would clarify intent. cpflow-review-app-help.yml using pull_request_target is correct and well-documented. deploy job in cpflow-deploy-staging.yml re-runs cpflow-setup-environment unnecessarily (only needs cpflow deploy-image) — minor redundancy, acceptable for simplicity. |
| echo "🗑️ Deleting application: $APP_NAME" | ||
| cpflow delete -a "$APP_NAME" --org "$CPLN_ORG" --yes | ||
|
|
||
| echo "✅ Successfully deleted application: $APP_NAME" |
There was a problem hiding this comment.
The case pattern matching over stderr is the right approach given cpflow exists doesn't expose a structured signal, and the inline TODO is the right way to track this. One additional defensive measure worth considering: log the full exists_output to the runner even when the "not found" path is taken, so there's an audit trail if unexpected output silently passed through the filter. Right now the output is printed only if non-empty, but it's easy to miss in logs.
Also worth noting: the pattern list doesn't include "timeout", "connection refused", or "ENOTFOUND" — all plausible network failure messages. If cpflow's Ruby HTTP client times out, it may surface Errno::ECONNREFUSED or Net::ReadTimeout, which would match "Net::", so that's covered. But curl-style timeouts would not. Low risk given this is a Ruby gem, but worth keeping in mind when bumping the cpflow version.
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| cpflow cleanup-stale-apps -a "${{ vars.REVIEW_APP_PREFIX }}" --org "${{ vars.CPLN_ORG_STAGING }}" --yes |
There was a problem hiding this comment.
No failure notification exists for this cron job. If cpflow cleanup-stale-apps fails (auth error, API hiccup, etc.), stale review apps accumulate silently across days.
Consider appending a step:
- name: Notify on failure
if: failure()
run: |
echo "::error::Stale review app cleanup failed. Check the workflow logs."Or, if the team monitors GitHub issues, use gh issue create to open a tracking issue on failure so it's not lost in workflow history.
| } >> "$GITHUB_STEP_SUMMARY" | ||
| exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
When a collaborator comments /deploy-review-app on a fork PR, execution reaches this exit 1 with only a runner log message — no feedback is posted to the PR thread. The collaborator sees a red workflow run with no explanation in the PR.
Consider posting a comment before exiting. The GH_TOKEN is available from the Resolve PR ref and commit step but not inherited here; you'd need to add GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} to this step's env block. Something like:
gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
--method POST \
--field body="Fork PRs cannot use /deploy-review-app because this workflow uses repository secrets. Ask a maintainer to deploy from a branch in the base repository." \
--silent || true
There was a problem hiding this comment.
Fixed in 1e46bfe: fork PR /deploy-review-app rejections now post a PR comment before exiting, while still keeping the deployment blocked because the workflow uses repository secrets.
| run: | | ||
| set -euo pipefail | ||
|
|
||
| sudo npm install -g @controlplane/cli@${{ inputs.cpln_cli_version }} |
There was a problem hiding this comment.
Version-pinned but not hash-verified. A malicious actor who gains write access to the @controlplane/cli npm package (or re-tags a release) could substitute a compromised binary. Same applies to gem install cpflow below.
For production-adjacent pipelines, consider adding a post-install checksum check, or at minimum document the expected SHA256 for each pinned version in a comment so drift is detectable during future version bumps.
| run: | | ||
| # Best-effort rollback: try every workload, aggregate failures, exit non-zero at the end | ||
| # if any failed. A single cpln hiccup shouldn't leave other workloads mid-promotion. | ||
| set -uo pipefail |
There was a problem hiding this comment.
The intentional omission of -e here (using set -uo pipefail instead of set -euo pipefail) is the correct design — it lets rollback continue across all workloads even if one cpln call fails, with rollback_failures tracking the aggregate. Worth a brief comment explaining the omission so a future reader doesn't add -e thinking it was forgotten:
| set -uo pipefail | |
| # Intentionally no -e: we want every workload to attempt rollback even if earlier ones fail. | |
| set -uo pipefail |
There was a problem hiding this comment.
Fixed in 1e46bfe with a comment explaining that set -e is intentionally omitted so rollback can attempt every workload and aggregate failures.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Token not persisted for subsequent workflow steps
- The composite action now writes CPLN_TOKEN to GITHUB_ENV so subsequent cpflow and cpln workflow steps inherit authentication without passing the token on the command line.
Or push these changes by commenting:
@cursor push f1eee564c7
Preview (f1eee564c7)
diff --git a/.github/actions/cpflow-setup-environment/action.yml b/.github/actions/cpflow-setup-environment/action.yml
--- a/.github/actions/cpflow-setup-environment/action.yml
+++ b/.github/actions/cpflow-setup-environment/action.yml
@@ -63,6 +63,12 @@
exit 1
fi
+ {
+ echo "CPLN_TOKEN<<__CPLN_TOKEN__"
+ echo "$CPLN_TOKEN"
+ echo "__CPLN_TOKEN__"
+ } >> "$GITHUB_ENV"
+
create_output=""
if ! create_output="$(cpln profile create default --org "$ORG" 2>&1)"; then
if ! echo "$create_output" | grep -qi "already exists"; thenYou can send follow-ups to the cloud agent here.
Code Review — PR #732: Generated cpflow GitHub Actions FlowOverviewThis is a well-structured replacement of hand-written CI/CD workflows with the generated Significant Improvements ✅
IssuesBug:
|
| fi | ||
| deploy_args+=(--org "${CPLN_ORG}" --verbose) | ||
|
|
||
| cpflow deploy-image "${deploy_args[@]}" |
There was a problem hiding this comment.
Bug: deploy runs even when build fails.
When a job has an explicit if:, GitHub Actions drops the implicit success() guard. Because this condition only checks validate-branch's output, the deploy job will start even if the build job failed, wasting runner minutes and attempting to deploy a missing image.
| cpflow deploy-image "${deploy_args[@]}" | |
| if: needs.validate-branch.outputs.is_deployable == 'true' && needs.build.result == 'success' |
| echo "🗑️ Deleting application: $APP_NAME" | ||
| cpflow delete -a "$APP_NAME" --org "$CPLN_ORG" --yes | ||
|
|
||
| echo "✅ Successfully deleted application: $APP_NAME" |
There was a problem hiding this comment.
The string-matching heuristic on cpflow exists error output (lines 43–54) is the main reliability risk in this file. The consequence of an unmatched error pattern is silently exiting 0 ("app not found") when the API actually returned an error, meaning the delete is skipped without any visible failure.
An alternative that inverts the logic: treat any non-zero exit with non-empty output as a failure unless it matches a known "not found" pattern:
# instead of matching known error strings, match known "not found" patterns
case "$exists_output" in
*"not found"*|*"does not exist"*)
echo "⚠️ Application does not exist: $APP_NAME"
exit 0
;;
*)
echo "❌ ERROR: failed to determine whether application exists" >&2
printf '%s\n' "$exists_output" >&2
exit 1
;;
esacThis is safer by default — unknown output is treated as an error rather than a no-op. The tradeoff is that you'd need to know the exact "not found" wording from cpflow exists, which has the same coupling problem in the other direction. The TODO comment already captures this; just flagging it for awareness.
| echo "ready=false" >> "$GITHUB_OUTPUT" | ||
| { | ||
| echo "Control Plane review app automation is not configured yet." | ||
| echo |
There was a problem hiding this comment.
This top-level APP_NAME is computed from the raw event payload and is later overwritten in $GITHUB_ENV by the Resolve PR ref and commit step (echo "APP_NAME=..." >> "$GITHUB_ENV"). The step-level write wins, so this declaration is dead configuration and can be confusing. Consider removing it and relying solely on the step-computed value, which is already the authoritative one (it handles all three event types correctly).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review: cpflow Generated GitHub Actions FlowThis is a meaningful security and reliability upgrade over the handwritten workflows. Several issues from the old setup are fixed here; I've also flagged a few new items worth addressing. ✅ Notable Improvements
🔴 Issues1. The delete workflow triggers on 2. The same fragile string-matching logic ( 🟡 Warnings3. In 4. Production health check accepts 301/302 by default
5. Nightly cleanup has no failure notification
ℹ️ Informational6.
7. These will pick up any non-breaking updates automatically, which is generally fine. However, |
| name: Delete Review App | ||
|
|
||
| on: | ||
| pull_request_target: |
There was a problem hiding this comment.
pull_request_target deserves an explanation comment here.
pull_request_target runs in the context of the base repo (write permissions, access to secrets) and is required for fork PR cleanup — pull_request can't post comments on fork PRs because the token is read-only. This is safe here because no PR code is ever checked out (the actions/checkout@v4 below fetches base-branch code only).
cpflow-review-app-help.yml has a good comment explaining this exact pattern. Without a parallel comment here, a future maintainer may "fix" this to pull_request thinking it's safer, silently breaking fork PR cleanup.
| pull_request_target: | |
| # pull_request_target is intentional: it has write permission to comment on PRs from | |
| # forks, where `pull_request` would be read-only. This is safe because no PR code is | |
| # checked out — the job only calls cpflow with base-repo credentials. | |
| # Do not switch this to `pull_request` or add a checkout of the PR ref without re-evaluating. | |
| pull_request_target: |
| # Make the token available to later workflow steps without putting it on argv. | ||
| token_delimiter="CPLN_TOKEN_$(openssl rand -hex 8)" | ||
| { | ||
| echo "CPLN_TOKEN<<${token_delimiter}" | ||
| printf '%s\n' "$CPLN_TOKEN" | ||
| echo "${token_delimiter}" | ||
| } >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Writing CPLN_TOKEN to GITHUB_ENV propagates it to every subsequent step in the calling workflow — not just the steps that explicitly use it. This is a necessary tradeoff (cpflow picks it up from the environment automatically), but it's worth documenting so future maintainers don't inadvertently add steps that log environment state.
| # Make the token available to later workflow steps without putting it on argv. | |
| token_delimiter="CPLN_TOKEN_$(openssl rand -hex 8)" | |
| { | |
| echo "CPLN_TOKEN<<${token_delimiter}" | |
| printf '%s\n' "$CPLN_TOKEN" | |
| echo "${token_delimiter}" | |
| } >> "$GITHUB_ENV" | |
| # Persist CPLN_TOKEN to GITHUB_ENV so all subsequent steps can authenticate | |
| # with cpflow/cpln without putting the token on the command line. | |
| # Trade-off: the token is present in every subsequent step's environment. | |
| # Do not add steps that dump env vars (e.g., `env | sort`) after this point. | |
| token_delimiter="CPLN_TOKEN_$(openssl rand -hex 8)" | |
| { | |
| echo "CPLN_TOKEN<<${token_delimiter}" | |
| printf '%s\n' "$CPLN_TOKEN" | |
| echo "${token_delimiter}" | |
| } >> "$GITHUB_ENV" |
| # TODO: replace this string-matching with a structured signal once `cpflow exists` exposes one | ||
| # (e.g. a distinct exit code for "not found" vs. API/auth errors, or `cpflow exists --json`). | ||
| # Until then, keep this list in sync if `cpflow exists` starts emitting new error patterns — | ||
| # any unmatched error string would otherwise be silently treated as "app not found". | ||
| exists_output="" | ||
| if ! exists_output="$(cpflow exists -a "$APP_NAME" --org "$CPLN_ORG" 2>&1)"; then | ||
| case "$exists_output" in | ||
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) |
There was a problem hiding this comment.
The same cpflow exists error-pattern matching appears verbatim in the Check if review app exists step in cpflow-deploy-review-app.yml (line ~195). The TODO comment here is accurate but only covers this file — the workflow copy is uncovered.
If cpflow adds a new error pattern (e.g. "rate limit" or "timeout"), both locations need updating in sync. Consider extracting this into a shared script (e.g. .github/scripts/cpflow-app-exists.sh) that both the action and the workflow call, so the pattern list is maintained in one place.
| # repo variable to tighten this for apps that expose a dedicated health endpoint | ||
| # (e.g. "200" for a plain /health, or "200 401 403" for apps that auth-gate / without | ||
| # redirecting). | ||
| HEALTH_CHECK_ACCEPTED_STATUSES: ${{ vars.HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }} |
There was a problem hiding this comment.
Accepting 301/302 as "healthy" means a redirect to a login page, maintenance page, or even a misconfigured CDN passes the health check. The comment correctly explains why (curl without -L, Rails auth-gating /), but a redirect can pass even when the destination is broken.
Consider adding a step summary warning when a non-200 status is accepted, so operators reviewing the promotion log have visibility:
if [[ "${http_status}" == "${accepted}" ]]; then
if [[ "${http_status}" != "200" ]]; then
echo "::warning::Health check passed with HTTP ${http_status} (not 200) — verify the redirect destination is healthy."
fi
echo "healthy=true" >> "$GITHUB_OUTPUT"
exit 0
fiAlso worth documenting in .controlplane/shakacode-team.md that setting HEALTH_CHECK_ACCEPTED_STATUSES=200 and adding a /health endpoint is the recommended production posture.
| on: | ||
| workflow_dispatch: | ||
| schedule: | ||
| - cron: "0 0 * * *" |
There was a problem hiding this comment.
Scheduled workflows that fail silently are easy to miss — GitHub does send an email for failed scheduled runs but only to the repo owner by default, and only after 7 days of consecutive failures (as of current GitHub behavior).
Consider adding a failure notification step, or at minimum documenting that the team should verify GitHub's "Notify me of failed workflow runs" notification setting is enabled. Stale review apps that aren't cleaned up accrue Control Plane costs.
| - cron: "0 0 * * *" | |
| - cron: "0 3 * * *" # 3 AM UTC — avoids exact midnight when many repos run scheduled jobs simultaneously |
(The cron timing is a minor suggestion — staggering off midnight reduces GitHub's scheduler contention, making the run more likely to start on time.)
Code Review: Replace hand-written CI/CD with generated
|
| Old | New |
|---|---|
setup-environment action |
cpflow-setup-environment action |
build-docker-image action |
cpflow-build-docker-image action |
delete-control-plane-app action (no prefix guard) |
cpflow-delete-control-plane-app action (prefix-guarded) |
deploy-to-control-plane-review-app.yml |
cpflow-deploy-review-app.yml |
delete-review-app.yml (pull_request closed) |
cpflow-delete-review-app.yml (pull_request_target closed) |
deploy-to-control-plane-staging.yml (triggered on all branches) |
cpflow-deploy-staging.yml (main/master only) |
nightly-remove-stale-review-apps.yml |
cpflow-cleanup-stale-review-apps.yml |
promote-staging-to-production.yml |
cpflow-promote-staging-to-production.yml |
help-command.yml |
cpflow-help-command.yml + cpflow-review-app-help.yml |
Security Improvements ✅
- Token never on argv:
cpflow-setup-environmentpassesCPLN_TOKENvia environment variable rather than--tokenon the command line, avoiding/proc/<pid>/cmdlineandpsleakage. The oldsetup-environmentusedcpln profile update default --org "$ORG" --token "$TOKEN". - SSH key isolation:
cpflow-build-docker-imagededicates a separate step for writing the SSH key soDOCKER_BUILD_SSH_KEYis not present in the main build step's environment, even underACTIONS_STEP_DEBUG=true. - Prefix guard on deletes:
cpflow-delete-control-plane-app/delete-app.shchecks thatAPP_NAMEstarts withREVIEW_APP_PREFIX-before callingcpflow delete. The old action had no such guard. - Fork PR gating:
cpflow-deploy-review-app.ymlexplicitly blocks fork PRs onpull_requestevents (cannot access secrets anyway) and addsauthor_associationchecks for comment-triggered deploys. The old workflow triggered on any PRsynchronize/reopenedwithout fork checks. pull_request_targetused correctly:cpflow-delete-review-app.ymlusespull_request_targetfor the closed-PR cleanup path (to access secrets from fork PRs) and checks out only the base repo — the comment in the file explains this. This is a well-known footgun that has been handled correctly here.
Robustness Improvements ✅
cancel-in-progress: falseon all deploy/delete concurrency groups. Mid-rollout cancellation ofcpflow deploy-imagecan leave workloads in a partially-deployed state; letting in-flight runs finish is the right call.- Multi-workload rollback: The production promotion workflow now captures per-container image state for all workloads and restores each one on failure, rather than only rolling back the single
railsworkload image. - Randomized heredoc delimiters in
cpflow-promote-staging-to-production.ymlprevent a strayEOFin a variable value from truncating the multi-lineGITHUB_OUTPUTwrite. --run-release-phaseis now opt-in:cpflow-deploy-review-app.ymlandcpflow-deploy-staging.ymlinspectcpflow configto decide whether to pass--run-release-phase, rather than always passing it. This avoids failures when the flag isn't configured.- Health check window is larger: 24 retries × 25s = ~10 min (old: 12 × 10s = ~2 min). More headroom for slow Rails cold boots.
Issues and Suggestions
Medium: Stale cpflow exists error-matching is fragile
delete-app.sh and cpflow-deploy-review-app.yml both parse free-form stderr output from cpflow exists to distinguish "app not found" from authentication/API errors. The comment in the file acknowledges this with a TODO. This approach is inherently brittle if cpflow changes its error output. Consider filing an issue against cpflow to request a structured signal (distinct exit code or --json output) and linking it from the TODO so it doesn't get forgotten.
Low: cpflow-deploy-review-app.yml — steps.source.outputs.allowed can be empty on workflow_dispatch
The Validate review app deployment source step sets allowed=true for same-repo branches and allowed=false for fork PRs via pull_request events. For workflow_dispatch, steps.resolve-pr.outputs.same_repo could be "false" if the PR is from a fork, which falls into the issue_comment branch of the condition and calls gh pr comment — but workflow_dispatch on a fork PR should perhaps just fail rather than try to comment. Low risk since workflow_dispatch is manual, but the code path is slightly inconsistent.
Low: cpflow-setup-environment — redundant null checks
The Setup Control Plane profile and registry login step validates that CPLN_TOKEN and ORG are non-empty via shell if [[ -z ... ]] guards. These variables are both declared required: true in the action's inputs: block, so Actions will already refuse to call the action without them. The guards add no safety but don't hurt anything.
Low: Old staging workflow triggered on branches: ['*']
The deleted deploy-to-control-plane-staging.yml ran on every push to every branch and relied on a validate-branch job to short-circuit. The new cpflow-deploy-staging.yml correctly restricts the on.push.branches filter to ["main", "master"] with a comment explaining the GitHub limitation on vars in branch filters. This is an improvement, but it means pushes to a non-main/master STAGING_APP_BRANCH will not trigger the workflow unless the file is edited. The comment in the file explains this trade-off clearly — no action needed, just confirming the approach is understood.
Nit: cpflow-cleanup-stale-review-apps.yml — no contents: write permission for release creation
The promote workflow needs contents: write to create GitHub releases; the cleanup workflow does not. Both look correct on permissions.
Nit: cpflow-deploy-review-app.yml line 1011 — long if: conditions
Several steps have near-identical if: conditions like:
if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success')
This is repetitive across 10+ steps. A job-level output or an environment variable set once would be cleaner, but this is a style point and not a functional issue in generated code.
Documentation ✅
.controlplane/readme.mdandshakacode-team.mdare updated to reference the new workflow names, slash commands, andREVIEW_APP_PREFIXnaming convention.- Required/optional repository secrets and variables are now fully documented in
shakacode-team.md. - The
pull_request_targetusage incpflow-delete-review-app.ymlandcpflow-review-app-help.ymlis explained with inline comments that document why the trigger was chosen, which is important for future maintainers.
Summary
The new generated flow is substantially more secure and robust than what it replaces. The main open item is the cpflow exists output-parsing fragility, which the code already acknowledges. The PR is in good shape to merge once the draft status is resolved and tests can be run.
| exists_output="" | ||
| if ! exists_output="$(cpflow exists -a "$APP_NAME" --org "$CPLN_ORG" 2>&1)"; then | ||
| case "$exists_output" in | ||
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) |
There was a problem hiding this comment.
The TODO comment here is well-placed. To make this easier to track, consider filing an issue against the cpflow gem linking to this file, so the workaround has a visible upstream ticket. The risk of silent misclassification (an unrecognized error string treated as "app not found") is the main hazard — a regression test that mocks cpflow exists returning an exit-1 with a novel error string would catch drift before it reaches production.
| echo "Failed to post fork rejection comment to PR #${PR_NUMBER}" >&2 | ||
| fi | ||
|
|
||
| echo "${message}" >&2 |
There was a problem hiding this comment.
When the trigger is workflow_dispatch and the PR being deployed is from a fork (same_repo == "false"), neither the pull_request branch (line 143) nor the same_repo == "true" branch (line 138) matches, so execution falls through to this gh pr comment call followed by exit 1. That is technically correct — the deployment is blocked — but it reaches the gh pr comment path that was written for the issue_comment case, which may be surprising. A small guard makes the intent explicit:
| echo "${message}" >&2 | |
| # workflow_dispatch with a fork PR: block the run without attempting a PR comment. | |
| if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then | |
| echo "${message}" >&2 | |
| exit 1 | |
| fi | |
| message="Review app deploys from fork pull requests are not allowed because this workflow uses repository secrets." | |
| if ! gh pr comment "${PR_NUMBER}" --body "${message}"; then | |
| echo "Failed to post fork rejection comment to PR #${PR_NUMBER}" >&2 | |
| fi | |
| echo "${message}" >&2 |
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if [[ -z "$CPLN_TOKEN" ]]; then |
There was a problem hiding this comment.
Both token and org are declared required: true in inputs:, so Actions will already refuse to invoke this composite action without them. These null checks will never fire. They're harmless, but if left in they may mislead a future reader into thinking there's a code path where these are empty.
PR #732 Review: cpflow Generated GitHub Actions FlowOverviewThis PR replaces hand-written Control Plane GitHub Actions with the output of Security Improvements (Notable)
Correctness Improvements
Issues FoundMisleading comment:
|
| # Override these by editing this file or by setting the matching repository variable. | ||
| # Worst-case wall time per attempt is HEALTH_CHECK_INTERVAL plus the curl --max-time below | ||
| # (10s), so the defaults give a ~10 minute window (24 × (15 + 10) = 600s) — enough for | ||
| # most Rails cold boots (asset precompile + db:migrate + workload readiness). | ||
| HEALTH_CHECK_RETRIES: 24 | ||
| HEALTH_CHECK_INTERVAL: 15 |
There was a problem hiding this comment.
The comment says these can be overridden "by setting the matching repository variable", but unlike HEALTH_CHECK_ACCEPTED_STATUSES (line 28 which reads ${{ vars.HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }}), these two are hardcoded literals — setting a repo variable with the same name has no effect.
Either wire them up to match the pattern used for HEALTH_CHECK_ACCEPTED_STATUSES:
| # Override these by editing this file or by setting the matching repository variable. | |
| # Worst-case wall time per attempt is HEALTH_CHECK_INTERVAL plus the curl --max-time below | |
| # (10s), so the defaults give a ~10 minute window (24 × (15 + 10) = 600s) — enough for | |
| # most Rails cold boots (asset precompile + db:migrate + workload readiness). | |
| HEALTH_CHECK_RETRIES: 24 | |
| HEALTH_CHECK_INTERVAL: 15 | |
| HEALTH_CHECK_RETRIES: ${{ vars.HEALTH_CHECK_RETRIES || 24 }} | |
| HEALTH_CHECK_INTERVAL: ${{ vars.HEALTH_CHECK_INTERVAL || 15 }} |
…or remove the "setting the matching repository variable" claim from the comment above so the next person who tries to tune these doesn't spend time debugging why their repo var is ignored.
| # Until then, keep this list in sync if `cpflow exists` starts emitting new error patterns — | ||
| # any unmatched error string would otherwise be silently treated as "app not found". | ||
| exists_output="" | ||
| if ! exists_output="$(cpflow exists -a "$APP_NAME" --org "$CPLN_ORG" 2>&1)"; then | ||
| case "$exists_output" in | ||
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) |
There was a problem hiding this comment.
The same error-token list and detection logic is duplicated verbatim in cpflow-deploy-review-app.yml (the "Check if review app exists" step at line ~204). When this list needs to grow (e.g., a new cpflow exists error pattern), both files need to change in sync — which the TODO already flags.
A practical option: extract the detection to a small shared shell script (e.g., .github/actions/cpflow-delete-control-plane-app/cpflow-app-exists.sh) that returns 0/1/2 for exists/not-found/error, then source or call it from both places. That makes the error-token list a single source of truth and keeps the TODO resolution contained to one file.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unnecessary CI work on every new PR open
- Removed the opened pull_request trigger so the deploy workflow only runs on synchronize and reopened events while PR-open help remains handled by the help workflow.
Or push these changes by commenting:
@cursor push 2c303a005e
Preview (2c303a005e)
diff --git a/.github/workflows/cpflow-deploy-review-app.yml b/.github/workflows/cpflow-deploy-review-app.yml
--- a/.github/workflows/cpflow-deploy-review-app.yml
+++ b/.github/workflows/cpflow-deploy-review-app.yml
@@ -4,7 +4,7 @@
on:
pull_request:
- types: [opened, synchronize, reopened]
+ types: [synchronize, reopened]
issue_comment:
types: [created]
workflow_dispatch:
@@ -35,7 +35,7 @@
jobs:
deploy:
- # Skip synchronize/opened events from fork PRs at the job level — they cannot access
+ # Skip pull_request events from fork PRs at the job level — they cannot access
# repository secrets anyway, so running any steps just burns billable minutes. Users
# can still manually deploy a fork PR via `/deploy-review-app` (gated below by
# author_association) or workflow_dispatch.You can send follow-ups to the cloud agent here.
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
Unnecessary CI work on every new PR open
Low Severity
Adding opened to the pull_request types (the old workflow only had synchronize, reopened) causes the full workflow to run on every new PR — including Ruby setup, npm install of the CLI, and gem install — only to determine the review app doesn't exist yet and write a step summary. That summary is only visible in the Actions tab, while the separate cpflow-review-app-help.yml already posts a help comment directly on the PR. A brand-new PR can never have an existing review app, so opened adds several minutes of billable CI time per PR with no deployment benefit.
Reviewed by Cursor Bugbot for commit 9a390c1. Configure here.
Code ReviewOverviewThis PR replaces hand-written Control Plane GitHub Actions with the generated Significant ImprovementsSecurity
Correctness — fork PR cleanup (critical fix)
Safety — delete guard
Nightly cleanup — hardcoded prefix bug fixed
Staging workflow scope
Concurrency safety
Release phase opt-in
Production rollback
Issues FoundMinor
Non-blocking / suggestions
Test CoverageThe PR description notes that SummaryThis is a well-executed migration with meaningful security, correctness, and safety improvements. The identified issues are minor; the fork |
|
|
||
| echo "${message}" >&2 | ||
| exit 1 | ||
|
|
There was a problem hiding this comment.
The fork PR rejection for issue_comment exits 1 (job fails, red ✗), but the equivalent pull_request path exits 0 with allowed=false (job succeeds, subsequent steps are skipped). A collaborator triggering /deploy-review-app on a fork PR will see a failed workflow run, which is noisy and confusing when the PR comment already explains why it was blocked.
Consider aligning the two paths:
| echo "allowed=false" >> "$GITHUB_OUTPUT" | |
| message="Review app deploys from fork pull requests are not allowed because this workflow uses repository secrets." | |
| if [[ "${{ github.event_name }}" == "issue_comment" ]]; then | |
| if ! gh pr comment "${PR_NUMBER}" --body "${message}"; then | |
| echo "Failed to post fork rejection comment to PR #${PR_NUMBER}" >&2 | |
| fi | |
| fi | |
| { | |
| echo "${message}" | |
| } >> "$GITHUB_STEP_SUMMARY" |
Then all subsequent steps already gate on steps.source.outputs.allowed == 'true', so no other changes are needed.
| run: | | ||
| set -euo pipefail | ||
|
|
||
| staging_vars="$(CPLN_TOKEN="${CPLN_TOKEN_STAGING}" cpln gvc get "${STAGING_APP_NAME}" --org "${CPLN_ORG_STAGING}" -o json | jq -r '.spec.env // [] | .[].name' | sort)" |
There was a problem hiding this comment.
This check compares GVC-level environment variable names only. Variables set at the workload level (inside a workload spec's env: block rather than the GVC's env: block) or backed by Control Plane secret stores will not be caught by this check.
This is an improvement over the old code (same scope), but worth documenting so operators understand the guarantee: the check ensures no GVC-level staging var is absent from production at the GVC level — it does not cover workload-scoped vars or secret references.
A comment on the if [[ -z "${staging_vars}" ]] branch might also help — if staging uses only workload-level vars, the check silently skips entirely rather than flagging a misconfiguration.
| # Do not switch this to `pull_request` or add a checkout step without re-evaluating. | ||
| pull_request_target: | ||
| types: [opened] | ||
|
|
There was a problem hiding this comment.
This fires on every PR open, including draft PRs. Teams that routinely open drafts before they're ready for review will see an auto-comment on every draft, which can become noise.
Consider restricting to non-draft PRs:
| pull_request_target: | |
| types: [opened] | |
| # Remove the filter below if you want the help comment on draft PRs too |
…and adding a job-level guard:
if: github.event.pull_request.draft == falseOr document this behavior in cpflow-help.md if the team intentionally wants drafts included.
Code Review: cpflow GitHub Actions MigrationThis PR replaces the hand-written Control Plane GitHub Actions with the generated cpflow-* composite action set. The overall direction is sound - the generated actions are more robust, better commented, and fix several real issues in the old code. Strengths: Token passed via CPLN_TOKEN env var (not --token arg, prevents process-list exposure). SSH key isolated to dedicated step with umask 077 and chmod 600. Heredoc delimiter randomization via openssl rand. Fork PR safety: deploy uses pull_request; delete help use pull_request_target. Deletion guard enforces REVIEW_APP_PREFIX allowlist. Production rollback covers all workloads. Issues:
Regressions fixed: delete workflow pull_request -> pull_request_target (fork PR secrets access); deploy command substring match -> exact match; staging cancel-in-progress: true -> false; --token arg -> CPLN_TOKEN env; name-contains delete guard -> REVIEW_APP_PREFIX allowlist. Verdict: Ready to merge with minor notes. Clear improvement over old workflows across security, fork PR safety, and production rollback. Draft status appropriate while repo vars/secrets are verified. |
| - name: Validate required secrets and variables | ||
| id: config | ||
| uses: ./.github/actions/cpflow-validate-config | ||
| env: |
There was a problem hiding this comment.
The Initial checkout step runs unconditionally before the fork origin guard at the Validate review app deployment source step. Under pull_request this is safe — GitHub always runs the workflow in the base repository's context, so even fork PRs check out base-repo code, not fork code. A comment here would make this safe ordering explicit and prevent a future maintainer from accidentally reversing the steps:
| env: | |
| # NOTE: pull_request events always execute in the base-repo context, so | |
| # checking out here before the fork guard is intentional and safe. | |
| - name: Initial checkout |
|
|
||
| build: | ||
| needs: validate-branch | ||
| if: needs.validate-branch.outputs.is_deployable == 'true' |
There was a problem hiding this comment.
This checkout in validate-branch is only needed because cpflow-validate-config is a local composite action (./.github/actions/...). However, cpflow-validate-config only performs indirect bash variable lookups — it never reads files from the working tree. The checkout could be dropped from this job and the validate config step could be moved to the build job (which already checks out), saving ~15 seconds per staging push.
If keeping the checkout here for simplicity, this comment would clarify intent:
| if: needs.validate-branch.outputs.is_deployable == 'true' | |
| # Checkout is required to reference the local ./.github/actions/cpflow-validate-config action. | |
| - name: Checkout repository |
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: |
There was a problem hiding this comment.
The contents: write permission is only required by the gh release create step near the end of the job. All other steps need at most contents: read. Since this is a workflow_dispatch-only workflow the blast radius is low, but a comment would help auditors understand why write is needed here:
| inputs: | |
| permissions: | |
| contents: write # Required for `gh release create` in the promotion summary step |
| CPFLOW_PR_FRIENDLY: ${{ inputs.pull_request_friendly }} | ||
| CPFLOW_EVENT_NAME: ${{ github.event_name }} | ||
| run: | | ||
| set -euo pipefail |
There was a problem hiding this comment.
These parameter expansion trims strip exactly one leading/trailing space. An entry with two or more leading spaces (e.g. " secret:FOO") would not be fully trimmed and the subsequent ${!name} indirect lookup would silently fail (looking up a variable named " secret:FOO" instead of "FOO").
Since the required: input is author-controlled YAML the practical risk is low, but a more robust trim would be:
| set -euo pipefail | |
| entry="${entry#"${entry%%[! ]*}"}" | |
| entry="${entry%"${entry##*[! ]}"}" |
Or with extglob: entry="${entry##*([[:space:]])}"; entry="${entry%%*([[:space:]])}" after shopt -s extglob.
Code Review: Replace hand-written GitHub Actions with generated cpflow-* flowOverviewThis PR replaces all the hand-written Control Plane CI/CD actions and workflows (+1790 / -1147 lines) with a generated Security Fixes (significant wins)
Other Improvements
Issues / Observations1. The 2. Fragile error-pattern matching in The 3. Verbose step-level conditions in The guard 4. Each job calls Minor / Nits
Overall: The security fixes alone make this worth merging. The issues noted are non-blocking: observability noise on first deploy (issue 1), a known TODO that should be tracked as an issue (issue 2), and two style/performance suggestions. |
| cpln_cli_version: ${{ vars.CPLN_CLI_VERSION }} | ||
| cpflow_version: ${{ vars.CPFLOW_VERSION }} | ||
|
|
||
| - name: Detect release phase support |
There was a problem hiding this comment.
Ordering issue: release phase detection runs before app existence is known
cpflow config -a "${APP_NAME}" will fail (and print to stderr) if the app doesn't yet exist — which is always the case on a first /deploy-review-app. Because the command is inside an if, set -e doesn't abort, and flag= is safely emitted as the fallback. But the error output appears in the log and can mislead operators.
Consider moving this step after setup-review-app (or gating it with steps.check-app.outputs.exists == 'true'), so it only runs against an app that's confirmed to exist:
| - name: Detect release phase support | |
| - name: Detect release phase support | |
| if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') | |
| id: release-phase | |
| uses: ./.github/actions/cpflow-detect-release-phase | |
| with: | |
| app_name: ${{ env.APP_NAME }} |
| exists_output="" | ||
| if ! exists_output="$(cpflow exists -a "$APP_NAME" --org "$CPLN_ORG" 2>&1)"; then | ||
| case "$exists_output" in | ||
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) |
There was a problem hiding this comment.
Fragile string-matching (acknowledged TODO — needs a tracking issue)
This case pattern distinguishes real API/auth errors from app not found by matching substrings of cpflow exists output. If cpflow ever changes or adds error messages, a real failure would silently fall through as "app does not exist" and exit 0 — meaning a delete triggered on an app whose token is invalid would do nothing and report success.
The TODO comment documents this well, but a GitHub issue to track the upstream cpflow exists --json or structured exit-code request would make sure it doesn't get forgotten:
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) | |
| *"Double check your org"*|*"Unknown API token format"*|*"ERROR"*|*"Error:"*|*"Traceback"*|*"Net::"*) |
Suggestion: open a cpflow upstream issue requesting a distinct exit code for "not found" vs "auth/API error" and link it here.
| }); | ||
|
|
||
| - name: Build Docker image | ||
| if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') |
There was a problem hiding this comment.
Repeated gate condition is copy-paste fragile
This if: guard appears on ~10 consecutive steps:
steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success')
A newly added step that accidentally omits it will run unconditionally — silently posting a PR comment or creating a GitHub deployment even when config isn't ready.
A structural fix would be to split this into two jobs:
preflight— validates config, resolves PR ref, checks app existence, creates app if neededbuild-and-deploy— gated vianeeds: preflightwith a job-levelif: needs.preflight.outputs.ready == 'true'
This pushes the gate up to needs: so individual step omissions can't escape it.
|
|
||
| # Later workflow steps call cpflow/cpln, so persist the token without putting it on argv. | ||
| # This intentionally gives subsequent trusted steps in the same job a CPLN_TOKEN env var. | ||
| token_delimiter="CPLN_TOKEN_$(openssl rand -hex 8)" |
There was a problem hiding this comment.
Correct pattern — worth a note for future maintainers
The randomized delimiter (CPLN_TOKEN_$(openssl rand -hex 8)) is the right way to write a multiline secret to $GITHUB_ENV. A fixed delimiter like EOF would be vulnerable to heredoc injection if the token value happened to contain that string on a line by itself (unlikely for a base64-encoded token, but still the right habit).
One thing to be aware of: once this step completes, CPLN_TOKEN is in the process environment for all subsequent steps in the same job. This is intentional (documented in the comment above), but if ACTIONS_STEP_DEBUG=true is ever enabled on a run, GitHub will dump each step's env before commands execute. GitHub's secret masking should redact the token value in those dumps since it was originally sourced from ${{ secrets.* }}, but it's worth confirming this in your GitHub org's audit settings if you operate in a high-security context.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Health check
cplnfailure aborts retry loop prematurely- Guarded the workload endpoint lookup so transient
cplnorjqfailures clear the endpoint and allow the polling loop to retry.
- Guarded the workload endpoint lookup so transient
- ✅ Fixed: Error message header repeats for each missing item
- Split the error output into one header line followed by a separate printf for all missing items.
Or push these changes by commenting:
@cursor push f419c0c97c
Preview (f419c0c97c)
diff --git a/.github/actions/cpflow-validate-config/action.yml b/.github/actions/cpflow-validate-config/action.yml
--- a/.github/actions/cpflow-validate-config/action.yml
+++ b/.github/actions/cpflow-validate-config/action.yml
@@ -71,5 +71,6 @@
exit 0
fi
- printf 'Missing required GitHub configuration:\n- %s\n' "${missing[@]}" >&2
+ echo "Missing required GitHub configuration:" >&2
+ printf -- '- %s\n' "${missing[@]}" >&2
exit 1
diff --git a/.github/actions/cpflow-wait-for-health/action.yml b/.github/actions/cpflow-wait-for-health/action.yml
--- a/.github/actions/cpflow-wait-for-health/action.yml
+++ b/.github/actions/cpflow-wait-for-health/action.yml
@@ -61,7 +61,10 @@
for attempt in $(seq 1 "${CPFLOW_MAX_RETRIES}"); do
echo "Health check attempt ${attempt}/${CPFLOW_MAX_RETRIES}"
- endpoint="$(cpln workload get "${CPFLOW_WORKLOAD_NAME}" --gvc "${CPFLOW_APP_NAME}" --org "${CPFLOW_ORG}" -o json | jq -r '.status.endpoint // empty')"
+ if ! endpoint="$(cpln workload get "${CPFLOW_WORKLOAD_NAME}" --gvc "${CPFLOW_APP_NAME}" --org "${CPFLOW_ORG}" -o json | jq -r '.status.endpoint // empty')"; then
+ echo "Unable to fetch workload endpoint; will retry if attempts remain."
+ endpoint=""
+ fi
if [[ -n "${endpoint}" ]]; then
http_status="$(curl -s -o /dev/null -w '%{http_code}' --max-time "${CPFLOW_CURL_MAX_TIME}" "${endpoint}" 2>/dev/null || echo 000)"
echo "Endpoint: ${endpoint}, HTTP status: ${http_status}"You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit fee6ce9. Configure here.
| for attempt in $(seq 1 "${CPFLOW_MAX_RETRIES}"); do | ||
| echo "Health check attempt ${attempt}/${CPFLOW_MAX_RETRIES}" | ||
|
|
||
| endpoint="$(cpln workload get "${CPFLOW_WORKLOAD_NAME}" --gvc "${CPFLOW_APP_NAME}" --org "${CPFLOW_ORG}" -o json | jq -r '.status.endpoint // empty')" |
There was a problem hiding this comment.
Health check cpln failure aborts retry loop prematurely
Medium Severity
The cpln workload get call on line 64 runs under set -euo pipefail with no error guard. A transient API or network failure causes the entire script to exit immediately instead of retrying on the next loop iteration. The curl call on line 66 is correctly guarded with || echo 000, showing retry resilience was intended, but the cpln call lacks similar protection. In the production promotion workflow, this health check failure triggers a rollback of a potentially healthy deployment.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fee6ce9. Configure here.
| exit 0 | ||
| fi | ||
|
|
||
| printf 'Missing required GitHub configuration:\n- %s\n' "${missing[@]}" >&2 |
There was a problem hiding this comment.
Error message header repeats for each missing item
Low Severity
printf 'Missing required GitHub configuration:\n- %s\n' "${missing[@]}" repeats the entire format string for every element in the missing array. When multiple secrets or variables are missing, the "Missing required GitHub configuration:" header is printed once per item, producing confusing duplicate output instead of a single header followed by a list.
Reviewed by Cursor Bugbot for commit fee6ce9. Configure here.
1 participant
Summary
cpflow-*action and workflow set fromcpflow generate-github-actions.cpflow-*action/workflow names.Verification
BUNDLE_GEMFILE=/tmp/cpflow-pr-278/Gemfile bin/conductor-exec bundle exec ruby /tmp/cpflow-pr-278/bin/cpflow github-flow-readiness— no blocking readiness issues detected.bin/conductor-exec ruby -e 'require "yaml"; paths = Dir[".github/**/*.yml"] + Dir[".github/**/*.yaml"] + Dir[".controlplane/**/*.yml"]; paths.sort.each { |path| YAML.load_file(path, aliases: true); puts path }'bin/conductor-exec git diff --cached --checkbin/conductor-exec bundle exec rubocopNot Run / Blocked
bin/conductor-exec bundle exec rspeccould not complete locally because PostgreSQL was not running at/tmp/.s.PGSQL.5432; after the first Rails load failure, Coveralls setup also reported repeated coverage initialization errors.Note
Medium Risk
Replaces core deployment automation (review apps, staging deploys, production promotion) and introduces new secret/variable handling plus deletion safeguards, so misconfiguration could block deploys or affect environments.
Overview
Migrates Control Plane CI/CD from the older hand-written GitHub Actions to the generated
cpflow-*composite actions and workflows, covering review-app deploy/delete, staging deploys, production promotion, and nightly stale review-app cleanup.The new flow makes review apps opt-in via the exact
/deploy-review-appcomment (with auto-redeploy on new commits once created), adds a/delete-review-apppath plus PR-close cleanup, and gates fork PR behavior to avoid secret exposure.Updates the build/deploy plumbing with configurable
@controlplane/cli+cpflowversions, optional Docker build SSH/extra args, safer token handling, a release-phase auto-detect flag, and stronger deletion safety viaREVIEW_APP_PREFIXchecks; docs are updated to match the new workflow names and required repo secrets/variables.Reviewed by Cursor Bugbot for commit fee6ce9. Bugbot is set up for automated code reviews on this repo. Configure here.