refactor(llm): promote decorator chain settings from NearAiConfig to top-level LlmConfig - #1749
Conversation
…top-level LlmConfig
There was a problem hiding this comment.
Code Review
This pull request refactors the LLM configuration by introducing top-level fields for retries, circuit breaker settings, and response caching. It adds environment variable parsing for these new fields with fallbacks to maintain backward compatibility. A review comment correctly identified an inconsistency in an error message regarding the circuit breaker threshold, suggesting it be updated to reflect that non-negative integers are valid.
| .transpose() | ||
| .map_err(|e| ConfigError::InvalidValue { | ||
| key: "LLM_CIRCUIT_BREAKER_THRESHOLD".to_string(), | ||
| message: format!("must be a positive integer: {e}"), |
There was a problem hiding this comment.
The error message states that LLM_CIRCUIT_BREAKER_THRESHOLD must be a positive integer. However, a value of 0 is a valid u32 and represents a valid configuration (tripping the breaker on the first failure). To be more accurate, this message should indicate a "non-negative integer".
| message: format!("must be a positive integer: {e}"), | |
| message: format!("must be a non-negative integer: {e}"), |
zmanian
left a comment
There was a problem hiding this comment.
Review -- REQUEST_CHANGES
The refactoring rationale is sound -- these decorator chain settings apply to all LLM backends, not just NearAI, so LLM_* prefixed env vars make more sense. The backward compatibility approach (LLM_* takes priority, falls back to NEARAI_*) is correct.
Medium
-
No tests for the new env var parsing paths -- 6 new
optional_env("LLM_*")blocks with fallback logic, but zero new tests verifying: LLM_* overrides NEARAI_, fallback works when only NEARAI_ is set, invalid LLM_* values produce correctConfigError::InvalidValue. This is the core behavior the PR introduces. -
Module spec (
src/llm/CLAUDE.md) is stale -- Lines 112, 130, 170-171, 204-206 still referenceNearAiConfigfields andNEARAI_*env vars as the configuration source. The "Provider Chain Construction" diagram still saysNEARAI_CIRCUIT_BREAKER_THRESHOLD. -
NearAiConfig fields not deprecated -- The 6 fields still exist on
NearAiConfigwith no deprecation marker. Future code can accidentally readconfig.nearai.max_retriesinstead ofconfig.max_retries. Consider#[deprecated]or a follow-up tracking issue. -
Failover settings inconsistency --
failover_cooldown_secs,failover_cooldown_threshold,fallback_modelare still read fromconfig.nearai.*. Same rationale applies -- users on non-NearAI backends face the same confusing prefix.
Low
.env.examplenot updated with the newLLM_*vars.response_cache_max_entriesdefault is100infor_testing()but1000everywhere else (pre-existing, but propagated).
Required
- Add at least one config-level test for the LLM_* override / NEARAI_* fallback behavior
- Update
src/llm/CLAUDE.mdto reflect the new env vars
zmanian
left a comment
There was a problem hiding this comment.
Re-Review -- APPROVE
Both requested items addressed in 5d6a74e:
- Tests added: 3 new tests covering LLM_* override, NEARAI_* fallback, and invalid value error. Well-structured with lock_env() guards.
- Module spec updated: src/llm/CLAUDE.md now documents LLM_* env vars for circuit breaker, retry, and response cache settings, including fallback behavior. Provider chain ASCII diagram updated.
Both branches added new tests at the end of the test module: - PR: decorator chain env var tests (LLM_MAX_RETRIES, etc.) - staging: DB > ENV priority tests (builtin_overrides) Kept both sets of tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Re-Review -- APPROVE
The two new commits since my last review (5d6a74e: tests + docs, 5bf240a: merge conflict resolution) address both required items from my previous review:
-
Tests: 3 new tests added --
llm_max_retries_overrides_nearai,nearai_max_retries_used_as_fallback,llm_max_retries_invalid_value_produces_error. These cover the override, fallback, and error paths for the new env var parsing. Properly guarded withlock_env(). -
Module spec updated:
src/llm/CLAUDE.mdnow documentsLLM_*env vars with fallback behavior for circuit breaker, retry, and response cache settings. Provider chain diagram updated. -
Merge commit (5bf240a): Clean conflict resolution keeping both the new decorator chain tests and the staging DB>ENV priority tests.
Minor (non-blocking)
-
src/config/llm.rs:130-- circuit breaker threshold error message still says "must be a positive integer" butu32accepts 0. Should be "non-negative integer" for consistency with the other error messages. (Gemini flagged this too.) -
Medium items from previous review (NearAiConfig deprecation markers, failover settings inconsistency, .env.example,
for_testing()cache entries default of 100 vs 1000 elsewhere) remain open but are not blocking for this PR.
…top-level LlmConfig (nearai#1749) * refactor(llm): promote decorator chain settings from NearAiConfig to top-level LlmConfig * review: add env var override/fallback tests and update module spec --------- Co-authored-by: Firat Sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Retry, circuit breaker, and response cache settings were nested under
NearAiConfigwithNEARAI_*env var prefixes, but are applied to all LLM backends viabuild_provider_chain(). This confused users on non-NearAI backends (Gemini, OpenAI, Bedrock, etc.) who had to setNEARAI_MAX_RETRIESto control retry behavior.LlmConfig:max_retries,circuit_breaker_threshold,circuit_breaker_recovery_secs,response_cache_enabled,response_cache_ttl_secs,response_cache_max_entriesLLM_*env vars resolve these fields, with automatic fallback to existingNEARAI_*/ bare-name vars for backward compatibilitybuild_provider_chain()now reads fromLlmConfigdirectly instead ofconfig.nearaiFixes #1554
Test plan
cargo check(default features)cargo fmtNEARAI_MAX_RETRIESstill works whenLLM_MAX_RETRIESis unset