Skip to content

Validate required_rubygems_version for content addressable gems - #6844

Merged
jenshenny merged 1 commit into
masterfrom
remove-rubygems-requirement-injection
Sep 9, 2026
Merged

Validate required_rubygems_version for content addressable gems#6844
jenshenny merged 1 commit into
masterfrom
remove-rubygems-requirement-injection

Conversation

@jenshenny

@jenshenny jenshenny commented Sep 7, 2026

Copy link
Copy Markdown
Member

We currently set the correct required_rubygems_version for content addressable gems during push by normalizing the current value. However in the RubyGems side ruby/rubygems#9773, we decided it would be better to set the required_rubygems_version in the gemspec during build. The server shouldn't need to adjust the gemspec value.

So instead of normalizing and injecting CONTENT_ADDRESSABLE_REQUIRED_RUBYGEMS_VERSION for require_rubygems_version, we should be solely validating the value and raising an error if it doesn't satisfy.

The alternative is to let the gem be registered as a fat gem if required_rubygems_version isn't satisfied, but if a gem is platformed and scoped to a Ruby ABI already ~> x.y.0, there's a high probability that they are creating a content addressable gem, and the gem owner would need to yank and release a new content addressable gem after finding that out.

Also changes CONTENT_ADDRESSABLE_REQUIRED_RUBYGEMS_VERSION to >= 4.1.0.a to align with the client requirement.

@jenshenny
jenshenny requested a balanced review from Copilot September 7, 2026 23:23
@jenshenny
jenshenny force-pushed the remove-rubygems-requirement-injection branch from 8da5d9f to 1c71f60 Compare September 7, 2026 23:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The push-path requirement handling lacks direct acceptance and rejection coverage, and the new user-facing error bypasses localization.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Validates that content-addressable gems require a compatible RubyGems version instead of modifying gemspec metadata server-side.

Changes:

  • Raises the required RubyGems floor to >= 4.1.0.a.
  • Adds model validation and copies the gemspec requirement before push validation.
  • Updates content-addressable test fixtures for the new requirement.
File summaries
File Description
app/models/version.rb Adds RubyGems floor validation.
app/models/pusher.rb Copies gemspec requirements before validation and removes normalization.
test/models/version_test.rb Tests floor validation and updates fixtures.
test/models/rubygem_test.rb Updates content-addressable fixtures.
test/models/pusher_test.rb Updates push mocks and fixtures.
test/models/gem_info_test.rb Updates compact-index fixture.
test/models/deletion_test.rb Updates deletion fixture.
test/models/concerns/compact_index_versions_test.rb Updates compact-index version fixtures.
test/integration/pusher_test.rb Replaces normalization expectations with preservation behavior.
test/functional/api/v2/versions_controller_test.rb Updates API version fixtures.
test/functional/api/v2/contents_controller_test.rb Updates content API fixtures.
test/functional/api/v1/deletions_controller_test.rb Updates deletion API fixtures.
Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/models/pusher.rb
Comment thread app/models/version.rb
@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.80%. Comparing base (0df4671) to head (d2a3219).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
app/models/version.rb 96.42% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6844   +/-   ##
=======================================
  Coverage   97.80%   97.80%           
=======================================
  Files         534      534           
  Lines       11774    11778    +4     
=======================================
+ Hits        11516    11520    +4     
  Misses        258      258           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jenshenny
jenshenny force-pushed the remove-rubygems-requirement-injection branch from 1c71f60 to d4e2564 Compare September 7, 2026 23:40
@jenshenny
jenshenny requested a balanced review from Copilot September 7, 2026 23:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The validation and push flow are consistent with the client requirement and adequately tested.

Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@jenshenny
jenshenny force-pushed the remove-rubygems-requirement-injection branch from d4e2564 to 744939c Compare September 7, 2026 23:56
Comment thread app/models/version.rb
requirements = required_rubygems_version.presence&.split(/\s*,\s*/) || [">= 0"]
requirement = Gem::Requirement.new(requirements)

requirement.requirements.any? do |operator, required_version|

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is a particularly a big deal because who would be doing this in a real world scenario, but figure it's worth calling out. Currently this field will be marked valid with any requirement as long as the requirement as a minimim floor of 4.1.0.a.

So a requirement >= 4.2, < 4.1.0.a would technically be valid... but why would you 🤷🏻

Is there a particular reason we would want to not allow this that i can't think of?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No reason, it's a good catch, we raise an error if there's a conflicting version in the client on build. So I updated this to match the same logic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, actually I revert that decision - we never really validate conflicting rubygems requirements in general (eg. = 4.2, != 4.2), maybe we should? I think that should be in another PR then. It makes sense to raise an error when building because we are modifying the requirement and maybe the user is unaware.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The validator calls an undefined helper and accepts some contradictory requirements.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread app/models/version.rb Outdated
Comment thread app/models/version.rb Outdated
@jenshenny
jenshenny force-pushed the remove-rubygems-requirement-injection branch from 5e6440b to d2a3219 Compare September 9, 2026 10:19
@jenshenny
jenshenny requested a balanced review from Copilot September 9, 2026 10:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The validation matches the RubyGems client requirement and is covered across model and push integration paths.

Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@jenshenny
jenshenny force-pushed the remove-rubygems-requirement-injection branch from d2a3219 to 744939c Compare September 9, 2026 10:29
@jenshenny
jenshenny merged commit 9d5892f into master Sep 9, 2026
34 checks passed
@jenshenny
jenshenny deleted the remove-rubygems-requirement-injection branch September 9, 2026 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants