fix: reorder release-please jobs and fix enhance-release-pr workflow #15
Conversation
- Reorder jobs: handle-untagged-releases now runs FIRST before release-please This prevents Release Please from aborting with 'untagged PRs outstanding' - Replace grep -oP with sed -E for portability (BSD grep doesn't support -P) - Fix enhance-release-pr: Use github.event.pull_request.head.ref instead of github.head_ref - Add workflow_dispatch trigger to enhance-release-pr for manual triggering - Improve version detection with multi-method fallback (base branch manifest, GitHub releases, Git tags) - Update verify-release to check both release-please and handle-untagged-releases outputs
![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.
ℹ️ 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".
| - name: 🏷️ Extract Version from PR Title | ||
| id: version | ||
| run: | | ||
| VERSION=$(echo "${{ github.event.pull_request.title }}" | grep -oP '\d+\.\d+\.\d+' || echo "") | ||
| # Extract version using sed -E (extended regex, more portable than grep -P) | ||
| VERSION=$(echo "${{ github.event.pull_request.title }}" | sed -E 's/.*([0-9]+\.[0-9]+\.[0-9]+).*/\1/' || echo "") |
There was a problem hiding this comment.
The new sed-based extraction returns the original PR title when no X.Y.Z match is found, so VERSION is non-empty even for titles without a semantic version. That flips skip to false and drives the rest of the workflow with an arbitrary string instead of skipping as before (e.g., if the release PR is retitled or manual dispatch lacks a version).
Useful? React with 👍 / 👎.
| outputs: | ||
| release_created: ${{ steps.process.outputs.release_created }} | ||
| tag_name: ${{ steps.process.outputs.tag_name }} | ||
| version: ${{ steps.process.outputs.version }} |
There was a problem hiding this comment.
The job exports release_created, tag_name, and version from steps.process, but no step in the job has id: process. As a result those outputs are always empty, so downstream verify-release never runs when this job creates a tag/release and cannot check the produced artifact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes critical release workflow orchestration issues by reordering jobs and improving workflow triggers. The primary change is moving the handle-untagged-releases job to run before release-please to prevent "untagged PRs outstanding" errors. However, the implementation contains several critical bugs that will prevent the workflows from functioning correctly.
Key Changes:
- Reorders
handle-untagged-releasesto run beforerelease-please(instead of after) to handle edge cases proactively - Adds
workflow_dispatchtrigger toenhance-release-pr.ymlfor manual execution - Improves portability by replacing
grep -Pwithsed -Ein version extraction - Enhances previous version detection with multiple fallback methods
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/workflows/release-please.yml |
Reorders jobs to handle untagged releases before running release-please; adds outputs to track release creation; updates verify-release to handle releases from either job |
.github/workflows/enhance-release-pr.yml |
Adds manual trigger support; improves portability with sed instead of grep; enhances version detection with multiple fallback strategies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| types: [opened, synchronize, reopened] | ||
| branches: | ||
| - main | ||
| workflow_dispatch: # Added to allow manual triggering |
There was a problem hiding this comment.
Adding workflow_dispatch trigger without handling the case when github.event.pull_request is null will cause failures. When manually triggered, github.event.pull_request.number (line 12), github.event.pull_request.head.ref (lines 24, 30, 145, 319), github.event.pull_request.title (line 38), and github.event.pull_request.base.ref (lines 58-59) will all be undefined, causing the workflow to fail. Either add inputs to workflow_dispatch to provide these values, or guard the job execution with a condition that checks if the trigger is pull_request.
| run: | | ||
| VERSION=$(echo "${{ github.event.pull_request.title }}" | grep -oP '\d+\.\d+\.\d+' || echo "") | ||
| # Extract version using sed -E (extended regex, more portable than grep -P) | ||
| VERSION=$(echo "${{ github.event.pull_request.title }}" | sed -E 's/.*([0-9]+\.[0-9]+\.[0-9]+).*/\1/' || echo "") |
There was a problem hiding this comment.
The sed pattern replacement may not work as intended. Unlike grep -oP which only outputs matching text, sed 's/.../.../ will output the entire line unchanged if the pattern doesn't match. If the PR title doesn't contain a version number, VERSION will be set to the entire title instead of being empty. Consider using: sed -n 's/.*\([0-9]\+\.[0-9]\+\.[0-9]\+\).*/\1/p' with the -n flag and p command to only print when there's a match, or add a pattern test first.
| VERSION=$(echo "${{ github.event.pull_request.title }}" | sed -E 's/.*([0-9]+\.[0-9]+\.[0-9]+).*/\1/' || echo "") | |
| VERSION=$(echo "${{ github.event.pull_request.title }}" | sed -n -E 's/.*([0-9]+\.[0-9]+\.[0-9]+).*/\1/p') |
| # ============================================================================= | ||
| # Verify Release | ||
| # ============================================================================= |
There was a problem hiding this comment.
Duplicate comment headers. Lines 215-217 say "Step 3: Verify Release" and lines 219-221 repeat "Verify Release". Remove one set of these duplicate headers.
| # ============================================================================= | |
| # Verify Release | |
| # ============================================================================= |
| echo "✅ Processed PR #$PR_NUM" | ||
| done | ||
| done < <(echo "$PENDING_PRS" | jq -c '.') |
There was a problem hiding this comment.
Conflicting input redirection for the while loop. The loop at line 76 already receives input via pipe (echo "$PENDING_PRS" | jq -c '.' | while read -r pr; do), but line 165 attempts to redirect input again with done < <(...). This will cause a syntax error or unexpected behavior. Either use the pipe on line 76 and just use done at line 165, or remove the pipe on line 76 and keep the process substitution at line 165. The process substitution approach (while read -r pr; do ... done < <(...)) is preferred to avoid the subshell issue mentioned in the comment for lines 109-111.
| release_created: ${{ steps.process.outputs.release_created }} | ||
| tag_name: ${{ steps.process.outputs.tag_name }} | ||
| version: ${{ steps.process.outputs.version }} |
There was a problem hiding this comment.
The job outputs reference steps.process.outputs.*, but there is no step with id: process in this job. The step at line 54 ("🔍 Find and process untagged merged release PRs") needs id: process added so these outputs can be properly referenced.
Fixes release workflow orchestration issues