fix(install): quote AGENT_BROWSER_EXECUTABLE_PATH for spaced paths - #57249
fix(install): quote AGENT_BROWSER_EXECUTABLE_PATH for spaced paths#57249pnascimento9596 wants to merge 2 commits into
Conversation
|
Adding the rationale for the double-quote form, since it wasn't spelled out in Hermes reads
The double-quote form also matches Hermes' own Flagging for #57449 as well (same fix, single-quote helper) — happy to fold |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing all four installer writes and keeping the Snap cleanup aligned.
Problems
scripts/install.sh:2109andscripts/install.ps1:347,352insert the raw path between double quotes. Unlike the existing.envserializer (hermes_cli/config.py:7622), they do not escape embedded"or\\; a valid POSIX executable path containing"yields malformed sourced.envsyntax.tests/test_install_sh_browser_install.py:337suppresses source stderr with2>&1, and the tests do not assert the returnedrcorstderr. This does not verify the stated no-stderr contract.- The new harness reads and regex-extracts
install.sh(tests/test_install_sh_browser_install.py:307), contrary to the source-reading-test prohibition inAGENTS.md:1358-1411.
Suggested changes
- Add proper double-quote/backslash escaping and an embedded-double-quote round-trip case.
- Preserve stderr and assert
rc == 0plusstderr == "". - Move the serialization behind a callable helper or test the supported installer boundary rather than extracting source text.
Automated hermes-sweeper review.
|
The earlier comment said the double-quote form "matches Reproduced with python-dotenv 1.2.2 before this fix:
The underlying constraint is that python-dotenv escape-decodes double-quoted values, while The honest limitation remains: paths containing All three review points are addressed: both installers now escape backslashes and double quotes; source stderr is preserved and the tests assert return code and stderr; and the source-extraction harness is gone—the tests drive the real writer through the supported CI on |
ee3b81c to
3fcfa6d
Compare
|
Note for reviewers: the Jul 26 head move (ee3b81c4a to 3fcfa6dc6) was a rebase onto current main pushed by a local sync tool. The fix commits are patch-identical (range-diff clean), so the three review threads above show as outdated purely because of the rebase, not because of new changes. All three were addressed before the rebase and remain addressed. |
…aths source cleanly configure_browser_env_from_system_browser writes the browser override as AGENT_BROWSER_EXECUTABLE_PATH=$browser_path with no surrounding quotes. Post-NousResearch#50852 this path is opt-in only: it runs when the user has exported AGENT_BROWSER_EXECUTABLE_PATH explicitly. When that override contains a space (macOS /Applications/Google Chrome.app/Contents/MacOS/Google Chrome, Windows C:\Program Files\...), the written line is invalid under POSIX shell source: the shell parses the first word after the space as a command, emits "no such file or directory", and leaves the var empty. install.ps1 has the same bug at both writer sites. The Hermes runtime is unaffected (it loads .env via python-dotenv, which parses the unquoted value), but anyone who sources ~/.hermes/.env from a shell — which override users are especially likely to do — gets a stderr error and a silently-empty var. Fix: wrap the value in double quotes at all four writer sites (install.sh + install.ps1). Generalize the strip_snap_browser_override regexes from '^AGENT_BROWSER_EXECUTABLE_PATH=/snap/' to '="?/snap/' so the Snap-stripper still fires on the new quoted form — without this the migration silently no-ops on a quoted snap path, a regression the naive one-line fix would introduce. Update the test assertion pinning the old regex and add behavioral tests that write a spaced path and re-source the .env under both bash and POSIX sh.
Mirror the canonical _quote_env_value escaping in both installers and replace source-extraction tests with supported-stage behavior coverage. Accept hand-written single- or double-quoted Snap overrides defensively. The writer itself cannot emit a Snap path because find_system_browser rejects /snap/*.
3fcfa6d to
697baf1
Compare
Fixes #57247
Problem
See #57247. The installer persists the user's explicit browser override into
~/.hermes/.envunquoted. Spaced paths (macOS app bundles, Windows Program Files) produce a line that fails POSIX shellsource, leaving the var empty with a stderr error.Opt-in only: post-#50852 the installer no longer auto-detects a browser. Default installs are unaffected.
Why naive quoting is wrong
Quoting the value is exactly what makes
python-dotenvescape-decode it, so the value must be escaped. Naive double-quoting corrupts real Windows paths under python-dotenv 1.2.2:"..."(no escapes)dotenv_valuesC:\Users\p\scoop\apps\chrome.exeC:\Users\p\scoop+ BEL +pps\chrome.exe(\a→ BEL)D:\bin\firefox.exeD:+ BS +in+ FF +irefox.exe(\b/\fcontrol chars)E:\tools\tbrowser.exeE:+ TAB +ools+ TAB +browser.exe(\t→ tab)Core constraint
python-dotenvescape-decodes double-quoted values whileshexpands$/backtick, and dotenv leaves\$literal, so no double-quoted form is safe for both readers.Escaped double quotes are correct for Hermes because:
python-dotenvis the reader Hermes actually uses (hermes_cli/env_loader.py)_quote_env_valuealready writes (hermes_cli/config.py)Single quotes are shell-perfect but silently drop apostrophe paths under
python-dotenv.Changes
scripts/install.sh:244-dotenv_double_quotehelper next tojson_escape(backslash +"escape, then wrap in")scripts/install.sh:2118- writer uses the helper instead of rawechoscripts/install.sh:1888,1892- snap strip regex generalized to['\"]?/snap/(tolerates',", or unquoted)scripts/install.sh:1861-find_system_browseralready rejects/snap/*, so quoted-snap stripping is defensive migration onlyscripts/install.ps1:340-ConvertTo-DotEnvDoubleQuotedusing ordinal.Replace(not-replace; regex would mangle backslashes)scripts/install.ps1:354,359- bothWrite-BrowserEnvwriter sites use the helpertests/test_install_sh_browser_install.py- behavioral coverage via supported--stage config/--stage node-depsboundaries (no source reading; per AGENTS.md)Testing
Boundary-driven tests (no installer source extraction):
On this head (
ee3b81c4a): 14 collected - 12 passed, 2 xfailed (strict) documenting the$/backtick shell limitation.Coverage:
shround-trips (spaces, Windows backslash paths, apostrophe, embedded")$/ backtick pathsxfails: POSIXshcannot round-trip$/ backtick under any dotenv-compatible double-quoted form--stage node-deps--stage configrc == 0and empty stderr asserted on installer andshsourceAlso verified:
ee3b81c4a: 23 SUCCESS / 7 SKIPPED / 1 NEUTRALLimitation
$/ backtick paths remain shell-source hostile in any dotenv-compatible double-quoted form. Documented by strictxfailtests; dotenv still preserves the literal path.Platforms tested
ConvertTo-DotEnvDoubleQuotedexecuted with dotenv round-tripUpdate (2026-08-04): rebased onto upstream/main
(36cb5ae) following the triage disposition
on #57247 recommending this PR as the fix. Installer
fix content is unchanged (per-file patch-id verified
on scripts/install.sh and scripts/install.ps1). The
test file was merged with the current upstream suite:
all upstream tests retained, this PR's round-trip
suite added, and 4 earlier interim tests dropped
as superseded (mapping in the table below). All
checks green at 697baf1. Ready for review.
test_quoted_spaced_browser_path_round_trips_under_bashtest_config_stage_browser_path_round_trips_through_dotenv_and_shtest_quoted_spaced_browser_path_round_trips_under_shtest_config_stage_browser_path_round_trips_through_dotenv_and_shtest_quoted_browser_env_write_is_double_quotedtest_config_stage_browser_path_round_trips_through_dotenv_and_sh(via_expected_serialized_line)test_install_script_strips_stale_snap_browser_overridetest_node_deps_stage_strips_quoted_snap_override_run_configure_and_source(helper)_run_config_stage+_source_with_posix_sh