Skip to content

Add plugin hooks for Gemfile evaluation and source fetching#9488

Open
hsbt wants to merge 7 commits intomasterfrom
add-plugin-hooks-eval-fetch
Open

Add plugin hooks for Gemfile evaluation and source fetching#9488
hsbt wants to merge 7 commits intomasterfrom
add-plugin-hooks-eval-fetch

Conversation

@hsbt
Copy link
Copy Markdown
Member

@hsbt hsbt commented Apr 16, 2026

What was the end-user or developer problem that led to this PR?

Bundler's plugin hook mechanism only covers install and require operations. PR #6961 proposed eval hooks and PR #8162 proposed fetch hooks, but both have been open for years without a conclusion.

What is your fix for the problem, implemented in this PR?

Rebases #6961 and #8162 onto the current codebase and consolidates them into one cohesive set of hook points, with the following adjustments after review.

For eval hooks, the unlock argument was removed from before-eval since it exposes internal state that plugins should not depend on.

For fetch hooks, the hook position was moved from fetch_gem_if_possible to Source::Rubygems#download_gem so the hooks fire only on actual network downloads, not on cache hits or bundle cache runs. The source argument was dropped because spec.source already provides it. The hook invocations are wrapped in begin/ensure so the after hook fires even when the download raises, matching the behavior of the existing GEM_AFTER_INSTALL hook.

For git fetch hooks, the event names were renamed from git-before-fetch / git-after-fetch to before-git-fetch / after-git-fetch for consistency with the existing before-* / after-* naming pattern. The redundant GEM_BEFORE_FETCH / GEM_AFTER_FETCH invocations in Source::Git#install were removed since using gem fetch events for git sources is semantically inconsistent. As with the gem fetch hooks, the invocations are wrapped in begin/ensure.

Closes #6961
Closes #8162

Make sure the following tasks are checked

ccutrer and others added 5 commits April 16, 2026 13:25
Adds four new hook points:
- before-fetch / after-fetch: fires in Source::Rubygems#download_gem
  around actual network downloads, avoiding noise from cache hits.
- before-git-fetch / after-git-fetch: fires in Source::Git#specs
  around fetch/checkout operations.

Based on the original proposal in #8162 with adjustments:
- Moved gem fetch hooks from fetch_gem_if_possible to download_gem
  so they only fire on actual network I/O.
- Dropped the source argument since spec.source provides it.
- Renamed git hooks to before-git-fetch / after-git-fetch for
  consistency with the existing before-*/after-* pattern.
- Removed GEM_BEFORE_FETCH/GEM_AFTER_FETCH from Source::Git#install
  since using gem fetch events for git sources is semantically
  inconsistent.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers the four new hook events added in the previous commit:
before-fetch, after-fetch, before-git-fetch, after-git-fetch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Arrange events.rb by actual firing order so the file reads as a
timeline of bundler's lifecycle: Gemfile eval, install-all bracket
(with per-gem fetch, git fetch, and install nested inside), then
require-all bracket (with per-gem require nested inside).

Also clarify the git fetch hook docstrings: the hook fires around
both remote fetch and checkout, not only fetch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap the fetch/checkout operations in begin/ensure so the after
hook fires even when the underlying fetch raises. This matches
the existing GEM_AFTER_INSTALL hook, which fires on both success
and failure paths (via internal error-to-state conversion in
ParallelInstaller#do_install).

Without this, plugins relying on before/after pairs for cleanup
or timing would see unbalanced hook invocations whenever a
network or checkout error occurs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 16, 2026 06:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends Bundler’s plugin event system to let plugins hook into Gemfile evaluation and into actual gem/git fetching operations, enabling tooling such as timing/verification plugins.

Changes:

  • Add new plugin events for before-eval / after-eval, gem fetch, and git fetch.
  • Invoke eval hooks from Bundler::Definition.build.
  • Invoke fetch hooks around rubygems downloads and git source fetch/checkout with ensure to guarantee after-* events run on errors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
spec/plugins/hook_spec.rb Adds plugin integration specs asserting new eval/fetch hook events are emitted.
bundler/lib/bundler/source/rubygems.rb Wraps remote gem downloads with GEM_BEFORE_FETCH / GEM_AFTER_FETCH hooks.
bundler/lib/bundler/source/git.rb Wraps git fetch/checkout with GIT_BEFORE_FETCH / GIT_AFTER_FETCH hooks.
bundler/lib/bundler/plugin/events.rb Defines and documents new plugin event constants.
bundler/lib/bundler/definition.rb Emits eval hooks around Gemfile evaluation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bundler/lib/bundler/plugin/events.rb Outdated
Comment on lines +53 to +59
# Includes a Gem::Specification. Does not fire on cache hits.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"

# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a Gem::Specification. Does not fire on cache hits.
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GEM_BEFORE_FETCH / GEM_AFTER_FETCH docs say the hook receives a Gem::Specification, but in this code path Bundler passes a Bundler spec proxy (e.g., Bundler::RemoteSpecification), which delegates many Gem::Specification methods via method_missing but is not a Gem::Specification instance. Plugins that do spec.is_a?(Gem::Specification) or rely on concrete class will break; consider documenting the actual object type (or “spec-like object responding to the Gem::Specification API”).

Suggested change
# Includes a Gem::Specification. Does not fire on cache hits.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"
# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a Gem::Specification. Does not fire on cache hits.
# Includes a spec-like object responding to the Gem::Specification API
# (for example, a Bundler specification proxy such as Bundler::RemoteSpecification).
# Does not fire on cache hits.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"
# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a spec-like object responding to the Gem::Specification API
# (for example, a Bundler specification proxy such as Bundler::RemoteSpecification).
# Does not fire on cache hits.

Copilot uses AI. Check for mistakes.
Comment thread bundler/lib/bundler/plugin/events.rb Outdated
Comment on lines +53 to +59
# Includes a Gem::Specification. Does not fire on cache hits.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"

# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a Gem::Specification. Does not fire on cache hits.
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs state these fetch hooks “do not fire on cache hits”, but Bundler.rubygems.download_gem has its own early return when the .gem file already exists (e.g., due to a race), so hooks can fire even when no network download occurs. Either adjust the wording to reflect this (“does not fire when the gem is already present at the initial download-cache check”) or change the hook placement to only wrap the actual network fetch path.

Suggested change
# Includes a Gem::Specification. Does not fire on cache hits.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"
# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a Gem::Specification. Does not fire on cache hits.
# Includes a Gem::Specification. Does not fire when the gem is already
# present at the initial download-cache check.
# GEM_BEFORE_FETCH = "before-fetch"
define :GEM_BEFORE_FETCH, "before-fetch"
# @!parse
# A hook called after each individual gem is downloaded from a remote source.
# Includes a Gem::Specification. Does not fire when the gem is already
# present at the initial download-cache check.

Copilot uses AI. Check for mistakes.
Comment on lines +480 to 487
Plugin.hook(Plugin::Events::GEM_BEFORE_FETCH, spec)
begin
Gem.time("Downloaded #{spec.name} in", 0, true) do
Bundler.rubygems.download_gem(spec, uri, download_cache_path, gem_remote_fetcher)
end
ensure
Plugin.hook(Plugin::Events::GEM_AFTER_FETCH, spec)
end
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new begin/ensure behavior is intended to guarantee GEM_AFTER_FETCH fires even when the download raises, but there’s no spec covering the error path. Consider adding a plugin hook spec that forces a gem download to fail (e.g., by stubbing Bundler.rubygems.download_gem or using an Artifice endpoint that returns a 404) and asserting the after-fetch hook still ran.

Copilot uses AI. Check for mistakes.
Comment on lines +194 to +200
Plugin.hook(Plugin::Events::GIT_BEFORE_FETCH, self)
begin
fetch unless use_app_cache?
checkout
ensure
Plugin.hook(Plugin::Events::GIT_AFTER_FETCH, self)
end
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since GIT_AFTER_FETCH is now guaranteed via ensure, it would be good to add a spec that exercises the failure path (e.g., invalid git URI or stubbing fetch/checkout to raise) and asserts the after-git-fetch hook still runs. That prevents regressions where future refactors accidentally remove the ensure semantics.

Copilot uses AI. Check for mistakes.
hsbt and others added 2 commits April 16, 2026 16:49
doctor_spec.rb strict-stubs File.writable? and File.readable? with
specific paths, which was safe as long as the doctor command did
not trigger Plugin.hook. The new before-eval/after-eval hooks
fire during Bundler.definition, which the doctor command calls,
and Plugin.hook initializes Plugin::Index, which touches
Bundler.user_home and calls File.writable? on the test home path.
Those calls do not match the stubs and RSpec raises.

Match the existing File.exist? pattern and add and_call_original
fallbacks so unrelated paths fall through to the real methods,
while the specific stubs continue to control the paths under test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The hook receives a Bundler spec proxy (Bundler::EndpointSpecification
or Bundler::RemoteSpecification) that responds to the Gem::Specification
API but is not itself a Gem::Specification instance. Plugins doing
strict is_a? checks would break on the previous wording.

Also clarify the cache-hit language: the hook does not fire when the
initial download-cache check in fetch_gem finds the .gem already on
disk, but Bundler.rubygems.download_gem also has a race-protection
early return on the same path, which the previous wording obscured.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants