feat(bridge/p2): stateful browser actions with mobile approval flow#491
Merged
Conversation
Enable fill_field/click behind Layer 2 halt, Expo Push approval cards, REST approve-action, and a WS-decoupled auth observer with 5-min TTL. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
cloud-agent/src/handlers/expoPushToken.test.ts (1)
5-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the missing-user path.
This mock only proves that
set()saw the token. It would still pass ifupsertExpoPushTokenmatched zero rows, which is the failure mode the public route needs to guard against.🤖 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 `@cloud-agent/src/handlers/expoPushToken.test.ts` around lines 5 - 20, Add a regression test for the missing-user path in upsertExpoPushToken. Extend the existing expoPushToken.test.ts coverage so the fakeDb/update chain can simulate a where() that matches zero rows, then assert the function detects that no user was updated and returns or throws the expected failure. Use the upsertExpoPushToken helper and the fakeDb.update().set().where() mock to verify the route cannot silently succeed when no row is matched.
🤖 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 `@app/_layout.tsx`:
- Around line 140-143: The push token registration in useRegisterExpoPushToken
is still passing an empty string when the Expo config projectId is missing.
Update the projectId selection in this call to prefer
Constants?.expoConfig?.extra?.eas?.projectId and fall back to
Constants?.easConfig?.projectId, and make sure registration is skipped or an
error is thrown when neither value exists so Notifications.getExpoPushTokenAsync
is never called with an invalid projectId.
In `@cloud-agent/src/handlers/authApprovalObserver.ts`:
- Around line 46-50: The auth approval flow in settle() currently clears the
observer and then only logs wakeExtension() failures, which can let an approved
task remain stuck in awaiting_auth while still calling onResolved(). Update the
settle/wakeExtension path in authApprovalObserver.ts so a failed wake is treated
as a failure state: capture the wake result, and if wakeExtension() rejects,
abort or retry the task before invoking onResolved(), keeping the observer/task
recovery logic intact in the affected approval branch.
In `@cloud-agent/src/handlers/expoPushToken.ts`:
- Around line 6-12: `upsertExpoPushToken` currently only performs an `UPDATE` on
`users` and returns success even when no row matches the given `firebaseUid`.
Update this function to detect the affected row count from the
`db.update(users)...where(eq(users.firebaseUid, firebaseUid))` call and throw an
error or otherwise fail when zero rows are updated, so the
`/agent/user/expo-push-token` path does not acknowledge a token write that never
happened.
In `@cloud-agent/src/handlers/wsBrowserAgentHandler.ts`:
- Around line 165-182: The auth approval flow in wsBrowserAgentHandler should
not continue when getDeviceFcmToken() returns null because
startAuthApprovalObserver() can resolve approval without any way to wake the
extension or finalize the task. In the handler where deviceFcmToken is read and
startAuthApprovalObserver() is called, add an early abort path that marks the
session/task as terminal and exits before registering the observer when no
device token is available. Use the existing symbols deviceFcmToken,
startAuthApprovalObserver, sendSessionEndIfOpen, and dispatchedIntent to locate
the flow and ensure the user never ends up stuck in awaiting_auth.
- Around line 147-180: Validate the incoming awaitingAuth frame in
onAwaitingAuth against the currently dispatchedIntent before calling
fs.haltForAuth or startAuthApprovalObserver, using the taskId from
dispatchedIntent as the source of truth. Reject stale or malformed frames where
parsed.data.taskId does not match the active task, and add bounds checks for
haltedStepIndex so it cannot exceed the dispatched intent’s step list. Keep the
guard close to awaitingAuthFrameSchema parsing and the startAuthApprovalObserver
call so the task pairing remains consistent.
In `@cloud-agent/src/index.ts`:
- Around line 354-365: The /agent/browser/approve-action handler in the app.post
route should be guarded the same way as the other browser-bridge endpoints.
Before calling admin.firestore() or handleApproveAction, check that the browser
bridge/Admin app is available and return the controlled 503 response when it is
not, instead of allowing an uninitialized Admin app to throw; use the existing
register-device flow as the pattern and keep the fix localized to the
approve-action route.
- Around line 407-410: The resolveUserId implementation in
cloud-agent/src/index.ts is doing the users.id lookup correctly but returns the
Firebase UID instead of the database UUID. Update the resolveUserId callback to
return the queried user’s id value from the db select result, and keep the null
fallback when no matching row is found; use the existing users.id lookup and
resolveUserId symbol to locate the fix.
- Around line 362-367: The approval-token parsing in the `rawToken` handling
should match `requireAuth` exactly, since the current `replace('Bearer ', '')`
only strips a capitalized prefix and does not trim whitespace. Update the token
extraction near `handleApproveAction` to use the same case-insensitive bearer
parsing and trimming logic as `requireAuth`, so headers like `authorization:
bearer <token>` are normalized before being passed as `idToken`/`approvalToken`.
In `@cloud-agent/src/services/fcmDispatcher.ts`:
- Around line 10-16: The Expo POST in expoPush() can stall and block the
auth-halt path before the approval observer starts. Update the fetchImpl() call
to use a short timeout/abort mechanism, and handle timeout failures as a
best-effort notification error so the state transition can continue. Keep the
fix localized to expoPush() in fcmDispatcher.ts and preserve the existing non-OK
response handling.
In `@cloud-agent/src/services/firestoreSession.ts`:
- Around line 139-151: The auth document write in firestoreSession’s state
transition flow is happening after batch.commit(), which can expose
awaiting_auth/pending_auth before the auth doc exists. Update the same db.batch
path in firestoreSession so the task update, session update, and
db.doc(authPath).set(...) are all included in one batch before commit, while
keeping the non-batch fallback in sync if needed.
In `@docs/superpowers/plans/2026-06-29-phase2-browser-bridge-stateful-actions.md`:
- Around line 1969-1977: The Firestore rules example in the document is fenced
without a language label, which will fail the MD040 lint rule. Update the
markdown fence around the rules snippet to use a concrete label such as
firestore or text, keeping the same content inside the block. The change is in
the Firestore rules example that includes the match
/users/{uid}/sessions/{sessionId}/auth/{taskId} block.
- Around line 1959-1963: The runbook uses a machine-specific absolute search
path for locating firestore.rules, which makes it non-portable across checkouts.
Update the Step 1 command in the plans document to use a repo-relative root like
find . -name "firestore.rules" while preserving the existing node_modules
exclusion, or switch to an env-var-based root if needed. Keep the guidance
aligned with the surrounding “Locate the rules file” step so it works from any
clone.
In `@extension/src/background/task-dispatcher.ts`:
- Around line 24-35: The awaiting_auth early return in task-dispatcher’s step
loop drops previously collected state, so preserve the accumulated data and
activeUrl when exiting on auth pause. Update the task-dispatcher flow around
runInActiveTab/openTab to include partial state in the awaiting_auth response or
halt record, and make the resume path use that state when restarting from
haltedStepIndex so earlier results are merged into the final TaskResult.
In `@extension/src/content/executor.ts`:
- Around line 39-40: The Layer 2 auth gate in classifyElement is missing
destructive input controls because it only checks textContent and submit-in-form
behavior. Update classifyElement to inspect form-control labels like input.value
and, if available, accessible labels before returning a safe classification, so
controls such as button-like inputs with destructive text are marked
requires_auth. Keep the executor.ts check that depends on classifyElement
unchanged, and ensure the same fix covers the related logic referenced by the
other affected location.
- Around line 37-43: The fill_field path in executor.ts currently assumes any
querySelector result can accept value assignment, which can silently fail on
non-form elements and bypass controlled input tracking. Update the fill logic in
the executor action handling to first verify the matched element is a supported
form control, then use the native value setter from the element prototype before
dispatching input and change events. Keep the existing selector lookup, auth
check, and event dispatch flow, but add the guard and native setter use around
the element handling.
In `@src/hooks/useBrowserActionApproval.ts`:
- Around line 34-45: The approval request in useBrowserActionApproval’s fetch
call is treating non-2xx responses as success because fetch does not reject on
HTTP errors. Update the approve-action flow to explicitly check the response
status after the request in the same hook and throw or surface an error for any
non-2xx result, so callers can retry or fail the browser action properly. Keep
the change localized around useBrowserActionApproval and the approve-action
request handling.
- Around line 24-25: `TaskManager.defineTask(...)` is currently registered
inside the `useBrowserActionApproval` hook, which can delay task setup until
after React mounts. Move the `APPROVAL_TASK` registration to module scope in
`useBrowserActionApproval.ts`, keeping the `TaskManager.isTaskDefined(...)`
guard with the `defineTask` call so the background task is always registered
before any hook runs.
- Around line 27-44: The useBrowserActionApproval handler is treating Expo’s
default notification-body tap as a deny action because it only checks for
actionIdentifier presence before sending the approval request. Update the logic
in useBrowserActionApproval so it returns early unless actionIdentifier is
exactly APPROVE or DENY, and only then build the approve value and call the
/agent/browser/approve-action request. Keep the existing
currentUser/sessionId/taskId flow intact, but gate the fetch on valid action
identifiers only.
In `@src/hooks/useRegisterExpoPushToken.ts`:
- Around line 14-30: The push-token registration flow in
useRegisterExpoPushToken currently ignores non-OK fetch responses and lets async
failures bubble unhandled, so update the effect to explicitly handle errors
around Notifications.getPermissionsAsync, requestPermissionsAsync,
getExpoPushTokenAsync, user.getIdToken, and the fetch call. In the same hook,
check the fetch response status and treat 401/500-style failures as registration
errors by logging or surfacing them and returning early, so the Expo push token
is only considered registered when the request truly succeeds.
---
Nitpick comments:
In `@cloud-agent/src/handlers/expoPushToken.test.ts`:
- Around line 5-20: Add a regression test for the missing-user path in
upsertExpoPushToken. Extend the existing expoPushToken.test.ts coverage so the
fakeDb/update chain can simulate a where() that matches zero rows, then assert
the function detects that no user was updated and returns or throws the expected
failure. Use the upsertExpoPushToken helper and the
fakeDb.update().set().where() mock to verify the route cannot silently succeed
when no row is matched.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45e5e9d4-995c-48e5-af11-ab420dce9b58
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
__tests__/useBrowserActionApproval.test.ts__tests__/useRegisterExpoPushToken.test.tsapp/_layout.tsxcloud-agent/src/db/schema.tscloud-agent/src/handlers/approveAction.test.tscloud-agent/src/handlers/approveAction.tscloud-agent/src/handlers/authApprovalObserver.test.tscloud-agent/src/handlers/authApprovalObserver.tscloud-agent/src/handlers/expoPushToken.test.tscloud-agent/src/handlers/expoPushToken.tscloud-agent/src/handlers/wsBrowserAgentHandler.test.tscloud-agent/src/handlers/wsBrowserAgentHandler.tscloud-agent/src/index.tscloud-agent/src/services/fcmDispatcher.test.tscloud-agent/src/services/fcmDispatcher.tscloud-agent/src/services/firestoreSession.test.tscloud-agent/src/services/firestoreSession.tscloud-agent/src/tools/browserAction.test.tscloud-agent/src/tools/browserAction.tsdocs/superpowers/plans/2026-06-29-phase2-browser-bridge-stateful-actions.mdextension/src/background/content-bridge.tsextension/src/background/service-worker.tsextension/src/background/task-dispatcher.test.tsextension/src/background/task-dispatcher.tsextension/src/background/ws-client.test.tsextension/src/background/ws-client.tsextension/src/content/executor.test.tsextension/src/content/executor.tsfunctions/drizzle/0017_expo_push_token.sqlfunctions/src/db/schema.tspackage.jsonshared/dsl-types.tssrc/hooks/useBrowserActionApproval.tssrc/hooks/useRegisterExpoPushToken.ts
- Expo projectId: add Constants.easConfig fallback - useBrowserActionApproval: module-scope defineTask; skip DEFAULT_ACTION_IDENTIFIER; throw on non-2xx approve-action response - useRegisterExpoPushToken: try/catch + throw on non-2xx - authApprovalObserver: abort task when FCM wake fails after approval - expoPushToken: throw USER_NOT_FOUND when no row matched by upsert - wsBrowserAgentHandler: validate awaiting_auth taskId + haltedStepIndex bounds; abort early when deviceFcmToken is null; carry partialData/partialActiveUrl and merge into resumed task result - index: guard approve-action with browserBridgeAvailable 503; fix Bearer token parsing (case-insensitive + trim); fix resolveUserId to return u?.id not firebaseUid - fcmDispatcher: 5s AbortController timeout on Expo push POST - firestoreSession: add FirestoreBatch.set; batch auth doc write with state flip; persist partialData/partialActiveUrl on halt - task-dispatcher/ws-client/service-worker: carry partial state through awaiting_auth - safety-classifier: check input.value and aria-label for destructive patterns - executor: guard fill_field to form controls; use native value setter - docs: repo-relative find path; language tag on Firestore rules fence Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
/fix-pr follow-upCommit: `3fe2c20b` Review resolution
Verification
|
This was referenced Jun 29, 2026
Merged
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.
Description
Phase 2 of the MV3 browser bridge: stateful
fill_fieldandclickactions behind a mobile approval flow. The extension halts at destructive steps (Layer 2 classifier), the Cloud Agent fires an Expo Push approval card, the mobile app approves via REST, and the extension resumes fromhaltedStepIndexafter FCM wake.Spec: 2026-06-29-mv3-browser-extension-bridge-design.md (Phase 2 sections)
Plan: 2026-06-29-phase2-browser-bridge-stateful-actions.md
Type of Change
Related Issue
N/A — phase gate tracked in plan Task 13 (manual staging E2E).
Changes Made
AuthDoc,haltForAuth/watchAuth, Expo Push (sendApprovalCard,sendTaskComplete),POST /agent/user/expo-push-token,POST /agent/browser/approve-action,awaiting_authWS handling, decoupledauthApprovalObserver(5-min TTL, denial/timeout feedback),browserActionteardown onawaiting_authfill_field/clickwith Layer 2 classifier, task-dispatcher halt,sendAwaitingAuth, service-worker suspend on haltuseRegisterExpoPushToken,useBrowserActionApproval(lock-screen APPROVE/DENY), wired inapp/_layout.tsxexpo_push_tokenmigration (functions/drizzle/0017_expo_push_token.sql) + schema mirrorsexpo-notifications,expo-task-managerTesting
Platforms Tested
Test Steps
cd cloud-agent && npm test— 168 passcd extension && npm test— 30 passnpm test -- __tests__/useRegisterExpoPushToken.test.ts __tests__/useBrowserActionApproval.test.tsfill_field+clicksubmit through mobile APPROVE tap (plan Task 13)Screenshots
N/A — backend/bridge changes; staging E2E pending.
Checklist
Additional Notes
staging; no rules diff in this PR.0017_expo_push_token.sqlbefore deploying cloud-agent to staging.Made with Cursor