Address cpflow review app follow-ups

Author: justin808

.controlplane/readme.md

@@ -381,7 +381,8 @@ Use `/delete-review-app` to delete it manually; closing the PR deletes it
 automatically. Pushes to the staging branch deploy staging, and production
 promotion is manual from the `cpflow-promote-staging-to-production` workflow.
 The production promotion workflow checks that production has all environment
-variable names present in staging; it does not compare secret values.
+variable names present in staging; it does not compare secret values, workload
+environment variables, or Control Plane secret references.
 
 The repository variables and secrets must match the app names in
 `.controlplane/controlplane.yml`. In particular, `REVIEW_APP_PREFIX` should
@@ -426,7 +427,17 @@ bundle exec rubocop
 ```
 
 Then open a normal PR and let GitHub Actions prove the generated review-app,
-staging, lint, JS, and RSpec workflows before merging. If a project needs to
-track generator changes automatically, use a scheduled maintenance PR or
-Renovate-style workflow that bumps the `cpflow` version, regenerates these files,
-and runs the same validation commands.
+staging, lint, JS, and RSpec workflows before merging. For review-app workflow
+changes, test both the local workflow syntax and a real deployment. GitHub runs
+`issue_comment` workflows from the default branch, so a `/deploy-review-app`
+comment on the PR does not fully exercise slash-command changes that are only on
+the PR branch. Before merge, run the PR branch workflow explicitly:
+
+```bash
+gh workflow run cpflow-deploy-review-app.yml --ref <branch> -f pr_number=<pr-number>
+```
+
+After the workflow reports a review-app URL, verify the URL returns HTTP 200.
+If a project needs to track generator changes automatically, use a scheduled
+maintenance PR or Renovate-style workflow that bumps the `cpflow` version,
+regenerates these files, and runs the same validation commands.

.github/workflows/cpflow-deploy-review-app.yml

@@ -51,6 +51,8 @@ jobs:
     timeout-minutes: 45
 
     steps:
+      # `pull_request` events run in the base repository context, so this initial
+      # checkout fetches trusted workflow code before the fork-origin guard below.
       - name: Initial checkout
         uses: actions/checkout@v4
 
@@ -107,6 +109,8 @@ jobs:
             same_repo="true"
           fi
 
+          # Re-derive these from the resolved PR number because workflow-level env
+          # values are empty for issue_comment events.
           {
             echo "PR_NUMBER=$pr_number"
             echo "APP_NAME=${REVIEW_APP_PREFIX}-$pr_number"

.github/workflows/cpflow-promote-staging-to-production.yml

@@ -9,15 +9,15 @@ on:
         type: string
 
 permissions:
-  contents: write
+  contents: write  # Required for `gh release create` after a successful promotion.
 
 env:
   # 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 }}
   # Space-separated list of HTTP statuses considered healthy. The default accepts 301/302
   # because `curl` is invoked without `-L`, so a root `/` that redirects to a login page
   # (common for Rails apps that auth-gate `/`) would otherwise be reported as unhealthy
@@ -26,8 +26,8 @@ env:
   # (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' }}
-  ROLLBACK_READINESS_RETRIES: 24
-  ROLLBACK_READINESS_INTERVAL: 15
+  ROLLBACK_READINESS_RETRIES: ${{ vars.ROLLBACK_READINESS_RETRIES || 24 }}
+  ROLLBACK_READINESS_INTERVAL: ${{ vars.ROLLBACK_READINESS_INTERVAL || 15 }}
   PRIMARY_WORKLOAD: ${{ vars.PRIMARY_WORKLOAD }}
 
 concurrency:
@@ -128,19 +128,27 @@ jobs:
           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)"
           production_vars="$(CPLN_TOKEN="${CPLN_TOKEN_PRODUCTION}" cpln gvc get "${PRODUCTION_APP_NAME}" --org "${CPLN_ORG_PRODUCTION}" -o json | jq -r '.spec.env // [] | .[].name' | sort)"
 
+          # This compares GVC-level env var names only. Workload-level env entries
+          # and secret references are outside this pre-flight check's scope.
           if [[ -z "${staging_vars}" ]]; then
             echo "Staging GVC exposes no environment variables; skipping parity check."
             exit 0
           fi
 
           missing_vars="$(comm -23 <(printf '%s\n' "${staging_vars}") <(printf '%s\n' "${production_vars}"))"
+          extra_vars="$(comm -13 <(printf '%s\n' "${staging_vars}") <(printf '%s\n' "${production_vars}"))"
 
           if [[ -n "${missing_vars}" ]]; then
             echo "::error::Production is missing environment variables that exist in staging"
             echo "${missing_vars}"
             exit 1
           fi
 
+          if [[ -n "${extra_vars}" ]]; then
+            echo "::warning::Production has GVC environment variables that do not exist in staging; verify they are intentional:"
+            echo "${extra_vars}"
+          fi
+
       - name: Capture current production image
         id: capture-current
         env:
@@ -258,6 +266,8 @@ jobs:
             rollback_args=()
 
             while IFS=$'\t' read -r index image; do
+              # cpln parses --set as key=value; current image URI formats do not
+              # contain "=", but re-check this if digest-pinned refs ever add one.
               rollback_args+=(--set "spec.containers[${index}].image=${image}")
             done < <(echo "${previous_images}" | jq -r 'to_entries[] | "\(.key)\t\(.value)"')