Conversation
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR modifies client library code (browser session client) rather than API endpoints or Temporal workflows in packages/api/ To monitor this PR anyway, reply with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Bind browser subresource calls to a browser session's base_url and expose raw HTTP through fetch so metro-routed access feels like normal JavaScript networking. Made-with: Cursor
Fail fast when browser-scoped clients do not have a session base_url, route subresource calls through the browser session base directly, and clean up browser-vm wording. Made-with: Cursor
Fail fast when browser-scoped clients are missing a browser session base_url, route subresource calls through the session base consistently, and keep lint output clean. Made-with: Cursor
Replace the handwritten Node browser-scoped façade with deterministic generated bindings from the browser resource graph, and enforce regeneration during lint and build. Made-with: Cursor
rgarcia
force-pushed
the
raf/browser-scoped-client
branch
from
c5731cb to
e730af8
Compare
| const kernel = new Kernel(); | ||
|
|
||
| const created = await kernel.browsers.create({}); | ||
| const browser = kernel.forBrowser(created); |
There was a problem hiding this comment.
I'm not opposed to this tbc but I spent a bit of time scheming up if we could do all these routing decisions right in the sdk without the users having to think about it. Something like a:
- response interceptor -> cache base urls from creating / acquiring browsers
- request interceptor -> dynamically route to base url depending on the operation
here's what opus vibed up as a poc #100
wdyt?
Route direct-to-VM browser requests through the shared client cache so the SDK no longer needs the generated browser session wrapper layer. Made-with: Cursor
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
rgarcia
changed the title
Shorten the browserRouting allowlist field to subresources so the direct-to-VM configuration reads more cleanly without changing behavior. Made-with: Cursor
Keep the node browser-routing example showing both direct subresource routing and the cache-backed /curl/raw path. Made-with: Cursor
Bring back the cache-backed browser fetch helper so raw HTTP stays on the SDK's language-native surface instead of falling through to manual /curl/raw requests. Made-with: Cursor
| throw new KernelError(`browser.fetch unsupported HTTP method: ${method}`); | ||
| } | ||
| return methodLower as HTTPMethod; | ||
| } |
There was a problem hiding this comment.
normalizeMethod allows methods outside HTTPMethod type
Medium Severity
normalizeMethod accepts 'head' and 'options' in its allowed set and returns them cast as HTTPMethod, but the HTTPMethod type is defined as 'get' | 'post' | 'put' | 'patch' | 'delete' — it does not include those two methods. The as HTTPMethod cast hides this mismatch. When the result is passed into FinalRequestOptions.method, any downstream code relying on the HTTPMethod union will silently receive a value outside its domain.
Reviewed by Cursor Bugbot for commit b09434e. Configure here.
Remove the unnecessary generated resource and dependency diffs from the node branch and keep BrowserRouteCache.set() as a direct assignment without trimming user input. Made-with: Cursor
Tighten the browser routing files to the repo's formatter expectations so the node CI lint job passes cleanly again. Made-with: Cursor
Split browser.fetch into its own helper, remove unused browser transport code, and simplify withOptions cache sharing so the routing layer stays easier to reason about. Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issues. You can view the agent here.
Reviewed by Cursor Bugbot for commit fdd3adf. Configure here.
|
|
||
| function joinURL(baseURL: string, path: string): string { | ||
| return `${baseURL.replace(/\/+$/, '')}${path.startsWith('/') ? path : `/${path}`}`; | ||
| } |
There was a problem hiding this comment.
Duplicate joinURL helper in two new files
Low Severity
Both browser-fetch.ts and browser-routing.ts define identical private joinURL functions with the same signature and implementation. This duplicated logic increases maintenance burden — a fix in one copy could easily be missed in the other. One shared utility would be cleaner.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fdd3adf. Configure here.
| throw new KernelError(`browser.fetch unsupported HTTP method: ${method}`); | ||
| } | ||
| return methodLower as HTTPMethod; | ||
| } |
There was a problem hiding this comment.
normalizeMethod allows methods outside HTTPMethod type union
Low Severity
normalizeMethod validates and passes through 'head' and 'options', then casts the result as HTTPMethod. However, HTTPMethod is defined as 'get' | 'post' | 'put' | 'patch' | 'delete' — it does not include those two methods. The as cast silently lies to the type system, which could mask issues if any downstream code narrows on HTTPMethod variants.
Reviewed by Cursor Bugbot for commit fdd3adf. Configure here.
2 participants
Summary
browserRoutingclient config plus a sharedbrowserRouteCachefor debugging and cache reusesrc/lib/browser-routing.tsdown to the cache + request interception flow used to route allowlisted browser subresources directly to the VMsubresourcesfor the direct-to-VM allowlistTest plan
./node_modules/.bin/jest tests/lib/browser-routing.test.ts --runInBand./node_modules/typescript/bin/tsc --noEmitNote
Medium Risk
Changes core client request dispatch by optionally wrapping
fetchto rewrite allowlisted/browsers/:id/:subresource/*calls to VMbase_urlwith JWT injection and auth header stripping; routing/caching bugs could break browser subresource requests or leak requests to the wrong destination.Overview
Adds
browserRoutingclient config that, when enabled, wraps the client’sfetchto populate a sharedBrowserRouteCachefrom JSON browser responses and rewrite allowlisted browser subresource requests from the API origin to the browser VMbase_url(injectingjwtand removingAuthorization).Introduces
Kernel.browserRouteCache(reused acrosswithOptions) and a newbrowsers.fetch()API implemented viabrowser-fetch.tsto issue raw HTTP requests through the VM using the cached route. Includes a new example (examples/browser-routing.ts) and Jest coverage for cache warming, allowlist behavior,withOptionscache reuse, and cache-miss errors.Reviewed by Cursor Bugbot for commit fdd3adf. Bugbot is set up for automated code reviews on this repo. Configure here.