feat(browser): make the built-in browser the default for chat - #2082
Conversation
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS The reachability/bootstrap fixes are sound; removing the per-session consent gate rests on a contestable "strictly weaker" argument the PR's own retained comment contradicts. Watch
Suggestions
[DESIGN-REVIEWED] b32beed |
UX Review (Fable 5) — 🟡 CONCERNSAdvisory UX-level review of UX-Verdict: CONCERNS Agent control of the user's logged-in browser is now invisible and unstoppable at the point of action — the deleted badge was the only status signal. Watch
Suggestions
[UX-REVIEWED] b32beed |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/electron/main.js:1377 -- A configured SSH tunnel still uses a localhost backend, so False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've reviewed the full diff and the relevant surrounding code (main.js dispatch/gate, mcp_playwright_proxy.py native routing, browser-control.js predicates, ChatPage/hook wiring, and both AUTOSDE rule snapshots). Key checks performed:
No reachable crash, data-loss, guard removal without compensation, or normal-path correctness break survives falsification. No findings. [OPUS-REVIEWED] b32beed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
e29587a to
336c7b3
Compare
Dispositions for
|
336c7b3 to
11c98d0
Compare
11c98d0 to
ffe778a
Compare
Dispositions for
|
ffe778a to
bcc5a3a
Compare
Dispositions for
|
3fa4a30 to
65a6f68
Compare
Disposition for
|
65a6f68 to
73b3aa6
Compare
|
Independent verification of the current GPT blocking finding — it is legitimate, please do not override it. I traced all three legs of the "consent survives dashboard reload" scenario against current main plus this diff:
Net effect: grant slots A and B → reload → the effect unregisters only A (active slot, now-false consent) → B stays registered, and the agent can bootstrap a fully consented native browser for B with no live user grant. This is a fourth route into the same hard-failure class this PR's review history already closed three times (unconsented-but-reachable sessions), except this one fails open rather than hard. Fix sketch (small, contained to // Consent lives in the renderer; a reload destroys it there, so mirror the
// destruction here. Re-registration happens naturally from fresh consent.
win.webContents.on("did-start-navigation", (_e, _url, isInPlace, isMainFrame) => {
if (!isMainFrame || isInPlace) return;
agentSessions.clear();
for (const entry of browserPanels.values()) {
entry.agentAct = false;
try { void entry.control.release(); } catch { /* mid-teardown */ }
}
});( Everything else in this PR checked out under review: all three original defects verified real against main (the |
73b3aa6 to
2444345
Compare
|
Reviewed SHA
The report's "only A unregisters" detail is exactly right, and it is the crux. The if (agentActEnabled) void api.registerSession(activeSlot)
else void api.unregisterSession?.(activeSlot)A reloaded renderer starts with an empty consent map, so the effect fires once for the Two points reinforce the severity rather than reduce it:
Fix: Two design points worth stating explicitly:
666 Electron tests (+2), 11681 frontend, |
|
Reviewed SHA Why it moved. The all-gated shape did not deliver this PR's own headline. A fresh The boundary is now drawn in three parts, and the middle one is a precondition for the
On (3) I want to be explicit, because my first attempt was wrong and the repo caught it. So the fallback is narrowed to one structural case: Electron emits Falling back is not an escalation: Playwright runs an isolated profile with an empty Known gap, stated rather than hidden: clicking the grant does not retroactively 668 Electron, 11789 frontend (876 files), 24 native-routing tests, tsc / eslint / isort / |
|
To be explicit about what the label normally asserts vs. what is true here. The gate's own The label is here only to unblock the rest of the round — Why the frame is not attached yet: the native view exists only in the Electron main What lands before merge, replacing this label:
Both will be committed under |
The native path had never executed: a constant-true gate verdict refused every bootstrap navigate even with consent granted, the command channel never polled for a session whose Browser tab was unopened, and a late-mounting panel silently revoked the grant by mirroring its own false default. Fixes all three, inverts the consent handshake to a pull so ordering cannot matter, removes dead chooseControlTransport, and corrects the web-browse skill which still described the panel as a Playwright mirror. No authorization boundary changes: every browser_* op still requires the per-session grant.
|
Reviewed SHA GPT — BLOCKING:
|
Problem
Ask the chat to open a page and you get a screenshot of a page, not a page. The
right-hand Browser panel paints frames streamed from a headless Playwright Chromium:
nothing in it is clickable, it carries none of your logins, and the built-in Electron
browser that was built for exactly this sits unused.
That was not a configuration mistake. The native path was unreachable code:
preis the verdict fromcanAgentControl, which returns{allowed, reason}— it hasno
agentActEnabledfield. The condition is therefore always true and the bootstrapwas always refused, even with the grant switched on. The expression had no test, so
it stayed wrong.
Only mounted panels were reported. A fresh chat has no panel, so no session key was
ever polled, so the gateway answered 503 and the very first "open this page" fell back
to the mirror before any gate code ran.
Why it matters
The built-in browser is the feature. A screenshot mirror cannot be operated, cannot use
a site you are logged into, and cannot be handed to the user to keep browsing. Two
defects — neither of them visible as an error — meant that no user on any platform had
ever reached the native path from chat.
Fix (symptoms → root cause → change)
Symptom: every chat-opened page renders as a Playwright screenshot.
Root cause: two independent hard blockers above, plus a consent model that could not
be satisfied in the default flow.
Change: make the native path reachable, then make it the default.
mayBootstrapView(verdict)— the constant-true expression is extracted into anamed predicate with tests in both directions. It tolerates exactly one refusal,
no-browser-view, which is the precondition the caller is about to satisfy byopening a view; every other refusal still stops the bootstrap.
browser:track-session+reachableSessions. The command channel can only deliver an op for a session key itpolls for, and it must poll before any URL is known, so reachability cannot depend
on anything about the request. The renderer declares its active slot; that is what
makes a fresh chat's first navigate arrive at all.
browser:agent-opened— when the agent bootstraps a view, the main process asksthe SPA to surface the Browser panel so it mounts, measures and reports bounds.
Without it the view exists but is composited nowhere and the user sees an empty panel.
Browser Mode is the authorization
Enabling Browser Mode in Settings is already a keystone-level grant. From
src/kiro_crew/security.py:So the repository already treats one deliberate Settings action as consent for a
stronger capability than the one at issue here: driving the operator's own logged-in
browser. Requiring a second, per-session gesture before the agent may open a page in
the built-in view gated a strictly weaker capability, and that inconsistency is what kept
the default path on the mirror. The per-session gate is removed; the
viewOpenprecondition and
mayBootstrapVieware unchanged.Removing it also removed the control that advertised it. The panel's "Let the agent act"
button could not have kept its meaning: the dispatch re-acquires LIGHT on every op, so
switching the button off produced one
release()and the next op took control straightback. A control that reads as a denial but is not must not ship, so the button, its four
i18n keys across all catalogs, and the now-dead wiring behind it
(
PREVIEW_ENABLE_BROWSE_EVENT, theBROWSE_MODE_EVENTmirror, the pull handshake, thehook's
agentActEnabledoption) are deleted. The human's own panel controls — addressbar, navigate, back, close — are untouched.
Native routing yields to extension mode
extension_mode(exposed by/api/browser/config) attaches Playwright to the operator'sown running browser.
_EXTENSION_MODEwas consulted only where mirror frames aresent, never in the routing decision — harmless while native required a per-session grant,
and a silent hijack the moment native becomes the default.
_try_native_tool_callnowyields immediately in extension mode, before any POST, so an explicit choice of an
external browser is honoured.
The resulting ladder, in precedence order:
extension_modeon — an explicit choice, now honoured in the routing pathLoopback targets are exempt from any gate
isLoopbackUrlkeepslocalhostungated. Cookies are host-scoped, so a dev server holdsno third-party session and there is no identity to protect. It is judged against the
target of a navigate, never the page already loaded, so a local page cannot launder
the agent onto a real site; lookalikes (
localhost.evil.com), non-http schemes andunparseable input are not exempt, and the exemption never lifts the "a view must exist"
precondition.
Tests
website/electron/test/browser-control.test.js—mayBootstrapViewin both directions(the predicate that shipped broken); an agent op is authorized with no per-session
grant while the view precondition still applies; the loopback exemption including the
localhost.evil.comspoof,file://, and unparseable input. The deadchooseControlTransportcases are dropped with the function.test/test_browser_native_routing.py—extension_mode_never_routes_to_the_native_viewasserts no POST is even attempted. The pre-existing
test_refusal_returns_an_mcp_error_and_does_not_fall_backpasses unchanged: a deny mustnever become an allow by another route.
place explaining why the coverage is gone rather than leaving a silent hole.
666 Electron tests, 11786 frontend tests (876 files, including the
catalogParity/deadKeys/dynamicKeysi18n ratchets that verify the key removal is consistent acrossall 13 catalogs), 25 native-routing tests.
tsc -b, eslint (0 errors), flake8, isort andthe brand gate are clean.
Manual verification
Not yet done — needs a rebuilt desktop app, and it matters more than usual here: every
defect in this PR survived because this path had no integration coverage, and the unit
tests pin gate logic rather than the gateway-bus ↔ poller ↔ panel-creation chain.
built-in browser and surfaces the panel. No per-session gesture anywhere.
browser stays logged in.
extension_modeon → the same request drives your own browser and does not openthe built-in view.
http://localhost:5173) opens natively too.Screenshots