Skip to content

fix: guard float(os.getenv()) against non-numeric env var values - #30274

Open
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/float-env-guard
Open

fix: guard float(os.getenv()) against non-numeric env var values#30274
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/float-env-guard

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Problem

float(os.getenv('VAR', '1800')) raises ValueError when the env var is set to a non-numeric string like "abc". The or default pattern doesn't help because non-empty strings are truthy — float("abc" or 1800) still raises.

This affects 5 call sites in the core request path (hit on every API call):

File Env Var Impact
run_agent.py HERMES_API_TIMEOUT Every request timeout resolution
run_agent.py HERMES_API_CALL_STALE_TIMEOUT Stale detector base timeout
chat_completion_helpers.py HERMES_API_TIMEOUT Streaming request timeout
chat_completion_helpers.py HERMES_STREAM_READ_TIMEOUT Stream read timeout
chat_completion_helpers.py HERMES_STREAM_STALE_TIMEOUT Stream stale detector

Fix

Add _safe_float_env(var_name, default) helper that wraps float() in try/except (ValueError, TypeError) with a warning log. Applied to all 5 unguarded call sites in the core request path.

Additional unguarded sites exist in gateway platforms (discord, wecom, telegram) and CLI auth — those only affect startup and can be addressed in a follow-up.

Tests

  • py_compile passes on both files
  • Helper returns default when env var is None
  • Helper returns default when env var is non-numeric
  • Helper returns parsed value when env var is valid numeric string

float(os.getenv('VAR', '1800')) raises ValueError when the env var
is set to a non-numeric string like 'abc'. The 'or default' pattern
doesn't help because non-empty strings are truthy.

Add _safe_float_env() helper to run_agent.py and
agent/chat_completion_helpers.py that wraps float() in try/except
(ValueError, TypeError) with a warning log.

Fixed sites (core request path — hit on every API call):
- run_agent.py: HERMES_API_TIMEOUT, HERMES_API_CALL_STALE_TIMEOUT
- chat_completion_helpers.py: HERMES_API_TIMEOUT, HERMES_STREAM_READ_TIMEOUT,
  HERMES_STREAM_STALE_TIMEOUT
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels May 22, 2026

@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 identifying the malformed environment-value failure mode. The remaining non-stream stale-timeout path is still live on current main, but most of this PR has since been covered by the shared helper.

Problems

  • run_agent.py:1257-1259 still directly calls float(env_timeout), but the other four proposed sites already use canonical env_float after a7dd98c8609c0d944e3c5dd0c5b9ee31dd99eb29 (run_agent.py:1233; agent/chat_completion_helpers.py:2171, 2178, 2998).
  • The PR's stale fallback is 300.0, whereas current main's documented and tested default is 90.0 (run_agent.py:1242-1246, 1274; tests/hermes_cli/test_timeouts.py:267-268).
  • tests/hermes_cli/test_timeouts.py:228-268 lacks a malformed-value case.

Suggested changes

  • Salvage the one live branch with env_float("HERMES_API_CALL_STALE_TIMEOUT", 90.0) and retain its explicit-value flag.
  • Add a malformed-env regression case asserting (90.0, False).

Automated hermes-sweeper review.

Comment thread run_agent.py
@@ -897,7 +909,7 @@ def _resolved_api_call_stale_timeout_base(self) -> tuple[float, bool]:

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.

When salvaging this branch onto current main, use the canonical env_float helper and preserve the current 90-second default. Current main documents and tests 90.0 for the implicit stale timeout (run_agent.py:1242-1246, tests/hermes_cli/test_timeouts.py:267-268), so this PR's 300.0 fallback would regress that contract.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants