[STG-2407] fix(cli): honor BROWSERBASE_API_KEY passed to an already-running daemon#2280
Open
shrey150 wants to merge 1 commit into
Open
[STG-2407] fix(cli): honor BROWSERBASE_API_KEY passed to an already-running daemon#2280shrey150 wants to merge 1 commit into
shrey150 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 25323a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
2 issues found across 11 files
Confidence score: 3/5
- In
packages/cli/src/lib/driver/daemon/credentials.ts, credential forwarding only adds/updates values and never removes unset keys, so the daemon can keep using an old API key after the caller deletes it; this creates real auth/init inconsistency for users — propagate key removals (or rebuild the forwarded credential set) before merging. - In
packages/cli/src/lib/driver/session-manager.ts, the matching-signature early return can preserve stale init backoff when credentials change mid-init, so retries with the new key are delayed instead of immediate; this can make credential fixes feel ineffective — reset/override backoff when credential state changes before merge.
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/cli/src/lib/driver/session-manager.ts">
<violation number="1" location="packages/cli/src/lib/driver/session-manager.ts:115">
P2: Matching-signature early return can keep stale init backoff after credentials changed during an in-flight init. This delays retry with the new key, contradicting immediate-retry behavior.</violation>
</file>
<file name="packages/cli/src/lib/driver/daemon/credentials.ts">
<violation number="1" location="packages/cli/src/lib/driver/daemon/credentials.ts:30">
P2: Forwarded-credential sync is one-way: unset keys are never propagated, so daemon env keeps stale credentials. This can make init/retry behavior use an old API key after the caller has removed it.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as CLI Client Process
participant DaemonClient as daemon/client.ts
participant Socket as Local Socket (Unix)
participant DaemonServer as daemon/server.ts
participant SessionMgr as DriverSessionManager
participant Remote as remote.ts (capability)
participant Browserbase as Browserbase Cloud
Note over Client,Browserbase: Remote session initiation via daemon (warm or cold)
Client->>DaemonClient: openViaDaemon(url, options)
DaemonClient->>DaemonClient: collectClientCredentials(process.env)
DaemonClient->>Remote: getRemote().forwardedCredentialKeys()
Remote-->>DaemonClient: ["BROWSERBASE_API_KEY", "BROWSERBASE_PROJECT_ID"]
DaemonClient->>DaemonClient: Filter env to {"BROWSERBASE_API_KEY": "...", "BROWSERBASE_PROJECT_ID": "..."}
DaemonClient->>Socket: sendDriverRequest({ type: "open", url, credentials, id })
Socket-->>DaemonServer: deserialize request
Note over DaemonServer,SessionMgr: NEW: Forward credentials before session init
DaemonServer->>SessionMgr: applyForwardedCredentials(request.credentials)
SessionMgr->>SessionMgr: applyForwardedCredentials(credentials)
SessionMgr->>SessionMgr: applyForwardedCredentials() sets process.env vars
SessionMgr->>SessionMgr: compute credential signature
alt No existing stagehand (not initialized)
alt Credential signature changed
SessionMgr->>SessionMgr: Clear initFailure and consecutiveInitFailures
Note over SessionMgr: Enables immediate retry instead of backoff replay
end
SessionMgr->>Remote: remoteStagehandOptions()
Remote->>Remote: Read process.env.BROWSERBASE_API_KEY (now forwarded)
Remote-->>SessionMgr: StagehandConstructorOptions with valid key
SessionMgr->>Browserbase: stagehand.init() with forwarded key
Browserbase-->>SessionMgr: Browser session established
SessionMgr-->>DaemonServer: Open result
else Already initialized (warm session)
Note over SessionMgr: NEW: Early return skips credential reset
SessionMgr->>SessionMgr: if stagehand && context exists, return immediately
SessionMgr-->>DaemonServer: Reuse existing browser session
end
DaemonServer-->>Socket: writeResponse({ data: result, id })
Socket-->>DaemonClient: deserialize response
DaemonClient-->>Client: OpenResult
Note over Client,Browserbase: Later command reusing warm daemon
Client->>DaemonClient: runDriverCommandViaDaemon(command, params)
DaemonClient->>DaemonClient: collectClientCredentials(process.env)
DaemonClient->>Socket: sendDriverRequest({ type: "command", command, params, credentials, id })
Socket-->>DaemonServer: deserialize request
DaemonServer->>SessionMgr: applyForwardedCredentials(request.credentials)
SessionMgr->>SessionMgr: Same signature as before → no-op
SessionMgr->>SessionMgr: execute(command, params) on warm session
SessionMgr-->>DaemonServer: Command result
DaemonServer-->>Socket: writeResponse
Socket-->>DaemonClient: deserialize
DaemonClient-->>Client: Command output
Note over Client,Browserbase: Key-less first call then keyed retry (the bug fix)
Client->>DaemonClient: openViaDaemon(url) [no API key set]
DaemonClient->>DaemonClient: collectClientCredentials returns undefined
DaemonClient->>Socket: sendDriverRequest({ credentials: undefined })
Socket-->>DaemonServer: deserialize
DaemonServer->>SessionMgr: applyForwardedCredentials(undefined)
SessionMgr->>SessionMgr: init() fails → Missing BROWSERBASE_API_KEY
SessionMgr->>SessionMgr: Cache initFailure with backoff
SessionMgr-->>DaemonServer: Error response
Note over Client: User sets BROWSERBASE_API_KEY=xxx
Client->>DaemonClient: openViaDaemon(url) [now with key]
DaemonClient->>DaemonClient: collectClientCredentials returns {"BROWSERBASE_API_KEY": "xxx"}
DaemonClient->>Socket: sendDriverRequest({ credentials: {"BROWSERBASE_API_KEY": "xxx"} })
Socket-->>DaemonServer: deserialize
DaemonServer->>SessionMgr: applyForwardedCredentials({BROWSERBASE_API_KEY: "xxx"})
SessionMgr->>SessionMgr: New credential signature, clear cached failure & backoff
SessionMgr->>SessionMgr: init() runs immediately with forwarded key
SessionMgr->>Browserbase: stagehand.init() with valid key
Browserbase-->>SessionMgr: Session created successfully
SessionMgr-->>DaemonServer: Success
DaemonServer-->>Socket: Success response
Socket-->>DaemonClient: deserialize
DaemonClient-->>Client: SUCCESS remote https://...
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
4563ebc to
0c5cb08
Compare
The detached driver daemon captures process.env once at spawn time, so an inline or exported BROWSERBASE_API_KEY set after the daemon started never reached it and remote sessions kept failing with "Missing BROWSERBASE_API_KEY". The client now forwards its credentials over the (localhost, owner-only) driver socket with every open/command, and the daemon threads them straight into the Stagehand constructor at session init — never into its own process.env, so the key's only home is the live session. A cold session whose credentials changed clears the cached init-failure backoff so the retry runs immediately; warm sessions are untouched (credentials only matter at init). The local-only (CDP-only) build forwards nothing and stays free of any API-key code path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0c5cb08 to
25323a4
Compare
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
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.
TL;DR
BROWSERBASE_API_KEY=xxx browse open <url> --remotewas silently ignored when the driver daemon was already running, so the CLI kept printingMissing BROWSERBASE_API_KEY. The daemon froze a copy ofprocess.envat spawn time and never saw the late key. Fix: the client now forwards the key with every command, and the daemon threads it straight into the Stagehand session at init — no restart, nobrowse stop, warm sessions untouched.Closes STG-2407. Reported via the partner AX update (partner-2027dev, Jun 22); confirmed still live on
main.Symptom
This burns 3–4 retries per session and blocks the documented recover-after-interruption flow.
Root cause
The CLI is a thin client that talks to a long-lived background daemon (which holds the warm browser session). The key never reached that daemon:
detachedwithenv: process.envcaptured once at spawn time (daemon/client.ts). A key exported/inlined in a later shell never propagates to it.process.env.BROWSERBASE_API_KEYthere (remote.ts→session-manager.ts).daemon/protocol.ts). Since--remoteis explicit, the client never blocked on the key — so it happily started a doomed key-less daemon.The fix
Make the client the source of truth for the key; never trust the daemon's frozen env.
daemon/credentials.ts(new) —collectClientCredentials()reads the caller's env;credentialSignature()is a secret-freesha256fingerprint used only to detect key changes.protocol.ts/client.ts— everyopen/commandrequest carries the caller'scredentials.server.ts/session-manager.ts— the daemon threads the forwarded key into the Stagehand constructor at init. It is never written into the daemon'sprocess.env. When the fingerprint changes on a cold session, the stale init backoff is cleared so the retry runs immediately.Why thread into the constructor instead of writing
process.env?The key is read exactly once per session (at init); afterward the live
Stagehandinstance holds it. Writing it into the daemon's global env would leave a stray secret with no reader. Threading keeps the credential scoped to the session, and the change-detector is hashed so the raw key isn't kept in a second field. There's no perf cost — forwarding ~100 bytes is free next to cloud session creation; the only thing cached for speed is the warm session, which is independent of the key value.Rejected alternatives: writing the key into the daemon's env (stray secret, one-way env accumulation); auto-restarting the daemon (kills warm sessions, racy); a mere actionable error (the ask is for it to work, not just guide).
Security contract (local-only build)
BROWSERBASE_API_KEYmust not appear in the CDP-only artifact. The forwardable-key list is capability-gated:forwardedCredentialKeys()returns the key in the full build (remote.ts) and[]inremote.disabled.ts.collectClientCredentials/credentialSignatureiterate the received object's own keys, so they stay key-name-free. Net: the literal lives only indist/lib/driver/remote.js, andtests/local-only-build.test.tsstill passes.Testing
Local full build (
pnpm build) of the code under review, real Browserbase key, exercising the exact repro against an already-running key-less daemon.open … --remote(spawns daemon)Missing BROWSERBASE_API_KEY(daemon stays up)BROWSERBASE_API_KEY=… open … --remote, same daemon, no project id anywhereSUCCESS—"title": "Example Domain","url": "https://example.com/"open https://www.iana.org --remoteSUCCESS—"Internet Assigned Numbers Authority", sametargetIdvitestdriver-foundation + remote-disabledprocess.envtsc -p tsconfig.local-only.json+ local-only-build testpnpm lintFollow-ups (not in this PR)
browse openECONNREFUSED (also AX-flagged):sendDriverRequest(client.ts) has no connect-retry, so a transientECONNREFUSED/ENOENT(stale socket / daemon mid-shutdown) propagates raw. Couldn't reproduce under load — tracking separately rather than shipping unverified.--remoteguard (defense in depth): make explicit--remoteresolve the key client-side likeautoSelectRemoteTargetalready does, so a key-less first call fails fast instead of spawning a doomed daemon. Forwarding alone fixes the reported bug; the guard would just improve the first-call error.🤖 Generated with Claude Code