Document cpflow workflow testing#737
Conversation
Review app commands
For setup details, comment |
|
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 (5)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds a new testing guide for cpflow-generated GitHub Actions, updates related docs to reference the guide and clarify default-branch sourcing of trusted composite actions, and introduces a Changescpflow GitHub Actions Testing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review: Document cpflow workflow testingOverviewDocumentation-only PR that adds a testing guide for cpflow GitHub Actions changes and updates three existing docs to clarify the trusted-default-branch checkout behavior. The content is accurate and fills a real gap — the "PR-branch composite actions are not exercised until merge" gotcha is non-obvious and worth documenting. Strengths
IssuesInconsistent
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| actionlint .github/workflows/cpflow-*.yml | ||
| bundle exec rubocop | ||
| bin/conductor-exec bundle exec rubocop | ||
| ``` |
There was a problem hiding this comment.
The new testing-cpflow-github-actions.md guide runs actionlint -ignore 'SC2129', but this line still omits that flag. The two docs will produce different output for the same workflow files, which is confusing for anyone switching between them.
| ``` | |
| actionlint -ignore 'SC2129' .github/workflows/cpflow-*.yml |
| from `master` until the fix is merged there. | ||
|
|
||
| ## Local Checks |
There was a problem hiding this comment.
The preamble introduces the unreleased-checkout case, but every command in the block below uses a hard-coded path — there is no hint for readers using a normally-installed gem. Consider clarifying:
| from `master` until the fix is merged there. | |
| ## Local Checks | |
| After regenerating the flow, run these checks from the repository root. If | |
| `cpflow` is installed as a gem, use `bin/conductor-exec cpflow <subcommand>` | |
| directly. When testing an unreleased upstream `control-plane-flow` checkout, | |
| replace `cpflow` with `ruby /path/to/control-plane-flow/bin/cpflow`: |
Greptile SummaryThis PR adds a focused testing guide for cpflow GitHub Actions changes and propagates the "trusted local actions load from
Confidence Score: 4/5Safe to merge — all changes are documentation only, with no impact on production code or workflow logic. The readme.md actionlint command is missing the -ignore SC2129 flag that the new guide introduces, so following the two docs in parallel produces inconsistent results. The same file's pre-merge wording still reads as a complete validation step while the new guide and cpflow-help.md both qualify it as a partial smoke test. .controlplane/readme.md — two minor inconsistencies with the new guide worth a quick pass before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Local as Local Checks
participant GH as GitHub Actions
participant Master as master branch
Dev->>Local: bin/conductor-exec cpflow generate-github-actions
Dev->>Local: actionlint -ignore SC2129
Dev->>Local: Ruby YAML + metadata-description checks
Local-->>Dev: Pass / Fail
Dev->>GH: Open PR CI runs
GH-->>Dev: Syntax and CI validation (partial)
Dev->>GH: gh workflow run --ref PR-branch (workflow_dispatch)
GH->>Master: Checkout trusted local composite actions
Master-->>GH: ".github/actions/* from master"
GH-->>Dev: Partial smoke test (top-level workflow only)
Note over Dev,Master: .github/actions/* changes NOT fully tested until merged
Dev->>Master: Merge PR
Dev->>GH: Rerun review-app deploy on canary PR
GH->>Master: Checkout trusted local composite actions (merge commit)
GH-->>Dev: Full end-to-end validation
|
| testing an unreleased upstream `control-plane-flow` checkout, invoke that | ||
| checkout's `bin/cpflow` directly: | ||
|
|
||
| ```sh |
There was a problem hiding this comment.
This one-liner is ~200 characters and will wrap unpredictably in rendered Markdown, making it easy to mis-copy (a truncated command silently does nothing or raises a syntax error). The "Ways To Make This Easier" section already suggests extracting these checks into bin/test-cpflow-github-flow. Moving this command there now — even as a thin wrapper — would be more robust and reduce copy-paste risk for future readers.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| bundle exec ruby /path/to/control-plane-flow/bin/cpflow github-flow-readiness | ||
| bin/conductor-exec ruby /path/to/control-plane-flow/bin/cpflow generate-github-actions --staging-branch master | ||
| bin/conductor-exec ruby /path/to/control-plane-flow/bin/cpflow github-flow-readiness | ||
| actionlint .github/workflows/cpflow-*.yml |
There was a problem hiding this comment.
actionlint command in readme.md omits the -ignore 'SC2129' flag that the new testing-cpflow-github-actions.md guide adds. Anyone following readme.md will get SC2129 warnings that the new guide explicitly silences, causing a confusing inconsistency between the two docs.
| actionlint .github/workflows/cpflow-*.yml | |
| actionlint -ignore 'SC2129' .github/workflows/cpflow-*.yml |
![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: 117d7052b8
ℹ️ 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".
| bin/conductor-exec ruby /path/to/control-plane-flow/bin/cpflow generate-github-actions --staging-branch master | ||
| bin/conductor-exec ruby /path/to/control-plane-flow/bin/cpflow github-flow-readiness | ||
| bin/conductor-exec ruby -e 'require "yaml"; Dir[".github/actions/**/action.yml", ".github/workflows/*.yml"].sort.each { |path| YAML.load_file(path, aliases: true); puts "parsed #{path}" }' | ||
| bin/conductor-exec ruby -e 'require "yaml"; bad=[]; Dir[".github/actions/**/action.yml"].sort.each { |path| doc=YAML.load_file(path, aliases: true); doc.fetch("inputs", {}).each { |name, spec| bad << "#{path}:#{name}" if spec["description"].to_s.include?("${{") } }; }; abort bad.join("\n") unless bad.empty?; puts "no action metadata descriptions contain GitHub expressions"' |
There was a problem hiding this comment.
The Ruby command in this checklist has an extra }; before abort, so running it as documented raises a SyntaxError instead of validating action metadata descriptions. Anyone following the new local test steps will fail at this point and may skip the intended guard entirely; removing the extra block terminator makes the command executable.
Useful? React with 👍 / 👎.
|
+review-app-deploy |
Review app ready |
justin808
force-pushed
the
jg-codex/cpflow-testing-docs
branch
from
117d705 to
e4302c5
Compare
| abort bad.join("\n") unless bad.empty? | ||
| puts "no action metadata descriptions contain GitHub expressions" | ||
| RUBY | ||
|
|
There was a problem hiding this comment.
If no cpflow-*.yml files exist (e.g. in a fresh checkout or on a branch before generation), bash passes the literal glob string cpflow-*.yml to actionlint with set -euo pipefail active, causing a confusing "no such file" failure rather than a clean "nothing to lint" exit.
Consider guarding with shopt -s nullglob or an explicit check:
| shopt -s nullglob | |
| workflow_files=(.github/workflows/cpflow-*.yml) | |
| if [[ ${#workflow_files[@]} -eq 0 ]]; then | |
| echo "no cpflow workflow files found, skipping actionlint" | |
| else | |
| actionlint -ignore "SC2129" "${workflow_files[@]}" | |
| fi |
| puts "no action metadata descriptions contain GitHub expressions" | ||
| RUBY | ||
|
|
There was a problem hiding this comment.
No check that actionlint is installed before invoking it. A missing binary produces command not found with no hint about where to get it. A quick guard would give a clearer developer experience:
| puts "no action metadata descriptions contain GitHub expressions" | |
| RUBY | |
| echo "==> actionlint" | |
| if ! command -v actionlint &>/dev/null; then | |
| echo "actionlint not found — install it (https://github.com/rhysd/actionlint#installation) and rerun" >&2 | |
| exit 1 | |
| fi | |
| actionlint -ignore "SC2129" .github/workflows/cpflow-*.yml |
|
|
||
| cpflow_cmd=(cpflow) | ||
| if [[ $# -gt 0 ]]; then | ||
| cpflow_cmd=("$@") |
There was a problem hiding this comment.
When no arguments are supplied the script silently falls back to looking up cpflow in $PATH. If it isn't installed, conductor-exec will fail with a generic "not found" error.
A small guard here mirrors what you're already doing for actionlint (or could do — see other comment) and makes the failure message more actionable:
| cpflow_cmd=(cpflow) | |
| if [[ $# -gt 0 ]]; then | |
| cpflow_cmd=("$@") | |
| cpflow_cmd=(cpflow) | |
| if [[ $# -gt 0 ]]; then | |
| cpflow_cmd=("$@") | |
| elif ! command -v cpflow &>/dev/null; then | |
| echo "cpflow not found in PATH — install it or pass the path as an argument: $0 ruby /path/to/cpflow" >&2 | |
| exit 1 | |
| fi |
|
|
||
| ## Troubleshooting Signals | ||
|
|
||
| ### Composite action metadata fails before setup |
There was a problem hiding this comment.
The APP_NAME: ${REVIEW_APP_PREFIX}-${PR_NUMBER} line is shell-interpolation syntax inside a fenced text block, so it renders literally in markdown — which is exactly what the following paragraph explains. No change needed, just confirming the intent is clear.
Code Review — PR #737: Document cpflow workflow testingOverall: solid documentation PR with a useful local test script. Two minor bugs in What the PR does
Issues
|
![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.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Ruby
fetchreturns nil for nil-valued YAML keys- Updated the action metadata check to iterate over an empty hash for nil inputs and treat nil input specs as empty specs before reading descriptions.
Or push these changes by commenting:
@cursor push 4f71316d0b
Preview (4f71316d0b)
diff --git a/bin/test-cpflow-github-flow b/bin/test-cpflow-github-flow
--- a/bin/test-cpflow-github-flow
+++ b/bin/test-cpflow-github-flow
@@ -29,8 +29,8 @@
bad = []
Dir[".github/actions/**/action.yml"].sort.each do |path|
doc = YAML.load_file(path, aliases: true)
- doc.fetch("inputs", {}).each do |name, spec|
- bad << "#{path}:#{name}" if spec["description"].to_s.include?("${{")
+ (doc["inputs"] || {}).each do |name, spec|
+ bad << "#{path}:#{name}" if (spec || {})["description"].to_s.include?("${{")
end
endYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit e4302c5. Configure here.
| Dir[".github/actions/**/action.yml"].sort.each do |path| | ||
| doc = YAML.load_file(path, aliases: true) | ||
| doc.fetch("inputs", {}).each do |name, spec| | ||
| bad << "#{path}:#{name}" if spec["description"].to_s.include?("${{") |
There was a problem hiding this comment.
Ruby fetch returns nil for nil-valued YAML keys
Low Severity
doc.fetch("inputs", {}) only falls back to {} when the "inputs" key is entirely absent. If an action YAML file contains inputs: with no sub-keys, the key exists but maps to nil, so fetch returns nil and nil.each raises a NoMethodError. Similarly, if an individual input spec is nil (e.g. token: with no properties), spec["description"] crashes on nil. Using (doc["inputs"] || {}) and guarding spec would prevent both crashes.
Reviewed by Cursor Bugbot for commit e4302c5. Configure here.
|
✅ Review app for PR #737 is deleted |
1 participant
Adds a focused guide for testing generated cpflow GitHub Actions changes, including the default-branch trusted-actions gotcha we hit while validating PR 734.
Also updates the existing Control Plane docs and generated review-app help text so
workflow_dispatch --refis described as a partial smoke test: it loads the workflow file from the ref, but the deploy workflow still checks out trusted local composite actions frommasterbefore using secrets.Included guidance:
CPLN_TOKEN_STAGINGValidation:
git diff --checkNote
Low Risk
Low risk: changes are documentation plus a new local test script; no production code paths or CI workflows are modified.
Overview
Adds a dedicated guide for validating and troubleshooting changes to generated
cpflow-*GitHub Actions, emphasizing that review-app deploys load trusted composite actions from the default branch before using secrets.Updates existing Control Plane docs/help text to frame
workflow_dispatch --refruns as a partial smoke test (workflow file from the ref, but local actions still frommaster), and introducesbin/test-cpflow-github-flowto runcpflow github-flow-readiness, parse generated YAML, fail on${{ ... }}in action input descriptions, and runactionlint.Reviewed by Cursor Bugbot for commit e4302c5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores