[BRE-1815] Add PR mode to cherry-pick workflow

[gitclonebrian]

🎟️ Tracking

BRE-1815

📔 Objective

Feedback from testing of this workflow is that the workflow opening a PR into the dest branch, instead of a direct cherry-pick, would be helpful. This PR adds a PR mode to the workflow that does exactly this.

[github-actions]

Checkmarx One – Scan Summary & Details – 8fe6e14b-340b-41f6-891b-4c64a7d84b7a

Great job! No new security vulnerabilities introduced in this pull request

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[pixman20]

@claude Review this

[claude]

Claude finished @pixman20's task in 1m 54s —— View job

---

Claude PR Review

• Read changed files
• Analyze cherry-pick workflow changes
• Analyze test workflow changes
• Analyze action definition changes
• Post review feedback

---

Summary

Nice, focused addition. The create_pr input is wired cleanly through the wrapper → reusable workflow → output chain, and the PR-mode branching keeps the existing direct-push flow untouched. Tests cover both modes and the cleanup job has been extended for the new branch. A few suggestions and one notable edge case below.

---

🟡 Suggestions / Edge cases

1. Cherry-pick branch collision on re-run — .github/workflows/_cherry-pick.yml:52,175

The branch is hardcoded as cherry-pick/${{ inputs.source_pr }}-to-${{ inputs.dest_branch }}. If the workflow is re-run for the same source PR + dest branch (e.g. after a conflict fix or a previous run failed mid-flight, leaving the branch on origin), git checkout -b "$_CHERRY_PICK_BRANCH" will succeed locally but git push origin "$_CHERRY_PICK_BRANCH" may push diverged history, or gh pr create will fail with "a pull request already exists". Worth either:

• detecting the existing remote branch / PR up front and failing fast with a clear error, or
• suffixing the branch with a short run id / timestamp (-${GITHUB_RUN_ID}) to guarantee uniqueness.

Fix this →

2. Local cherry-pick branch is created before the cherry-pick step, but the conditional gating is split — .github/workflows/_cherry-pick.yml:171-194

Today the order is: Create cherry-pick branch (if create_pr) → Configure git → Cherry pick (always) → conditional push step. That's correct, but it means when create_pr=false, the Configure git step now runs unconditionally while the cherry-pick happens on the dest branch directly. That's actually the intended behavior — just worth a quick sanity check the existing direct-push flow still picks up the bot identity. (It does, since Configure git is unconditional.) No change needed; flagging for review awareness.

3. PR-mode failure path leaves no cleanup hook in non-test usage — .github/workflows/_cherry-pick.yml:189-193, 218-228

If git cherry-pick fails after the branch is created but never pushed, there's nothing to clean up (good). However, if git push origin "$_CHERRY_PICK_BRANCH" succeeds but gh pr create fails, the orphan branch is left on origin with no PR. Consider either creating the PR first via gh pr create --head against an unpushed branch (not supported) or, more practically, deleting the pushed branch in the error path:

if ! new_pr_url=$(gh pr create ...); then
  echo "::error::Failed to create PR."
  git push origin --delete "$_CHERRY_PICK_BRANCH" || true
  exit 1
fi

4. pr_title is captured but only used in PR mode — .github/workflows/_cherry-pick.yml:128,161,222,237

Minor: pr_title is always captured in Validate PR but only consumed when create_pr=true. Harmless, just noting it.

5. PR title interpolation is safe — verified — .github/workflows/_cherry-pick.yml:222,237

PR_TITLE is passed via env and quoted in --title "Cherry pick: $PR_TITLE". Shell handles special chars correctly; no injection risk via the title content. Looks good.

6. Test ordering rationale is implicit — .github/workflows/test-cherry-pick.yml:108-112

The comment from commit 68515c7 explains why cherry-pick-pr runs before cherry-pick (so the direct push doesn't make the dest branch already contain the commit when the PR-mode test runs). Consider adding a one-line YAML comment above needs: [setup, cherry-pick-pr] so future maintainers understand the dependency isn't arbitrary.

7. Cleanup sleep 120 — .github/workflows/test-cherry-pick.yml:148

Existing 2-minute sleep is preserved. With the new cherry-pick branch also being cleaned, this is the right place. No change.

8. Cleanup ordering and idempotency look correct — .github/workflows/test-cherry-pick.yml:138-201

The cherry-pick branch is deleted before the dest branch, so the PR closes cleanly (head removed). All git ls-remote --exit-code guards handle the case where steps failed before the branch was pushed. 👍

9. Wrapper action template — cherry-pick/cherry-pick.yml:19-23,43

The pass-through for create_pr is correct. Note the wrapper still lists rc-2024-02 as a choice (pre-existing) — out of scope for this PR but worth a future cleanup.

---

✅ What's good

• Clean separation of the two modes via a single boolean input and step-level if: guards — no duplicated cherry-pick logic.
• Output cherry_pick_branch is correctly threaded through jobs.cherry-pick.outputs → workflow outputs so the test cleanup can consume it.
• git cherry-pick -x preserves provenance in the PR-mode commit message.
• Test workflow exercises both modes in the same run with proper sequencing.
• PR-mode comment on source PR includes the new PR URL — good UX.

---

Verdict

Approach is solid. The collision case (#1) and the orphan-branch case (#3) are the two items I'd want addressed before merge; the rest are minor.
• Branch: BRE-1815_add-pr-to-cpick