Skip to content

fix: use _env_float() helper for env var timeout parsing in chat_completion_helpers.py - #45511

Closed
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/unsafe-env-float-chat-completion
Closed

fix: use _env_float() helper for env var timeout parsing in chat_completion_helpers.py#45511
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/unsafe-env-float-chat-completion

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Bug

Three call sites in chat_completion_helpers.py use raw float(os.getenv(...)) instead of the existing _env_float() helper (line 118). If a user sets any of these env vars to a non-numeric value, the streaming API call crashes with unhandled ValueError.

Changes

Line Env Var Purpose
1764 HERMES_API_TIMEOUT Base timeout for chat completions
1771 HERMES_STREAM_READ_TIMEOUT httpx read timeout
2511 HERMES_STREAM_STALE_TIMEOUT Stale stream detector

All 3 replaced with _env_float("VAR", default) which already exists at line 118.

Test Plan

  • py_compile passes
  • HERMES_API_TIMEOUT=abc falls back to 1800.0 instead of crashing

Three call sites in chat_completion_helpers.py used raw float(os.getenv(...))
instead of the existing _env_float() helper (line 118). If a user sets
HERMES_API_TIMEOUT, HERMES_STREAM_READ_TIMEOUT, or HERMES_STREAM_STALE_TIMEOUT
to a non-numeric value, the streaming API call crashes with ValueError.

_env_float() already handles this gracefully with try/except (TypeError, ValueError).

Lines fixed:
- 1764: HERMES_API_TIMEOUT (base timeout for chat completions)
- 1771: HERMES_STREAM_READ_TIMEOUT (httpx read timeout)
- 2511: HERMES_STREAM_STALE_TIMEOUT (stale stream detector timeout)
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists labels Jun 13, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #40938 — routing the three chat_completion_helpers.py timeout sites through _env_float() is exactly what open PR #40938 already does (also covered by #30274/#35790).

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Uses _env_float() helper instead of direct float() call on env var in chat_completion_helpers.py. Trivial 3-line fix — same pattern applied consistently across the codebase.


Reviewed by Hermes Agent (cron batch)

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the malformed timeout-environment handling.

Automated hermes-sweeper review found that the requested behavior is already implemented on current main:

  • agent/chat_completion_helpers.py:2171 and :2178 use shared utils.env_float() for HERMES_API_TIMEOUT and HERMES_STREAM_READ_TIMEOUT.
  • agent/chat_completion_helpers.py:2998 uses the same helper for HERMES_STREAM_STALE_TIMEOUT.
  • utils.py:421-429 returns the supplied default for empty, non-numeric, or otherwise invalid values.
  • The implementation is present in d682f320b35a13084371a541a835e1d988c982b8, contained in v2026.7.1.

The member discussion identifying overlap with #40938 is consistent with this canonical implementation. Closing as implemented on main.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 14, 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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants