fix(ai-sandbox): reject host tool bridge listener startup errors - #1334
Open
LadyBluenotes wants to merge 2 commits into
Open
fix(ai-sandbox): reject host tool bridge listener startup errors#1334LadyBluenotes wants to merge 2 commits into
LadyBluenotes wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe host-tool bridge now preserves HTTP listener startup errors. Unit and end-to-end tests verify invalid binding, recovery with valid configuration, authenticated endpoint behavior, and cleanup. Documentation and release metadata describe the updated behavior. ChangesHost-tool bridge startup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Host-tool bridge startup now returns the original listener error to callers instead of terminating the host process, while successful startup and recovery after a failed attempt are covered. The change is ready to merge. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant E2E as E2E child process
participant Bridge as startHostToolBridge
participant Server as HTTP server
E2E->>Bridge: Start with invalid bindAddress
Bridge->>Server: listen(0, invalid bindAddress)
Server-->>Bridge: ENOTFOUND error
Bridge-->>E2E: Reject with ENOTFOUND
E2E->>Bridge: Start with valid configuration
Bridge->>Server: listen(0, valid bindAddress)
Server-->>Bridge: listening event
Bridge-->>E2E: Return bridge URL
E2E->>Bridge: fetch(bridge.url)
Bridge-->>E2E: Return 401
E2E->>Bridge: Close bridge
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 753a38d
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 8a82e54
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-byteplus
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-cloudflare
@tanstack/ai-code-mode
@tanstack/ai-code-mode-snippets
@tanstack/ai-codex
@tanstack/ai-cohere
@tanstack/ai-compaction
@tanstack/ai-devtools-core
@tanstack/ai-durable-stream
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-daytona
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-isolate-quickjs-bun
@tanstack/ai-llmgateway
@tanstack/ai-lovable
@tanstack/ai-mcp
@tanstack/ai-memory
@tanstack/ai-mistral
@tanstack/ai-octane
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-perplexity
@tanstack/ai-persistence
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-reactor
@tanstack/ai-remix
@tanstack/ai-sandbox
@tanstack/ai-sandbox-blaxel
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-upstash-box
@tanstack/ai-sandbox-vercel
@tanstack/ai-skills
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vercel-gateway
@tanstack/ai-vertex
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
@tanstack/svelte-ai-devtools
commit: |
github-actions
Bot
added
the
waiting-on: author
Contributor
|
Thanks for the PR, @LadyBluenotes! 🙌 @tombeckenham will take a look. Automated pre-review checks
Automated triage — a human review follows. |
github-actions
Bot
added
waiting-on: maintainer
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
2 participants
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.
When the host-tool bridge cannot start its HTTP listener, an unhandled
errorevent can terminate the Node process. This fix rejects startup with the original Node.js error so callers can handle the failure.🎯 Changes
Use
node:events.once()to await listener startup. Add a regression test, a process-level E2E test, a documentation clarification, and a patch changeset.✅ Checklist
pnpm run test:pr, or these tests do not apply to this pull request.docs/for this change, or this change is not user-facing.pnpm changeset), or this PR does not change a published package.🚀 Release Impact
Root cause
Issue. A failed listener startup can crash the host process even when the caller awaits
startHostToolBridge()insidetry/catch.Cause. The startup promise resolves on
listeningbut has no listener for theerrorevent.Fix.
events.once()rejects onerrorwhile it awaitslistening. Successful startup retains the existing bridge behavior.Possible alternatives
Explicit
errorandlisteninghandlers can also settle the promise. The standard library already provides rejection and event-listener cleanup.Testing
Commands run:
pnpm run build:all: passed.pnpm exec nx run-many --projects=@tanstack/ai-sandbox --targets=build,test:lib,test:types,test:oxlint,test:build: passed; 974 tests.pnpm run test:pr: passed, including the declaration scan.CI=true pnpm --filter @tanstack/ai-e2e test:e2e --workers=4 --retries=0: 665 passed, 13 failed, 1 skipped. All 13 failing cases also reproduce on clean main. A final focused run oftests/sandbox-tool-bridge-startup.spec.tspasses.git diff --check: passed.The new regression test fails on the original implementation with an unhandled
ENOTFOUNDerror and a timeout. It passes after the fix. The E2E test exercises the built package in a real Node process, catches startup failure, and starts another bridge that serves a request.The broader E2E failures are in
tests/multi-turn-structured.spec.ts:101: the final snapshot still containsVegan Spaghetti Pomodorowhen the test expectsGluten-Free. On clean main atc9681f7, 11 cases fail in the focused group. Repeating the two remaining provider cases reproduces both failures. No assertions or test settings in the repository changed.Manual reproduction, using Node 24:
c9681f7, run the following command from the repository root after dependency installation.node --input-type=module -e 'import { startHostToolBridge } from "./packages/ai-sandbox/src/tool-bridge.ts"; try { await startHostToolBridge([], { hostForSandbox: "127.0.0.1", bindAddress: "not a valid host" }); process.exitCode = 2; } catch (error) { console.log("caught startup error:", error.code); const bridge = await startHostToolBridge([], { hostForSandbox: "127.0.0.1" }); console.log("subsequent listener status:", (await fetch(bridge.url)).status); await bridge.close(); }'Clean main, exit 1:
Fixed branch at
8a82e54, exit 0:Risk / rollback
The change only affects listener startup. Existing authenticated MCP calls and bearer-token rejection pass. Revert this PR to restore the previous implementation.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests