Skip to content

fix: guard int()/float() env var casts and unguarded flock(LOCK_UN) - #35871

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

fix: guard int()/float() env var casts and unguarded flock(LOCK_UN)#35871
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/guard-env-var-casts

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Summary

Three categories of defensive-coding fixes across 9 files:

1. int()/float() env var casts without ValueError guard (6 files)

  • plugins/platforms/irc/adapter.py: IRC_PORT
  • plugins/platforms/discord/adapter.py: HERMES_DISCORD_TEXT_BATCH_DELAY_SECONDS, HERMES_DISCORD_TEXT_BATCH_SPLIT_DELAY_SECONDS
  • plugins/platforms/google_chat/adapter.py: GOOGLE_CHAT_MAX_MESSAGES, GOOGLE_CHAT_MAX_BYTES
  • plugins/browser/firecrawl/provider.py: FIRECRAWL_BROWSER_TTL
  • tools/browser_tool.py: BROWSER_INACTIVITY_TIMEOUT
  • tools/checkpoint_manager.py: HERMES_CHECKPOINT_TIMEOUT

A non-numeric env var (e.g. typo) crashes the entire gateway on startup. Each cast is wrapped in try/except (ValueError, TypeError) with the default as fallback.

This follows the established pattern from prior PRs #29304 and #32458 which fixed the same pattern in core files but missed these plugin/tool locations.

2. Unguarded flock(LOCK_UN) in hermes_cli/kanban_db.py

If fcntl.flock() raises OSError (e.g. EBADF), the exception propagates past the try/finally, potentially crashing the caller. Added try/except OSError: pass around the unlock call, matching the pattern in all other flock locations (8 other files already have this guard).

3. logging.basicConfig() in library __init__ (2 files)

  • trajectory_compressor.py
  • mini_swe_runner.py

basicConfig() hijacks the root logger config. If these classes are instantiated before the main app configures logging, it overrides everything. Removed basicConfig(), kept getLogger(__name__). The entry point (cli.py, gateway) is responsible for root logger config.

Testing

  • All 9 modified files pass ast.parse() syntax validation
  • Follows established codebase patterns from prior PRs

Three categories of defensive-coding fixes across 9 files:

1. int()/float() env var casts without ValueError guard (6 files):
   - plugins/platforms/irc/adapter.py: IRC_PORT
   - plugins/platforms/discord/adapter.py: HERMES_DISCORD_TEXT_BATCH_DELAY_SECONDS,
     HERMES_DISCORD_TEXT_BATCH_SPLIT_DELAY_SECONDS
   - plugins/platforms/google_chat/adapter.py: GOOGLE_CHAT_MAX_MESSAGES,
     GOOGLE_CHAT_MAX_BYTES
   - plugins/browser/firecrawl/provider.py: FIRECRAWL_BROWSER_TTL
   - tools/browser_tool.py: BROWSER_INACTIVITY_TIMEOUT
   - tools/checkpoint_manager.py: HERMES_CHECKPOINT_TIMEOUT

   A non-numeric env var (e.g. typo) crashes the entire gateway on startup.
   Wrap each in try/except (ValueError, TypeError) with the default as fallback.
   This follows the established pattern from prior PRs NousResearch#29304 and NousResearch#32458 which
   fixed the same pattern in core files but missed these plugin/tool locations.

2. Unguarded fcntl.flock(LOCK_UN) in hermes_cli/kanban_db.py:
   If flock raises OSError (e.g. EBADF), the exception propagates past the
   try/finally, potentially crashing the caller. Add try/except OSError: pass
   around the unlock call, matching the pattern in all other flock locations.

3. logging.basicConfig() in library __init__ (2 files):
   - trajectory_compressor.py
   - mini_swe_runner.py

   basicConfig() hijacks the root logger config. If these classes are
   instantiated before the main app configures logging, it overrides
   everything. Remove basicConfig(), keep getLogger(__name__).
   The entry point (cli.py, gateway) is responsible for root logger config.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Overlaps with open PRs #35790 (same env var guard pattern for same files) and #35204/#35793 (same flock(LOCK_UN) guard for kanban_db.py). The env var guards are a superset of #35790; the flock fix duplicates #35204. logging.basicConfig removal is new.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the defensive cleanup work. Most of this PR is now superseded on current main, but the kanban release guard remains worth salvaging.

Problems

  • Current main still has an unguarded POSIX unlock at hermes_cli/kanban_db.py:1410; the surrounding finally only closes the handle at :1411-1412, so an OSError from release still propagates.
  • The PR adds no regression coverage. Current bounded-lock tests (tests/hermes_cli/test_kanban_init_lock_bounded.py:75-92) cover acquisition timeout, not an unlock failure.
  • For a cross-platform non-fatal cleanup policy, the Windows release call is also unguarded at hermes_cli/kanban_db.py:1406; the sibling dispatch-lock cleanup already suppresses OSError at :1494-1495.

Suggested changes

  • Salvage only the current _cross_process_init_lock() release handling rather than cherry-picking the stale nine-file diff. The environment fallbacks are already provided by utils.py:410-429 and current affected call sites; trajectory_compressor.py:355 is already free of root logger setup.
  • Add a test that makes release raise OSError and confirms the context exits normally.

Automated hermes-sweeper review.

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

Labels

comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants