Skip to content

fix(nous): ignore non-finite rate limit resets - #51951

Open
lin-hongkuan wants to merge 1 commit into
NousResearch:mainfrom
lin-hongkuan:codex/fix-nous-rate-limit-nonfinite
Open

fix(nous): ignore non-finite rate limit resets#51951
lin-hongkuan wants to merge 1 commit into
NousResearch:mainfrom
lin-hongkuan:codex/fix-nous-rate-limit-nonfinite

Conversation

@lin-hongkuan

Copy link
Copy Markdown
Contributor

Summary

  • reject non-finite Nous rate-limit reset values from headers and error context
  • avoid persisting Infinity breaker state and clean up non-finite state files when read
  • ignore non-finite bucket values when deciding whether a Nous 429 is a genuine account limit

Why

Malformed or non-finite rate-limit headers such as retry-after: inf could be recorded as an infinite cross-session cooldown. The stored Infinity state then made nous_rate_limit_remaining() return inf, and format_remaining() or bucket parsing could raise OverflowError instead of falling back safely.

Tests

  • python -m pytest tests/agent/test_nous_rate_guard.py -q
  • python -m pytest tests/agent/test_nous_rate_guard.py tests/agent/test_rate_limit_tracker.py tests/run_agent/test_nous_429_fallback_reentry.py -q
  • python -m py_compile agent/nous_rate_guard.py
  • python scripts/check-windows-footguns.py --all

@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 provider/nous Nous Research API (OAuth) labels Jun 24, 2026

@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.

Hermes Agent Review

Verdict: Approved — Clean, well-scoped change with appropriate tests.

Reviewed as part of batch review session 2026-06-24d.

Reviewed by Hermes Agent

@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 addressing the malformed-header cooldown path; the current-main premise is real (agent/nous_rate_guard.py:62-64 accepts positive infinity, which reaches persistence at lines 111-114).

Problems

  • agent/nous_rate_guard.py:160 converts persisted reset_at with float(...), but the enclosing handler does not catch ValueError. Valid JSON containing a nonnumeric string will now escape instead of failing closed. Please catch it and clean up/ignore that invalid state; add a regression test.
  • The same header class remains in agent/rate_limit_tracker.py:78-89: _safe_int("inf") raises OverflowError, while _safe_float("inf") can feed _fmt_seconds() at line 148. Please reject non-finite values there too and add parser/display coverage.

Suggested changes

  • Extend the guard-state test matrix with a nonnumeric persisted reset_at.
  • Add non-finite rate-limit-tracker tests for both integer and reset fields.

Automated hermes-sweeper review.

Comment thread agent/nous_rate_guard.py
with open(path, encoding="utf-8") as f:
state = json.load(f)
reset_at = state.get("reset_at", 0)
reset_at = float(state.get("reset_at", 0) or 0)

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.

float() raises ValueError for a valid JSON value such as "reset_at": "bad", but this function's except clause still excludes ValueError. Please fail closed here (and remove or ignore the invalid state) so a corrupted-but-parseable state file cannot escape this guard.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Two PRs touch the Nous rate-limit guard but address distinct failure classes: #18782 makes text I/O explicitly UTF-8 across several modules, while #51951 rejects non-finite rate-limit values that could create an infinite cooldown or formatting failures.

Related pull requests

  • #18782 [closed] related — (+197/-12) — merged via #71078: This broader encoding fix adds explicit UTF-8 reads and writes across Copilot, model metadata, the Nous rate guard, cron, and delivery paths; it remains relevant because its commits were cherry-picked into the class-wide encoding cleanup with authorship preserved.
  • #51951 related — (+118/-9) — keep open pending fixes: The diff correctly rejects non-finite reset and bucket values, removes persisted infinite cooldowns, and guards duration formatting. The contributor keep_open review identifies two remaining gaps: a nonnumeric persisted reset_at can still raise ValueError, and the equivalent non-finite parsing/display paths in agent/rate_limit_tracker.py are not covered.

Suggested consolidation

Merge #51951 after addressing the contributor keep_open review with invalid-string persisted-state cleanup and corresponding rate_limit_tracker.py guards and tests; #18782 is not a duplicate and needs no reopening because its relevant changes were merged through #71078.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 23 kB of PR diffs, 5 kB of issue/PR text, 4 kB of discussion (5 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 provider/nous Nous Research API (OAuth) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

5 participants