Skip to content

Resolve spawned proxy paths from the server root, not the calling module - #217

Merged
milind-soni merged 1 commit into
mainfrom
fix/proxy-paths-after-bundling
Aug 18, 2026
Merged

Resolve spawned proxy paths from the server root, not the calling module#217
milind-soni merged 1 commit into
mainfrom
fix/proxy-paths-after-bundling

Conversation

@milind-soni

Copy link
Copy Markdown
Owner

Follow-up to #198. That fix traded one silent packaging failure for a subtler one, caught by an adversarial review of the release candidate before publishing.

The defect

esbuild inlines drivers/claude.ts and drivers/acp/core.ts into index.js at the server root, so the ".." each wrote to reach a sibling proxy started climbing from the bundle's directory rather than its own — one level too high, two for the ACP driver. Measured in a packaged layout:

constant resolved to exists
PROXY_PATH <Resources>/computer-proxy.js no
PERM_PROXY_PATH <Resources>/permission-proxy.js no
DWEB_PROXY_PATH <Resources>/drivers/dweb-proxy.js no
COMPUTER_PROXY_PATH <Resources>/../computer-proxy.js no

The resolver only stats the .ts branch (existsSync(ts) ? ts : ts.replace(...)), so the missing .js was returned unchecked and nothing failed until a child was spawned.

Why every gate missed it

The server still booted and /api/health still answered. The smoke test added in #198 and the new Windows packaging step both passed this build. A green run proved nothing about the feature that was dead.

Impact had it shipped

  • permission Allow/Deny cards never appear — decodeConfig defaults permissionMode to acceptEdits, so every Claude turn takes the branch that spawns the permission proxy
  • cloud-box bots lose mcp__computer
  • dweb bots lose mcp__dweb
  • every ACP engine (grok, gemini, kimi, droid, qwen, hermes, opencode) loses its computer proxy

The fix

One anchor: server/proxy-paths.ts exports SERVER_ROOT and SPAWNED_PROXIES. It sits at the server root and is only ever inlined into root-level entries — the nested drivers/* entries import nothing local — so the anchor is correct in the dev tree and in the bundle alike. All five call sites now go through it, including the two that happened to survive bundling, so there is exactly one way a proxy is located.

Verification

  • pnpm test — 721 passed / 8 skipped across 78 files
  • pnpm typecheck clean
  • the smoke test now asserts every SPAWNED_PROXIES entry exists inside the staged copy, not merely that /api/health answers
  • mutation-checked: restoring the old ".." resolution fails the smoke test and names the missing proxies

🤖 Generated with Claude Code

The bundling fix in #198 traded one silent packaging failure for a
subtler one. esbuild inlines drivers/claude.ts and drivers/acp/core.ts
into index.js at the server root, so the `".."` each wrote to reach a
sibling proxy started climbing from the bundle's directory instead of
its own — one level too high, two for the ACP driver:

  PROXY_PATH           <Resources>/computer-proxy.js      missing
  PERM_PROXY_PATH      <Resources>/permission-proxy.js    missing
  DWEB_PROXY_PATH      <Resources>/drivers/dweb-proxy.js  missing
  COMPUTER_PROXY_PATH  <Resources>/../computer-proxy.js   missing

The resolver only stats the .ts branch, so the missing .js was returned
unchecked and nothing failed until a child was spawned. The server still
booted and /api/health still answered — which is exactly why the new
smoke test and the Windows gate both passed the broken build.

Impact had it shipped: permission Allow/Deny cards never appear (the
default permissionMode is acceptEdits, so every Claude turn takes that
branch), cloud-box bots lose mcp__computer, dweb bots lose mcp__dweb,
and every ACP engine — grok, gemini, kimi, droid, qwen, hermes — loses
its computer proxy.

Resolve all five through one anchor in server/proxy-paths.ts, which sits
at the server root and is only ever inlined into root-level entries, so
the anchor is right in the dev tree and in the bundle. The smoke test now
asserts every path in SPAWNED_PROXIES exists inside the staged copy;
mutation-checked by restoring the old "..", which fails it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@milind-soni, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Limit details: You’ve used all 3 included reviews currently available under your plan.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 38dfeb24-76a2-46c4-82fe-cac39d7656d5

📥 Commits

Reviewing files that changed from the base of the PR and between 1e458b1 and b8c249a.

📒 Files selected for processing (6)
  • scripts/smoke-packaged-server.mjs
  • server/container-computer.ts
  • server/drivers/acp/core.ts
  • server/drivers/claude.ts
  • server/index.ts
  • server/proxy-paths.ts

Comment @coderabbitai help to get the list of available commands.

@milind-soni
milind-soni merged commit 3922e06 into main Aug 18, 2026
6 checks passed
@milind-soni
milind-soni deleted the fix/proxy-paths-after-bundling branch August 18, 2026 05:01
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.

1 participant