fix(browser): guard module-level int() against non-numeric env var - #35789
Closed
annguyenNous wants to merge 1 commit into
Closed
fix(browser): guard module-level int() against non-numeric env var#35789annguyenNous wants to merge 1 commit into
annguyenNous wants to merge 1 commit into
Conversation
BROWSER_SESSION_INACTIVITY_TIMEOUT was cast via int() at module level without a try/except. Setting BROWSER_INACTIVITY_TIMEOUT to a non-numeric string (e.g. "abc") caused an unhandled ValueError on import, breaking all browser tool functionality. Wrap in try/except (ValueError, TypeError) with fallback to the default of 300 seconds.
Collaborator
tonydwb
approved these changes
May 31, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
🔍 What this does
Wraps the module-level int(os.environ.get(...)) call in a try/except (ValueError, TypeError) so a non-numeric BROWSER_INACTIVITY_TIMEOUT env var doesn't crash the entire browser_tool module at import time.
✅ Looks Good
- Correctness: Falls back to the documented default (300s) on any parsing failure — matches the existing
_env_float()defensive pattern inchat_completion_helpers.py. - Edge cases: Handles both
ValueError(non-numeric string like"abc") andTypeError(e.g.Nonefromos.environ.getwith no default — though the default is provided here, theexcept TypeErroris a good belt-and-suspenders guard). - Impact: Prevents a silent full-outage of browser tooling due to misconfiguration.
💡 Suggestion (non-blocking)
Consider extracting this pattern into a reusable helper like _env_int(name, default) to match _env_float() and apply consistently across the codebase. Not needed in this PR — can be a follow-up refactor.
Reviewed by Hermes Agent (cron)
Contributor
|
Thanks for identifying this import-time failure mode. This is an automated hermes-sweeper review; the requested guarantee is already present on current
Closing as implemented on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: Guard module-level
int()against non-numeric env varFile:
tools/browser_tool.py, line 1181Bug:
BROWSER_SESSION_INACTIVITY_TIMEOUTis cast viaint()at module level without any error handling. If a user setsBROWSER_INACTIVITY_TIMEOUTto a non-numeric string (e.g."abc"or an empty string), the entirebrowser_toolmodule fails to import with an unhandledValueError, breaking all browser functionality.Fix: Wrap the module-level
int()call intry/except (ValueError, TypeError)with a fallback to the default value of 300 seconds. This matches the defensive pattern used elsewhere in the codebase (e.g._env_float()helper inchat_completion_helpers.py).Impact: High — any misconfigured env var currently causes a complete browser tool outage with no recovery path.