Skip to content

Conversation

@philippe-granet
Copy link

@philippe-granet philippe-granet commented Nov 12, 2025

Description

Enhance setup_proxy Invoke-Git functions to handle no_proxy settings

Motivation and Context

Closes #6549

How Has This Been Tested?

In our network, where we need to use proxy to access to Internet, and have internal buckets that can't be reached with the proxy, and this configuration in Scoop config.json:

{
    "proxy":  "<proxy-server>:<proxy-port>",
    "no_proxy":  "localhost, 127.0.0.1, .private.domain.com"
}

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Summary by CodeRabbit

  • Chores
    • Enhanced proxy handling to honor NO_PROXY: specified hosts/domains now bypass proxy for Git operations (including background jobs) and for web requests; NO_PROXY values are propagated into background processes and applied to the default web proxy bypass list.

✏️ Tip: You can customize this high-level summary in your review settings.

Add support for NO_PROXY environment variable in git operations.
Add support for no_proxy configuration in setup_proxy function.
@coderabbitai
Copy link

coderabbitai bot commented Nov 12, 2025

Walkthrough

Adds NO_PROXY handling: read from config, propagate into Git background jobs (set $env:NO_PROXY inside jobs), and configure the system web proxy bypass list from NO_PROXY in setup_proxy.

Changes

Cohort / File(s) Summary
Git proxy propagation
lib/core.ps1
Retrieve NO_PROXY via get_config NO_PROXY; when starting background jobs for Git operations, pass using:no_proxy into the job and set $env:NO_PROXY inside the job if present.
Web proxy bypass list
lib/download.ps1
Read NO_PROXY into $no_proxy; split comma-separated entries into a bypass list and assign to [net.webrequest]::defaultwebproxy.BypassList (applied after proxy credential handling).

Sequence Diagram(s)

sequenceDiagram
    participant Config as Config
    participant Core as core.ps1\n(Invoke-Git)
    participant Job as Background Job\n(Git worker)
    participant Download as download.ps1\n(setup_proxy)
    participant Proxy as System Proxy

    Config->>Core: get_config NO_PROXY
    Core->>Job: Start-Job (pass using:no_proxy)
    Job->>Job: if no_proxy set -> $env:NO_PROXY = no_proxy

    Config->>Download: get_config NO_PROXY
    Download->>Download: if no_proxy -> split by ","
    Download->>Proxy: [net.webrequest]::defaultwebproxy.BypassList = list
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus review on:
    • Start-Job variable capture and correct use of using: to ensure jobs receive the value.
    • Trimming whitespace and filtering empty entries when splitting NO_PROXY.
    • Behavior and compatibility of assigning BypassList on [net.webrequest]::defaultwebproxy across supported PowerShell/.NET versions.

Poem

🐰 I nibble configs, tidy and spry,
NO_PROXY hops where proxies shouldn't ply.
Jobs learn the path, bypassing the fence,
Downloads skirt tunnels, nimble and tense,
A cheerful patch — a rabbit's small pry. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding no_proxy configuration support to handle proxy bypass functionality.
Linked Issues check ✅ Passed The PR implementation aligns with issue #6549 requirements: NO_PROXY configuration is retrieved, propagated to background jobs, and integrated into proxy setup to bypass specified domains.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing no_proxy support in core.ps1 and download.ps1 as required by the linked issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b5fc91 and f1f9b5f.

📒 Files selected for processing (2)
  • lib/core.ps1 (2 hunks)
  • lib/download.ps1 (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/core.ps1
  • lib/download.ps1

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.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
lib/download.ps1 (1)

576-579: Improve string handling for NO_PROXY bypass list.

The current implementation could produce unexpected results if the NO_PROXY configuration contains leading/trailing whitespace or empty values. Consider trimming the entire string and filtering out empty entries.

Apply this diff to improve robustness:

         if ($no_proxy) {
-            $bypass_list = $no_proxy -split "\s*,\s*"
+            $bypass_list = $no_proxy.Trim() -split "\s*,\s*" | Where-Object { $_ -ne '' }
             [net.webrequest]::defaultwebproxy.BypassList = $bypass_list
         }

This ensures:

  • Leading/trailing whitespace from the entire string is removed
  • Empty elements from consecutive commas or malformed input are filtered out
  • The BypassList receives only valid, non-empty entries
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e698cc and 69dd3de.

📒 Files selected for processing (2)
  • lib/core.ps1 (2 hunks)
  • lib/download.ps1 (2 hunks)
🔇 Additional comments (2)
lib/download.ps1 (1)

554-554: Good: NO_PROXY configuration retrieval.

The retrieval of the NO_PROXY configuration is correctly placed and follows the same pattern as the PROXY configuration.

lib/core.ps1 (1)

235-235: NO_PROXY implementation is correct and properly propagated.

Verification confirms the implementation follows best practices:

  • Line 235 correctly retrieves NO_PROXY from configuration using the standard get_config pattern
  • Line 250 properly passes NO_PROXY to the background job via $using: scope
  • Line 256-258 conditionally sets $env:NO_PROXY only when the value is non-empty—more robust than the unconditional HTTP_PROXY/HTTPS_PROXY handling
  • NO_PROXY is applied only within background jobs for network-dependent Git operations (clone, checkout, pull, fetch, ls-remote)

The implementation is complete and correct. No changes required.

Trimmed whitespace from no_proxy variable before splitting, as reported by @coderabbitai
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.

1 participant