Document cpflow upstream testing#746
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
WalkthroughAdds a ref-pinning CLI ( Changescpflow Ref Pinning Tooling and Testing Guidance
Sequence Diagram(s)sequenceDiagram
participant CLI as bin/pin-cpflow-github-ref
participant Repo as cpflow-wrapper-file(s)
participant Validator as bin/test-cpflow-github-flow
CLI->>Repo: update `uses: ...@<ref>` and `control_plane_flow_ref: <ref>`
Repo->>Validator: validator loads `cpflow-*.yml`
Validator->>Repo: extract `uses:` `@ref` and `with.control_plane_flow_ref`
Validator->>Validator: compare refs and check secrets input
Validator-->>CLI: report single ref found or abort on mismatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Greptile SummaryThis PR documents how downstream apps can test unreleased
Confidence Score: 4/5Safe to merge; the only gap is that the new ref-consistency check does not fail when different workflow files carry different upstream refs. The new bin/pin-cpflow-github-ref script is well-scoped and safe. The consistency guard in bin/test-cpflow-github-flow correctly catches intra-file ref mismatches, but it only logs — and does not abort on — cross-file divergence. Because the primary stated goal of the guard is to prevent exactly that split-ref situation from silently passing CI, the partial coverage is a real gap in the tooling being introduced. bin/test-cpflow-github-flow — the new ref-consistency check block (lines 41-65) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Developer: upstream PR ready to test] --> B[bin/pin-cpflow-github-ref SHA]
B --> C[Rewrites uses: @ref in cpflow-*.yml]
B --> D[Rewrites control_plane_flow_ref in cpflow-*.yml]
C --> E[bin/test-cpflow-github-flow]
D --> E
E --> F{cpflow github-flow-readiness}
E --> G{Parse YAML}
E --> H{Check action input descriptions}
E --> I{Check ref consistency per-file}
I -->|intra-file mismatch| J[abort]
I -->|cross-file mismatch| K[prints multiple refs — no abort]
I -->|all consistent| L[actionlint]
L --> M[Open downstream PR + comment +review-app-deploy]
M --> N{Review app deploy succeeds?}
N -->|yes| O[Upstream PR merges + releases]
O --> P[Regenerate or repin to release tag]
N -->|no| Q[Debug upstream PR]
Reviews (1): Last reviewed commit: "Document cpflow upstream testing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| echo "==> check cpflow reusable workflow refs" | ||
| bin/conductor-exec ruby <<'RUBY' | ||
| refs = Hash.new { |hash, key| hash[key] = [] } | ||
|
|
||
| Dir[".github/workflows/cpflow-*.yml"].sort.each do |path| | ||
| text = File.read(path) | ||
| uses_refs = text.scan(%r{uses:\s+shakacode/control-plane-flow/\.github/workflows/[^@\s]+@([^\s]+)}).flatten | ||
| input_refs = text.scan(/^\s*control_plane_flow_ref:\s*(\S+)/).flatten | ||
|
|
||
| (uses_refs + input_refs).each { |ref| refs[ref] << path } | ||
|
|
||
| next if uses_refs.empty? && input_refs.empty? | ||
|
|
||
| expected = uses_refs.first | ||
| mismatches = (uses_refs + input_refs).uniq | ||
| abort "#{path}: mismatched cpflow refs: #{mismatches.join(', ')}" if mismatches.length > 1 | ||
| abort "#{path}: missing control_plane_flow_ref for #{expected}" if input_refs.empty? && path !~ /cpflow-(help-command|review-app-help)\.yml\z/ | ||
| end | ||
|
|
||
| if refs.empty? | ||
| puts "no upstream cpflow reusable workflow refs found" | ||
| else | ||
| puts "cpflow refs: #{refs.keys.sort.join(', ')}" | ||
| end | ||
| RUBY |
There was a problem hiding this comment.
The guard aborts when uses: and control_plane_flow_ref disagree within a single file, but if file_a.yml pins sha_a and file_b.yml pins sha_b, the loop passes both without aborting — the only evidence is the printed cpflow refs: sha_a, sha_b output. Any CI job that doesn't inspect stdout carefully would pass silently with mismatched refs across the generated wrappers, which is the exact split-ref scenario the guard is meant to prevent.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| exit 1 | ||
| end | ||
|
|
||
| unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/) |
There was a problem hiding this comment.
The validation regex \A[0-9A-Za-z._\/-]+\z allows forward slashes, so branch names like main or feature/my-branch pass silently. The docs explicitly warn against mutable branch refs, but the script doesn't enforce it.
Consider adding a soft warning (or hard failure) when the ref is neither a full 40-char SHA nor a v-prefixed semver tag:
| unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/) | |
| unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/) | |
| warn "Ref contains unsupported characters: #{ref.inspect}" | |
| exit 1 | |
| end | |
| if ref.include?("/") || (ref !~ /\A[0-9a-f]{40}\z/ && ref !~ /\Av\d/) | |
| warn "Warning: #{ref.inspect} looks like a branch name — use a full commit SHA or release tag instead." | |
| end |
| expected = uses_refs.first | ||
| mismatches = (uses_refs + input_refs).uniq | ||
| abort "#{path}: mismatched cpflow refs: #{mismatches.join(', ')}" if mismatches.length > 1 | ||
| abort "#{path}: missing control_plane_flow_ref for #{expected}" if input_refs.empty? && path !~ /cpflow-(help-command|review-app-help)\.yml\z/ |
There was a problem hiding this comment.
The exempt workflow name list (cpflow-(help-command|review-app-help)) is hardcoded inline. If a new wrapper legitimately omits control_plane_flow_ref, this regex must be updated and is easy to miss. Consider extracting it to a named constant near the top of the heredoc:
EXEMPT_WORKFLOWS = /cpflow-(help-command|review-app-help)\.yml\z/
# ...later...
abort "#{path}: missing control_plane_flow_ref for #{expected}" if input_refs.empty? && path !~ EXEMPT_WORKFLOWS
|
|
||
| next if uses_refs.empty? && input_refs.empty? | ||
|
|
||
| expected = uses_refs.first |
There was a problem hiding this comment.
Logic gap: if uses_refs is empty but input_refs is non-empty (a control_plane_flow_ref value with no corresponding reusable uses: line), the next guard is skipped, expected is nil, and neither abort fires — a dangling control_plane_flow_ref is silently accepted.
Consider adding an explicit guard after the next line:
next if uses_refs.empty? && input_refs.empty?
abort "#{path}: has control_plane_flow_ref but no reusable uses: line" if uses_refs.empty?![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: 7d767699f9
ℹ️ 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".
| next if uses_refs.empty? && input_refs.empty? | ||
|
|
||
| expected = uses_refs.first | ||
| mismatches = (uses_refs + input_refs).uniq | ||
| abort "#{path}: mismatched cpflow refs: #{mismatches.join(', ')}" if mismatches.length > 1 |
There was a problem hiding this comment.
uses: ref per wrapper
The new ref-consistency check only validates mismatches after parsing refs, but it never fails when control_plane_flow_ref exists and the cpflow uses: line is missing or unparsable. In that case uses_refs is empty, mismatches has one value, and the script exits successfully, so a broken wrapper can pass local validation and CI. This can happen if the uses: line is reformatted (e.g., quoted/templated) or accidentally changed away from the expected repo path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: Cross-file ref consistency check never enforced
- Added an abort when generated workflow wrappers contain more than one upstream cpflow ref across files.
Or push these changes by commenting:
@cursor push 20d24f1de7
Preview (20d24f1de7)
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
@@ -59,6 +59,9 @@
if refs.empty?
puts "no upstream cpflow reusable workflow refs found"
+elsif refs.keys.length > 1
+ details = refs.keys.sort.map { |ref| "#{ref}: #{refs[ref].uniq.sort.join(', ')}" }
+ abort "mismatched cpflow refs across workflows:\n#{details.join("\n")}"
else
puts "cpflow refs: #{refs.keys.sort.join(', ')}"
endYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 7d76769. Configure here.
|
placeholder |
Code ReviewOverall: Low-risk, well-scoped PR. The documentation improvements are clear and well-structured, and the new What the PR does
Issues (inline comments posted)
Minor notes
|
justin808
force-pushed
the
jg-codex/document-cpflow-upstream-testing
branch
from
7d76769 to
1bef2ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bef2ca1d6
ℹ️ 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_refs + input_refs).each { |ref| refs[ref] << path } | ||
|
|
||
| next if uses_refs.empty? && input_refs.empty? |
There was a problem hiding this comment.
The early next lets a cpflow-*.yml file pass with neither a parsed reusable-workflow uses: ref nor a control_plane_flow_ref, so the new guard can silently skip broken wrappers. In practice, if a non-exempt wrapper is edited so both patterns are absent/unmatched (for example by changing the repo path in uses: and dropping the input), this check exits successfully instead of failing, which defeats the intended ref-consistency validation.
Useful? React with 👍 / 👎.
Code ReviewOverall: Low-risk, well-scoped PR. Documentation quality is noticeably improved, and the new helper scripts add real validation value. A few things worth addressing before merge. What this PR does
Issues
changed << Pathname.new(path).relative_path_from(root).to_s
Minor observations
DocumentationThe refactored |
| next if updated == text | ||
|
|
||
| File.write(path, updated) | ||
| changed << path.delete_prefix("#{root}/") |
There was a problem hiding this comment.
String prefix stripping is fragile here — if Dir[] and Pathname#expand_path produce subtly different paths (symlink resolution, trailing slash, etc.) the full absolute path leaks silently into the output. Use relative_path_from instead:
| changed << path.delete_prefix("#{root}/") | |
| changed << Pathname.new(path).relative_path_from(root).to_s |
| WORKFLOWS_WITHOUT_CONTROL_PLANE_FLOW_REF = [ | ||
| ".github/workflows/cpflow-help-command.yml", | ||
| ".github/workflows/cpflow-review-app-help.yml" | ||
| ].freeze |
There was a problem hiding this comment.
This hardcoded allowlist will silently drift as workflows are added, renamed, or removed — a new workflow missing control_plane_flow_ref would falsely pass, and a renamed exempted workflow would start failing.
Consider deriving the exemption from the file content itself: a workflow that contains no uses: shakacode/control-plane-flow line at all legitimately needs no control_plane_flow_ref. That way the guard stays correct without manual maintenance:
# instead of checking an explicit list, skip the check when there are no uses: refs
if input_refs.empty? && uses_refs.any?
abort "#{path}: missing control_plane_flow_ref for #{expected}"
endIf the two listed workflows genuinely have a uses: line but still don't need the input, add a comment explaining why so the intent survives future edits.
| uses_refs = text.scan(%r{uses:\s+shakacode/control-plane-flow/\.github/workflows/[^@\s]+@([^\s]+)}).flatten | ||
| input_refs = text.scan(/^\s*control_plane_flow_ref:\s*(\S+)/).flatten | ||
|
|
||
| abort "#{path}: has control_plane_flow_ref but no control-plane-flow reusable workflow" if uses_refs.empty? && input_refs.any? |
There was a problem hiding this comment.
Minor ordering nit: the refs accumulation at line 53 runs even for the empty && empty case (where it's a no-op), then the next at line 55 skips the rest. Moving the next guard before the accumulation makes the control flow easier to follow — "skip this file entirely if there's nothing to track" — without changing behaviour.
| uses_refs = text.scan(%r{uses:\s+shakacode/control-plane-flow/\.github/workflows/[^@\s]+@([^\s]+)}).flatten | |
| input_refs = text.scan(/^\s*control_plane_flow_ref:\s*(\S+)/).flatten | |
| abort "#{path}: has control_plane_flow_ref but no control-plane-flow reusable workflow" if uses_refs.empty? && input_refs.any? | |
| next if uses_refs.empty? && input_refs.empty? | |
| (uses_refs + input_refs).each { |ref| refs[ref] << path } | |
justin808
force-pushed
the
jg-codex/document-cpflow-upstream-testing
branch
from
1bef2ca to
29aec12
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29aec1267d
ℹ️ 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".
| unless uses_match | ||
| abort "#{path}:#{job_name} has control_plane_flow_ref but no control-plane-flow reusable workflow" if input_ref | ||
|
|
||
| next |
There was a problem hiding this comment.
uses ref
Don’t silently next when uses_match is missing and control_plane_flow_ref is absent, because this lets a broken cpflow-*.yml wrapper pass the new guard entirely. If a reusable-workflow call is edited away from shakacode/control-plane-flow/...@ref (or otherwise becomes unparsable) in a job that doesn’t carry control_plane_flow_ref (for example wrappers without secrets: inherit), this check exits successfully and no ref-consistency error is reported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bin/test-cpflow-github-flow`:
- Around line 70-71: The guard currently fires for any presence of the secrets
key; change it to only abort when secrets is explicitly set to inherit by
replacing the condition that references job.key?("secrets") with a check that
secrets equals "inherit" (e.g., job["secrets"] == "inherit" or equivalent),
leaving the abort message that mentions control_plane_flow_ref and uses_ref
intact so only jobs with secrets: inherit trigger the missing-ref error.
🪄 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: 227fa940-7c9d-4add-ab5a-d4598317fd00
📒 Files selected for processing (5)
.controlplane/docs/testing-cpflow-github-actions.md.controlplane/readme.md.github/testing-github-actions.mdbin/pin-cpflow-github-refbin/test-cpflow-github-flow
✅ Files skipped from review due to trivial changes (1)
- .controlplane/readme.md
| elsif job.key?("secrets") | ||
| abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}" |
There was a problem hiding this comment.
Enforce the missing-ref guard only for secrets: inherit
Line 70 currently triggers on any secrets key, not specifically inherited secrets. That can cause false failures for wrappers that pass explicit secrets mappings.
Suggested fix
- elsif job.key?("secrets")
+ elsif job["secrets"] == "inherit"
abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elsif job.key?("secrets") | |
| abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}" | |
| elsif job["secrets"] == "inherit" | |
| abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}" |
🤖 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 `@bin/test-cpflow-github-flow` around lines 70 - 71, The guard currently fires
for any presence of the secrets key; change it to only abort when secrets is
explicitly set to inherit by replacing the condition that references
job.key?("secrets") with a check that secrets equals "inherit" (e.g.,
job["secrets"] == "inherit" or equivalent), leaving the abort message that
mentions control_plane_flow_ref and uses_ref intact so only jobs with secrets:
inherit trigger the missing-ref error.
Code Review: Document cpflow upstream testingOverall: Low-risk, well-scoped PR. Clear documentation and new scripts address a real operational gap. What this PR does well
Issues1. Double-entry in refs tracking hash (bin/test-cpflow-github-flow) When a job has both a valid uses: match and a control_plane_flow_ref, the same path:job_name is appended to refs[uses_ref] twice - once unconditionally, and again inside 2. gsub pattern can silently strip YAML quotes (bin/pin-cpflow-github-ref) The replacement pattern 3. FULL_COMMIT_SHA accepts uppercase SHAs (minor) The 4. --help exits with error (minor UX)
VerdictApprove after addressing items 1 and 2. Items 3 and 4 can be deferred. |
| refs[uses_ref] << "#{path}:#{job_name}" | ||
|
|
||
| if input_ref | ||
| refs[input_ref] << "#{path}:#{job_name}" |
There was a problem hiding this comment.
When uses_ref == input_ref (the valid case), refs[uses_ref] already received this path:job_name on line 65. Appending it again here means every valid job appears twice under the same key. paths.uniq in the display compensates, but the double-entry is confusing and could skew refs.length in edge cases.
The mismatch guard on the next line already handles the error case, so this append can be dropped entirely:
| refs[input_ref] << "#{path}:#{job_name}" | |
| abort "#{path}:#{job_name} mismatched cpflow refs: #{uses_ref}, #{input_ref}" if uses_ref != input_ref |
| text = File.read(path) | ||
| updated = text | ||
| .gsub(%r{(uses:\s+shakacode/control-plane-flow/\.github/workflows/[^@\s]+@)[^\s]+}, "\\1#{ref}") | ||
| .gsub(/(\bcontrol_plane_flow_ref:\s*)\S+/, "\\1#{ref}") |
There was a problem hiding this comment.
\S+ matches the entire non-whitespace token, including surrounding YAML quotes if the value was written as control_plane_flow_ref: "3e0e7e1f...". The replacement would then emit a bare unquoted ref, silently changing the YAML style.
A tighter pattern that explicitly handles both quoted and unquoted forms would be safer:
| .gsub(/(\bcontrol_plane_flow_ref:\s*)\S+/, "\\1#{ref}") | |
| .gsub(/(\bcontrol_plane_flow_ref:\s*)['"]?[0-9A-Za-z._\/-]+['"]?/, "\\1#{ref}") |
The generated workflow files currently use unquoted values so this is not an active bug, but it is a latent risk if the generator ever changes style.
✅ Review App DeletedReview app for PR #746 is deleted |
1 participant
Summary
Verification
Note
Low Risk
Low risk: changes are documentation plus local helper scripts/validation for generated workflow refs; no production runtime code paths are modified.
Overview
Clarifies how downstream repos should pin and validate
cpflow-*GitHub Actions when testing unreleasedcontrol-plane-flowchanges, including a step-by-step process for using an upstream PR commit SHA and keepinguses:andcontrol_plane_flow_refaligned.Adds
bin/pin-cpflow-github-refto bulk-update all generated wrapper refs (rejecting moving branch refs by default) and extendsbin/test-cpflow-github-flowto fail if wrappers use inconsistent upstream refs or if secret-inheriting reusable workflow jobs omitcontrol_plane_flow_ref.Reviewed by Cursor Bugbot for commit 29aec12. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation