Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The change is a focused fix that triggers the existing primary connection retry immediately after pairing succeeds. Because it modifies an authentication-directory file, the sensitive-code review requirement calls for human review. Notes:
You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAfter credential submission succeeds, PairingRouteSurface retries the primary environment when its ID is available, then calls onAuthenticated in a transition. Failed submissions still set an error and return before these actions. ChangesPairing authentication
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Successful pairing retries the connection with an established session before navigation. No material merge risk remains. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change requests an earlier connection attempt only after pairing succeeds. It reuses the existing primary-environment retry mechanism without changing credentials, permissions or server authorization. No material security risk was found in the changed flow. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Closing this. #9785 already fixes the same bug on main: after pairing succeeds, the pair page now reloads the whole page, and a fresh page starts a fresh connection with no leftover retry timer. I retested on main 30cc788 with a production web build in headless Chromium. A tab that sat on |
If a browser sat on the pair page for a while before you pasted a pairing token, the app said "Still connecting" for up to 3 minutes after you clicked Continue. Pairing had worked. The app was waiting out a retry timer.
While a browser is unpaired, the server rejects its WebSocket, so the connection keeps retrying with a delay that doubles each time. Nothing told the connection that pairing had succeeded, so it waited for the next scheduled try. After pairing, the pair page now asks the primary environment to retry right away, the same as the Reconnect button does.
This is a small fix for an obvious bug under the contributing guide: the bug comes from one missing call after a successful pairing, and the change adds that call on the success path only.
Testing
I ran a production web build (
vp build) served bynode apps/server/src/bin.ts --mode webagainst a fresh base dir. Playwright drove headless Chromium: open/pairwith no session, wait, mint a token witht3 pair, paste it, click Continue, then measure when the app's/wssocket opens. An overlay in the page shows real time and the last socket events.Screenshots 6 seconds after Continue: all three runs on main show "Still connecting", and all three with the fix show the app.
Recordings of the 3-minute run, at 6x (the overlay clock is real time):
Full MP4s: before, after.
Other cases, run with the fix:
/pair. The retry only runs after a successful pairing, so neither starts an extra socket.registerPairing, not this page, so they're unchanged. Desktop authenticates the primary with a bearer and doesn't show this page.The retry itself is the existing
retryNow, covered by "explicit retry interrupts the current backoff" and "explicit retry starts a fresh backoff sequence" insupervisor.test.ts. I didn't add a component test. It would only assert that the callback is wired.tsc --noEmitinapps/webpasses. Lint on the changed file has no new findings.🤖 Generated with Claude Code (Claude Opus 5.5)