Skip to content

fix: avoid DNS lookup after proxied release CONNECT#506

Merged
jahooma merged 1 commit intoCodebuffAI:mainfrom
YoungSx:fix/release-proxy-dns-regression
Apr 18, 2026
Merged

fix: avoid DNS lookup after proxied release CONNECT#506
jahooma merged 1 commit intoCodebuffAI:mainfrom
YoungSx:fix/release-proxy-dns-regression

Conversation

@YoungSx
Copy link
Copy Markdown
Contributor

@YoungSx YoungSx commented Apr 16, 2026

Summary

  • extract the release launcher HTTPS proxy logic into dedicated http.js helpers for codebuff, codecane, and freebuff
  • switch proxied requests from a raw createConnection callback to a custom https.Agent, so requests reuse the CONNECT tunnel without falling back to local DNS lookup
  • add a regression test covering all three release helpers and include the helper file in each published npm package

Verification

  • bun --cwd cli test src/__tests__/release/proxy-http-get.test.ts
  • bun run --cwd cli typecheck
  • manual live check: patched freebuff/cli/release/http.js fetched https://registry.npmjs.org/freebuff/latest successfully through the configured proxy (status 200)

Root cause

The release launchers established the proxy CONNECT tunnel correctly, but then passed a raw createConnection callback into https.get. In proxy environments like WSL, Node still attempted a fresh hostname lookup for registry.npmjs.org, causing first-run startup to fail with EAI_AGAIN even though the proxy tunnel was already open.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 16, 2026

Greptile Summary

This PR fixes a DNS-lookup regression in the release launchers for codebuff, codecane, and freebuff: the launchers were correctly establishing a CONNECT tunnel through the configured HTTP proxy, but then supplying a raw createConnection callback to https.get, which still triggered a local DNS resolution for the target hostname (registry.npmjs.org) in environments like WSL where that lookup fails with EAI_AGAIN.

The fix extracts all proxy logic into a new ./http.js helper (createReleaseHttpClient) and replaces the createConnection callback with a custom TunnelAgent that subclasses https.Agent. Because https.Agent owns the connection lifecycle, Node no longer performs a DNS lookup — it calls TunnelAgent.createConnection directly, which wraps the already-open tunnel socket in TLS.

Issues found:

  • The http and https Node built-in imports remain in all three index.js files after the proxy functions were moved to http.js; both are now unused and should be removed
  • Each redirect hop establishes a new CONNECT tunnel (fine for a release launcher, but worth being aware of)
  • The test calls createConnection without a callback, so the settled-guard / event-listener code path inside TunnelAgent.createConnection is not covered

Confidence Score: 5/5

Safe to merge — the core fix is correct, well-tested, and addresses a real proxy environment regression without changing any non-release-launcher code paths.

The approach (custom https.Agent instead of createConnection callback) is the standard Node.js idiom for tunneled HTTPS and correctly eliminates the spurious DNS lookup. Tests cover the primary invariant across all three packages. The only remaining items are cosmetic (dead imports) and a minor test coverage gap in the callback path — neither affects runtime correctness.

The three index.js files (cli/release/index.js, cli/release-staging/index.js, freebuff/cli/release/index.js) each retain unused http and https require calls that should be cleaned up.

Important Files Changed

Filename Overview
cli/release/http.js New helper: extracts proxy logic into createReleaseHttpClient; fixes the DNS-lookup-after-CONNECT bug by using a custom TunnelAgent instead of createConnection callback. Core logic is correct.
cli/release-staging/http.js Identical copy of cli/release/http.js for the staging release package. Same fix applied correctly.
freebuff/cli/release/http.js Identical copy of cli/release/http.js for the freebuff release package. Same fix applied correctly.
cli/release/index.js Removes inline proxy functions and delegates to createReleaseHttpClient. Leaves unused http and https require calls (confirmed by search).
cli/release-staging/index.js Same cleanup as cli/release/index.js; also retains unused http and https imports.
freebuff/cli/release/index.js Same cleanup as cli/release/index.js; also retains unused http and https imports.
cli/src/tests/release/proxy-http-get.test.ts Good regression test covering all three helpers; verifies agent-based approach over createConnection. Callback path of TunnelAgent.createConnection is not exercised.
cli/release/package.json Adds http.js to the published files list so it's included in the npm package.
cli/release-staging/package.json Adds http.js to the published files list for the staging package.
freebuff/cli/release/package.json Adds http.js to the published files list for the freebuff package.

Sequence Diagram

sequenceDiagram
    participant L as release/index.js
    participant H as http.js (createReleaseHttpClient)
    participant P as HTTP Proxy
    participant R as registry.npmjs.org

    L->>H: httpGet(url)
    H->>H: buildRequestOptions(url)
    H->>H: getProxyUrl() → HTTPS_PROXY
    H->>P: HTTP CONNECT registry.npmjs.org:443
    P-->>H: 200 Connection established (tunnelSocket)
    H->>H: new TunnelAgent({ keepAlive: false })
    H->>L: reqOptions (with agent, no createConnection)
    L->>H: httpsModule.get(reqOptions, cb)
    Note over H,R: Agent.createConnection called internally by Node
    H->>H: TunnelAgent.createConnection()
    H->>H: tls.connect({ socket: tunnelSocket, servername })
    H->>R: TLS handshake over tunnel (no DNS lookup)
    R-->>H: 200 OK / JSON response
    H-->>L: resolve(res)
Loading

Comments Outside Diff (2)

  1. cli/release/index.js, line 5-6 (link)

    P2 Dead http / https imports after refactor

    After moving all proxy and HTTP logic into ./http.js, the http and https require calls in index.js have no remaining usages — confirmed by searching the whole file for http. / https. references. The same stale imports are present in cli/release-staging/index.js:5-6 and freebuff/cli/release/index.js:5-6.

    (Remove const http = require('http') and const https = require('https') from all three index.js files.)

  2. cli/src/__tests__/release/proxy-http-get.test.ts, line 816-818 (link)

    P2 Test exercises createConnection without a callback

    The test invokes options.agent.createConnection(options) synchronously, with no second argument. In production, Node's HTTPS internals always pass a callback to createConnection, which triggers the settled / secureConnect + error listener logic. The test verifies the TLS options are correct but does not exercise the callback path at all, so a regression there (e.g. the finish closure or the settled guard) would go undetected.

    Consider adding a variant that passes a callback:

    options.agent.createConnection(options, (err, sock) => {
      expect(err).toBeNull()
      expect(sock).toBe(tlsSocket)
    })

Reviews (1): Last reviewed commit: "fix: avoid DNS lookup after proxied rele..." | Re-trigger Greptile

Comment thread cli/release/http.js
@jahooma jahooma merged commit 93959cb into CodebuffAI:main Apr 18, 2026
@jahooma
Copy link
Copy Markdown
Contributor

jahooma commented Apr 18, 2026

Thanks for the fix!

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.

2 participants