feat(extension): harden browser target bootstrap for the resident flow - #2802
Open
miguelg719 wants to merge 4 commits into
Open
feat(extension): harden browser target bootstrap for the resident flow#2802miguelg719 wants to merge 4 commits into
miguelg719 wants to merge 4 commits into
Conversation
Port of #2464 onto main without the ordinary-session regression. - Target.setAutoAttach (root and per-page) now uses the page/iframe/exclude filter so workers, service workers and background pages are never attached. - New resident-only BrowserContext options: restrictToWebTargets ignores top-level pages without an injectable DOM (chrome://, chrome-extension://) except Stagehand's own blank page, detaches them via the reporting parent session, and re-attaches them from Target.targetInfoChanged once they reach web content (top-level pages only, coalesced). deferPageInstrumentation postpones Page/Runtime/Network enables until prepareForInitialization(), with a snapshot per attach and a post-registration catch-up so pages attached while preparation is in flight are still instrumented. - BrowserContext.create gains onConnected/onDisconnected hooks and ensureInitialPage (false skips the inline newPage() fallback); the connection is closed when bootstrap fails. - hasInjectableDOM accepts about:blank?query. - Default (non-resident) flow keeps the #1924 rule: every top-level page is tracked, including chrome://newtab; covered by new tests.
…not TargetInfo.attached Review finding on the step-B port: onTargetInfoChanged gated the restrictToWebTargets re-attach on `info.attached`, but Chrome reports `attached` when ANY DevTools client has a session on the target. In the resident deployment the SDK (Playwright connectOverCDP), Browserbase tooling or DevTools are attached to every page, so the flag is always true and an ignored chrome://newtab / chrome-extension:// page was never re-attached after navigating to web content — the only re-attach path was dead where it is meant to run. - Track deliberately ignored top-level pages in an `ignoredTargets` set (populated from the resume+detach branch in onAttachedToTarget and from the bootstrap sweep; cleared on successful attach, supported attach, Target.targetDestroyed and connection reset). `conn.sessionToTarget` is not used as the signal because Chrome does not echo Target.detachedFromTarget to the client that issued the detach. - onTargetInfoChanged re-attaches only targets in that set; the `info.attached` check is gone. - Tests: re-attach with another client attached (attached=true), no re-attach for never-ignored pages, no re-attach after targetDestroyed; the coalescing test now ignores the page first.
…typechecks
tsc (packages/protocol/tsconfig.json, run by pnpm check) rejected the
mockImplementation returning Promise<void> against the inferred
Promise<{}>; widen the mock's return type.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 6 files
Confidence score: 2/5
- In
packages/extension/understudy/context.ts, failed page setup can clearignoredTargetstoo early or continue after late instrumentation failure, leaving pages unprepared and preventing retry; retain retry state and propagate or retry initialization failures. - In
packages/extension/understudy/page.ts,Page.prepareForInitialization()rethrows a browser-provided CDP error throughBrowserContext, potentially exposing unsanitized error details; sanitize the propagated message before surfacing it. - In
packages/extension/understudy/cdp.tsandpackages/extension/understudy/context.ts, default auto-attach filtering and earlyNetwork.enablecan change target behavior and header instrumentation order; preserve unfiltered default behavior and defer header setup with the remaining instrumentation. - In
packages/extension/understudy/context.tsandpackages/extension/understudy/networkManager.ts, callback failures can leak the CDP transport, while late OOPIF and transient enable failures lack focused coverage; close the connection on callback failure and add retry/concurrency tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/extension/understudy/networkManager.ts">
<violation number="1" location="packages/extension/understudy/networkManager.ts:167">
P2: This new stateful enable path lacks focused coverage for late OOPIF sessions and transient `Network.enable` failures. Add tests that assert the loop picks up a session added while enabling is in flight and retries only sessions that are still owned, so future changes cannot silently leave network events disabled.
(Based on your team's feedback about focused tests for new behavior.)</violation>
</file>
<file name="packages/extension/understudy/cdp.ts">
<violation number="1" location="packages/extension/understudy/cdp.ts:202">
P2: In default mode, `enableAutoAttach()` now filters out non-page targets, changing the previously unfiltered bootstrap behavior. Apply this filter only when `restrictToWebTargets` is enabled and preserve the unfiltered payload otherwise.</violation>
</file>
<file name="packages/extension/understudy/context.ts">
<violation number="1" location="packages/extension/understudy/context.ts:224">
P2: If `onConnected` throws, `BrowserContext.create` exits before its cleanup handler and leaves the newly opened CDP transport active. Invoke the callback inside the cleanup path or close `conn` when the callback fails.</violation>
<violation number="2" location="packages/extension/understudy/context.ts:783">
P1: When an ignored page is reattached, `attachToTarget` clears `ignoredTargets` before `onAttachedToTarget` finishes page setup. If `Page.create` or pre-resume setup fails, later `Target.targetInfoChanged` events cannot retry the page; retain the marker until `pagesByTarget` registration succeeds.</violation>
<violation number="3" location="packages/extension/understudy/context.ts:903">
P2: When `deferPageInstrumentation` is enabled with context headers configured, this path still sends `Network.enable` before `prepareForInitialization()`. Defer header setup with the other instrumentation and apply it during preparation.</violation>
<violation number="4" location="packages/extension/understudy/context.ts:1090">
P2: If late instrumentation fails after context preparation, this catch lets initialization continue with an uninstrumented `Page`. Propagate the failure or retain the page in a retryable preparation set instead of swallowing it.</violation>
</file>
<file name="packages/extension/understudy/page.ts">
<violation number="1" location="packages/extension/understudy/page.ts:250">
P2: Custom agent: **Exception and error message sanitization**
When a CDP enable call fails during `Page.prepareForInitialization()`, this line rethrows the generic browser-provided error through `BrowserContext.prepareForInitialization()` and `runtime.initialize()`. Wrap it in a dedicated typed error with a sanitized message before exposing the bootstrap failure to callers.</violation>
</file>
Architecture diagram
sequenceDiagram
participant App as Stagehand/Init Script
participant Context as BrowserContext
participant Conn as CdpConnection
participant Browser as Browser (CDP)
participant Page as Page / NetworkManager
Note over App,Browser: Bootstrap Phase (Resident Flow)
App->>Context: create(url, { restrictToWebTargets: true, deferPageInstrumentation: true })
Context->>Conn: connect(wsUrl)
Conn->>Browser: NEW: Target.setAutoAttach (Filter: Page, Iframe, Exclude)
Conn->>Browser: Target.getTargets
Browser-->>Conn: List of targets (tabs, workers, etc.)
loop For each existing target
Context->>Context: NEW: isSupportedWebTarget(info)
alt is injectable (e.g. https://)
Context->>Conn: Target.attachToTarget(id)
else NEW: non-injectable (e.g. chrome://)
Context->>Context: Track in ignoredTargets set
end
end
Note over Context,Page: Page Attachment Flow
Browser-->>Conn: Target.attachedToTarget
Conn->>Context: onAttachedToTarget()
Context->>Page: NEW: create(..., deferInstrumentation: true)
opt CHANGED: deferPageInstrumentation == true
Note over Page: Skip Page.enable, Runtime.enable, Network.enable
end
Note over Context,Browser: Target Re-attachment Flow
Browser-->>Conn: NEW: Target.targetInfoChanged (URL changed)
Conn->>Context: onTargetInfoChanged(info)
alt NEW: Ignored target navigated to injectable URL
Context->>Conn: Target.attachToTarget(id)
Context->>Context: Remove from ignoredTargets
end
Note over App,Page: Explicit Initialization Flow
App->>Context: NEW: prepareForInitialization()
Context->>Page: prepareForInitialization()
Page->>Page: NEW: NetworkManager.enable()
Page->>Browser: Page.enable
Page->>Browser: Runtime.enable
Page->>Browser: Page.setLifecycleEventsEnabled
loop For each OOPIF (out-of-process iframe)
Page->>Browser: NEW: Catch-up instrumentation for child sessions
end
Page-->>Context: Ready
Context-->>App: Initialization Complete
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.pendingTargetAttachments.add(targetId); | ||
| try { | ||
| await this.conn.attachToTarget(targetId); | ||
| this.ignoredTargets.delete(targetId); |
Contributor
There was a problem hiding this comment.
P1: When an ignored page is reattached, attachToTarget clears ignoredTargets before onAttachedToTarget finishes page setup. If Page.create or pre-resume setup fails, later Target.targetInfoChanged events cannot retry the page; retain the marker until pagesByTarget registration succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/context.ts, line 783:
<comment>When an ignored page is reattached, `attachToTarget` clears `ignoredTargets` before `onAttachedToTarget` finishes page setup. If `Page.create` or pre-resume setup fails, later `Target.targetInfoChanged` events cannot retry the page; retain the marker until `pagesByTarget` registration succeeds.</comment>
<file context>
@@ -650,6 +763,29 @@ export class BrowserContext {
+ this.pendingTargetAttachments.add(targetId);
+ try {
+ await this.conn.attachToTarget(targetId);
+ this.ignoredTargets.delete(targetId);
+ } finally {
+ this.pendingTargetAttachments.delete(targetId);
</file context>
| } | ||
|
|
||
| /** Enable network events after resident bootstrap begins initialization. */ | ||
| public async enable(): Promise<void> { |
Contributor
There was a problem hiding this comment.
P2: This new stateful enable path lacks focused coverage for late OOPIF sessions and transient Network.enable failures. Add tests that assert the loop picks up a session added while enabling is in flight and retries only sessions that are still owned, so future changes cannot silently leave network events disabled.
(Based on your team's feedback about focused tests for new behavior.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/networkManager.ts, line 167:
<comment>This new stateful enable path lacks focused coverage for late OOPIF sessions and transient `Network.enable` failures. Add tests that assert the loop picks up a session added while enabling is in flight and retries only sessions that are still owned, so future changes cannot silently leave network events disabled.
(Based on your team's feedback about focused tests for new behavior.) </comment>
<file context>
@@ -155,6 +163,40 @@ export class NetworkManager {
}
+ /** Enable network events after resident bootstrap begins initialization. */
+ public async enable(): Promise<void> {
+ if (this.enabled) return;
+ if (this.enableTask) return await this.enableTask;
</file context>
| autoAttach: true, | ||
| flatten: true, | ||
| waitForDebuggerOnStart: true, | ||
| filter: STAGEHAND_WEB_TARGET_FILTER, |
Contributor
There was a problem hiding this comment.
P2: In default mode, enableAutoAttach() now filters out non-page targets, changing the previously unfiltered bootstrap behavior. Apply this filter only when restrictToWebTargets is enabled and preserve the unfiltered payload otherwise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/cdp.ts, line 202:
<comment>In default mode, `enableAutoAttach()` now filters out non-page targets, changing the previously unfiltered bootstrap behavior. Apply this filter only when `restrictToWebTargets` is enabled and preserve the unfiltered payload otherwise.</comment>
<file context>
@@ -192,6 +199,7 @@ export class CdpConnection implements CDPSessionLike {
autoAttach: true,
flatten: true,
waitForDebuggerOnStart: true,
+ filter: STAGEHAND_WEB_TARGET_FILTER,
});
await this.send("Target.setDiscoverTargets", { discover: true });
</file context>
| const corePreResumeOps = [ | ||
| queuePreResume("Page.enable"), | ||
| queuePreResume("Runtime.enable"), | ||
| ...(deferInstrumentation |
Contributor
There was a problem hiding this comment.
P2: When deferPageInstrumentation is enabled with context headers configured, this path still sends Network.enable before prepareForInitialization(). Defer header setup with the other instrumentation and apply it during preparation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/context.ts, line 903:
<comment>When `deferPageInstrumentation` is enabled with context headers configured, this path still sends `Network.enable` before `prepareForInitialization()`. Defer header setup with the other instrumentation and apply it during preparation.</comment>
<file context>
@@ -752,12 +900,14 @@ export class BrowserContext {
const corePreResumeOps = [
- queuePreResume("Page.enable"),
- queuePreResume("Runtime.enable"),
+ ...(deferInstrumentation
+ ? []
+ : [queuePreResume("Page.enable"), queuePreResume("Runtime.enable")]),
</file context>
| opts.onDisconnected?.(); | ||
| }); | ||
| } | ||
| opts.onConnected?.(); |
Contributor
There was a problem hiding this comment.
P2: If onConnected throws, BrowserContext.create exits before its cleanup handler and leaves the newly opened CDP transport active. Invoke the callback inside the cleanup path or close conn when the callback fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/context.ts, line 224:
<comment>If `onConnected` throws, `BrowserContext.create` exits before its cleanup handler and leaves the newly opened CDP transport active. Invoke the callback inside the cleanup path or close `conn` when the callback fails.</comment>
<file context>
@@ -167,10 +204,24 @@ export class BrowserContext {
+ opts.onDisconnected?.();
+ });
+ }
+ opts.onConnected?.();
const ctx = new BrowserContext(
conn,
</file context>
| ) { | ||
| // Context-level preparation finished while this target was attaching; it | ||
| // missed the rescan, so enable its domains now. | ||
| await page.prepareForInitialization().catch((error: unknown) => { |
Contributor
There was a problem hiding this comment.
P2: If late instrumentation fails after context preparation, this catch lets initialization continue with an uninstrumented Page. Propagate the failure or retain the page in a retryable preparation set instead of swallowing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/context.ts, line 1090:
<comment>If late instrumentation fails after context preparation, this catch lets initialization continue with an uninstrumented `Page`. Propagate the failure or retain the page in a retryable preparation set instead of swallowing it.</comment>
<file context>
@@ -927,6 +1080,21 @@ export class BrowserContext {
+ ) {
+ // Context-level preparation finished while this target was attaching; it
+ // missed the rescan, so enable its domains now.
+ await page.prepareForInitialization().catch((error: unknown) => {
+ this.logger.debug("Failed to instrument page attached during initialization", {
+ category: "ctx",
</file context>
| this.instrumentedSessions.add(session); | ||
| } catch (error) { | ||
| if (session.id && !this.sessions.has(session.id)) return; | ||
| throw error; |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Exception and error message sanitization
When a CDP enable call fails during Page.prepareForInitialization(), this line rethrows the generic browser-provided error through BrowserContext.prepareForInitialization() and runtime.initialize(). Wrap it in a dedicated typed error with a sanitized message before exposing the bootstrap failure to callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/page.ts, line 250:
<comment>When a CDP enable call fails during `Page.prepareForInitialization()`, this line rethrows the generic browser-provided error through `BrowserContext.prepareForInitialization()` and `runtime.initialize()`. Wrap it in a dedicated typed error with a sanitized message before exposing the bootstrap failure to callers.</comment>
<file context>
@@ -214,10 +221,58 @@ export class Page {
+ this.instrumentedSessions.add(session);
+ } catch (error) {
+ if (session.id && !this.sessions.has(session.id)) return;
+ throw error;
+ }
+ }),
</file context>
miguelg719
changed the title
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
1 participant
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.
Summary
Stack B/5:
-a-transport→feat/resident-main-b-target-safety(this PR) →-c-gateway-runtime→-d-artifacts→-e-browserbase-opt-in. Supersedes #2464.Makes
BrowserContextbootstrap safe for the resident flow, where the extension attaches to a browser that Core and other extensions already own:BrowserContext.createoptions:restrictToWebTargets,deferPageInstrumentation,ensureInitialPage,onConnected,onDisconnected.type=pagetargets tracked, incl.chrome://newtab) is preserved; the strict page/iframe filter applies only underrestrictToWebTargets. Test pins headful+-tab tracking in default mode. (The v4-spike version applied the filter globally — the review's top finding.)hasInjectableDOMacceptsabout:blank?query;Target.targetInfoChangedno longer root-attaches iframes of ignored pages; adopted OOPIFs getRuntime.enable+ lifecycle events during deferred bootstrap;setAutoAttachfilter order[page, iframe, exclude]asserted literally.ignoredTargets), notTargetInfo.attached(which means "any DevTools client attached" and never fires while the SDK is connected).Note:
go run ./internal/extensionpack --checkis red on this PR by design (the bundle changed); stack D regenerates the embedded zip.Validation
pnpm check, extension tests incl. existing understudy/context lifecycle tests — green.Summary by cubic
Hardens browser bootstrap for the resident flow by ignoring non‑web targets and deferring page instrumentation until initialization. Default behavior is unchanged: all top‑level pages remain tracked (including chrome://newtab), while workers and background targets never auto‑attach.
blankPageUrl(e.g., the extension’s blank page).packages/sdk-goto reflect these changes.Migration for resident deployments:
blankPageUrlwhen using restrictToWebTargets.Written for commit a9a17d6. Summary will update on new commits.