Reviewed proxy setup PR for AI and coding-agent

Summary

In Progress

Understand
Read PR, linked issues, and comments.
Understand
Understand
Analyzed relevant source files and changelogs.
Understand
Understand
Investigated SDK fetch and proxy setup behavior.
Understand
Verify
Prepared detailed review with findings and recommendations.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/939

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Now let me read the linked issue #931:

Let me read the relevant source files to understand the context:

Readmain.ts

Readgenerate-models.ts

Let me check the changelog for packages/ai and packages/coding-agent:

ReadCHANGELOG.md

Now let me check the coding-agent README and docs to see if proxy documentation would be needed:

The current README refers to "proxy" in the context of API endpoint proxies (like baseUrl), not HTTP_PROXY environment variables. Let me check if undici is already a dependency:

undici is not currently a dependency. Let me check if Node's native fetch uses undici under the hood:

Let me now understand the issue better by checking the original issue description. The issue #931 mentions that for fetch based SDKs, they suggest using undici to set up the dispatcher. Let me see if the approach is correct by checking how undici.setGlobalDispatcher works:

The PR adds undici which is not currently a dependency. This is a problem because:

The PR doesn't add undici to any package.json
The imports will fail at runtime

Let me verify the PR doesn't touch package.json files:

Confirmed. The PR imports undici but doesn't add it as a dependency. Let me also check if undici is bundled with newer Node.js versions:

The PR has a fundamental issue. Now let me also check if this approach even works for all SDK calls. The SDKs (Anthropic, OpenAI, Google GenAI) have their own HTTP clients:

Readanthropic.ts50 lines

Now let me see if the Anthropic SDK respects setGlobalDispatcher:

The Anthropic SDK uses its own fetch implementation. The setGlobalDispatcher from undici affects the global fetch, but the SDK might use its own bundled fetch. Let me check the issue description more carefully:

Looking at issue #931:

for http based SDK (e.g., amazon bedrock client), use proxy-agent to set up http agent.

for fetch based SDK (e.g., openai and anthropic),

in node, use undici to set up the dispatcher of fetch

in bun, there is built-in support for these environment variables

The key insight is that Node.js 18+ uses undici under the hood for its native fetch. So calling setGlobalDispatcher should affect the native fetch. However, the SDKs might use their own fetch implementation.

Let me check if this approach is valid:

The Anthropic SDK uses the global fetch by default (typeof fetch !== 'undefined'). This means setGlobalDispatcher should work because Node.js 18+'s native fetch uses undici internally.

However, there's still the issue that undici needs to be added as a dependency.

Let me also check the PR's placement of the proxy setup code. The issue is that the code is placed at the top of files, which means it runs at import time:

Now I have enough information to provide the review.

PR: https://github.com/badlogic/pi-mono/pull/939

Changelog:

Missing. No changelog entry exists in either packages/ai/CHANGELOG.md or packages/coding-agent/CHANGELOG.md. A changelog entry is required before merge and will be added if the user decides to merge.

Good:

Addresses a real need for users behind corporate proxies (issue #931)
Uses EnvHttpProxyAgent which automatically reads standard proxy environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY, and lowercase variants)
Conditional check avoids creating the proxy agent when not needed
Placed early in the entry points before any network calls
The approach is sound for Node.js 18+ where native fetch uses undici internally

Bad:

Critical: Missing dependency - The PR imports undici but doesn't add it to any package.json. The code will fail at runtime with Cannot find module 'undici'.
Missing package.json changes - Need to add undici to:
- packages/ai/package.json (for generate-models.ts)
- packages/coding-agent/package.json (for main.ts)
Incomplete coverage - Issue #931 mentions Amazon Bedrock uses http based SDK requiring proxy-agent. This PR only handles fetch-based providers.
Missing Bun consideration - Issue #931 notes that Bun has built-in proxy support, but this PR always uses undici even on Bun (where it's unnecessary and might not work correctly)
Missing documentation - No README updates documenting the new proxy support feature

Ugly:

Duplicate code - The same proxy setup logic is duplicated in two files with slightly different formatting (tabs vs spaces in generate-models.ts, different variable placement). Should be centralized.
generate-models.ts is a build script - This script only runs at build time to generate model definitions. Adding proxy support here has limited value since it only affects the npm run generate-models command, not runtime usage.

Questions or Assumptions:

Was this tested with an actual HTTP proxy to verify it works?
Should proxy support be in packages/ai instead, so all downstream packages get it automatically?
Does this approach work with all providers (Anthropic, OpenAI, Google, Bedrock)?
What about Bun runtime detection to avoid unnecessary undici usage?

Change summary:

Adds HTTP proxy support via undici.EnvHttpProxyAgent for Node.js native fetch
Modifies two entry points: packages/ai/scripts/generate-models.ts and packages/coding-agent/src/main.ts
Checks for HTTP_PROXY, HTTPS_PROXY, NO_PROXY (both cases) and sets global fetch dispatcher

Tests:

No tests added
No existing tests modified
Needs manual testing with a real proxy setup