Skip to content

fix: resolve proxy transport false-positives and windows linter issues - #3900

Open
Piyush0049 wants to merge 1 commit into
docker:mainfrom
Piyush0049:fix/proxy-and-windows-linters
Open

fix: resolve proxy transport false-positives and windows linter issues#3900
Piyush0049 wants to merge 1 commit into
docker:mainfrom
Piyush0049:fix/proxy-and-windows-linters

Conversation

@Piyush0049

Copy link
Copy Markdown
Contributor

Description

This pull request addresses a critical logic bug in the Docker Desktop proxy transport and resolves multiple linter failures specific to the Windows build.

Proxy Error Classification Fix

Before:
The isProxySocketError function used a broad string match for "connect: connection refused". When an agent attempted to reach an offline target service (e.g. dial tcp 127.0.0.1:8080: connect: connection refused), the transport misclassified this target host failure as a dead Docker Desktop proxy socket. This falsely triggered a 30-second global disablement of the proxy, routing all subsequent outbound agent traffic directly and bypassing Docker Desktop networking.

After:
Added an explicit guard to exclude direct target TCP dial errors. The function now correctly distinguishes between target host connection failures (dial tcp) and upstream proxy tunnel failures (proxyconnect tcp), ensuring the proxy is only disabled during genuine proxy socket outages.

Windows Linter Fixes

Resolved 7 linting issues on the Windows build:

  • pkg/selfupdate/exec_windows.go: Fixed errorlint warnings by changing %v to %w in fmt.Errorf calls. Added nolint:noctx annotation for exec.Command re-exec behavior.
  • pkg/tools/builtin/backgroundjobs/cmd_windows.go: Added nolint:gosec annotations for unsafe.Pointer Win32 struct conversions and process PID uint32 conversions.
  • pkg/tools/builtin/shell/cmd_windows.go: Added nolint:gosec annotations for identical Windows Job Object API calls.
  • pkg/model/provider/gemini/schema_boolean_test.go: Replaced implicit nil checks with require.NoError and require.NotNil to satisfy the staticcheck SA5011 control-flow analyzer.

Testing

  • Added 3 new unit test boundary cases in transport_test.go to explicitly verify target TCP refusal and proxy tunnel refusal logic.
  • Verified golangci-lint run passes with 0 issues across the entire repository.
  • Verified go test ./... passes cleanly on all modified packages.

@Piyush0049
Piyush0049 requested a review from a team as a code owner August 3, 2026 16:43
@aheritier aheritier added area/providers/gemini Google Gemini provider support area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Aug 3, 2026
@aheritier
aheritier requested a review from docker-agent August 4, 2026 07:23

@docker-agent docker-agent 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.

Assessment: 🟡 NEEDS ATTENTION

The proxy transport logic fix is well-motivated and the guard correctly preserves the main code paths (Unix socket and Windows HTTP CONNECT). One edge case remains: bare-TCP proxy connection failures (without the proxyconnect tcp prefix) are now silently classified as target-host errors rather than proxy failures, because "connect: connection refused" was removed from proxyErrorPatterns. See inline comment for details.


// Check for common proxy socket failure patterns
// Target TCP connection errors are target host errors, not proxy socket failures.
if strings.Contains(errStr, "dial tcp") && !strings.Contains(errStr, "proxyconnect tcp") {

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.

[medium] Removal of "connect: connection refused" pattern may leave bare-TCP proxy failures undetected

The PR removes "connect: connection refused" from proxyErrorPatterns (previously the safety net for the case where a socket exists but nothing is listening). The replacement relies on two assumptions: (1) every genuine proxy connection failure will contain "proxyconnect tcp", and (2) every "dial tcp … connect: connection refused" without "proxyconnect tcp" is a target-host error.

Assumption (1) holds for standard Go net/http HTTP-CONNECT proxy paths — those always produce a proxyconnect tcp: … prefix. However if the proxy transport is configured as a direct TCP dial (not an HTTP CONNECT tunnel), Go wraps the failure without that prefix, yielding a bare "dial tcp 127.0.0.1:<port>: connect: connection refused" error. That string hits the new early-return guard at line 152 and returns false, so disableProxy() is never called and the agent keeps retrying against the dead proxy.

In practice this codebase appears to go through HTTP CONNECT, so the risk is low — but the gap is real and there is no test covering the bare-TCP proxy case. Consider re-adding the pattern with a tighter guard (e.g. only match when the error also contains "proxyconnect tcp" or "dial unix"), or add a comment documenting why bare-TCP proxy failures cannot occur here.

Confidence Score
🟡 moderate 60/100

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

The proxy transport fix (isProxySocketError) correctly distinguishes target-host TCP dial errors from upstream proxy tunnel failures using the "dial tcp" / "proxyconnect tcp" guard. The three new test cases cover the key boundary conditions. Windows linter suppressions (nolint:gosec, nolint:noctx) are appropriate for the contexts. The %w error-wrapping changes are valid under Go 1.26.5.

Lower-confidence findings (not posted inline)

  • [medium] pkg/desktop/transport/transport.go:152"connect: connection refused" removed from proxyErrorPatterns — may affect proxy fallback when Unix socket exists but is not listening (confidence: 🟠 weak 52/100)

    The PR removes the "connect: connection refused" pattern. For Unix socket errors, Go's net package typically formats them as "dial unix /path/to/socket: connect: connection refused", which still matches the remaining "dial unix" pattern — so on Linux/macOS this is likely safe. However, the safety of the removal depends on Go runtime error-message formatting across all supported platforms; if an OS or wrapper ever produces a bare "connect: connection refused" without the "dial unix" prefix, proxy socket outages during startup (socket file present, process not yet listening) would now fail hard rather than falling back to direct transport. Worth a manual cross-platform spot-check before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/providers/gemini Google Gemini provider support area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants