Skip to content

fix(tools): handle infinite output limits - #104998

Open
ishuowang wants to merge 1 commit into
NousResearch:mainfrom
ishuowang:fix/non-finite-tool-limits
Open

ishuowang wants to merge 1 commit into
NousResearch:mainfrom
ishuowang:fix/non-finite-tool-limits

Conversation

@ishuowang

@ishuowang ishuowang commented Sep 7, 2026 •

Copy link
Copy Markdown

What does this PR do?

Treats positive and negative infinity as malformed integer limits instead of allowing int() to raise OverflowError. This preserves the documented never-raises behavior when PyYAML parses .inf or -.inf in tool_output or hook spill settings.

Related Issue

N/A — found during source audit; no matching issue or PR.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/tool_output_limits.py: fall back to the configured default on OverflowError.
  • tests/tools/test_tool_output_limits.py: cover positive and negative infinity for all three output limits.

How to Test

  1. Parse this configuration with PyYAML and return it from load_config():

    tool_output:
      max_bytes: .inf
  2. Call get_tool_output_limits().

  3. Verify max_bytes is 50000 rather than an OverflowError.

The focused test passes all 17 tests:

scripts/run_tests.sh tests/tools/test_tool_output_limits.py -q

Ruff and the Windows-footgun check pass for both changed files. The full suite was also attempted with .[all,dev]; 26 unrelated integration files still fail because optional providers and machine-local state are not hermetic here, so the full-suite checkbox remains unchecked.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu Linux (x86_64), Python 3.12

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A — configuration coercion is covered by regression tests.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels Sep 7, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary (Non-blocking)

_coerce_int now also catches OverflowError, so inf/-inf config values for tool-output limits fall back to defaults instead of crashing. (int(float("inf")) raises OverflowError, not ValueError — easy to miss; good catch.) Test pins both infinities.

Notes

  • tools/tool_output_limits.py:23 — float("nan") raises ValueError on int() (already covered) and huge-but-finite floats (e.g. 1e400 → inf) now land here too. Coercion is total across non-numeric inputs. Nothing further needed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants