Skip to content

fix(plugins): drop a trailing slash from pypi_url before adding /simple - #4081

Merged
mcosgriff merged 1 commit into
2867-pypi_url-not-passed-to-uv-during-docker-image-builds-breaking-air-gappedproxy-environmentsfrom
pypi-url-trailing-slash
Oct 16, 2026
Merged

mcosgriff merged 1 commit into
2867-pypi_url-not-passed-to-uv-during-docker-image-builds-breaking-air-gappedproxy-environmentsfrom
pypi-url-trailing-slash

Conversation

@jmthomas

@jmthomas jmthomas commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

Stacked on #3891.

#3891 makes the image builds tolerate a trailing slash on PYPI_URL (${PYPI_URL%/}/simple), but runtime plugin and package installs appended /simple to the raw pypi_url setting or PYPI_URL value. A value ending in / gave https://mirror//simple. https://pypi.org/ also stopped matching PypiUrl::DEFAULT, so uv got --no-config (and --no-sources) for the public index.

seed_database copies PYPI_URL into the setting verbatim, so a trailing slash the build now accepts would reach runtime installs.

Changes

  • New PypiUrl.index_url strips trailing slashes, then adds /simple
  • PluginModel.resolve_pypi_url and PythonPackageModel.install use it for both the setting and the PYPI_URL fallback

Testing

  • New specs for index_url plus trailing-slash cases for both models; the 4 new model specs fail without the fix
  • bundle exec rspec spec/utilities/pypi_url_spec.rb spec/models/plugin_model_spec.rb spec/models/python_package_model_spec.rb: 135 examples, 0 failures

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • PyPI package installation now handles index URLs with trailing slashes consistently, avoiding malformed package index addresses.
    • The default PyPI address is recognized consistently when configured with a trailing slash, preserving the standard installation behavior.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 22:05

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7117ecc2-10e7-4832-9559-426067b36a2b
📥 Commits

Reviewing files that changed from the base of the PR and between d97ecfc and 83fb9dc.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0a89b91c-bcb5-4857-b0cf-0189e3a633a1
📥 Commits

Reviewing files that changed from the base of the PR and between 77879cf and d97ecfc.

📒 Files selected for processing: 6
  • openc3/lib/openc3/models/plugin_model.rb
  • openc3/lib/openc3/models/python_package_model.rb
  • openc3/lib/openc3/utilities/pypi_url.rb
  • openc3/spec/models/plugin_model_spec.rb
  • openc3/spec/models/python_package_model_spec.rb
  • openc3/spec/utilities/pypi_url_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

The change adds a shared PyPI index URL normalizer. Plugin URL resolution and Python package installation use it for configured and environment-provided URLs. Specs cover trailing-slash normalization, nil input, and the public index default.

Changes: PyPI index URL normalization

Layer / File(s) Summary
Normalize PyPI index URLs
openc3/lib/openc3/utilities/pypi_url.rb, openc3/spec/utilities/pypi_url_spec.rb
PypiUrl.index_url removes trailing slashes and appends /simple. Specs cover the normalized URL, the public index default, and nil input.
Use normalized URLs in PyPI workflows
openc3/lib/openc3/models/plugin_model.rb, openc3/lib/openc3/models/python_package_model.rb, openc3/spec/models/plugin_model_spec.rb, openc3/spec/models/python_package_model_spec.rb
Plugin URL resolution and package installation pass configured and environment-provided URLs through PypiUrl.index_url. Specs cover trailing slashes and confirm that the public index default omits --no-config.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mcosgriff

Merge Risk: ⚪ Minimal · up to d97ec

Configured and environment-provided package indexes are normalized consistently, preventing a trailing slash from producing a doubled /simple path. The reviewed runtime paths are covered, and no actionable merge-blocking risk remains.

Security architecture risk: 🔵 Low · up to d97ec

This is a narrow change, but recognizing a trailing-slash public PyPI URL as the default also changes which package-source settings installers honor. Private indexes retain their existing restrictions, and final URL validation remains in place. No introduced security defect was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The inspected installation sinks include per-plugin virtual environments and the legacy shared Python environment. Plugin installation can fall back to the shared environment after per-plugin installation fails, so the existing workflow is not confined to a per-plugin security sandbox.

Trust Boundaries and Controls

  • observed — Final validation still requires an HTTP(S) URI with a non-empty host and falls back to DEFAULT for invalid URLs. Non-default normalized indexes still receive --no-config; resolving wrapper commands derive --no-sources from that flag. Normalization does not introduce another input source or remove this validation boundary.

Resilience and Maintainability Implications

  • observed — The inspected retry paths retain their selected source arguments: offline-to-online retry reuses the same command arguments, and plugin fallback to shared installation reuses the previously built PyPI arguments. Normalization does not add a retry-specific policy downgrade.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: removing a trailing slash from pypi_url before appending /simple.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6 files.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Comment thread openc3/lib/openc3/utilities/pypi_url.rb Dismissed
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.52%. Comparing base (c64afad) to head (83fb9dc).

Additional details and impacted files
@@                                                         Coverage Diff                                                         @@
##           2867-pypi_url-not-passed-to-uv-during-docker-image-builds-breaking-air-gappedproxy-environments    #4081      +/-   ##
===================================================================================================================================
- Coverage                                                                                            80.57%   80.52%   -0.06%     
===================================================================================================================================
  Files                                                                                                  904      904              
  Lines                                                                                                68996    68993       -3     
  Branches                                                                                              2681     2681              
===================================================================================================================================
- Hits                                                                                                 55592    55554      -38     
- Misses                                                                                               12741    12773      +32     
- Partials                                                                                               663      666       +3     
Flag Coverage Δ
frontend 67.30% <ø> (-0.13%) ⬇️
python 80.53% <ø> (+<0.01%) ⬆️
ruby-api 82.80% <ø> (-0.43%) ⬇️
ruby-backend 86.10% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@mcosgriff
mcosgriff added this pull request to stack #4084 October 16, 2026 18:09

@mcosgriff mcosgriff 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.

A pypi_url setting or PYPI_URL ending in / produced //simple, and
https://pypi.org/ no longer matched the default, so uv got --no-config
for the public index. Normalize the same way the image builds do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mcosgriff
mcosgriff force-pushed the pypi-url-trailing-slash branch from d97ecfc to 83fb9dc Compare October 16, 2026 20:08
@mcosgriff
mcosgriff merged commit 7ad1f7f into main Oct 16, 2026
28 of 29 checks passed
@mcosgriff
mcosgriff deleted the pypi-url-trailing-slash branch October 16, 2026 20:10
@sonarqubecloud

Copy link
Copy Markdown

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.

4 participants