[codex] Move generated GitHub Actions logic upstream#305
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
<review_stack_artifact> </review_stack_artifact> WalkthroughRefactors generated GitHub Actions into thin wrapper workflows that delegate to upstream reusable workflows, fixes cpflow-setup-environment metadata and install behavior, adds reusable workflows for deploy/cleanup/help/promote flows, updates generator/template variables, adapts wrapper templates, extends specs, updates docs, and tightens token/org handling. ChangesReusable Workflows Architecture Refactoring
Sequence Diagram(s)sequenceDiagram
participant GitHubEvent as GitHub Event
participant Wrapper as Generated Wrapper Workflow
participant ReusableFlow as shakacode/control-plane-flow Reusable Workflow
participant CPFlowCLI as cpflow CLI
participant GitHubAPI as GitHub API
GitHubEvent->>Wrapper: trigger (issue_comment/pull_request/workflow_dispatch)
Wrapper->>ReusableFlow: workflow_call (control_plane_flow_ref, inputs, secrets: inherit)
ReusableFlow->>CPFlowCLI: setup + run cpflow commands (exists/deploy/delete/cleanup)
ReusableFlow->>GitHubAPI: create/update comments, reactions, Deployment Status
CPFlowCLI-->>ReusableFlow: command results / workload URLs
ReusableFlow-->>Wrapper: outputs (status, environment_url)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
from
2b7e8cf to
fae07ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/cpflow-deploy-staging.yml (1)
63-63: ⚡ Quick win
actions/checkoutuses@v6tags—only pin to commit SHAs if this repo enforces it.
.github/workflows/cpflow-deploy-staging.ymlusesuses: actions/checkout@v6(mutable tags) at lines 63, 90, 95, 127, and 132. This repo’s own workflow template code explicitly allows floating major tags for generated templates and suggests downstream repos with stricter supply-chain requirements should replaceuses: actions/...@vNwith immutable commit SHAs—so if “unpinned-uses” is an active policy for this repo, update these steps; otherwise treat SHA pinning as an optional hardening improvement.🤖 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 @.github/workflows/cpflow-deploy-staging.yml at line 63, The workflow currently uses mutable tags for the checkout action via the string "uses: actions/checkout@v6"; if your repo enforces the "unpinned-uses" policy, replace each "uses: actions/checkout@v6" occurrence with the corresponding immutable commit SHA (e.g., "uses: actions/checkout@<commit-sha>") by looking up the exact commit for the v6 release on the actions/checkout GitHub repo, or otherwise document that floating major tags are acceptable; update all occurrences of the "uses: actions/checkout@v6" token in the workflow to use the chosen SHA-pinned ref or leave as-is if floating tags are allowed..github/workflows/cpflow-delete-review-app.yml (1)
44-44: ⚡ Quick winAction SHA pinning isn’t required here (repo intentionally uses floating major tags).
These
uses: actions/github-script@v8(lines 44/95/108/137) anduses: actions/checkout@v6(line 63) align with the repo’s documented approach in.github/actions/cpflow-setup-environment/action.yml, which intentionally pins third-party actions to floating major tags rather than immutable SHAs; SHA pinning is positioned as an option for downstream repos with stricter requirements. Consider immutable SHA pinning only as optional supply-chain hardening.🤖 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 @.github/workflows/cpflow-delete-review-app.yml at line 44, Keep the existing floating major tags instead of pinning to immutable SHAs: leave the uses entries as actions/github-script@v8 (all occurrences) and actions/checkout@v6 rather than replacing them with specific SHAs; ensure the workflow matches the repo convention described in .github/actions/cpflow-setup-environment/action.yml so only downstream repos opt into SHA pinning if desired.
🤖 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/workflows/cpflow-cleanup-stale-review-apps.yml:
- Line 29: The workflow currently uses the mutable tag "uses:
actions/checkout@v6"; replace that with a pinned immutable commit SHA (full
40-character commit) for actions/checkout to avoid supply-chain risks. Locate
the "uses: actions/checkout@v6" entry in the CPFlow cleanup workflow and update
it to "uses: actions/checkout@<COMMIT_SHA>" where <COMMIT_SHA> is the exact
commit hash from the actions/checkout repository corresponding to the v6 release
you want to lock to.
In @.github/workflows/cpflow-deploy-review-app.yml:
- Line 54: The workflow uses mutable action tags (actions/github-script@v8 and
actions/checkout@v6) which should be SHA-pinned for the privileged deploy path;
locate every occurrence of "uses: actions/github-script@v8" and "uses:
actions/checkout@v6" in the YAML and replace the tag with the corresponding full
commit SHA (e.g., actions/github-script@<commit-sha> and
actions/checkout@<commit-sha>) by looking up the desired release commit on the
action's GitHub repo and substituting that SHA so each "uses:" entry is pinned.
In @.github/workflows/cpflow-help-command.yml:
- Line 31: Replace mutable action tags with immutable commit SHAs: update every
"uses: actions/github-script@v8" occurrence (both spots) to the corresponding
full commit SHA for that release and update "uses: actions/checkout@v6" to its
full commit SHA; ensure you commit the exact SHA strings (not tags) so the
workflow references the pinned commits instead of v-tags.
In @.github/workflows/cpflow-promote-staging-to-production.yml:
- Line 56: Replace the loose version tag for actions/checkout (currently
"actions/checkout@v6") in the two checkout steps ("Checkout repository" and
"Checkout control-plane-flow actions") with the corresponding immutable commit
SHAs; locate the uses: "actions/checkout@v6" entries in the workflow and change
them to "actions/checkout@<commit-sha>" using the official checkout repository
commit SHA you want to pin to so both checkout steps reference the exact same
pinned SHA for supply-chain integrity.
In `@lib/github_flow_templates/.github/workflows/cpflow-delete-review-app.yml`:
- Around line 29-32: The reusable-workflow wrapper forwards a mutable branch ref
via `@__CPFLOW_GITHUB_ACTIONS_REF__` and the generator uses
ENV.fetch("CPFLOW_GITHUB_ACTIONS_REF", "main"); change the generator and CI to
only accept immutable refs (commit SHAs or release tags) by validating
cpflow_github_actions_ref against a SHA/tag regex and rejecting branch names
(remove the "main" default), and update usages in
lib/github_flow_templates/.github/workflows/* (e.g.,
cpflow-delete-review-app.yml referencing `@__CPFLOW_GITHUB_ACTIONS_REF__`) to
require a pinned ref; also add a CI check that fails if
CPFLOW_GITHUB_ACTIONS_REF is a branch name so generation/consumers cannot supply
mutable refs.
---
Nitpick comments:
In @.github/workflows/cpflow-delete-review-app.yml:
- Line 44: Keep the existing floating major tags instead of pinning to immutable
SHAs: leave the uses entries as actions/github-script@v8 (all occurrences) and
actions/checkout@v6 rather than replacing them with specific SHAs; ensure the
workflow matches the repo convention described in
.github/actions/cpflow-setup-environment/action.yml so only downstream repos opt
into SHA pinning if desired.
In @.github/workflows/cpflow-deploy-staging.yml:
- Line 63: The workflow currently uses mutable tags for the checkout action via
the string "uses: actions/checkout@v6"; if your repo enforces the
"unpinned-uses" policy, replace each "uses: actions/checkout@v6" occurrence with
the corresponding immutable commit SHA (e.g., "uses:
actions/checkout@<commit-sha>") by looking up the exact commit for the v6
release on the actions/checkout GitHub repo, or otherwise document that floating
major tags are acceptable; update all occurrences of the "uses:
actions/checkout@v6" token in the workflow to use the chosen SHA-pinned ref or
leave as-is if floating tags are allowed.
🪄 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: 470e4dcf-4211-482c-b5cf-2207cfcde67e
📒 Files selected for processing (26)
.github/actions/cpflow-build-docker-image/action.yml.github/actions/cpflow-delete-control-plane-app/action.yml.github/actions/cpflow-delete-control-plane-app/delete-app.sh.github/actions/cpflow-detect-release-phase/action.yml.github/actions/cpflow-setup-environment/action.yml.github/actions/cpflow-validate-config/action.yml.github/actions/cpflow-wait-for-health/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-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.ymldocs/ai-github-flow-prompt.mddocs/ci-automation.mddocs/commands.mdlib/command/generate_github_actions.rblib/github_flow_templates/.github/workflows/cpflow-cleanup-stale-review-apps.ymllib/github_flow_templates/.github/workflows/cpflow-delete-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-staging.ymllib/github_flow_templates/.github/workflows/cpflow-help-command.ymllib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.ymllib/github_flow_templates/.github/workflows/cpflow-review-app-help.ymlspec/command/generate_github_actions_spec.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/command/generate_github_actions_spec.rb (1)
589-592: ⚡ Quick winAdd coverage for a non-default
CPFLOW_GITHUB_ACTIONS_REF.Current assertions lock in the default
"main"path, but they don’t verify that a custom env ref is actually propagated into generated wrappers. A small env-scoped example would protect the new feature from regressions.🤖 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 `@spec/command/generate_github_actions_spec.rb` around lines 589 - 592, The spec currently only asserts the default replacement for __CPFLOW_GITHUB_ACTIONS_REF__ ("main"); add a focused example in generate_github_actions_spec.rb that sets a non-default ENV value for CPFLOW_GITHUB_ACTIONS_REF (e.g. "refs/heads/custom"), runs the same generation flow that produces actual = playground.join(relative_path).read, and asserts the generated wrapper contains that custom ref instead of "main"; ensure you set and restore the ENV within the example (or use with_modified_env) so other tests are unaffected and reference the same relative_path/playground read code path used in the existing examples.
🤖 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 `@lib/command/generate_github_actions.rb`:
- Around line 66-67: The cpflow_github_actions_ref method currently returns
ENV.fetch("CPFLOW_GITHUB_ACTIONS_REF", "main") which still yields an empty
string if the env var is set to "", so update cpflow_github_actions_ref to guard
against empty/whitespace-only values by reading the env var
(ENV["CPFLOW_GITHUB_ACTIONS_REF"] or ENV.fetch), trimming it (to_s.strip) and
returning the trimmed value if non-empty, otherwise falling back to "main";
ensure you reference the cpflow_github_actions_ref method when applying this
change.
In `@lib/github_flow_templates/.github/workflows/cpflow-review-app-help.yml`:
- Around line 16-18: The show-help step currently uses the upstream workflow via
"uses:
shakacode/control-plane-flow/.github/workflows/cpflow-review-app-help.yml@__CPFLOW_GITHUB_ACTIONS_REF__"
but omits the usual with: and secrets: blocks; update the show-help job to
include a with: block passing control_plane_flow_ref:
__CPFLOW_GITHUB_ACTIONS_REF__ (and any other required inputs the upstream
workflow expects) and add a secrets: block to forward necessary secrets (e.g.,
GITHUB_TOKEN or other repo secrets) while preserving the existing if:
vars.REVIEW_APP_PREFIX != '' condition so the wrapper matches other templates
like cpflow-help-command.yml.
---
Nitpick comments:
In `@spec/command/generate_github_actions_spec.rb`:
- Around line 589-592: The spec currently only asserts the default replacement
for __CPFLOW_GITHUB_ACTIONS_REF__ ("main"); add a focused example in
generate_github_actions_spec.rb that sets a non-default ENV value for
CPFLOW_GITHUB_ACTIONS_REF (e.g. "refs/heads/custom"), runs the same generation
flow that produces actual = playground.join(relative_path).read, and asserts the
generated wrapper contains that custom ref instead of "main"; ensure you set and
restore the ENV within the example (or use with_modified_env) so other tests are
unaffected and reference the same relative_path/playground read code path used
in the existing examples.
🪄 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: d912a0ef-4c09-435b-840a-ef3adfcb87c9
📒 Files selected for processing (26)
.github/actions/cpflow-build-docker-image/action.yml.github/actions/cpflow-delete-control-plane-app/action.yml.github/actions/cpflow-delete-control-plane-app/delete-app.sh.github/actions/cpflow-detect-release-phase/action.yml.github/actions/cpflow-setup-environment/action.yml.github/actions/cpflow-validate-config/action.yml.github/actions/cpflow-wait-for-health/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-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.ymldocs/ai-github-flow-prompt.mddocs/ci-automation.mddocs/commands.mdlib/command/generate_github_actions.rblib/github_flow_templates/.github/workflows/cpflow-cleanup-stale-review-apps.ymllib/github_flow_templates/.github/workflows/cpflow-delete-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-staging.ymllib/github_flow_templates/.github/workflows/cpflow-help-command.ymllib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.ymllib/github_flow_templates/.github/workflows/cpflow-review-app-help.ymlspec/command/generate_github_actions_spec.rb
✅ Files skipped from review due to trivial changes (2)
- docs/commands.md
- docs/ci-automation.md
Greptile SummaryThis PR migrates the heavy GitHub Actions logic from generated downstream repositories into reusable upstream workflows (
Confidence Score: 4/5Safe to merge; no functional regressions found. The architecture shift is clean and the key security properties (same-repo gate, author_association checks, no PR code on trusted runners) are preserved in the upstream workflows. The upstream workflows correctly inherit The two cleanups are self-contained: Important Files Changed
Sequence DiagramsequenceDiagram
participant DW as Downstream Wrapper<br/>(generated .github/workflows/)
participant UW as Upstream Reusable Workflow<br/>(shakacode/control-plane-flow)
participant CA as Composite Actions<br/>(.cpflow/.github/actions/)
Note over DW: Event triggers, permissions,<br/>safety-gate if condition
DW->>UW: "uses: cpflow-deploy-review-app.yml@ref"
DW->>UW: with: control_plane_flow_ref / secrets: inherit
UW->>UW: React to comment command
UW->>CA: actions/checkout (shakacode/control-plane-flow → .cpflow)
UW->>CA: cpflow-validate-config
UW->>UW: Resolve PR ref / same-repo gate
UW->>UW: Checkout PR commit → app/
UW->>CA: cpflow-setup-environment
UW->>CA: cpflow-detect-release-phase
UW->>CA: cpflow-build-docker-image
UW->>UW: cpflow deploy-image
UW->>CA: cpflow-wait-for-health
UW-->>DW: job result
Reviews (1): Last reviewed commit: "Move generated GitHub Actions logic upst..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def template_variables | ||
| { | ||
| "__CPFLOW_VERSION__" => ::Cpflow::VERSION, | ||
| "__CPFLOW_GITHUB_ACTIONS_REF__" => cpflow_github_actions_ref, | ||
| "__STAGING_BRANCH_FILTER__" => staging_branch_filter, | ||
| "__STAGING_APP_BRANCH_EXPRESSION__" => staging_app_branch_expression | ||
| "__STAGING_BRANCH_DEFAULT__" => staging_branch_default | ||
| } | ||
| end |
There was a problem hiding this comment.
__CPFLOW_VERSION__ key is no longer used by any template file — no file under lib/github_flow_templates/ contains this placeholder. The version is now hardcoded directly in .github/actions/cpflow-setup-environment/action.yml and kept in sync via the spec. Leaving the entry in template_variables is dead code that the substitute_template_variables helper iterates over on every generation run.
| def template_variables | |
| { | |
| "__CPFLOW_VERSION__" => ::Cpflow::VERSION, | |
| "__CPFLOW_GITHUB_ACTIONS_REF__" => cpflow_github_actions_ref, | |
| "__STAGING_BRANCH_FILTER__" => staging_branch_filter, | |
| "__STAGING_APP_BRANCH_EXPRESSION__" => staging_app_branch_expression | |
| "__STAGING_BRANCH_DEFAULT__" => staging_branch_default | |
| } | |
| end | |
| def template_variables | |
| { | |
| "__CPFLOW_GITHUB_ACTIONS_REF__" => cpflow_github_actions_ref, | |
| "__STAGING_BRANCH_FILTER__" => staging_branch_filter, | |
| "__STAGING_BRANCH_DEFAULT__" => staging_branch_default | |
| } | |
| end |
| template = template_root.join(relative_path).read | ||
| expected = template | ||
| .gsub("__CPFLOW_VERSION__", Cpflow::VERSION) | ||
| .gsub("__CPFLOW_GITHUB_ACTIONS_REF__", "main") | ||
| .gsub("__STAGING_BRANCH_FILTER__", %("main", "master")) |
There was a problem hiding this comment.
__CPFLOW_VERSION__ substitution in the drift test is now a no-op — no template file contains that placeholder. It should be removed in lockstep with the template_variables cleanup to keep the test an accurate mirror of what the generator actually does.
| template = template_root.join(relative_path).read | |
| expected = template | |
| .gsub("__CPFLOW_VERSION__", Cpflow::VERSION) | |
| .gsub("__CPFLOW_GITHUB_ACTIONS_REF__", "main") | |
| .gsub("__STAGING_BRANCH_FILTER__", %("main", "master")) | |
| expected = template | |
| .gsub("__CPFLOW_GITHUB_ACTIONS_REF__", "main") | |
| .gsub("__STAGING_BRANCH_FILTER__", %("main", "master")) | |
| .gsub("__STAGING_BRANCH_DEFAULT__", "") |
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
6 times, most recently
from
ee25ceb to
6188949
Compare
![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: 618894963c
ℹ️ 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".
| uses: ./.cpflow/.github/actions/cpflow-setup-environment | ||
| with: | ||
| token: ${{ secrets.CPLN_TOKEN_STAGING }} | ||
| org: ${{ vars.CPLN_ORG_STAGING }} | ||
| cpln_cli_version: ${{ vars.CPLN_CLI_VERSION }} |
There was a problem hiding this comment.
These jobs only check out shakacode/control-plane-flow into .cpflow, then call cpflow-setup-environment without a working_directory. In that action, ruby/setup-ruby auto-detects Ruby from the working directory, which defaults to .; here . is not the checked-out .cpflow tree, so Ruby resolution can fail or drift to the runner’s system Ruby and break review-app deletion/cleanup runs. Set working_directory: .cpflow (or check out the caller repo before setup) to make runtime deterministic; the same omission also appears in .github/workflows/cpflow-cleanup-stale-review-apps.yml.
Useful? React with 👍 / 👎.
![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: Missing user repo checkout breaks cleanup and delete workflows
- Added caller repository checkout steps before loading cpflow actions in both cleanup and delete reusable workflows so Ruby setup and cpflow run from the app repository root.
Or push these changes by commenting:
@cursor push 796f90efe2
Preview (796f90efe2)
diff --git a/.github/workflows/cpflow-cleanup-stale-review-apps.yml b/.github/workflows/cpflow-cleanup-stale-review-apps.yml
--- a/.github/workflows/cpflow-cleanup-stale-review-apps.yml
+++ b/.github/workflows/cpflow-cleanup-stale-review-apps.yml
@@ -25,6 +25,11 @@
runs-on: ubuntu-latest
timeout-minutes: 30
steps:
+ - name: Checkout repository
+ uses: actions/checkout@v6
+ with:
+ persist-credentials: false
+
- name: Checkout control-plane-flow actions
uses: actions/checkout@v6
with:
diff --git a/.github/workflows/cpflow-delete-review-app.yml b/.github/workflows/cpflow-delete-review-app.yml
--- a/.github/workflows/cpflow-delete-review-app.yml
+++ b/.github/workflows/cpflow-delete-review-app.yml
@@ -59,6 +59,11 @@
}
}
+ - name: Checkout repository
+ uses: actions/checkout@v6
+ with:
+ persist-credentials: false
+
- name: Checkout control-plane-flow actions
uses: actions/checkout@v6
with:You can send follow-ups to the cloud agent here.
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
from
6188949 to
25e80af
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| env: | ||
| CPLN_CLI_VERSION: ${{ inputs.cpln_cli_version }} | ||
| CPFLOW_VERSION: ${{ inputs.cpflow_version }} | ||
| CPFLOW_SOURCE_DIR: ${{ github.action_path }}/../../.. |
There was a problem hiding this comment.
Fragile relative path from github.action_path
github.action_path resolves to .cpflow/.github/actions/cpflow-setup-environment, so ../../.. correctly lands at .cpflow. However, this silently breaks if the action is ever nested differently (e.g., moved one level deeper).
Consider making this path more explicit and robust:
| CPFLOW_SOURCE_DIR: ${{ github.action_path }}/../../.. | |
| CPFLOW_SOURCE_DIR: ${{ github.action_path }}/../../../.. |
Wait — actually the current path is intentional (action is exactly 3 levels under the repo root: .cpflow/.github/actions/<action-name>). A self-documenting alternative:
| CPFLOW_SOURCE_DIR: ${{ github.action_path }}/../../.. | |
| # github.action_path = .cpflow/.github/actions/cpflow-setup-environment → 3 levels up = .cpflow | |
| CPFLOW_SOURCE_DIR: ${{ github.action_path }}/../../.. |
At minimum, adding a runtime sanity-check would catch misuse early:
if [[ ! -f "${CPFLOW_SOURCE_DIR}/cpflow.gemspec" ]]; then
echo "::error::cpflow.gemspec not found at ${CPFLOW_SOURCE_DIR}; check the action's path depth." >&2
exit 1
fi
| end | ||
| return @@api_token if @@api_token[:token].match?(API_TOKEN_REGEX) | ||
| token = @@api_token[:token] | ||
| return @@api_token if token && !token.empty? && !token.match?(/[\r\n]/) |
There was a problem hiding this comment.
Token validation now accepts almost any non-empty string
The old API_TOKEN_REGEX = /^[\w\-._]+$/ was too strict for scoped service-account tokens (which contain /, +, :, =). The new check correctly allows those, but the trade-off is that malformed tokens now surface at the first actual API call rather than upfront — potentially with a less obvious error message.
This is the right design decision for scoped tokens, but worth a brief comment explaining why the regex was dropped rather than relaxed, so future readers don't see it as accidentally too-permissive:
| return @@api_token if token && !token.empty? && !token.match?(/[\r\n]/) | |
| # Allow any token that doesn't contain line-breaks. Scoped service-account tokens | |
| # include characters like '/', '+', ':', '=' that the old word-char regex rejected; | |
| # format enforcement is deferred to the API, which returns a clear auth error on | |
| # bad tokens. | |
| return @@api_token if token && !token.empty? && !token.match?(/[\r\n]/) |
|
|
||
| # Scoped service-account tokens may not be allowed to list every org. Let | ||
| # the command's target API call report any org/resource problem instead. | ||
| return true unless result |
There was a problem hiding this comment.
nil from list_orgs silently bypasses org validation
Returning true when list_orgs returns nil is the right call for scoped tokens that can't enumerate orgs. The concern is that a transient API error that happens to return nil (rather than raising) would also bypass the check.
If ControlplaneApiDirect can return nil for network/HTTP failures (not just permission-denied responses), it may be worth distinguishing the two:
# nil means the token is scoped and can't list orgs — assume it's correct and let
# the subsequent API call surface any org/auth problem with a real error message.
return true unless resultThe comment is good. Just confirm that api.list_orgs only returns nil for 403/permission responses and raises (not returns nil) for genuine network errors — otherwise this broadens the bypass surface beyond intent.
| # Don't cancel an in-flight promotion: a half-finished `cpflow deploy-image` plus a | ||
| # rollback can leave production in a worse state than letting the first run finish. | ||
| cancel-in-progress: false | ||
| contents: write |
There was a problem hiding this comment.
Permissions elevated to contents: write at the workflow level
The upstream reusable workflow scopes contents: write narrowly to the create-github-release job. By granting contents: write here at the wrapper's workflow level, the promote-to-production job call also runs with write access — even though it only needs contents: read.
GitHub's reusable-workflow permission model requires the caller to grant any permission the callee uses, so this is technically necessary. Consider adding a comment to explain the elevation:
| contents: write | |
| permissions: | |
| # contents: write is required for the create-github-release job in the upstream reusable | |
| # workflow. The promote-to-production job only needs read, but callers must grant the union. | |
| contents: write |
|
|
||
| jobs: | ||
| delete-review-app: | ||
| if: | |
There was a problem hiding this comment.
pull_request_target security rationale was removed from the wrapper
The original template had a clear inline comment explaining why pull_request_target is used (fork PRs need write permission to delete the review app and update PR comments, and the checkout doesn't use the PR ref: so it's safe). That comment was intentionally moved to the upstream reusable workflow — but the wrapper no longer carries any explanation.
Downstream adopters reading this generated file will see pull_request_target without the security context. Consider keeping a brief note here:
jobs:
delete-review-app:
# pull_request_target is intentional: fork PR-close events need access to staging
# secrets to delete review apps and update PR comments. The upstream reusable workflow
# checks out trusted base-branch action code (not the fork head), keeping secrets safe.
if: |
| }); | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-deploy-review-app.yml@__CPFLOW_GITHUB_ACTIONS_REF__ | ||
| with: | ||
| control_plane_flow_ref: __CPFLOW_GITHUB_ACTIONS_REF__ |
There was a problem hiding this comment.
secrets: inherit passes all repository secrets upstream
This is the correct approach for reusable workflows that need CPLN_TOKEN_STAGING, DOCKER_BUILD_SSH_KEY, etc. However, secrets: inherit passes every secret in the calling repository — including any secrets unrelated to cpflow — to the upstream shakacode/control-plane-flow reusable workflow.
GitHub's reusable workflow trust model means the callee receives the secrets but can only access the ones it explicitly references. Still, it's worth documenting this in the generated file so downstream teams are aware:
secrets: inherit
# Note: `secrets: inherit` passes all repository secrets to the upstream reusable
# workflow. The upstream workflow only uses the secrets it explicitly references
# (CPLN_TOKEN_STAGING, DOCKER_BUILD_SSH_KEY, etc.), but downstream repos should
# be aware of this trust delegation when adding new secrets.
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
from
25e80af to
27a862e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/github_flow_templates/.github/workflows/cpflow-help-command.yml (1)
26-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
with:andsecrets:blocks for consistency with other wrappers.All other wrapper templates in this PR pass
control_plane_flow_refviawith:and forward secrets viasecrets: inherit. This template is missing both, which may cause issues if the upstream reusable workflow expects these inputs or needs access to secrets.Proposed fix
uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@__CPFLOW_GITHUB_ACTIONS_REF__ + with: + control_plane_flow_ref: __CPFLOW_GITHUB_ACTIONS_REF__ + secrets: inherit🤖 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 `@lib/github_flow_templates/.github/workflows/cpflow-help-command.yml` around lines 26 - 27, The reusable-workflow invocation that currently only has the uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@__CPFLOW_GITHUB_ACTIONS_REF__ needs to be updated to include a with: block passing control_plane_flow_ref (same input used by other wrappers) and a secrets: inherit block so secrets are forwarded to the upstream workflow; locate the line with the uses: value and add the with key supplying control_plane_flow_ref (using the same var/expr pattern as other templates) and add secrets: inherit beneath it.
🧹 Nitpick comments (1)
spec/core/controlplane_api_direct_spec.rb (1)
60-68: ⚡ Quick winAdd explicit carriage-return rejection coverage.
The implementation rejects both
\nand\r, but this spec only locks in\n. Please add a\rcase so the full guard remains protected.Proposed spec addition
it "rejects tokens containing newlines" do stub_env("CPLN_TOKEN", "token\nsecond-line") message = "Unknown API token format. " \ "Please re-run 'cpln profile login' or set the correct CPLN_TOKEN env variable." expect do described_instance.api_token end.to raise_error(RuntimeError, message) end + + it "rejects tokens containing carriage returns" do + stub_env("CPLN_TOKEN", "token\rsecond-line") + + message = "Unknown API token format. " \ + "Please re-run 'cpln profile login' or set the correct CPLN_TOKEN env variable." + expect do + described_instance.api_token + end.to raise_error(RuntimeError, message) + end🤖 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 `@spec/core/controlplane_api_direct_spec.rb` around lines 60 - 68, Add a second test case that mirrors the existing newline rejection spec but uses a carriage-return character: call stub_env("CPLN_TOKEN", "token\rsecond-line") and assert that described_instance.api_token raises the same RuntimeError with the same message; you can either duplicate the existing it "rejects tokens containing newlines" as a new it "rejects tokens containing carriage returns" or extend the current spec to include both "\n" and "\r" cases while still referencing described_instance.api_token and the same expected message.
🤖 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.
Outside diff comments:
In `@lib/github_flow_templates/.github/workflows/cpflow-help-command.yml`:
- Around line 26-27: The reusable-workflow invocation that currently only has
the uses:
shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@__CPFLOW_GITHUB_ACTIONS_REF__
needs to be updated to include a with: block passing control_plane_flow_ref
(same input used by other wrappers) and a secrets: inherit block so secrets are
forwarded to the upstream workflow; locate the line with the uses: value and add
the with key supplying control_plane_flow_ref (using the same var/expr pattern
as other templates) and add secrets: inherit beneath it.
---
Nitpick comments:
In `@spec/core/controlplane_api_direct_spec.rb`:
- Around line 60-68: Add a second test case that mirrors the existing newline
rejection spec but uses a carriage-return character: call stub_env("CPLN_TOKEN",
"token\rsecond-line") and assert that described_instance.api_token raises the
same RuntimeError with the same message; you can either duplicate the existing
it "rejects tokens containing newlines" as a new it "rejects tokens containing
carriage returns" or extend the current spec to include both "\n" and "\r" cases
while still referencing described_instance.api_token and the same expected
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 430468af-bf96-4ff0-8919-e2addda8d064
📒 Files selected for processing (30)
.github/actions/cpflow-build-docker-image/action.yml.github/actions/cpflow-delete-control-plane-app/action.yml.github/actions/cpflow-delete-control-plane-app/delete-app.sh.github/actions/cpflow-detect-release-phase/action.yml.github/actions/cpflow-setup-environment/action.yml.github/actions/cpflow-validate-config/action.yml.github/actions/cpflow-wait-for-health/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-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.ymldocs/ai-github-flow-prompt.mddocs/ci-automation.mddocs/commands.mdlib/command/generate_github_actions.rblib/core/controlplane.rblib/core/controlplane_api_direct.rblib/github_flow_templates/.github/workflows/cpflow-cleanup-stale-review-apps.ymllib/github_flow_templates/.github/workflows/cpflow-delete-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-staging.ymllib/github_flow_templates/.github/workflows/cpflow-help-command.ymllib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.ymllib/github_flow_templates/.github/workflows/cpflow-review-app-help.ymlspec/command/generate_github_actions_spec.rbspec/core/controlplane_api_direct_spec.rbspec/core/controlplane_spec.rb
✅ Files skipped from review due to trivial changes (1)
- docs/ci-automation.md
Code Review: PR 305OverviewWell-designed architectural shift. Moving deploy/delete/promote/cleanup logic into upstream reusable workflows eliminates 1,400+ lines of duplicated code from every downstream repo and centralizes future fixes. The structural decisions (thin wrappers, workflow_call, secrets: inherit, control_plane_flow_ref input) are all sound. Strengths
Issues and SuggestionsMedium: verify list_orgs nil semantics (inline comment on controlplane.rb:479) Returning Low: CPFLOW_SOURCE_DIR path relies on implicit depth (inline comment on action.yml:61) The Low: secrets: inherit scope (inline comment on deploy-review-app wrapper:35)
Nit: pull_request_target security rationale removed from wrapper (inline comment on delete-review-app:22) The explanation for why Nit: contents: write elevation in promote wrapper (inline comment on promote wrapper:12) The wrapper grants Testing GapThe PR notes the full RSpec suite was aborted. The SummaryArchitecturally sound, security posture well-maintained. The five items above are mostly documentation/defensive nits. The one substantive question before merge is confirming |
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
2 times, most recently
from
432eb65 to
7575e7b
Compare
Code ReviewOverviewThis PR is a well-structured architectural refactor. Moving all deploy/delete/staging/promotion logic upstream into The Ruby API hardening (ForbiddenError, Bearer prefix, token validation, safe SecurityToken validation loosening ( ForbiddenError in
ArchitectureRedundant
Bugs / Minor IssuesTemp gem file not cleaned up (
TestsSpec updates are thorough. The new SummarySolid refactor with correct security handling throughout. Three items worth addressing before merge:
|
| cd "${cpflow_source_dir}" | ||
| gem build cpflow.gemspec --output "${cpflow_gem}" | ||
| ) | ||
| gem install "${cpflow_gem}" --no-document |
There was a problem hiding this comment.
The temp file created by mktemp is never removed. On GitHub-hosted runners this is harmless (the VM is discarded), but on self-hosted runners it leaks a gem file per workflow run. Add a trap to clean up on exit:
| gem install "${cpflow_gem}" --no-document | |
| trap 'rm -f "${cpflow_gem}"' EXIT | |
| gem install "${cpflow_gem}" --no-document |
| comment_id: commentId, | ||
| body | ||
| }); | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-delete-review-app.yml@__CPFLOW_GITHUB_ACTIONS_REF__ |
There was a problem hiding this comment.
The if: guard duplicates the condition from the upstream reusable workflow. This is intentional — it prevents a billable workflow_call invocation when the event doesn't match — but it means both copies must stay in sync when trigger conditions change. A short comment here would help future editors understand the pattern:
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-delete-review-app.yml@__CPFLOW_GITHUB_ACTIONS_REF__ | |
| # This `if:` mirrors the upstream workflow's own job guard. | |
| # Its purpose is to short-circuit the workflow_call and avoid | |
| # burning billable minutes when the event doesn't match. | |
| # Keep this condition in sync with cpflow-delete-review-app.yml@upstream. | |
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-delete-review-app.yml@__CPFLOW_GITHUB_ACTIONS_REF__ |
| org = ControlplaneApiDirect.parse_org(url) | ||
| message = | ||
| if org | ||
| "Double check your org #{org}. #{response} #{response.body}" |
There was a problem hiding this comment.
The error message includes the raw response.body, which may contain sensitive CP API error payloads (internal identifiers, partial token context, etc.). Consider omitting or truncating the body, or at minimum stripping it from the message surfaced to users:
| "Double check your org #{org}. #{response} #{response.body}" | |
| message = | |
| if org | |
| "Double check your org #{org}. #{response}" | |
| else | |
| "Control Plane API request to #{url} was forbidden. #{response}" | |
| end |
| # expression literal cannot be broken by the generated branch name. | ||
| "${{ vars.STAGING_APP_BRANCH || '#{staging_branch}' }}" | ||
| def cpflow_github_actions_ref | ||
| ENV.fetch("CPFLOW_GITHUB_ACTIONS_REF", "main") |
There was a problem hiding this comment.
Defaulting to main means every downstream repo that omits CPFLOW_GITHUB_ACTIONS_REF always fetches the tip of main at CI time. A breaking upstream change silently breaks all callers. Consider using "v#{Cpflow::VERSION}" as the default so generated wrappers pin to the released version they were generated with, and let users opt in to tracking main.
| cd "${cpflow_source_dir}" | ||
| gem build cpflow.gemspec --output "${cpflow_gem}" | ||
| ) | ||
| gem install "${cpflow_gem}" --no-document |
There was a problem hiding this comment.
The temp .gem file is installed but never deleted. Since runners are ephemeral this does not leak, but adding rm -f "${cpflow_gem}" here keeps /tmp tidy and avoids confusion when inspecting failed runs.
| items = result.fetch("items", []) | ||
| items.any? { |item| item["name"] == org } | ||
| rescue ControlplaneApiDirect::ForbiddenError => e | ||
| raise unless e.url == "/org" |
There was a problem hiding this comment.
The string "/org" is tightly coupled to ControlplaneApi#list_orgs which calls api_json("/org", ...). If that endpoint path ever changes, this rescue clause silently stops matching and callers see the raw 403 message instead. Consider extracting LIST_ORGS_PATH = "/org" as a constant shared between the API layer and this check, or at minimum add an inline comment linking the two.
| # Don't cancel an in-flight promotion: a half-finished `cpflow deploy-image` plus a | ||
| # rollback can leave production in a worse state than letting the first run finish. | ||
| cancel-in-progress: false | ||
| contents: write |
There was a problem hiding this comment.
This contents: write grant is at the workflow level (not the job level) because GitHub's reusable-workflow permission model requires the caller to grant at least as much permission as the called workflow's most permissive job. The create-github-release job inside the upstream reusable workflow needs contents: write, so this wrapper cannot scope it to a single job. A comment here would help future maintainers avoid accidentally narrowing it back to read.
Code Review: Move generated GitHub Actions logic upstreamOverviewThis is a well-structured refactor that moves the heavy workflow logic from generated downstream files into upstream reusable workflows and composite actions in this repository. The thin-wrapper pattern is sound: downstream repos now contain only event triggers, permissions, and a single Positives
Issues to address1. Default
2.
3. Tempfile not cleaned up after gem install In 4. Duplicate The downstream templates ( 5.
6. Full test suite not confirmed green The PR description notes the full RSpec suite was stopped after it hung. Before merging, it would be worth confirming no regressions in SummaryThe architecture is solid and the security posture (env-routing, |
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
from
7575e7b to
547060f
Compare
Code ReviewOverviewThis PR moves the heavy GitHub Actions deployment/cleanup/promote logic from generated downstream files into reusable upstream workflows in this repo, producing thin wrapper workflows that delegate to Bug: cleanup-stale-apps is missing the consumer repo checkout
The staging and promote workflows correctly check out the caller's repo with a bare Concern: secrets: inherit with mutable version tagsAll generated wrapper templates use Minor: ruby/setup-ruby@v1 is a floating tagIn Positive: Ruby API hardening
TestingTest coverage for the generator is thorough and specs correctly validate both the generated wrappers and the upstream shared workflows. Note that the full RSpec suite was not completed per the PR description; a full run before merge would be prudent. |
| cpln_cli_version: ${{ vars.CPLN_CLI_VERSION }} | ||
| cpflow_version: ${{ vars.CPFLOW_VERSION }} | ||
|
|
||
| - name: Remove stale review apps |
There was a problem hiding this comment.
Bug: consumer repo is never checked out.
cpflow cleanup-stale-apps calls into lib/command/cleanup_stale_apps.rb, which reads config[:stale_app_image_deployed_days] and match_if_app_name_starts_with from .controlplane/controlplane.yml. Without that file on disk the command fails at config load time.
The staging and promote reusable workflows both have a bare actions/checkout step (no repository:) that, in a workflow_call context, checks out the caller's repository. The cleanup workflow should do the same. Suggested addition before the "Remove stale review apps" step:
- name: Checkout caller repository
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
with:
persist-credentials: falseWithout this, every scheduled cleanup run will fail silently with a config-not-found error.
There was a problem hiding this comment.
Confirmed. cleanup-stale-apps loads .controlplane/controlplane.yml, so the cleanup reusable workflow needs a caller-repo checkout before Remove stale review apps. I’ll add a pinned caller checkout there while keeping the .cpflow checkout for shared actions.
| # reusable workflows pin GitHub-owned actions to immutable SHAs. | ||
| steps: | ||
| - name: Set up Ruby | ||
| uses: ruby/setup-ruby@v1 |
There was a problem hiding this comment.
ruby/setup-ruby@v1 is a floating major tag, unlike every other third-party action in the privileged workflows which are pinned to immutable SHAs. The comment on line 42 notes this is intentional, but the previous comment that explained the full SHA-pinning rationale was removed in this PR. Consider adding a one-liner here to explain the exemption, e.g.:
# ruby/setup-ruby is exempt from SHA pinning: major-tag tracking is
# intentional so Ruby and toolcache updates roll out automatically.
# It is not a GitHub-owned action, so Dependabot/Renovate can manage it.
uses: ruby/setup-ruby@v1This makes the policy legible for future reviewers without relying on the git history.
There was a problem hiding this comment.
Confirmed. The workflow-level comment captures the policy, but the exemption is clearer if it sits directly above ruby/setup-ruby@v1. I’ll add an inline note that major-tag tracking is intentional for Ruby/toolcache updates.
| control_plane_flow_ref: __CPFLOW_GITHUB_ACTIONS_REF__ | ||
| # Delegates repository secrets to the trusted upstream cpflow workflow; it | ||
| # only references the named secrets it needs. | ||
| secrets: inherit |
There was a problem hiding this comment.
secrets: inherit delegates all of the downstream repo's secrets to the upstream reusable workflow. Combined with a mutable version tag (@v1.2.3 can be force-moved), this creates a supply-chain trust dependency: if this repo's tag infrastructure is ever compromised, every downstream secret is exposed.
This is an accepted tradeoff for usability (many popular Actions do the same), but the comment above only describes the functional purpose. Consider adding an explicit note about the trust boundary, e.g.:
# secrets: inherit passes all downstream repository secrets to the upstream
# reusable workflow. The upstream workflow only reads the named secrets it
# declares, but `inherit` cannot enforce that boundary at the GitHub level.
# Users with strict supply-chain requirements should set CPFLOW_GITHUB_ACTIONS_REF
# to an immutable commit SHA instead of a version tag.
secrets: inheritThere was a problem hiding this comment.
Confirmed. The current generated-wrapper comment explains which secrets the upstream workflow reads, but it does not state the GitHub-level trust boundary. I’ll expand it to call out that secrets: inherit grants all caller secrets to the trusted reusable workflow and that strict consumers can pin CPFLOW_GITHUB_ACTIONS_REF to an immutable SHA.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/workflows/cpflow-deploy-staging.yml:
- Around line 102-109: The Setup environment step uses the local action
./.cpflow/.github/actions/cpflow-setup-environment but doesn't set
working_directory, so Ruby/toolchain discovery may pick the repo root; add
working_directory: .cpflow to the step that has name: Setup environment and
uses: ./.cpflow/.github/actions/cpflow-setup-environment (apply the same change
to the other identical job near the other occurrence mentioned) so the action
runs from the checked-out .cpflow directory.
In @.github/workflows/cpflow-promote-staging-to-production.yml:
- Around line 98-126: The workflow resolves app_workloads into the variable
workloads but does not enforce a single effective primary workload or validate a
provided PRIMARY_WORKLOAD; update the Ruby block (where PRODUCTION_APP_NAME,
app_config, and app_workloads are used and the workloads variable is produced)
to: 1) parse app_config["app_workloads"] into an ordered list, 2) if the env
PRIMARY_WORKLOAD is set, validate it is present in that list and fail fast with
a clear error if not, 3) if PRIMARY_WORKLOAD is blank, select a single primary
workload deterministically (e.g., the only entry or fail if multiple to force
explicit selection), and 4) emit that single primary workload to GITHUB_OUTPUT
(e.g., names or primary_workload) so subsequent steps use the resolved output
instead of falling back to `${{ env.PRIMARY_WORKLOAD || 'rails' }}`; reference
the workloads-producing code and the echo "names=${workloads}" >>
"$GITHUB_OUTPUT" line when making the change.
In `@lib/command/generate_github_actions.rb`:
- Around line 65-68: The cpflow_github_actions_ref method currently only strips
edges allowing internal whitespace like "feature branch" which later breaks the
generated uses: ...@ref; update cpflow_github_actions_ref to validate the
resolved ref (ENV.fetch("CPFLOW_GITHUB_ACTIONS_REF",
default_cpflow_github_actions_ref).to_s) and if it is blank or contains any
whitespace characters (e.g. ref.match?(/\s/)) raise a clear ArgumentError or
logging+raise indicating an invalid CPFLOW_GITHUB_ACTIONS_REF, otherwise return
the validated ref (falling back to default_cpflow_github_actions_ref when
appropriate).
🪄 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: e5f1e2cc-d42b-4556-9ad5-0f51b83bafa8
📒 Files selected for processing (31)
.github/actions/cpflow-build-docker-image/action.yml.github/actions/cpflow-delete-control-plane-app/action.yml.github/actions/cpflow-delete-control-plane-app/delete-app.sh.github/actions/cpflow-detect-release-phase/action.yml.github/actions/cpflow-setup-environment/action.yml.github/actions/cpflow-validate-config/action.yml.github/actions/cpflow-wait-for-health/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-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.ymldocs/ai-github-flow-prompt.mddocs/ci-automation.mddocs/commands.mdlib/command/generate_github_actions.rblib/core/controlplane.rblib/core/controlplane_api.rblib/core/controlplane_api_direct.rblib/github_flow_templates/.github/workflows/cpflow-cleanup-stale-review-apps.ymllib/github_flow_templates/.github/workflows/cpflow-delete-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.ymllib/github_flow_templates/.github/workflows/cpflow-deploy-staging.ymllib/github_flow_templates/.github/workflows/cpflow-help-command.ymllib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.ymllib/github_flow_templates/.github/workflows/cpflow-review-app-help.ymlspec/command/generate_github_actions_spec.rbspec/core/controlplane_api_direct_spec.rbspec/core/controlplane_spec.rb
✅ Files skipped from review due to trivial changes (2)
- docs/commands.md
- docs/ci-automation.md
justin808
force-pushed
the
jg-codex/reusable-github-actions
branch
from
547060f to
2db67ab
Compare
|
Code Review: PR 305 - Move Generated GitHub Actions Logic Upstream Overview: This PR moves heavy GitHub Actions deployment logic from generated downstream repos into upstream reusable workflows. Generated repos get thin wrappers delegating to reusable workflows via secrets: inherit. The architecture eliminates duplicated workflow code across all consumer repos. The Ruby API-hardening changes (scoped token support, ForbiddenError, Bearer normalization) are solid and well-tested. Security - secrets: inherit trust boundary (medium risk, acknowledged) All six wrapper templates pass secrets: inherit to upstream reusable workflows. GitHub does not enforce that only referenced secrets are forwarded; the entire caller secret store is passed. Two concerns: (1) The default ref is v which is a mutable tag; republishing that tag could redirect all consumers to different code. SHA-pinning guidance in the generated comment is the right advice. (2) The promote-to-production template grants contents: write to the caller workflow. Combined with secrets: inherit, a compromised upstream ref gets write access to the caller repo AND all its secrets. The generated comment should explicitly note that contents: write is required specifically for the create-github-release job. Security - ForbiddenError no longer leaks response body (good) Previously response.body was included in the raised message. The new ForbiddenError only interpolates response.to_s (the HTTP status line), never response.body. CPln 403 bodies sometimes contain internal tracking IDs, so this is the right call. Security - Token validation relaxed correctly Removing API_TOKEN_REGEX and accepting any non-empty token without line breaks correctly supports scoped service account tokens with special characters. Format validation is correctly deferred to the CP API. Potential Issue 1 - Manual ref-sync in generated wrappers Every generated wrapper has both the uses: ref and control_plane_flow_ref set to the same value, and both must stay in sync manually. If a downstream user updates one but not the other, they silently run the wrong composite action version. The comment warns about this but does not enforce it. Consider having the upstream workflow echo both refs so a mismatch is immediately visible in the run log. Potential Issue 2 - cpflow cleanup-stale-apps has no explicit working-directory In .github/workflows/cpflow-cleanup-stale-review-apps.yml, the caller repo is checked out with no path:, landing at GITHUB_WORKSPACE. The cpflow step finds .controlplane/ correctly. However, without an explicit working-directory: directive, if a future commit adds path: app to that checkout step (mirroring the review-app workflow), the cpflow step would silently break. Potential Issue 3 - staging_branch_default relies on empty-string falsy behaviour When both vars.STAGING_APP_BRANCH and inputs.staging_app_branch_default are empty, the || expression evaluates to empty string. The downstream shell check correctly falls through to main/master. This works but a brief comment would make the intent clearer for future maintainers. Code Quality parse_org nil-safety is a good defensive fix -- the old code raised NoMethodError on /org (the list-orgs path itself), exactly the URL now handled in org_exists?. The is_a?(Hash) and payload exp guard with .to_i handles non-JWT service account tokens that decode without an exp claim. The authorization_header case-insensitive Bearer prefix check is correct. The cpflow_github_actions_ref whitespace guard aborts before writing invalid YAML refs. The ForbiddenError URL-equality check in org_exists? avoids fragile message-text matching. The new primary workload resolution in the promote workflow (explicit error when ambiguous rather than silently falling back to first_image) is a meaningful correctness improvement. Tests Generator specs are well-updated to cover the wrapper/reusable split. ForbiddenError, authorization_header, and token-validation tests are complete and correctly scoped. The acknowledgment that the full RSpec suite was not run to completion is notable -- the promote and cleanup workflows have non-trivial shell logic that benefits from integration coverage. |
| when Net::HTTPForbidden | ||
| org = self.class.parse_org(url) | ||
| raise("Double check your org #{org}. #{response} #{response.body}") | ||
| raise ForbiddenError.new(url: url, response: response) |
There was a problem hiding this comment.
Good: response here is the Net::HTTPForbidden object, so string interpolation calls response.to_s which produces the status line (e.g. "403 Forbidden"), not response.body. This deliberately prevents raw 403 bodies -- which can contain internal CPln tracking IDs -- from appearing in logs or error messages. The test in controlplane_api_direct_spec.rb verifies this behaviour explicitly.
| items = result.fetch("items", []) | ||
| items.any? { |item| item["name"] == org } | ||
| rescue ControlplaneApiDirect::ForbiddenError => e | ||
| raise unless e.url == ControlplaneApi::LIST_ORGS_PATH |
There was a problem hiding this comment.
This URL-equality check (e.url == LIST_ORGS_PATH) is the right approach -- it re-raises any ForbiddenError from other endpoints (e.g. a GVC or workload API call) while silently treating a 403 on the list-orgs endpoint as "token is scoped, proceed". Using the URL rather than matching on message text means this won't silently swallow unrelated 403s if error messages ever change.
| gem install cpflow -v "${CPFLOW_VERSION}" --no-document | ||
| else | ||
| cpflow_source_dir="$(cd "${CPFLOW_SOURCE_DIR}" && pwd)" | ||
| if [[ ! -f "${cpflow_source_dir}/cpflow.gemspec" ]]; then |
There was a problem hiding this comment.
This path traversal is correct given the action's location at .github/actions/cpflow-setup-environment/ -- navigating up three levels reaches the repo root, where cpflow.gemspec lives. The cpflow.gemspec existence check on line 102 provides the only validation of this path. Worth noting: this path is evaluated at runner time from github.action_path, so it's stable across runner OS versions. If this action were ever moved to a different nesting depth, this ../../.. would need to be updated.
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| cpflow cleanup-stale-apps -a "${REVIEW_APP_PREFIX}" --org "${CPLN_ORG_STAGING}" --yes |
There was a problem hiding this comment.
The caller repo checkout above (line 58) has no path:, so it lands at $GITHUB_WORKSPACE and cpflow correctly finds .controlplane/controlplane.yml. However, this step has no working-directory: directive. If a future change adds path: app to the checkout step (to mirror the review-app workflow), this step would silently run from the wrong directory and fail to find the app config. Suggest adding working-directory: ${{ github.workspace }} to make the intent explicit.
| env: | ||
| APP_NAME: ${{ vars.STAGING_APP_NAME }} | ||
| CPLN_ORG: ${{ vars.CPLN_ORG_STAGING }} | ||
| STAGING_APP_BRANCH: ${{ vars.STAGING_APP_BRANCH || inputs.staging_app_branch_default }} |
There was a problem hiding this comment.
When both vars.STAGING_APP_BRANCH and inputs.staging_app_branch_default are empty/unset, this expression evaluates to "". The downstream if [[ -n "${STAGING_APP_BRANCH}" ]] shell check correctly falls through to the main/master branch logic. This works because GitHub Actions treats empty strings as falsy in || expressions -- a brief inline comment on this line would make the intent clearer for future maintainers.
| cancel-in-progress: false | ||
| # The upstream reusable workflow's create-github-release job needs | ||
| # contents: write, and callers must grant the union of callee permissions. | ||
| contents: write |
There was a problem hiding this comment.
The comment is correct but could be more explicit about the risk. This is the only generated wrapper that requires contents: write. Combined with secrets: inherit, it means a compromised or misconfigured upstream ref (e.g. if v<VERSION> tag is republished) would have write access to both the caller repo's git contents and its entire secret store. Consider adding a note that this elevated permission scope is solely required for gh release create in the upstream create-github-release job, so downstream operators know exactly which upstream job drives this requirement.
1 participant
Summary
Moves the heavy generated GitHub Actions logic upstream into reusable workflows and shared composite actions.
Generated downstream repos now get thin wrappers that keep only:
shakacode/control-plane-flow.github/cpflow-help.mdThe deploy/delete/staging/promote/comment-formatting logic now lives in upstream
.github/workflows/cpflow-*.yml, and shared composite actions live in upstream.github/actions/cpflow-*.Also keeps the composite-action description fix from #297 in the upstream shared setup action so
vars.*examples are not embedded in action metadata descriptions.Testing
CPLN_ORG=dummy-test-org bundle exec rspec spec/command/generate_github_actions_spec.rbRUBOCOP_CACHE_ROOT=/private/tmp/rubocop-cache bundle exec rubocopactionlint -ignore 'SC2129' .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-help-command.yml .github/workflows/cpflow-promote-staging-to-production.yml .github/workflows/cpflow-review-app-help.yml lib/github_flow_templates/.github/workflows/cpflow-cleanup-stale-review-apps.yml lib/github_flow_templates/.github/workflows/cpflow-delete-review-app.yml lib/github_flow_templates/.github/workflows/cpflow-deploy-review-app.yml lib/github_flow_templates/.github/workflows/cpflow-deploy-staging.yml lib/github_flow_templates/.github/workflows/cpflow-help-command.yml lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml lib/github_flow_templates/.github/workflows/cpflow-review-app-help.ymlCPFLOW_GITHUB_ACTIONS_REF=jg-codex/reusable-github-actionsand verified only wrapper workflows plus.github/cpflow-help.mdare emittedI also started the full RSpec suite, but stopped it after it hung without output beyond the initial examples. The focused generator coverage above is green.
Note
Medium Risk
Medium risk because it rewires deployment/cleanup GitHub Actions to call upstream reusable workflows (with
secrets: inherit) and changes Control Plane API auth/error handling, which could affect CI deployments and API access behavior.Overview
Shifts the GitHub Actions generation model to upstream reusable workflows. Generated downstream repos now emit thin
cpflow-*workflow wrappers thatusesshakacode/control-plane-flow/.github/workflows/*@<ref>withsecrets: inherit, instead of embedding the full deploy/delete/staging/promote logic locally.Adds a new shared setup composite action and updates the generator. The new
.github/actions/cpflow-setup-environmentinstallscplnpluscpflow(either from RubyGems by version or built from the checked-out source), supports aworking_directory, and persistsCPLN_TOKENviaGITHUB_ENV;generate-github-actionsnow substitutes__CPFLOW_GITHUB_ACTIONS_REF__(configurable viaCPFLOW_GITHUB_ACTIONS_REF) and astaging_app_branch_defaultinstead of embedding expressions.Hardens Control Plane API interactions for scoped tokens. API calls now normalize Authorization to
Bearer ..., relax token-format validation (rejecting only newlines), introduceForbiddenErrorwithout leaking response bodies, and makeorg_exists?toleratenil/403 fromlist_orgsso scoped tokens can proceed.Docs and specs are updated accordingly (new wrapper behavior, SHA-pinned GitHub-owned actions, and composite-action metadata descriptions avoiding
${{ ... }}parsing pitfalls).Reviewed by Cursor Bugbot for commit 2db67ab. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests