fix(save_env_value): quote values with internal spaces to prevent shell-sourcing breakage (#66482) - #66583
Conversation
Duplicate of #66483, which already applies the same save_env_value whitespace-quoting repair with broader regression coverage. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Critical
- None
Assessment
Adds a created_by != "agent" guard to the background skill curator. Prevents the autonomous LLM consolidation pass from archiving manually authored skills (e.g. installed via URL or direct SKILL.md authoring). Correctly skips skills without the created_by: "agent" marker.
Looks Good
- Clean guard using
skill_usage._is_curator_managed_record - Error message is clear and actionable
- No impact on agent-created skills
Reviewed by Hermes Agent
…ll-sourcing breakage (NousResearch#66482) (NousResearch#66583)
3fa5ef5 to
24236b6
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused serializer fix. The current predicate still omits internal whitespace (hermes_cli/config.py:7776-7781), while save_env_value writes that helper's result on both update and append paths (hermes_cli/config.py:7827-7841), so the premise holds on current main.
Problems
- This PR changes the serialization contract without adding regression coverage. The adjacent tests cover hash/quote/backslash cases (
tests/hermes_cli/test_config.py:561-608) but not an internally spaced value.
Suggested changes
- Add a focused test that writes an internal-whitespace value, checks it is emitted as a double-quoted
.envassignment, and confirmsload_env()reads the original value back.
Automated hermes-sweeper review.
| or '"' in value | ||
| or "'" in value | ||
| or value != value.strip() | ||
| or any(c.isspace() for c in value) |
There was a problem hiding this comment.
Please add a regression test for an internal-space or tab value. Nearby coverage verifies hash/quote escaping, but not this new quoting trigger.
|
Superseded by #67192 for #66482 (salvages @pnascimento75's #66483 with a round-trip regression test). Closing as redundant. Thanks @webtecnica. |
Summary
One-line fix: extend the
needs_quotingtrigger in_quote_env_value()to also fire when any character in the value is whitespace (any(c.isspace() for c in value)).Root cause
needs_quotingchecked for#, quotes, and leading/trailing whitespace — but not internal whitespace. A path like/Users/paulo/Library/Application Support/hermes/keys/id_ed25519was written unquoted, which breaksset -a; . .env(POSIX word-splitting).Changes
hermes_cli/config.pyline 7781: addedor any(c.isspace() for c in value)to theneeds_quotingconditionreplace('\\', ...)/f'...'handles the quoting correctly)Closes #66482