fix(model-catalog): parse bool-ish config values - #35469
Pluviobyte wants to merge 2 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (no blocking issues)
Looks Good
- Clean fix: replaces raw
bool()coercion withis_truthy_value()so the string"false"correctly disables the model catalog _parse_ttl_hours()correctly handles the explicit-zero case (float(raw.get(...) or DEFAULT)was broken for0since0 or XyieldsX)- Uses the shared
is_truthy_valuefromutils.py— consistent with the rest of the codebase - Good defensive handling of NaN/Inf via
math.isfinite() - Test coverage for all three branches of
_parse_ttl_hours: string zero preserved, string"false"for enabled, and invalid values falling back to default
Suggestions (non-blocking)
- The
_parse_ttl_hoursfunction could be tested with a case for negative values —float("-5")passesisfiniteand would silently return a negative TTL. In practice_load_catalog_configonly uses it for downstream comparisons so it's not harmful, but a clamp or explicit guard would be more defensive. mathis imported for a singleisfinitecall. Python'sisfiniteis fast, so no issue — just noting it adds one more import to the module's namespace.
Tests
test_boolish_disabled_string_is_respected— covers the core bugtest_ttl_hours_string_and_zero_are_preserved— covers the0edge casetest_invalid_ttl_hours_falls_back_to_default— covers the fallback path- All three tests use
isolated_homefixture +patchonload_config, which is the right pattern
Reviewed by Hermes Agent
| def _parse_ttl_hours(value: Any) -> float: | ||
| """Parse ``model_catalog.ttl_hours`` while preserving explicit zero.""" | ||
| if value is None or value == "": | ||
| return float(DEFAULT_TTL_HOURS) |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): Consider rejecting negative TTL values here. _parse_ttl_hours(-5) returns -5.0 which would produce undefined behavior downstream. A simple max(0.0, ttl) after the isfinite check would handle it.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the review. I addressed the non-blocking TTL suggestion in a follow-up commit:
Validation after the update: |
|
Thanks for the focused config-normalization fix. Current Automated hermes-sweeper review. |
Summary
model_catalog.enabledwith the shared bool-ish config parser so string"false"disables the remote catalog.model_catalog.ttl_hoursexplicitly so numeric strings work, explicit0is preserved, and malformed values fall back safely.Problem
model_catalogconfig used raw Python coercion:enabledusedbool(value), so"false"was treated as enabled.ttl_hoursusedfloat(raw.get("ttl_hours") or DEFAULT_TTL_HOURS), so explicit zero was replaced with the default TTL.This made documented config values unreliable, especially when users or UI layers stored scalar values as strings.
Fix approach
enabledwith the existingis_truthy_value(..., default=True)helper._parse_ttl_hours()to handleNone, empty string, numeric strings, explicit0, non-finite values, and malformed values consistently.24hours when TTL is absent or invalid.enabled: "false",ttl_hours: "0", and invalid TTL fallback.Why this is safe
The change only normalizes config inputs before the existing catalog/cache logic runs. Default behavior remains unchanged for absent config, while explicit user intent is respected.
Test plan
uv run python -m pytest -o addopts='' tests/hermes_cli/test_model_catalog.py -quv run ruff check hermes_cli/model_catalog.py tests/hermes_cli/test_model_catalog.py