Fix Homebrew CI and add automated release workflow#1119
Conversation
- Fix homebrew.yml to use correct tap prefix (mflowcode/test/mfc) - Add homebrew-release.yml to automate formula updates on new tags - Update formula to v5.2.0 to match homebrew-mfc tap Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use TAP_REPO_TOKEN secret (already exists in MFlowCode/MFC) - Restore path restrictions in homebrew.yml for production Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When triggered by PR, runs in dry-run mode using v5.2.0 as test version. This allows testing the workflow from a PR before merging. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
codeant-ai
Bot
added
the
size:L
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Fork PRs cannot access repository secrets, so skip steps that require TAP_REPO_TOKEN. For PRs, only validate version parsing and SHA256. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Nitpicks 🔍
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
High-level Suggestion
Replace fragile sed and awk commands in the release workflow with Homebrew's built-in brew bump command to make formula updates more robust and maintainable. [High-level, importance: 8]
Solution Walkthrough:
Before:
# .github/workflows/homebrew-release.yml
- name: Update formula
run: |
VERSION="..."
SHA256="..."
FORMULA="homebrew-mfc/Formula/mfc.rb"
# Update URL with sed
sed -i "s|url ...|url ...|" "$FORMULA"
# Update SHA256 with awk
awk -v newsha="$SHA256" '
...
' "$FORMULA" > "$FORMULA.tmp" && mv "$FORMULA.tmp" "$FORMULA"
# Remove bottle block with awk
awk '
...
' "$FORMULA" > "$FORMULA.tmp" && mv "$FORMULA.tmp" "$FORMULA"
After:
# .github/workflows/homebrew-release.yml
- name: Update formula
run: |
VERSION="..."
FORMULA_PATH="homebrew-mfc/Formula/mfc.rb"
# Use Homebrew's developer commands to update the formula
# This command handles URL and SHA256 updates automatically
brew bump-formula-pr \
--version="${VERSION}" \
--no-browse \
--no-fork \
"${FORMULA_PATH}"
# The bottle block may still need to be removed separately
awk '
/^ bottle do/ { in_bottle=1; next }
in_bottle && /^ end/ { in_bottle=0; next }
!in_bottle { print }
' "${FORMULA_PATH}" > tmp && mv tmp "${FORMULA_PATH}"
| - name: Commit and push to homebrew-mfc | ||
| if: ${{ github.event_name != 'pull_request' && github.event.inputs.dry_run != 'true' }} | ||
| run: | | ||
| cd homebrew-mfc | ||
| VERSION="${{ steps.version.outputs.version }}" | ||
|
|
||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
|
|
||
| git add Formula/mfc.rb | ||
| git commit -m "Update MFC to v${VERSION}" | ||
|
|
||
| echo "Pushing to homebrew-mfc..." | ||
| git push origin main | ||
|
|
||
| echo "Successfully pushed formula update!" | ||
| echo "The bottle.yml workflow in homebrew-mfc will now build bottles automatically." |
There was a problem hiding this comment.
Suggestion: Improve the robustness of the conditional push logic by making the check for the dry_run input more explicit. [general, importance: 5]
| - name: Commit and push to homebrew-mfc | |
| if: ${{ github.event_name != 'pull_request' && github.event.inputs.dry_run != 'true' }} | |
| run: | | |
| cd homebrew-mfc | |
| VERSION="${{ steps.version.outputs.version }}" | |
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| git add Formula/mfc.rb | |
| git commit -m "Update MFC to v${VERSION}" | |
| echo "Pushing to homebrew-mfc..." | |
| git push origin main | |
| echo "Successfully pushed formula update!" | |
| echo "The bottle.yml workflow in homebrew-mfc will now build bottles automatically." | |
| - name: Commit and push to homebrew-mfc | |
| if: | | |
| github.event_name != 'pull_request' && | |
| (github.event_name != 'workflow_dispatch' || github.event.inputs.dry_run != 'true') | |
| run: | | |
| cd homebrew-mfc | |
| VERSION="${{ steps.version.outputs.version }}" | |
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| git add Formula/mfc.rb | |
| git commit -m "Update MFC to v${VERSION}" | |
| echo "Pushing to homebrew-mfc..." | |
| git push origin main | |
| echo "Successfully pushed formula update!" | |
| echo "The bottle.yml workflow in homebrew-mfc will now build bottles automatically." |
| - name: Run MFC test case | ||
| run: | | ||
| echo "Running a simple test case (1D Sod shock tube)..." | ||
| MFC_PREFIX="$(brew --prefix mflowcode/test/mfc)" | ||
| TESTDIR=$(mktemp -d) | ||
| cp $(brew --prefix mfc)/examples/1D_sodshocktube/case.py "$TESTDIR/" | ||
| cp "$MFC_PREFIX/examples/1D_sodshocktube/case.py" "$TESTDIR/" | ||
|
|
||
| echo "Running with $(sysctl -n hw.ncpu) processors..." | ||
| # Use absolute path and shorthand syntax (mfc auto-detects and prepends 'run') | ||
| mfc "$TESTDIR/case.py" -j $(sysctl -n hw.ncpu) | ||
|
|
||
| echo "Test case completed successfully!" |
There was a problem hiding this comment.
Suggestion: Avoid redefining the MFC_PREFIX variable by setting it as a job-level environment variable, making it available to all subsequent steps. [general, importance: 4]
| - name: Run MFC test case | |
| run: | | |
| echo "Running a simple test case (1D Sod shock tube)..." | |
| MFC_PREFIX="$(brew --prefix mflowcode/test/mfc)" | |
| TESTDIR=$(mktemp -d) | |
| cp $(brew --prefix mfc)/examples/1D_sodshocktube/case.py "$TESTDIR/" | |
| cp "$MFC_PREFIX/examples/1D_sodshocktube/case.py" "$TESTDIR/" | |
| echo "Running with $(sysctl -n hw.ncpu) processors..." | |
| # Use absolute path and shorthand syntax (mfc auto-detects and prepends 'run') | |
| mfc "$TESTDIR/case.py" -j $(sysctl -n hw.ncpu) | |
| echo "Test case completed successfully!" | |
| - name: Run MFC test case | |
| run: | | |
| echo "Running a simple test case (1D Sod shock tube)..." | |
| TESTDIR=$(mktemp -d) | |
| cp "$MFC_PREFIX/examples/1D_sodshocktube/case.py" "$TESTDIR/" | |
| echo "Running with $(sysctl -n hw.ncpu) processors..." | |
| # Use absolute path and shorthand syntax (mfc auto-detects and prepends 'run') | |
| mfc "$TESTDIR/case.py" -j $(sysctl -n hw.ncpu) | |
| echo "Test case completed successfully!" |
|
|
||
| # Verify URL is reachable | ||
| HTTP_CODE=$(curl -sI -w "%{http_code}" -o /dev/null "$URL") | ||
| if [[ "$HTTP_CODE" != "200" && "$HTTP_CODE" != "302" ]]; then |
There was a problem hiding this comment.
Suggestion: Add HTTP status code 301 (Moved Permanently) to the list of acceptable responses when checking the reachability of the release tarball URL. [general, importance: 6]
| if [[ "$HTTP_CODE" != "200" && "$HTTP_CODE" != "302" ]]; then | |
| if [[ "$HTTP_CODE" != "200" && "$HTTP_CODE" != "301" && "$HTTP_CODE" != "302" ]]; then |
| git add Formula/mfc.rb | ||
| git commit -m "Update MFC to v${VERSION}" |
There was a problem hiding this comment.
Suggestion: Prevent the workflow from failing by adding a guard to the git commit step, ensuring it only runs if there are staged changes. [general, importance: 7]
| git add Formula/mfc.rb | |
| git commit -m "Update MFC to v${VERSION}" | |
| git add Formula/mfc.rb | |
| if ! git diff --cached --quiet; then | |
| git commit -m "Update MFC to v${VERSION}" | |
| else | |
| echo "No changes to commit" | |
| fi |
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Homebrew CI test failure and adds automation for Homebrew formula releases. The primary issue was that the test workflow used an incorrect tap prefix when verifying the installation. Additionally, a new workflow automates formula updates in the homebrew-mfc tap when version tags are pushed.
Changes:
- Fixed homebrew.yml to use the full tap-qualified name
mflowcode/test/mfcinstead of justmfc - Added homebrew-release.yml workflow to automatically update the homebrew-mfc tap formula when new version tags are pushed
- Updated formula version from v5.1.5 to v5.2.0 with corresponding SHA256 hash
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
.github/workflows/homebrew.yml |
Fixed tap prefix references in test and cleanup steps, removed trailing whitespace |
.github/workflows/homebrew-release.yml |
New automated workflow for updating homebrew-mfc tap on version tag pushes |
packaging/homebrew/mfc.rb |
Version bump from v5.1.5 to v5.2.0 with updated SHA256 hash |
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "The full workflow will run when a \`v*\` tag is pushed after merge." >> $GITHUB_STEP_SUMMARY | ||
|
|
||
| - name: Checkout homebrew-mfc tap |
There was a problem hiding this comment.
The sed command uses -i flag without a backup suffix, which behaves differently between GNU sed (Linux) and BSD sed (macOS). On Ubuntu (this workflow's runner), GNU sed is used and -i works without an argument. However, this is fragile:
- If the workflow ever runs on macOS, this command would fail or behave unexpectedly
- The pattern
v[^\"]*\.tar\.gzcould potentially match unintended URLs if there are multiple similar URLs in the file
Consider:
- Using a more specific pattern that anchors to the exact line structure
- Or using the same awk-based approach used for SHA256 updates (lines 99-106), which would be more consistent and portable
- Or documenting that this workflow must run on Linux due to GNU sed dependency
| - name: PR test summary | ||
| if: ${{ github.event_name == 'pull_request' }} | ||
| run: | | ||
| echo "## PR Test Mode" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "Validated:" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
The workflow checks out the homebrew-mfc repository with TAP_REPO_TOKEN even during PR runs (line 79-84), but then doesn't push during PRs (line 132 condition). This means the token is being used unnecessarily during PR testing.
For better security, consider:
- Making the checkout step conditional:
if: github.event_name != 'pull_request' - Or using a separate step for PR testing that doesn't require token access
- Or checking out without token for PRs (which would fail if the repo is private, but would work if it's public)
This reduces the exposure window of the sensitive token during test runs where it's not needed.
| VERSION="${{ inputs.version }}" | ||
| elif [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| # Use existing version for PR testing | ||
| VERSION="5.2.0" |
There was a problem hiding this comment.
The hardcoded version "5.2.0" for PR testing mode will become outdated as new versions are released. This means PR tests will always test against version 5.2.0 regardless of what version is currently in the formula or what the PR is about.
Consider instead:
- Extracting the current version from the formula file in this repository
- Or using an environment variable that can be updated
- Or documenting that this version should be kept in sync with the latest release
This would ensure PR tests remain relevant as the project evolves.
| VERSION="5.2.0" | ||
| echo "::notice::PR test mode - using version $VERSION" | ||
| else | ||
| # Extract version from tag (remove 'v' prefix) |
There was a problem hiding this comment.
The version extraction uses ${GITHUB_REF#refs/tags/v} which removes the prefix refs/tags/v, but this doesn't validate that GITHUB_REF actually has this format. If the workflow is somehow triggered in an unexpected way (e.g., manual workflow_dispatch when not intended, or GitHub changes its ref format), the version extraction could produce unexpected results.
Consider adding validation that GITHUB_REF starts with refs/tags/v before attempting extraction, similar to how the workflow_dispatch path validates the version format. This would make the failure mode more explicit and debuggable.
| # Extract version from tag (remove 'v' prefix) | |
| # Extract version from tag (remove 'v' prefix) | |
| if [[ "$GITHUB_REF" != refs/tags/v* ]]; then | |
| echo "::error::Unexpected GITHUB_REF: '$GITHUB_REF' (expected to start with 'refs/tags/v')" | |
| exit 1 | |
| fi |
| FORMULA="homebrew-mfc/Formula/mfc.rb" | ||
|
|
||
| echo "Updating formula to v${VERSION}..." | ||
|
|
||
| # Update URL | ||
| sed -i "s|url \"https://github.com/MFlowCode/MFC/archive/refs/tags/v[^\"]*\.tar\.gz\"|url \"https://github.com/MFlowCode/MFC/archive/refs/tags/v${VERSION}.tar.gz\"|" "$FORMULA" | ||
|
|
There was a problem hiding this comment.
The bottle block removal logic assumes that bottle blocks are indented with exactly 2 spaces and the closing end is also at exactly 2 spaces. This pattern /^ bottle do/ and /^ end/ will fail to match if:
- The indentation uses tabs instead of spaces
- The indentation is different (e.g., 4 spaces)
- There's trailing whitespace
- There are nested
endstatements within the bottle block
The current logic could incorrectly remove content if there are nested blocks with end keywords inside the bottle block. A more robust approach would track brace depth or use a more sophisticated parser. At minimum, the comment should acknowledge this limitation.
Additionally, if there's no bottle block present (which is the case initially), this awk command will still succeed but do nothing, which is acceptable behavior.
| VERSION="${{ steps.version.outputs.version }}" | ||
| SHA256="${{ steps.sha256.outputs.sha256 }}" | ||
| FORMULA="homebrew-mfc/Formula/mfc.rb" | ||
|
|
There was a problem hiding this comment.
The comment states "This removes everything between 'bottle do' and the matching 'end'", but the awk logic doesn't actually verify that the end matches the bottle do block. If there are nested blocks within the bottle section (which is unlikely but possible in Ruby), the first end encountered will terminate the removal, potentially leaving partial bottle block content.
While bottle blocks in Homebrew formulas typically don't have nested structures, this edge case should be acknowledged or the logic should be made more robust to handle proper block matching.
| echo "::error::Invalid version format: $VERSION (expected X.Y.Z)" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The version validation regex ^[0-9]+\.[0-9]+\.[0-9]+$ is good for basic semver format, but it doesn't validate that the version numbers are reasonable. For example, it would accept 999999.999999.999999 or 0.0.0.
While this is unlikely to cause issues in practice (users won't typically enter such values), consider adding bounds checking or at least documenting expected version ranges if there are known constraints.
| # Additional sanity checks on version components: | |
| # - Disallow 0.0.0 | |
| # - Require each component to be between 0 and 999 | |
| IFS='.' read -r major minor patch <<< "$VERSION" | |
| if [[ "$major" -eq 0 && "$minor" -eq 0 && "$patch" -eq 0 ]]; then | |
| echo "::error::Invalid version: $VERSION (0.0.0 is not allowed)" | |
| exit 1 | |
| fi | |
| if (( major < 0 || major > 999 || minor < 0 || minor > 999 || patch < 0 || patch > 999 )); then | |
| echo "::error::Invalid version: $VERSION (each component must be between 0 and 999)" | |
| exit 1 | |
| fi |
| required: false | ||
| type: boolean | ||
| default: false | ||
|
|
There was a problem hiding this comment.
The workflow is missing a permissions: declaration at the job or workflow level. While the workflow uses secrets.TAP_REPO_TOKEN which provides its own authentication, it's a best practice to explicitly declare the minimum required permissions for security and clarity.
Consider adding:
permissions:
contents: readThis follows the principle of least privilege and makes the workflow's permission requirements explicit. Reference: .github/workflows/deploy-tap.yml:20-21 shows this pattern in the similar deploy-tap workflow.
| permissions: | |
| contents: read |
| on: | ||
| push: | ||
| tags: | ||
| - 'v*' |
There was a problem hiding this comment.
This workflow will conflict with the existing deploy-tap.yml workflow. Both workflows are triggered on tag pushes (v*):
deploy-tap.ymltriggers on tag creation (create:event) at line 17- This new
homebrew-release.ymlalso triggers on tag pushes (push: tags: - 'v*')
When a version tag is pushed, both workflows will attempt to update the homebrew-mfc tap repository simultaneously, which could result in:
- Race conditions where one workflow's push might conflict with the other's
- Duplicate commits or merge conflicts
- Unnecessary redundant operations
Consider either:
- Removing tag-based triggers from
deploy-tap.ymland consolidating into this workflow - Removing tag-based triggers from this new workflow if
deploy-tap.ymlis already handling releases - Or clearly separating their responsibilities with different trigger conditions
Reference: .github/workflows/deploy-tap.yml:16-17 shows the existing tag-triggered deployment.
| /^ bottle do/ { in_bottle=1; next } | ||
| in_bottle && /^ end/ { in_bottle=0; next } | ||
| !in_bottle { print } | ||
| ' "$FORMULA" > "$FORMULA.tmp" && mv "$FORMULA.tmp" "$FORMULA" |
There was a problem hiding this comment.
The condition logic has a potential issue. When workflow_dispatch is triggered with dry_run set to false, the condition github.event.inputs.dry_run != 'true' will evaluate the boolean false against the string 'true', which should work. However, GitHub Actions converts boolean inputs to strings, so when comparing, you're comparing string 'false' with string 'true'.
The safer and more explicit approach would be to use:
github.event.inputs.dry_run != true (without quotes for boolean comparison)
However, the current implementation should still work correctly because any string other than 'true' will pass the condition. This is just a style consideration for clarity.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Recreating PR from upstream branch to allow secret access for CI testing |
User description
User description
Summary
homebrew.ymlto use correct tap prefix (mflowcode/test/mfcinstead ofmfc)homebrew-release.ymlto automate formula updates when new version tags are pushedDetails
Bug fix (homebrew.yml)
The test step was using
$(brew --prefix mfc)but the formula is installed from a temporary tap asmflowcode/test/mfc. This caused the "Test MFC installation" step to fail.New release workflow (homebrew-release.yml)
When a new
v*tag is pushed:Supports
workflow_dispatchwith dry-run option for manual testing.Test plan
🤖 Generated with Claude Code
PR Type
Bug fix, Enhancement
Description
Fix homebrew.yml to use correct tap-qualified prefix for test installation
Add automated homebrew-release.yml workflow to update formula on version tags
Update MFC formula to v5.2.0 with new SHA256 hash
Support manual testing via workflow_dispatch with dry-run option
Diagram Walkthrough
File Walkthrough
mfc.rb
Update MFC formula to v5.2.0packaging/homebrew/mfc.rb
homebrew-release.yml
Add automated Homebrew formula release workflow.github/workflows/homebrew-release.yml
dispatch
homebrew.yml
Fix tap prefix and improve test assertions.github/workflows/homebrew.yml
mfctomflowcode/test/mfcfor testinstallation
$MFC_PREFIXvariable instead ofinline prefix calls
mflowcode/testafter uninstalling formulaformatting
CodeAnt-AI Description
Fix Homebrew CI and automate formula updates to v5.2.0
What Changed
Impact
✅ Fewer CI test failures for Homebrew installs✅ Automated Homebrew formula updates on tagged releases✅ Clearer formula release validation and dry-run testing💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Note
Medium Risk
Adds automation that pushes changes to an external tap repo on tag releases, so misconfiguration or secret/token issues could update the wrong formula or fail releases. The Homebrew CI changes are low risk but affect installation test reliability.
Overview
Adds a new GitHub Actions workflow (
homebrew-release.yml) that, onv*tags (or manual dispatch), computes the release tarball SHA, updatesMFlowCode/homebrew-mfc’smfc.rb(including removing anybottle doblock), validates it, and optionally commits/pushes the change.Fixes Homebrew CI (
homebrew.yml) to use the tap-qualified install prefix (mflowcode/test/mfc) for structure/tests and to uninstall/untap the correct formula.Bumps the in-repo Homebrew formula (
packaging/homebrew/mfc.rb) tov5.2.0with the corresponding SHA256.Written by Cursor Bugbot for commit 45403ff. Configure here.