Conversation
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR is marked [wip] with only test output in the body and no actual code changes provided; unable to assess if kernel API endpoints or Temporal workflows were modified. To monitor this PR anyway, reply with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9aab517. Configure here.
| inner: this.fetch, | ||
| cache: this.browserRouteCache, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Routing fetch double-wraps on withOptions calls
Medium Severity
When browserRouting is enabled and withOptions() is called, the routing fetch gets double-wrapped. withOptions passes fetch: this.fetch (the already-wrapped routing fetch), while this._options still carries browserRouting: { enabled: true }. The new constructor then wraps the already-wrapped fetch with another createRoutingFetch layer. Each withOptions call adds another nested routing layer, each with its own separate BrowserRouteCache, causing duplicate response cloning/parsing and cache isolation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9aab517. Configure here.
| * | ||
| * Usage (from the repo root): | ||
| * | ||
| * cd /Users/sayan/kernel/kernel-node-sdk |
There was a problem hiding this comment.
Hardcoded personal filesystem path in example file
Low Severity
The usage comment contains a hardcoded personal directory path (/Users/sayan/kernel/kernel-node-sdk). This leaks a developer's local filesystem layout and isn't useful for other developers.
Reviewed by Cursor Bugbot for commit 9aab517. Configure here.
| ); | ||
| cache.evict(match.id); | ||
| const fallback = await inner(input, init); | ||
| return sniffAndPopulateCache(fallback, cache); |
There was a problem hiding this comment.
Discarded metro-direct response body never consumed causing resource leak
Medium Severity
When the metro-direct call returns 401/403/404, the resp from that call is discarded without consuming or cancelling its body stream. The SDK itself explicitly cancels unconsumed response bodies before retrying (via Shims.CancelReadableStream(response.body) in makeRequest), but the routing fetch skips this step. Under high request volume, this can cause connection pool exhaustion as abandoned sockets wait to time out.
Reviewed by Cursor Bugbot for commit 9aab517. Configure here.
|
Thanks for drawing this up. I considered this and I go back and forth, but I think the question comes down to how much magic we want to accept in the SDK. Looking at this now, it doesn't seem like a lot of magic, so I think I'm more inclined to do this approach. I will keep exploring this idea and also see if it's possible to do something architecturally similar in the other SDKs so that there's not too much to juggle in terms of concepts |
Trim the node browser routing changes down to the cache/interceptor shape from PR #100 and remove the leftover browser-scoped example and priming surface. Made-with: Cursor
2 participants
example output:
Note
Medium Risk
Adds an opt-in
fetchrewrite layer that changes how browser subresource requests are routed and authenticated (JWT query param vs Authorization), which could affect request correctness and error handling despite being disabled by default.Overview
Introduces an opt-in “metro-direct” routing mode for browser subresource endpoints that transparently rewrites eligible
GET/POST /browsers/{id}/{subresource}/...calls to the per-browserbase_urlwith?jwt=..., and strips theAuthorizationheader.Adds an in-memory
BrowserRouteCachethat is auto-populated by sniffing Browser-shaped JSON responses (e.g.,browsers.create()), plus a fallback behavior that evicts cache entries and retries via the public API when metro responds with401/403/404.Includes a routing smoke-test script (
examples/browser-routing-smoke.ts) and a new test suite covering URL rewriting, allowlist behavior, cache population, and retry/eviction logic.Reviewed by Cursor Bugbot for commit 9aab517. Bugbot is set up for automated code reviews on this repo. Configure here.