feat(browser): use one global human-authenticated profile#810
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughChangesThe built-in browser now uses a global persistent profile with independent tab collections, actor-scoped leases, authenticated desktop bridge calls, persistent permissions and tab state, origin-access authorization, and profile diagnostics across CLI, IPC, preload, and renderer surfaces. Built-in browser platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/desktop/src/main/services/pty/ptyService.ts (1)
2613-2615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate revoke guard across three teardown paths.
The same
if (!chatSessionId && isTrackedCliToolType(...)) revokeBuiltInBrowserActorCapability(sessionId)guard is repeated verbatim incloseEntry, the orphaned-dispose branch, and the maindisposebranch. Extracting a small helper (e.g.revokeBrowserActorCapabilityIfStandalone(sessionId, chatSessionId, toolType)) would keep this security-relevant check consistent if the condition ever needs to change.♻️ Suggested helper
+function revokeBrowserActorCapabilityIfStandalone( + sessionId: string, + chatSessionId: string | null | undefined, + toolType: TerminalToolType | null, +): void { + if (!chatSessionId && isTrackedCliToolType(toolType)) { + revokeBuiltInBrowserActorCapability(sessionId); + } +}Then replace each of the three inline checks with a call to this helper.
Also applies to: 4961-4963, 4996-4998
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 2613 - 2615, Extract the repeated standalone tracked-CLI capability check into a helper such as revokeBrowserActorCapabilityIfStandalone, accepting sessionId, chatSessionId, and toolType. Replace the inline guards in closeEntry and both orphaned-dispose and main dispose branches with calls to this helper, preserving the existing revoke behavior.apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts (1)
2128-2221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider re-validating tab liveness after the human-approval await in
navigate/createTab.Both functions
await args.agentAccessController.requireUrlAccess(...)(which can pause for an async human-approval prompt) before mutatingexistingTab/claiming ownership and callingwc.loadURL(). A concurrentcloseTab/window-close during that pause could leavetab.webContentsdestroyed by the timeloadURLis called, surfacing a raw Electron "Object has been destroyed" error instead of a clear domain error.claimTabOwnerFromInputalready guards onisDestroyed(), but the finalwc.loadURL(targetUrl)call does not.♻️ Proposed guard
reclaimTabForHumanNavigation(tab, input); claimTabOwnerFromInput(tab, input); armAgentNavigationGuard(tab, input); const wc = tab.webContents; + if (wc.isDestroyed()) throw new Error("Browser tab was closed while waiting for access approval."); attachViewsToCurrentWindow(); await wc.loadURL(targetUrl);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts` around lines 2128 - 2221, Revalidate the target tab after the await on requireUrlAccess in both navigate and createTab, before claiming ownership, mutating tab state, or calling loadURL. Check that the tab and its webContents still exist and are not destroyed; if they are unavailable, throw a clear domain-level error rather than invoking Electron on a destroyed object.apps/desktop/src/main/services/builtInBrowser/builtInBrowserProfileMigration.ts (1)
29-103: 🚀 Performance & Scalability | 🔵 TrivialLegacy partition sessions stay resident for the app's lifetime.
args.getSession(persist:${partitionName})opens a full ElectronSessionfor each of up toMAX_LEGACY_PARTITIONS(64) legacy partitions during migration. Electron caches sessions by partition string with no unload API, so these stay memory-resident for the process's life even though they're only needed for this one-time cookie scan.This is a one-time startup cost bounded by 64 partitions, so likely acceptable in practice — flagging for awareness rather than as a blocking issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserProfileMigration.ts` around lines 29 - 103, Review the legacy partition migration in migrateLegacyBuiltInBrowserProfiles and avoid opening persistent Electron sessions for every legacy partition when the API provides a temporary or otherwise non-resident cookie-store access path. Preserve the existing cookie filtering, migration, retry, and completion behavior while ensuring one-time scans do not retain up to MAX_LEGACY_PARTITIONS sessions for the process lifetime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.ts`:
- Line 6: Update callBridge so authenticatedParams(params) validates the bridge
auth token before ensureClient() opens or reuses a connection and before
entering the connection-error handling path. Missing-token failures must return
without invoking drop(error) or emitting connection_dropped, while genuine
connection failures retain the existing cleanup behavior.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserAgentAccess.ts`:
- Around line 73-99: The origin authorization flow incorrectly short-circuits
before high-risk-host and authenticated-cookie checks can run. Update
authorizeUrl and isKnownSensitiveUrlSync so inspectOrigin reaches
isHighRiskHost, cookieDomainsContainHost, and the session-cookie scan for
origins where those checks are intended to affect authorization; otherwise
remove the unreachable branches, while preserving permission and
authenticated-origin handling.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 3324-3345: Update setBounds() to await tabRestorationPromise
before invoking ensureActiveTab() or otherwise manipulating tab bounds, matching
the existing gating used by navigate() and createTab(). Preserve the current
bounds behavior after restoration completes and prevent any tab creation during
restorePersistedTabs().
---
Nitpick comments:
In
`@apps/desktop/src/main/services/builtInBrowser/builtInBrowserProfileMigration.ts`:
- Around line 29-103: Review the legacy partition migration in
migrateLegacyBuiltInBrowserProfiles and avoid opening persistent Electron
sessions for every legacy partition when the API provides a temporary or
otherwise non-resident cookie-store access path. Preserve the existing cookie
filtering, migration, retry, and completion behavior while ensuring one-time
scans do not retain up to MAX_LEGACY_PARTITIONS sessions for the process
lifetime.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 2128-2221: Revalidate the target tab after the await on
requireUrlAccess in both navigate and createTab, before claiming ownership,
mutating tab state, or calling loadURL. Check that the tab and its webContents
still exist and are not destroyed; if they are unavailable, throw a clear
domain-level error rather than invoking Electron on a destroyed object.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 2613-2615: Extract the repeated standalone tracked-CLI capability
check into a helper such as revokeBrowserActorCapabilityIfStandalone, accepting
sessionId, chatSessionId, and toolType. Replace the inline guards in closeEntry
and both orphaned-dispose and main dispose branches with calls to this helper,
preserving the existing revoke behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f766bde9-5900-4154-826b-fdadac66819b
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/PRD.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/personal-chats/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**
📒 Files selected for processing (51)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.test.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeMethods.tsapps/desktop/build/entitlements.mac.plistapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserActorCapabilities.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserAgentAccess.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserAuthentication.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserConstants.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserNavigation.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserPermissions.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserPermissions.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserPersistence.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserProfileMigration.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserSecurity.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserStateStore.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.tsapps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.test.tsapps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.test.tsapps/desktop/src/main/services/remoteRuntime/runtimeRpcClient.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.test.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/builtInBrowser.ts
💤 Files with no reviewable changes (5)
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserConstants.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserPermissions.test.ts
- apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.test.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserNavigation.test.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserWebAuthn.test.ts
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28d7bd7edb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts (2)
2626-2644: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize against the tab that owns the selection
selectCurrent()andclearSelection()validate the active tab, butlastSelectedItemmay have been created byselectPoint()for a different, non-active tab. An agent owning the active tab can therefore read or clear another agent’s selected artifact. Track the selection’s owning tab and authorize against that tab before returning or clearing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts` around lines 2626 - 2644, Track the tab identity associated with lastSelectedItem when selectPoint() creates the selection, and update selectCurrent() and clearSelection() to authorize through that owning tab rather than only activeTab(). Return or clear the selection only after the owning-tab authorization succeeds, while preserving the existing selection and status emissions.
462-463: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel deferred tab restoration when disposing the service
A fallback or window service can be disposed while
tabRestorationPromiseis awaiting profile migration. Once migration completes,restorePersistedTabs()still creates newWebContentsViewinstances, resurrecting the disposed service and leaking browser resources. Add a disposed guard around restoration.Proposed fix
let restoringTabs = false; + let disposed = false; function dispose(): void { + disposed = true; inspecting = false; // ... } const restorePersistedTabs = async (): Promise<void> => { + if (disposed) return; const restored = args.restoredState; if (!restored?.tabs.length) return; restoringTabs = true; // ... const results = await Promise.allSettled(/* ... */); restoringTabs = false; + if (disposed) return; // log and emit status };Also applies to: 2647-2685, 3334-3365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts` around lines 462 - 463, Guard deferred tab restoration with the service’s disposed state: update restorePersistedTabs() and its tabRestorationPromise continuation so they exit without creating WebContentsView instances after disposal. Ensure dispose() marks the service disposed before clearing fallback services, and apply the same guard to the related restoration paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 2626-2644: Track the tab identity associated with lastSelectedItem
when selectPoint() creates the selection, and update selectCurrent() and
clearSelection() to authorize through that owning tab rather than only
activeTab(). Return or clear the selection only after the owning-tab
authorization succeeds, while preserving the existing selection and status
emissions.
- Around line 462-463: Guard deferred tab restoration with the service’s
disposed state: update restorePersistedTabs() and its tabRestorationPromise
continuation so they exit without creating WebContentsView instances after
disposal. Ensure dispose() marks the service disposed before clearing fallback
services, and apply the same guard to the related restoration paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 386d8640-3d9d-4b38-9cf4-ba0d422c4ef5
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/proof.mdis excluded by!docs/**
📒 Files selected for processing (13)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.test.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeMethods.tsapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserAgentAccess.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserSecurity.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.test.tsapps/desktop/src/main/services/computerUse/computerUseArtifactBrokerService.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/resources/agent-skills/ade-browser/SKILL.md
- apps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.ts
- apps/desktop/src/main/main.ts
- apps/ade-cli/src/adeRpcServer.test.ts
- apps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.test.ts
- apps/ade-cli/src/adeRpcServer.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts
|
Addressed the two CodeRabbit outside-diff findings in f51e6e0:
Focused result: 82 browser-service tests passed, desktop typecheck passed, and focused lint/diff checks are clean. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51e6e09cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a27caeb1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| profile, | ||
| collection, | ||
| restoredState: stateStore?.restore(collection.key) ?? null, | ||
| onStateChange: (state) => stateStore?.record(collection.key, state), |
There was a problem hiding this comment.
When the same project is open in more than one ADE window, each window gets a separate in-memory service, but persisted state is restored and recorded under only collection.key (the project hash). That makes both windows write to the same state-store entry, so whichever window emits status last overwrites the other window's tab list; after restart the supposedly independent per-window tab collections restore the last writer's tabs. Include a window-specific component in the persisted key for project/window collections.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Platform limitation
Electron Touch ID WebAuthn credentials are device-bound and session-partition-bound. ADE does not claim Apple Passwords or iCloud-synced passkey support.
Summary by CodeRabbit
Greptile Summary
This PR moves ADE browser automation onto one durable browser profile while tightening actor-scoped access. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Reviews (5): Last reviewed commit: "fix(browser): validate actor grants in E..." | Re-trigger Greptile