Skip to content

fix(config): preserve request_timeout_seconds and stale_timeout_seconds in custom_providers normalization - #28869

Open
flamiinngo wants to merge 1 commit into
NousResearch:mainfrom
flamiinngo:fix/custom-provider-timeout-normalization
Open

flamiinngo wants to merge 1 commit into
NousResearch:mainfrom
flamiinngo:fix/custom-provider-timeout-normalization

Conversation

@flamiinngo

@flamiinngo flamiinngo commented May 19, 2026 •

Copy link
Copy Markdown

Bug

request_timeout_seconds and stale_timeout_seconds are accepted custom-provider keys, but current main silently drops them from the runtime compatibility view. The v11 to v12 migration passes through the same normalizer and transfer helper, so it also deletes both values before removing custom_providers.

Fix

  • Preserve positive numeric timeout values in _normalize_custom_provider_entry().
  • Carry both fields into the migrated providers.<name> entry.
  • Register both fields in _VALID_CUSTOM_PROVIDER_FIELDS so config validation agrees with runtime behavior.
  • Extend the existing migration contract and add one schema/normalization invariant test.

Verification

  • Exact current-main probe was red before the fix and now preserves both values through normalization and migration.
  • scripts/run_tests.sh tests/hermes_cli/test_config.py -k TestCustomProviderCompatibility: 4 passed.
  • scripts/run_tests.sh tests/hermes_cli/test_runtime_provider_resolution.py -k timeout_fields: 1 passed.
  • Full runtime-provider file passed: 105 tests.
  • Ruff passed on all four changed files.

A broader two-file Windows run reached 278 passing tests; the only two failures were existing TestEnvWriteDenylist cases that expect lowercase POSIX environment names to remain case-sensitive on Windows, unrelated to this diff.

@flamiinngo
flamiinngo force-pushed the fix/custom-provider-timeout-normalization branch from 2404870 to b815be9 Compare May 19, 2026 18:15
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels May 19, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix. The core premise still holds on current main: hermes_cli/config.py:3803 accepts both timeout keys as known, but hermes_cli/config.py:3891-3895 only copies rate_limit_delay before moving on to discover_models.

Problems

  • The PR fixes the runtime normalizer, but the legacy migration path still drops the same fields. hermes_cli/config.py:4474 migrates v11 custom_providers through _custom_provider_entry_to_provider_config, and hermes_cli/config.py:3921-3930 copies context_length/rate_limit_delay/etc. but not request_timeout_seconds or stale_timeout_seconds before hermes_cli/config.py:4491 removes custom_providers.
  • The schema metadata set at hermes_cli/config.py:4128-4134 still omits both timeout keys even though hermes_cli/config.py:3803 treats them as supported custom provider fields.

Suggested changes

  • Add both timeout fields to _custom_provider_entry_to_provider_config's copied field tuple and extend the migration coverage in tests/hermes_cli/test_config.py:750-814.
  • Add both fields to _VALID_CUSTOM_PROVIDER_FIELDS, with a small invariant test near tests/hermes_cli/test_runtime_provider_resolution.py:2279.

Automated hermes-sweeper review.

Comment thread hermes_cli/config.py Outdated
if isinstance(rate_limit_delay, (int, float)) and rate_limit_delay >= 0:
normalized["rate_limit_delay"] = rate_limit_delay

request_timeout_seconds = entry.get("request_timeout_seconds")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This preserves the runtime compatibility view, but current main also migrates legacy custom_providers through _custom_provider_entry_to_provider_config; please copy request_timeout_seconds and stale_timeout_seconds there too, otherwise v11->v12 migration still discards them before removing custom_providers.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused normalization fix. The premise is verified on current main: hermes_cli/config.py:4707-4710 accepts both keys, while hermes_cli/config.py:4825-4829 copies only rate_limit_delay before returning the normalized entry.

Problems

  • The v11→v12 migration still drops both fields. hermes_cli/config.py:5592-5609 migrates entries through _custom_provider_entry_to_provider_config() and deletes custom_providers, but that helper's transfer tuple at hermes_cli/config.py:4872-4886 omits request_timeout_seconds and stale_timeout_seconds.
  • The added tests cover only direct normalization. The migration coverage in tests/hermes_cli/test_config.py:1063-1169 needs a legacy entry with both timeouts and assertions against the resulting providers entry.

Suggested changes

  • Add both timeout fields to the transfer tuple at hermes_cli/config.py:4872-4884.
  • Add a v11→v12 migration regression test preserving both fields.

Automated hermes-sweeper review.

@flamiinngo
flamiinngo force-pushed the fix/custom-provider-timeout-normalization branch from b815be9 to 1069610 Compare October 5, 2026 08:55
@flamiinngo

Copy link
Copy Markdown
Author

Implemented the requested changes on current main:

  • Both timeout fields now survive runtime normalization.
  • Both are preserved during the v11→v12 migration.
  • Both are registered as valid custom-provider fields.
  • Added focused normalization and migration regression coverage.

The branch is now conflict free and mergeable. Relevant tests and Ruff pass. Thanks for the review.

@flamiinngo

Copy link
Copy Markdown
Author

Implemented the requested follow-up on current main:

  • Both timeout fields now survive runtime normalization.
  • Both are preserved by the v11 to v12 migration before custom_providers is removed.
  • Both are registered in _VALID_CUSTOM_PROVIDER_FIELDS.
  • The existing migration contract and a focused schema/normalization invariant cover the complete path.

The branch is conflict-free and GitHub reports it as mergeable. The custom-provider compatibility tests and runtime-provider regression pass, and Ruff is clean.

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

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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