Skip to content

fix(cli): quote .env values with internal whitespace in save_env_value - #66483

Closed
pnascimento9596 wants to merge 1 commit into
NousResearch:mainfrom
pnascimento9596:fix/quote-env-value-spaces
Closed

fix(cli): quote .env values with internal whitespace in save_env_value#66483
pnascimento9596 wants to merge 1 commit into
NousResearch:mainfrom
pnascimento9596:fix/quote-env-value-spaces

Conversation

@pnascimento9596

@pnascimento9596 pnascimento9596 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What / why

Fixes #66482.

_quote_env_value (used by every save_env_value write) quoted only for # / " / ' / leading-trailing whitespace. Internal spaces (e.g. macOS Application Support paths for TERMINAL_SSH_KEY or GOOGLE_CHAT_SERVICE_ACCOUNT_JSON) were left unquoted:

TERMINAL_SSH_KEY=/Users/…/Application Support/…

python-dotenv still reads that correctly; POSIX set -a; . file word-splits:

.env: line 1: Support/…: No such file or directory

One-condition fix: extend needs_quoting with any(c.isspace() for c in value). Escaping dialect unchanged (\\ then \" then wrap — same dialect #57249 / sweeper already blessed for the installer writer).

Does not touch install.sh / install.ps1 (#57249's surface). This is the core serializer sibling of accepted #57247.

Residual write paths

This PR fixes the core serializer (_quote_env_value via save_env_value) that all standard config/setup flows route through. The following sites still write raw KEY=value lines outside _quote_env_value and are enumerated for follow-up (verified still present on current main):

  1. hermes_cli/memory_setup.py_write_env_vars
  2. plugins/memory/mem0/_setup.py_write_env
  3. plugins/memory/hindsight/__init__.py — setup path that rewrites Hermes .env lines in place
  4. plugins/memory/openviking/__init__.py_write_env_vars

Those provider/setup writers are intentionally out of scope here.

CI (at push time)

On head d90bfdfdb (run 29608254881): 23 SUCCESS / 9 SKIPPED / 0 FAILED. Numbers are the rollup at push time; re-check the checks tab for the current tip if the branch has moved.

Churn analysis (Gate 4)

No mass-requote of existing user .env files. save_env_value rewrites the file but only re-serializes the key being written; other lines pass through _sanitize_env_lines with their original text. A legacy unquoted spaced line stays unquoted until that key is saved again (one-time correction on re-save). Simple unquoted values stay unquoted (no rewrite churn). No caller depends on spaced values staying unquoted — shell source was already broken for them; dotenv load is unchanged for both quoted and unquoted forms.

Tests

tests/hermes_cli/test_config.py (TestSaveEnvValueSecure):

  • spaced path → quoted emit; dotenv + shell set -a; . file round-trip
  • tab in value + shell round-trip
  • pure-input double-save idempotent
  • read-back (dotenv_values) → re-save idempotent (setup path)
  • simple values stay unquoted; untouched lines not re-quoted
  • unrelated save does not requote legacy spaced lines
  • already-quoted raw input: stable + load_env dialect round-trip
  • \n/\r stripped before quote (single-line writer boundary)
  • existing hash / quote / backslash cases unchanged

Counts: TestSaveEnvValueSecure 16 passed; full test_config.py 163 passed. Footguns on changed files: 0.

One logical change

Production: single extra disjunct in needs_quoting. Tests: coverage + one pre-existing footgun encoding= in the same test file.

Merge-tree vs #57249

Report in PR comment / sync after both dual-order checks (installer branch only touches install scripts; expected clean either order).

Type of Change

  • 🐛 Bug fix
  • ✅ Tests

_quote_env_value previously left internal spaces unquoted (only #/"/'
and leading/trailing whitespace triggered). Spaced macOS paths written
via hermes setup SSH / Google Chat SA path / hermes config set produced
lines that python-dotenv still parsed but shell `set -a; . file` word-split.

Extend needs_quoting with any(c.isspace()); escaping dialect unchanged.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the core serializer path. The premise is confirmed on current main: hermes_cli/config.py:7776-7785 does not quote internal whitespace, and standard setup paths call save_env_value for SSH and Google Chat filesystem paths (hermes_cli/setup.py:1416; plugins/platforms/google_chat/adapter.py:3484).

Problems

  • The new shell round-trip tests invoke env and sh unconditionally (tests/hermes_cli/test_config.py:634 and :668 in this PR). Native Windows does not guarantee those executables. The existing POSIX file-mode tests in this class already guard Windows at current-main tests/hermes_cli/test_config.py:534 and :548.

Suggested changes

  • Gate the shell-source checks/tests on non-Windows platforms; keep the serializer, dotenv_values, and load_env() assertions cross-platform.

Automated hermes-sweeper review.

assert load_env()["TERMINAL_SSH_KEY"] == path

# Shell source must round-trip (this is what the bug broke).
r = subprocess.run(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please gate this shell-source assertion (and the analogous tab test) on non-Windows platforms. Native Windows does not guarantee env or sh; retain the serializer/dotenv assertions cross-platform and make only the POSIX shell round-trip conditional.

@OutThisLife

Copy link
Copy Markdown
Collaborator

Salvaged into #67192 (P2 config batch) with authorship preserved, rebased on current main, validated in a pristine checkout. Your any(c.isspace()) fix + the shell-source round-trip test were the strongest of the three — thanks @pnascimento75.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 2026
@pnascimento9596

Copy link
Copy Markdown
Contributor Author

Thanks @OutThisLife, happy the whitespace fix and round-trip test held up. #67192 was a tidy way to land the batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

save_env_value writes unquoted values with internal spaces - breaks shell-sourcing of .env (core-writer sibling of #57247)

4 participants