v0.5: sentinel errors, KDE Get/GetConfig, PAC in GetConfig, WithProxyMulti#9
Merged
Conversation
…Multi Bugfixes: - darwin: GetConfig returned SOCKS with http:// scheme instead of socks5:// - WithProxy restored only the HTTP field; now snapshots and restores the full ProxyConfig (HTTP, HTTPS, SOCKS, NoProxy, PAC) via GetConfig + SetMulti/SetPAC New API (additive, backwards compatible): - ErrProxyNotSet, ErrProxyNotEnabled, ErrUnsupportedPlatform, ErrToolMissing sentinel errors, matchable via errors.Is - RequiresElevation(err) alongside IsNonCritical(err) — distinguishes "sudo would have helped" from generic partial success - ProxyConfig.PAC — populated by GetConfig when the OS is in auto-proxy mode on all four platforms (macOS -getautoproxyurl, Windows AutoConfigURL, GNOME autoconfig-url, KDE Proxy Config Script) - WithProxyMulti — multi-protocol variant of WithProxy with the same full-config restore semantics KDE (Linux): - getGlobalKDE / getGlobalConfigKDE via kreadconfig5 / kreadconfig6 - Falls back from GNOME when gsettings is unavailable or mode=none - ProxyType mapping: 0=none, 1=manual, 2=PAC Tests: - FuzzParse, FuzzValidateProxyURL, FuzzValidatePACURL - internal/buildinfo covered 0% -> 100% - KDE normalize helpers, parseAutoProxyOutput, parseWindowsProxyServer / extractRegValue (moved to build-tag-free file so they run on every platform) - Coverage: main package 74.7% -> 75.4%
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR evolves sysproxy v0.5 by adding additive APIs (sentinel errors, ProxyConfig.PAC, WithProxyMulti, elevation classification), improving cross-desktop Linux support (KDE read-back), and enhancing GetConfig to surface PAC configuration across platforms while adding broader tests (unit + fuzz).
Changes:
- Add sentinel errors (
ErrProxyNotSet,ErrProxyNotEnabled,ErrUnsupportedPlatform,ErrToolMissing) plusRequiresElevation, and update callers/tests to useerrors.Is. - Add
ProxyConfig.PAC, implement PAC detection inGetConfig(macOS/Windows/GNOME/KDE), and addWithProxyMultiwith full-config snapshot/restore. - Add KDE (Plasma 5/6)
Get/GetConfigviakreadconfig{5,6}, plus new fuzz tests for parsing/validation surfaces.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| windows_parse.go | Introduces parsing helpers for Windows registry proxy values. |
| windows_parse_test.go | Unit tests for Windows parsing helpers. |
| sysproxy.go | Expands ProxyConfig with PAC and adds snapshot-based WithProxy* restore logic + WithProxyMulti. |
| sysproxy_windows.go | Adds PAC read-back (AutoConfigURL) and sentinel error returns for Windows Get/GetConfig. |
| sysproxy_test.go | Updates WithProxy tests to validate full snapshot/restore behavior, adds WithProxyMulti tests. |
| sysproxy_other.go | Wraps unsupported-platform errors with sentinel ErrUnsupportedPlatform. |
| sysproxy_linux.go | Adds KDE read-back fallback, sentinel errors, and GNOME auto-mode PAC support in GetConfig. |
| sysproxy_darwin.go | Fixes SOCKS scheme in GetConfig and adds PAC read-back on macOS. |
| sysproxy_darwin_test.go | Adds tests for parsing networksetup -getautoproxyurl output. |
| README.md | Documents ProxyConfig.PAC, WithProxy full snapshot behavior, WithProxyMulti, and sentinel error usage. |
| kde_linux.go | New KDE (kioslaverc) reader helpers for Get/GetConfig. |
| kde_linux_test.go | Unit tests for KDE URL normalization helpers. |
| internal/buildinfo/buildinfo_test.go | Adds coverage for buildinfo Summary() defaults/overrides. |
| fuzz_test.go | Adds fuzz tests for parse/validation functions to ensure no panics and consistent acceptance. |
| errors.go | Defines sentinel errors and elevation classification (RequiresElevation). |
| errors_test.go | Tests sentinel distinctness and elevation classification helpers. |
| appconfig.go | Wraps “tool missing” errors with ErrToolMissing for git/npm helpers. |
| appconfig_test.go | Updates tests to assert errors.Is(err, ErrToolMissing) for missing tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+289
to
+303
| if snap.PAC != "" { | ||
| if err := SetPAC(snap.PAC, scope); err != nil && !IsNonCritical(err) { | ||
| logf("WithProxy: restore SetPAC failed: %v", err) | ||
| } | ||
| return | ||
| } | ||
| if snap.HTTP == "" && snap.HTTPS == "" && snap.SOCKS == "" { | ||
| if err := Unset(scope); err != nil && !IsNonCritical(err) { | ||
| logf("WithProxy: restore Unset failed: %v", err) | ||
| } | ||
| return | ||
| } | ||
| if err := SetMulti(snap, scope); err != nil && !IsNonCritical(err) { | ||
| logf("WithProxy: restore SetMulti failed: %v", err) | ||
| } |
Comment on lines
+105
to
111
| func readAutoConfigURL(ctx context.Context) (string, error) { | ||
| out, err := exec.CommandContext(normalizeContext(ctx), "reg", "query", regKey, "/v", "AutoConfigURL").Output() | ||
| if err != nil { | ||
| return "", nil //nolint:nilerr | ||
| } | ||
| return extractRegValue(string(out), "AutoConfigURL"), nil | ||
| } |
Comment on lines
158
to
164
| server := extractRegValue(string(out), "ProxyServer") | ||
| if server == "" { | ||
| return ProxyConfig{}, fmt.Errorf("sysproxy: proxy not set") | ||
| return ProxyConfig{}, ErrProxyNotSet | ||
| } | ||
| cfg := parseWindowsProxyServer(server) | ||
| cfg.PAC = pac | ||
|
|
Comment on lines
+159
to
+161
| if pac, ok := readMacPAC(ctx, svc); ok { | ||
| cfg.PAC = pac | ||
| } |
Comment on lines
+79
to
+80
| return "", ErrProxyNotSet | ||
| } |
Comment on lines
+144
to
+145
| return ProxyConfig{}, ErrProxyNotSet | ||
| } |
Comment on lines
133
to
+137
| "proxy not set" / "proxy not enabled" even though PAC is active. | ||
|
|
||
| ### Temporary proxy | ||
|
|
||
| `WithProxy` sets the proxy for the duration of `fn` and restores the previous state on return — even if `fn` returns an error. | ||
| `WithProxy` sets the proxy for the duration of `fn` and restores the previous state on return — even if `fn` returns an error. The snapshot covers the full `ProxyConfig` (HTTP + HTTPS + SOCKS + NoProxy + PAC), not just the HTTP field. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
http://scheme instead ofsocks5://;WithProxyrestored only HTTP — now snapshots and restores the fullProxyConfig(HTTP + HTTPS + SOCKS + NoProxy + PAC).ErrProxyNotSet,ErrProxyNotEnabled,ErrUnsupportedPlatform,ErrToolMissing),RequiresElevation,ProxyConfig.PAC,WithProxyMulti.Get/GetConfigviakreadconfig5/kreadconfig6, fallback po GNOME.GetConfigna wszystkich platformach (macOS-getautoproxyurl, WindowsAutoConfigURL, GNOMEautoconfig-url, KDEProxy Config Script).FuzzParse,FuzzValidateProxyURL,FuzzValidatePACURL).internal/buildinfocoverage 0% → 100%; main package 74.7% → 75.4%.Breaking changes
None. All new API is additive. Error messages for the "tool not found in PATH" case were reworded but now wrap
ErrToolMissing— callers that were string-matching should switch toerrors.Is.Test plan
go test ./... -race -count=1on darwinGOOS={linux,windows,darwin,freebsd} go build ./...golangci-lint run ./...— 0 issuesgo test -fuzz Fuzz{Parse,ValidateProxyURL,ValidatePACURL} -fuzztime 3seach — passGetConfigwhenAutoConfigURLset)GetConfigoutput)