Make proxy mode default for npm based managers#148
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 37.38% 37.39% +0.01%
==========================================
Files 84 84
Lines 5107 5108 +1
==========================================
+ Hits 1909 1910 +1
Misses 3021 3021
Partials 177 177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Devin Review found 1 potential issue.
🟡 1 issue in files not directly in the diff
🟡 Info command displays incorrect Proxy Mode status by only checking ExperimentalProxyMode (cmd/setup/info.go:38)
The pmg setup info command displays the Proxy Mode status incorrectly by only checking ExperimentalProxyMode instead of using the IsProxyModeEnabled() helper function.
Click to expand
Root Cause
At cmd/setup/info.go:38, the code displays:
configEntries["Proxy Mode"] = strconv.FormatBool(cfg.Config.ExperimentalProxyMode)However, the actual proxy mode determination at config/config.go:160-161 uses:
func (r *RuntimeConfig) IsProxyModeEnabled() bool {
return (r.Config.ExperimentalProxyMode || r.Config.ProxyMode)
}Impact
With the new default configuration where ProxyMode: true and ExperimentalProxyMode: true, users who customize their config to only set proxy_mode: true (without experimental_proxy_mode) will see "Proxy Mode: false" in the info output even though proxy mode is actually enabled. This creates confusion about the actual system state.
Expected vs Actual
- Expected: Info command should show "Proxy Mode: true" when
IsProxyModeEnabled()returns true - Actual: Info command shows "Proxy Mode: false" if only
proxy_modeis set to true in config
Recommendation: Change line 38 to use the helper function: configEntries["Proxy Mode"] = strconv.FormatBool(cfg.IsProxyModeEnabled())
View issue and 4 additional flags in Devin Review.
SafeDep Report SummaryNo dependency changes detected. Nothing to scan. This report is generated by SafeDep Github App |
There was a problem hiding this comment.
Pull request overview
Updates PMG configuration and npm-family command execution so proxy-based interception is enabled by default, and aligns the E2E workflow/docs with that default.
Changes:
- Switch default proxy mode to enabled in the config template and runtime defaults.
- Adjust npm ecosystem command flows (npm/pnpm/yarn/bun and npx/pnpx) to treat proxy mode as the default execution path.
- Update README feature wording and E2E workflow steps to remove reliance on the experimental proxy flag.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| config/config.template.yml | Makes proxy_mode default to true and updates related config comments. |
| config/config.go | Changes runtime defaults around proxy mode. |
| cmd/npm/npm.go | Uses proxy flow by default when proxy mode is enabled. |
| cmd/npm/pnpm.go | Uses proxy flow by default when proxy mode is enabled. |
| cmd/npm/yarn.go | Uses proxy flow by default when proxy mode is enabled. |
| cmd/npm/bun.go | Uses proxy flow by default when proxy mode is enabled. |
| cmd/executors/npx.go | Uses proxy flow by default when proxy mode is enabled. |
| cmd/executors/pnpx.go | Uses proxy flow by default when proxy mode is enabled. |
| README.md | Updates feature bullet to reflect proxy-based analysis. |
| .github/workflows/pmg-e2e.yml | Removes --experimental-proxy-mode usage to reflect new default behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sahil Bansal <bansalsahil315@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sahil Bansal <bansalsahil315@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sahil Bansal <bansalsahil315@gmail.com>
Closes #140