Conversation
…browsing + local_browser arg
…_browser in test spies The two default-browser detectors call subprocess.run(text=True) without an explicit encoding, which the Windows-footgun linter (and its full-repo test, tests/scripts/test_footgun_subprocess_encoding.py) rejects. Pass encoding='utf-8', errors='replace' like the rest of the tree. Two existing tests replace browser_navigate / _navigation_session_key with positional-only lambdas; both callables now receive local_browser= from the registry handler and browser_navigate, so the spies raised TypeError. Accept the keyword with its default. Fixes the three CI failures on the PR head (Windows footguns lint, test_browser_extension_router_wiring x2, test_browser_open_timeout).
…talled-browser fallback
_detect_default_darwin matched a Chromium bundle id and the literal 'https'
anywhere in the whole LSHandlers dump, so a browser registered for ftp or a
content type was reported as the https default, and map order decided ties.
When nothing matched it fell back to the first installed Chromium app — with
Safari or Firefox as the actual default that drove a browser the user never
consented to, contradicting the docstring, the config comment and the desktop
copy ('a non-Chromium default fails with a clear message').
Parse the dump entry by entry, take the LSHandlerRoleAll/Viewer of the entry
whose LSHandlerURLScheme is https, and fail closed on anything else — an empty
handler list is what macOS stores while Safari is still the implicit default.
Tests feed real 'defaults read' output shapes instead of patching the detector
(reviewer fixture from the PR discussion: Safari on https, Chrome on ftp).
real_profile_data_dir hard-wired Linux to $XDG_CONFIG_HOME/<name>, and the xdg fragment map only knew the native package names. Ubuntu's default snap Chromium (xdg reports chromium_chromium.desktop, profile under ~/snap/chromium/common/chromium) and Flatpak builds (~/.var/app/<id>/config/…) therefore ended in 'profile directory was not found' for a browser the user runs every day, and Flatpak Chrome (com.google.Chrome.desktop) was reported as 'not a supported Chromium browser'. Try the native, snap and Flatpak locations and return the first that exists; fall back to the native path so the error message still names a concrete directory. Map the Flatpak application ids in the xdg lookup. Tests cover the xdg names for all four browsers in native and Flatpak form, and the directory preference order with a temp HOME.
…e existing local session _navigation_session_key returned the ::local sidecar key for local_browser before the cloud-provider and auto_local_for_private_urls checks. Two consequences: - Every private-URL gate in browser_navigate (credential-bearing query, _is_safe_url pre-nav, post-redirect) is keyed off the sidecar key, so a model-supplied browser_navigate(url, local_browser=True) opened LAN addresses in a host-side Chromium — with the real profile's cookies — even when the user had set browser.auto_local_for_private_urls: false. Consent to the profile is not consent to override the LAN routing opt-out; a private URL now follows auto_local_for_private_urls exactly as without the flag. - Without a cloud provider the bare session already is the local Chromium (and already carries the real profile when consented), so the flag created a second session for the same task on the same user-data-dir, which Chromium's process singleton refuses. The flag is now a no-op there. The existing consent/CDP-precedence tests keep their assertions; the sidecar test now states the cloud-provider precondition it silently relied on.
…-use CDP Copy the user's default-Chromium profile (auth state only) into a managed snapshot, launch Hermes' packaged Chromium on it via agent-browser, and hand the CDP endpoint to the Browser Use CLI (and built-in tools) to drive. The snapshot is a non-default dir, so it sidesteps Chrome 136+'s default-profile remote-debugging block and never contends with the user's running browser; launched without mock-keychain switches so keyring-encrypted cookies decrypt. - consent-gated browser_exec 'local' arg (schema only appears with consent) - fail-closed on non-Chromium default / snapshot failure - stale-session guard: reuse only when the live session is on our copy dir - snapshot excludes extensions/service-workers (renderer wedge) + caches
…reserve channel identity Addresses two P1 review blockers (kshitij / @kxee) on the real-profile feature: Credential-store lifecycle for ~/.hermes/browser-profile/ (copied Cookies/ Login Data): - exclude the singular 'browser-profile' dir from backup AND import (_EXCLUDED_DIRS drives both) — was silently archiving cookies/logins - add a browser-profile/ directory-PREFIX read-deny to agent/file_safety.py, same class as auth.json / mcp-tokens - secure the snapshot dir through the canonical hermes_cli.config._secure_dir (honors managed/NixOS group-share + HERMES_UID/GID), not a bespoke chmod Channel identity (NousResearch#95549 invariant — never normalize Beta/Dev/Canary to stable, which would drive a different account's profile): - detect recognized pre-release channels FIRST (Win ProgIds, macOS bundle ids, Linux .desktop) and return UNSUPPORTED_CHANNEL - macOS bundle match is now EXACT (was startswith); Linux/Win channel-before- stable ordering; real_profile_data_dir/chromium_executable reject the sentinel - _real_profile_cdp fails closed with a channel-specific message, never snapshots Tests: channel-not-normalized (linux/darwin/windows), wrong-principal fail-closed, backup exclusion, read-guard block/allow, snapshot dir secured. 187 browser + 222 backup/file_safety pass. Live re-verified: real Gmail inbox still loads.
…sions
The entry scanner splits the LSHandlers dump at brace depth 1, so a
handler entry still contains its nested LSHandlerPreferredVersions
dictionary. That block repeats LSHandlerRoleAll one level down with a
value that is not a bundle id, and the role scan took the first non-"-"
match it found.
Older macOS wrote "-" there, which the filter happened to skip. macOS 26
writes the real preferred version, so the scan returns "7559.97" instead
of "com.google.chrome": detect_default_chromium("Darwin") yields None and
real-profile browsing fails closed at consent resolution on a machine
that does have a Chromium default.
Flatten nested dictionaries out of the entry before reading the role, so
only keys at the entry's own level are considered. The test helper only
ever fed the "-" placeholder, which is why this shape was not covered;
a versioned helper now exercises both the Chromium and the non-Chromium
default.
Reported by @kshitijk4poor on NousResearch#95620 (macOS 26, Chrome 152). The regex
came in with NousResearch#95549, so the same defect is present there.
The snapshot reproduces the source layout, so a user whose session lives in "Profile 6" gets a copy whose Default is some other, usually signed-out profile. Chrome opens Default in a user-data-dir unless --profile-directory says otherwise, and agent-browser only emits that flag when --profile is a profile NAME (is_chrome_profile_name, no slash) — the real-profile path hands it a PATH, so the flag is never added. Result: the launch succeeds, cookies are present, and the agent is signed out anyway, because it is reading a profile the user does not use. Read profile.last_used from Local State and mirror that profile's auth files into the copy's Default slot, on both the full-copy and the refresh path. The original directories are left in place. Falls back to Default when Local State is missing, unparsable, or names a directory that is not there. Reported by @kshitijk4poor on NousResearch#95620 (LinkedIn login page instead of the feed, session in a non-Default profile).
_secure_snapshot_root() only ran inside the fresh-copy branch, so the permissions on a credential-bearing directory were asserted exactly once, at creation. _secure_dir is deliberately best-effort — the wrapper swallows and debug-logs — so a first attempt that failed left the copied Cookies / Login Data tree at whatever the umask gave it, permanently and silently. A snapshot dir created by an earlier build was never revisited either. Move the call ahead of the fresh/refresh split so it runs on every consented launch, and secure the browser-profile/ parent as well: makedirs creates it under the process umask, and it enumerates every browser Hermes holds cookies for.
_create_local_session() resolved the real-profile CDP unconditionally, so every local session got the user's cookie jar — including the hybrid sidecar that auto_local_for_private_urls creates. That sidecar exists for one reason: a URL resolved to a LAN/loopback host and must not be handed to the cloud backend. Routing it into the real-profile copy-browser instead means the agent visits an internal host — a router UI, a local dashboard, whatever the URL pointed at — with the user's full authenticated cookie jar attached. That is a strictly larger exposure than the routing rule was protecting against, and it happens without the user asking for a real-profile session on that URL. Pass allow_real_profile=False on the sidecar branch, where the caller already knows it is a sidecar (force_local). Bare local sessions are unchanged: consent still means the real profile there. A broken real-profile setup also no longer breaks private-URL routing, since the sidecar never calls the resolver.
…ally
_real_profile_cdp() launches agent-browser with --profile, and
agent-browser rejects that outright for the Lightpanda engine
("Profiles are not supported with Lightpanda",
cli/src/native/browser.rs:86).
A user with browser.engine: lightpanda and the consent toggle on
therefore gets "the real-profile browser failed to start: <last stderr
line>" — a message that names neither the setting that caused it nor the
fix. The in-place implementation caught this case explicitly; the
copy-based redo does not, so the guard is restored here against the new
entry point.
Checked before detection so a machine with no default browser at all
still reports the engine conflict, which is the actionable one.
alt-glitch
added
type/bug
Copilot stopped reviewing on behalf of
GottZ due to an error
August 26, 2026 15:34
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves “real-profile” and hybrid local browsing behavior by preventing unsafe real-profile reuse for private-URL sidecars, improving macOS default-Chromium detection, and ensuring the real-profile snapshot opens the user’s active Chromium profile.
Changes:
- Add an explicit configuration error when
browser.use_real_profileis enabled with the Lightpanda engine. - Prevent hybrid private-URL sidecar sessions (
::local) from attaching to the user’s real-profile snapshot. - Improve Chromium profile snapshot correctness (mirror active profile into
Default) and harden snapshot directory permissions on every refresh. - Fix macOS LaunchServices parsing to avoid confusing “preferred version” strings with bundle IDs; add tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/browser_tool.py | Adds Lightpanda guard for real-profile, and adds an allow_real_profile switch so ::local sidecars never use real-profile cookies. |
| hermes_cli/browser_connect.py | Fixes macOS handler parsing, determines “active” Chromium profile via Local State, mirrors active auth files into Default, and re-secures snapshot dirs on each launch. |
| tests/tools/test_browser_real_profile.py | Expands coverage for active-profile mirroring, Lightpanda conflict messaging, and sidecar real-profile opt-out. |
| tests/hermes_cli/test_browser_connect_default_chromium.py | Adds coverage for macOS LaunchServices dumps that include version strings in LSHandlerPreferredVersions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+578
to
+589
| for rel in _AUTH_REFRESH_FILES: | ||
| if "<profile>" not in rel: | ||
| continue | ||
| s = os.path.join(src, rel.replace("<profile>", active)) | ||
| if not os.path.isfile(s): | ||
| continue | ||
| d = os.path.join(dst, rel.replace("<profile>", "Default")) | ||
| try: | ||
| os.makedirs(os.path.dirname(d), exist_ok=True) | ||
| shutil.copy2(s, d) | ||
| except OSError as e: | ||
| logger.debug("real-profile: could not mirror %s as Default: %s", rel, e) |
Comment on lines
+368
to
+370
| flat = re.sub(r"\{[^{}]*\}", "", low) | ||
| while re.search(r"\{[^{}]*\}", flat): | ||
| flat = re.sub(r"\{[^{}]*\}", "", flat) |
Comment on lines
+1564
to
+1565
| "browser.use_real_profile needs the Chrome engine: agent-browser " | ||
| "does not support profiles with browser.engine: lightpanda. Set " |
teknium1
force-pushed
the
feat/real-profile-cdp
branch
from
August 26, 2026 15:58
dc5ba5d to
4b734e2
Compare
Contributor
Author
|
Superseded by 4b734e2, which implements all five findings from the review — thanks for picking them up. Closing this stack as redundant. |
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.
Stacked on
feat/real-profile-cdp(#95620), one commit per finding from my review over there. The copy-based design is right — everything here is inside it, nothing re-litigates the approach.Two of these are the E2E bugs @kshitijk4poor reported on #95620 and #95520, verified against the current head
dc5ba5d5rather than the reported symptom.489fcd15_launchservices_https_handler()reads the role out of the nestedLSHandlerPreferredVersionsblock, so it returns"7559.97"instead of"com.google.chrome"and the feature fails at consent resolution. The entry scanner splits at brace depth 1, so the nested dict is still in the entry; the only filter was!= "-", which older macOS happened to satisfy. Flattens nested dicts before reading the role.3d563e01Defaultin a user-data-dir unless--profile-directorysays otherwise, and agent-browser only emits that flag when--profileis a profile name (is_chrome_profile_name) — the real-profile path hands it a path. A user whose session lives inProfile 6gets a signed-out session. Mirrorsprofile.last_usedinto the copy'sDefaultslot on both the copy and refresh paths.c4080acf_secure_snapshot_root()sat inside the fresh-copy branch, and_secure_diris best-effort (swallows + debug-logs), so a first attempt that failed left a credential store loose permanently. Moves it ahead of the fresh/refresh split and secures thebrowser-profile/parent too.bd26e21c_create_local_session()resolved the real-profile CDP unconditionally, so the sidecar thatauto_local_for_private_urlscreates for a LAN/loopback host visited it with the user's full authenticated cookies. That sidecar exists to keep an internal host off the cloud backend; handing it the user's logins is a strictly larger exposure than the routing rule was protecting against. Bare local sessions are unchanged.35297291_real_profile_cdp()launches with--profile, which agent-browser rejects for Lightpanda (Profiles are not supported with Lightpanda,cli/src/native/browser.rs:86). A user withbrowser.engine: lightpandaand consent on gets a generic start failure naming neither the setting nor the fix. The in-place implementation caught this; restored against the new entry point.Tests
New coverage per finding, all against real file I/O where the code does real I/O:
LSHandlerPreferredVersionsshape, for a Chromium and a non-Chromium default (the existing helper only ever fed the"-"placeholder, which is why this shape was uncovered)active_profile_dir()including both fallbacks (unparsableLocal State, alast_usednaming a directory that is not there), the non-Defaultactive profile landing inDefaulton the copy path and staying there across a refresh, andDefaultbeing left alone when it already is the active profile_get_session_info("t1::local"), plus a bare session still opting in, plus a broken real-profile setup no longer breaking private-URL routing124 passedacrosstest_browser_real_profile.py,test_browser_connect_default_chromium.py,test_browser_open_timeout.py,test_browser_extension_router_wiring.py,test_browser_secret_exfil.pyand both footgun suites.ruff checkclean on all touched files.Not addressed here
local_browser=Falsesurvives in two test spies (test_browser_extension_router_wiring.py:42,test_browser_open_timeout.py:100) although neitherbrowser_navigatenor_navigation_session_keycarries that parameter any more. Harmless — an unused default — but it is dead surface from the salvaged commits, and I left it alone rather than touch files this stack has no other reason to open.