fix(config): quote .env values containing internal whitespace (#66482) - #66625
fix(config): quote .env values containing internal whitespace (#66482)#66625AlexFucuson9 wants to merge 1 commit into
Conversation
_quote_env_value only checked for '#', quotes, and leading/trailing whitespace when deciding whether to quote a value. Internal spaces and tabs were written unquoted, which breaks shell-sourcing of .env (e.g. 'source ~/.hermes/.env') because the shell word-splits on those spaces. Real-world impact: paths with spaces (macOS ~/Library/Application Support/, Windows Program Files/) stored via save_env_value for TERMINAL_SSH_KEY, GOOGLE_CHAT_SERVICE_ACCOUNT_JSON, etc. would silently truncate when shell-sourced. Add internal space and tab to the needs_quoting condition so values with whitespace are always double-quoted. Fixes NousResearch#66482
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved with Comment
PR #66625 — fix(config): quote .env values containing internal whitespace
- Adds whitespace detection (
" "and"\t") to the_quote_env_valuefunction inhermes_cli/config.py. - Fix is well-scoped: 2-line addition, targeted at a specific bug.
- Correctly handles tab characters alongside spaces.
- No security implications; purely config parsing logic.
Suggestions
- Consider a test case for values containing internal whitespace to prevent regression.
Reviewed by Hermes Agent
Duplicate of #66483, the earlier open canonical for quoting .env values with internal whitespace; it also supplies round-trip and idempotence coverage. |
tonydwb
left a comment
There was a problem hiding this comment.
{
"event": "COMMENT",
Code Review Summary
Verdict: Comment (prior COMMENT activity noted)
2-line fix adding space and tab to the quoting-triggering characters in _quote_env_value. Whitespace inside an env value was previously not detected as needing quotes — a bare export KEY=with spaces would be written unquoted and break on re-read. Fix adds or \" \" in value or \"\\t\" in value to the existing quote-detection logic.
Reviewed by Hermes Agent",
"comments": []
}
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the missing quote trigger. The production change targets the live serializer condition in hermes_cli/config.py:7776-7785 and addresses the current-main gap.
Problems
- This PR adds no regression coverage. Existing serializer tests at
tests/hermes_cli/test_config.py:561-608cover hash/quote/backslash values, not internal spaces or tabs.
Suggested changes
- If this branch is selected for salvage, add a real
save_env_valueround-trip test for spaces and tabs. The member-linked canonical duplicate #66483 already carries that coverage alongside the same production behavior.
This is an automated hermes-sweeper review.
| or "'" in value | ||
| or value != value.strip() | ||
| or " " in value | ||
| or " " in value |
There was a problem hiding this comment.
Please add a regression test for this new serialization trigger. Current tests cover hash/quote/backslash cases but not values containing an internal space or tab; a real save_env_value round-trip would protect the reported behavior.
|
Superseded by #67192 for #66482 (salvages @pnascimento75's #66483, which adds a shell-source round-trip regression test). Your fix was correct too — closing as redundant to consolidate. Thanks @AlexFucuson9. |
Summary
Fixes #66482 —
save_env_valuewrites unquoted values containing internal spaces, breaking shell-sourcing of.env.Root cause
_quote_env_valueonly checked for#, quotes, and leading/trailing whitespace. Internal spaces and tabs were written unquoted, causing shell word-splitting when.envis sourced.Fix
Add
" " in valueand"\t" in valueto theneeds_quotingcondition in_quote_env_value.Impact
Fixes shell-sourcing for paths with spaces:
~/Library/Application Support/...Program Files/...TERMINAL_SSH_KEY,GOOGLE_CHAT_SERVICE_ACCOUNT_JSONTesting
py_compilepassesKEY="path with spaces"