fix: guard float() env var casts, operator precedence, atomic writes, yaml fallback - #29304
Conversation
…writes
- Add try/except (ValueError, TypeError) guards for float(os.getenv())
across 6 locations in run_agent.py, auth.py, runtime_provider.py, and
chat_completion_helpers.py. A typo in env vars like HERMES_API_TIMEOUT
or HERMES_NOUS_TIMEOUT_SECONDS previously crashed the entire agent.
- Add _safe_float_env() helper to chat_completion_helpers.py and
runtime_provider.py for clean inline usage.
- Fix operator precedence bug in cli.py:7453 — 'or' + [:50] slice where
only the fallback was truncated, not the preview as intended.
- Replace non-atomic write_text() with atomic_json_write() in
checkpoint_manager.py and environments/base.py to prevent corruption
on crash/power-loss.
- Add 'or {}' fallback to yaml.safe_load() in profile_distribution.py
to prevent None return on empty manifest files.
alt-glitch
added
type/bug
|
Partial overlap with merged #17905 (yaml.safe_load + float() env var guards in gateway) and open #17967 (float() guards in platform adapters). The float() and yaml.safe_load fixes here target different files (run_agent.py, auth.py, runtime_provider.py, chat_completion_helpers.py) so not a strict duplicate, but the pattern is the same. The operator precedence fix (cli.py:7453) and atomic write fixes are net-new. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying several real defensive-coding issues. Current main has already absorbed part of the env-cast work through a7dd98c86, but this branch needs a focused salvage rather than a direct application.
Problems
a7dd98c86already uses sharedenv_floatfor the original agent/auth paths, while current unguarded cases are nowrun_agent.py:1259andhermes_cli/runtime_provider.py:1441,1765.- The preview expression moved to
hermes_cli/cli_commands_mixin.py:1043and remains inhermes_cli/commands.py:1930andgateway/slash_commands.py:2099; thecli.pyhunk no longer reaches the active handler. tools/checkpoint_manager.py:524remains a non-atomic metadata write in_touch_project, so changing only_register_projectleaves the same file vulnerable on normal snapshot updates.hermes_cli/profile_distribution.py:268already appliesdata or {}at the public manifest boundary, making the_load_yamlhunk redundant.
Suggested changes
- Rework the remaining current paths, preserve scoped provider reads, and add regressions for malformed timeout values, all personality preview surfaces, and both metadata writers.
Automated hermes-sweeper review.
| @@ -7451,7 +7451,7 @@ def _handle_personality_command(self, cmd: str): | |||
| print(f" {'none':<12} - (no personality overlay)") | |||
| for name, prompt in self.personalities.items(): | |||
| if isinstance(prompt, dict): | |||
There was a problem hiding this comment.
The active handler has since moved to hermes_cli/cli_commands_mixin.py:1043; please carry this parenthesization to that current path as well as the matching autocomplete and gateway preview paths (hermes_cli/commands.py:1930, gateway/slash_commands.py:2099).
| @@ -466,7 +467,7 @@ def _register_project(store: Path, working_dir: str) -> None: | |||
| pass | |||
There was a problem hiding this comment.
_touch_project writes the same project metadata file non-atomically (tools/checkpoint_manager.py:524 on current main) and is invoked on snapshot creation. Please convert that sibling write too, otherwise normal updates retain the corruption window.
| creds = resolve_nous_runtime_credentials( | ||
| min_key_ttl_seconds=max(60, int(os.getenv("HERMES_NOUS_MIN_KEY_TTL_SECONDS", "1800"))), | ||
| timeout_seconds=float(os.getenv("HERMES_NOUS_TIMEOUT_SECONDS", "15")), | ||
| timeout_seconds=_safe_float_env("HERMES_NOUS_TIMEOUT_SECONDS", 15.0), |
There was a problem hiding this comment.
On current main the Nous timeout resolution has two active float(_getenv(...)) paths (hermes_cli/runtime_provider.py:1441 and :1765). A salvage should cover both and retain the scoped _getenv source.
| return yaml.safe_load(text) | ||
| return yaml.safe_load(text) or {} | ||
|
|
||
|
|
There was a problem hiding this comment.
read_manifest() already calls DistributionManifest.from_dict(data or {}) on current main (hermes_cli/profile_distribution.py:268), so this internal fallback has no observable effect and can be omitted from a focused salvage.
teknium1
added
sweeper:risk-compatibility
4 participants
Summary
Fixes 4 classes of bugs found via static analysis:
1. float() on env vars without try/except (6 locations)
A typo in env vars like HERMES_API_TIMEOUT, HERMES_NOUS_TIMEOUT_SECONDS previously crashed the entire agent with ValueError.
Fix: Add try/except guards with fallback to defaults. Added _safe_float_env() helper.
Files: run_agent.py, hermes_cli/auth.py, hermes_cli/runtime_provider.py, agent/chat_completion_helpers.py
2. Operator precedence bug (cli.py:7453)
preview = prompt.get("description") or prompt.get("system_prompt", "")[:50]- slice binds tighter than or, only fallback was truncated.Fix: Parenthesize:
(description or system_prompt)[:50]3. Non-atomic write_text() (2 locations)
Replaced with atomic_json_write() from utils.py.
Files: tools/checkpoint_manager.py, tools/environments/base.py
4. yaml.safe_load() without fallback
Empty manifest files return None. Added
or {}.Test Plan