fix(importer): enable proxy auto-detection for git-based importers#2488
fix(importer): enable proxy auto-detection for git-based importers#2488mrrajan wants to merge 2 commits into
Conversation
Reviewer's GuideEnables libgit2-based Git importers to respect HTTP(S)_PROXY environment variables by configuring proxy auto-detection in fetch options, migrates logging in the Git walker to tracing, and adds a regression test that validates proxy handling behavior. Sequence diagram for GitWalker fetch options with proxy auto-detectionsequenceDiagram
participant GitWalker
participant FetchOptions
participant ProxyOptions
GitWalker->>GitWalker: run
GitWalker->>GitWalker: run_sync
GitWalker->>GitWalker: create_fetch_options
GitWalker->>FetchOptions: FetchOptions::new
GitWalker->>FetchOptions: remote_callbacks
GitWalker->>FetchOptions: depth
GitWalker->>ProxyOptions: ProxyOptions::new
ProxyOptions->>ProxyOptions: auto
ProxyOptions->>FetchOptions: proxy_options
GitWalker->>GitWalker: clone_or_update_repo
GitWalker->>GitWalker: find_changes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
unsafeblocks aroundstd::env::set_var/remove_varintest_walker_honors_proxyare unnecessary and misleading, since these APIs are safe; you can drop theunsafeand keep the explanatory comment. - The new
test_walker_honors_proxytest depends on real network access and an external GitHub repository plus shared process env vars, which can introduce flakiness and test ordering issues; consider isolating it via a local git server/mock or ensuring it is marked/structured to run serially and without external dependencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `unsafe` blocks around `std::env::set_var`/`remove_var` in `test_walker_honors_proxy` are unnecessary and misleading, since these APIs are safe; you can drop the `unsafe` and keep the explanatory comment.
- The new `test_walker_honors_proxy` test depends on real network access and an external GitHub repository plus shared process env vars, which can introduce flakiness and test ordering issues; consider isolating it via a local git server/mock or ensuring it is marked/structured to run serially and without external dependencies.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
The change itself looks good. I'd appreciate the tracing change to be in a dedicated commit, as they do not belong to the actual change (proxy). Settings env-vars during test is a no-go from my point of view.
| // Given: HTTP_PROXY is set to a non-routable address | ||
| // SAFETY: This test must not run concurrently with other tests that | ||
| // depend on proxy env vars. Run with `--test-threads=1` if needed. | ||
| unsafe { |
There was a problem hiding this comment.
This is a problem. Tests will run in parallel and so that would effect all other running tests. I know the comment states that. But we just can't to that.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — maintainer explicitly rejected process-global env var modification in tests due to parallel test safety. Sub-task created to rewrite the test without set_var/remove_var.
Verification Report for TC-5174 (commit 469af8a)
Overall: WARNProxy fix is approved — maintainer confirmed "The change itself looks good." Two issues must be resolved before merge:
Sub-tasks created:
This comment was AI-generated by sdlc-workflow/verify-pr v0.11.0. |
|
[sdlc-workflow/verify-pr] Re: @ctron review — Classified as code change request (2 items):
|
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — Classified as suggestion (2 items):
|
| /// is set. Uses a subprocess trampoline to avoid modifying process-global env vars: | ||
| /// the outer test spawns itself as a child process with HTTPS_PROXY pointing to a | ||
| /// non-routable address; the inner run performs the actual clone attempt. | ||
| #[test] |
There was a problem hiding this comment.
just an observation - this test might fail if git clone tests are run in an env with no egress ... maybe we have other tests that would fail similarly so.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as question — observation about network dependency in no-egress environments; no code change requested. No sub-task created.
| std::fs::remove_dir_all(&path).unwrap(); | ||
| } | ||
| let walker = | ||
| GitWalker::new("https://github.com/RConsortium/r-advisory-database", ()) |
There was a problem hiding this comment.
do we really want to bake this into test ?
There was a problem hiding this comment.
No, I don't think we should.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — maintainer agrees external URL should not be hardcoded in test. Covered by existing sub-task TC-5176 (updated: remove test entirely per maintainer direction).
rh-jfuller
left a comment
rh-jfuller
left a comment
There was a problem hiding this comment.
LGTM however it does impose proxy eg. maybe we need a switch to say --no-proxy to ensure to not use proxy when not desired - this might expand the scope of this fix.
| "expected clone to fail through unreachable proxy, but it succeeded" | ||
| ); | ||
| let stderr_lower = stderr.to_lowercase(); | ||
| assert!( |
There was a problem hiding this comment.
I don't like this one. I think the stderr should be predictable. That is a bit broad.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — stderr assertion is too broad and unpredictable across platforms. Covered by existing sub-task TC-5176 (updated: remove test entirely per maintainer direction).
| fn test_walker_honors_proxy() { | ||
| // When the sentinel env var is set, we are the inner subprocess — run the | ||
| // actual clone and let the result (panic or success) determine the exit code. | ||
| if std::env::var("__TRUSTIFY_TEST_PROXY_INNER").is_ok() { |
There was a problem hiding this comment.
I think the idea is valid. But I'd like to avoid building that ourselves. Here's an idea from ChatGPT:
| Crate | Style | Best for | Notable features |
|---|---|---|---|
subprocess-test |
Attribute macro | Running an individual test body in a subprocess | Process isolation, output/result verification, crash/exit testing |
sealed_test |
Attribute macro | Isolated tests with environment variables and filesystem setup | Declarative env vars, temporary directory, fixtures, setup/teardown |
rusty-fork |
Test-wrapping macro | General subprocess-isolated tests | Mature implementation, timeouts, crash/abort isolation |
Recommendation: use subprocess-test for straightforward process isolation, or sealed_test when declarative environment setup is useful.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes specific crates for test subprocess isolation. Moot since test is being removed per maintainer direction (see TC-5176). No sub-task created.
Add ProxyOptions::auto() to FetchOptions in GitWalker so that libgit2 honors HTTP_PROXY/HTTPS_PROXY environment variables when cloning repositories. Without this, git-based importers (CVE, OSV-GitHub) fail with connection timeouts in proxy-required environments because libgit2 defaults to GIT_PROXY_NONE. The reproducer test uses a subprocess trampoline to verify proxy settings are honored without modifying process-global environment variables. Implements TC-5174 Assisted-by: Claude Code
|
[sdlc-workflow/verify-pr] Re: @rh-jfuller review — Classified as suggestion — proposes adding a --no-proxy switch to control proxy behavior. This would expand scope beyond the current bug fix (TC-5174 addresses the missing auto-detection only). No sub-task created. |
Verification Report for TC-5174 (commit 8a4cdf6)
Overall: WARNThe core proxy fix is correct — WARN due to two blocking sub-tasks:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.2. |
The test's maintenance cost outweighs the regression risk for a 3-line proxy fix: it depends on network access to an external GitHub repository, its stderr assertion is fragile across platforms, and the subprocess trampoline is disproportionately complex. The git2 FetchOptions API does not expose proxy state, so there is no clean way to unit-test the configuration without hitting the network. This is consistent with HTTP-based importers (reqwest), which also rely on implicit proxy auto-detection with no proxy-specific tests. Implements TC-5176 Assisted-by: Claude Code
|
@rh-jfuller @ctron I have removed the unit tests for the proxy validation, Please let me know your thoughts. |
3 participants
Summary
ProxyOptions::auto()toFetchOptionsinGitWalker::create_fetch_options()so that libgit2 honorsHTTP_PROXY/HTTPS_PROXYenvironment variables when cloning repositorieslog::calls ingit.rstotracing::per project conventions (separate commit)test_walker_honors_proxyper maintainer feedback — the test's maintenance cost (network dependency, fragile assertions, subprocess complexity) outweighs the regression risk for a 3-line fixBug Context
Git-based importers (CVE, OSV-GitHub) fail with connection timeouts in environments that require an authenticated forward proxy. libgit2 defaults to
GIT_PROXY_NONEwhenProxyOptionsis not explicitly set, causing it to ignore all proxy environment variables. HTTP-based importers usingreqwestare unaffected becausereqwestauto-detects proxy settings.Commits
fix(importer): enable proxy auto-detection for git-based importers— proxy fix + reproducer testrefactor(importer): migrate log:: to tracing:: in git walker— tracing migration per CONVENTIONS.mdfix(importer): remove test_walker_honors_proxy per maintainer feedback— remove test per review feedbackTest plan
test_walkertest passestest_walker_fail_escapetest passescargo clippypasses with strict flagsProxyOptions::auto()) remains intactImplements TC-5174
Implements TC-5176