Skip to content

fix(config): trust APPS_SCHEMA float type over an int default in get_arg() (#4925) - #4938

Open
chalfontchubby wants to merge 1 commit into
mainfrom
fix/float-arg-truncation-4925
Open

chalfontchubby wants to merge 1 commit into
mainfrom
fix/float-arg-truncation-4925

Conversation

@chalfontchubby

Copy link
Copy Markdown
Collaborator

Fixes #4925.

Problem

get_arg() coerces its return value on the type of the default it was handed, not on the key's declared APPS_SCHEMA type, and applies that coercion to whatever value was resolved — real configured value or not. So get_arg("solcast_poll_hours", 8) ran a genuinely configured 4.8 through int(float(value)) and returned 4, shortening the Solcast poll TTL from 4.8h to 4h and pushing a two-site hobbyist account past its 10 poll/day quota into nightly HTTP 429s. Nothing warned: validate_config() checks the raw apps.yaml value against the float schema and passes it, so apps.yaml still reads 4.8 while the runtime behaves as 4.

Why not the issue's suggested fix

The issue proposes normalising int defaults inside Components.initialize(). That only covers the component-framework path. Auditing every float-declared APPS_SCHEMA key against its get_arg call sites found three more truncating reads that fix would miss — octopus_saving_session_rate and octopus_saving_session_min_octopoints_per_kwh are read directly from octopus.py, not through a component arg spec — plus a fifth affected component arg (alphaess_api_delay) the issue's table didn't list.

Fix

Fixed at the source in get_arg() (userinterface.py): promote an int default to float whenever APPS_SCHEMA declares the key float, before the coercion at the end of the function ever sees it. One normalisation point, mirroring where #4441 fixed the equivalent CONFIG_ITEMS-route bug in get_ha_config(), so no call site — present or future — can change a float-declared key's runtime type by writing 8 instead of 8.0.

Not included: the issue's second ask (a warning on lossy coercion). With the root cause fixed at the source, a remaining truncation means the schema genuinely declares the key an integer, so the warning would fire on intended behaviour every ~5 minutes on a routine sensor read.

Testing

  • test_get_arg_float_schema_int_default_not_truncated — the reported 4.8 → 288min case, an axle_pence_per_kwh money case, a direct (non-component) call site, unset-fallback typing, and negative controls for integer-declared and boolean keys.
  • test_component_arg_specs_resolve_float_declared_keys_as_float — sweeps every COMPONENT_LIST arg spec whose APPS_SCHEMA type is float, resolving each the way Components.initialize() does (currently 9 keys), so it stays correct for specs added later rather than asserting on today's literals.
  • Both verified to fail against unpatched userinterface.py and pass with the fix.
  • ./run_all --quick green, ./run_pre_commit green.

Written by Claude on behalf of @chalfontchubby.

🤖 Generated with Claude Code

…arg() (#4925)

get_arg() coerces its return value on the *type* of the default it was
handed, not on the key's declared APPS_SCHEMA type, and applies that
coercion to whatever value was resolved - real configured value or not.
So get_arg("solcast_poll_hours", 8) ran a genuinely configured 4.8
through int(float(value)) and returned 4, shortening the Solcast poll
TTL from 4.8h to 4h and pushing a two-site hobbyist account past its
10 poll/day quota into nightly HTTP 429s. Nothing warned: validate_config()
checks the raw apps.yaml value against the float schema and passes it, so
apps.yaml still reads 4.8 while the runtime behaves as 4.

The issue's own suggested fix (normalise int defaults inside
Components.initialize()) only covers the component-framework path.
Auditing every float-declared APPS_SCHEMA key against its get_arg call
sites found three more truncating reads that fix would miss -
octopus_saving_session_rate and octopus_saving_session_min_octopoints_per_kwh
are read directly from octopus.py, not through a component arg spec - plus
a fifth affected component arg (alphaess_api_delay) the issue's table
didn't list.

Fixed at the source in get_arg() instead: promote an int default to float
whenever APPS_SCHEMA declares the key float, before the coercion at the
end of the function ever sees it. One normalisation point, mirroring
where #4441 fixed the equivalent CONFIG_ITEMS-route bug in
get_ha_config(), and no call site - present or future - can change a
float-declared key's runtime type by writing 8 instead of 8.0.

Test suite: a direct regression test for the reported 4.8 -> 288min case
plus the money and non-component-path cases, and a sweep test that walks
every COMPONENT_LIST arg spec whose APPS_SCHEMA type is float and
confirms it resolves a fractional value intact - resolving each spec the
way Components.initialize() does, so it stays correct for specs added
later rather than asserting on today's literals.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@springfall2008

Copy link
Copy Markdown
Owner

Not sure about this, shouldn't we fix the caller for get_arg()?

@chalfontchubby

Copy link
Copy Markdown
Collaborator Author

(Posted by Rik; written by Claude.)

Fair challenge, and normally I would agree — fix the caller, not the accessor. Two things pushed me the other way here.

"The caller" is not one place. Auditing main: 15 float-declared APPS_SCHEMA keys, affected in two unrelated mechanisms.

Where Keys
components.py arg specs solcast_poll_hours, forecast_solar_max_age, open_meteo_forecast_max_age
octopus.py direct get_arg() calls octopus_saving_session_rate (x2), octopus_saving_session_min_octopoints_per_kwh

The original issue proposed normalising in Components.initialize(), which fixes the first three and cannot reach the octopus.py ones — those never go through the component framework. So the caller-side fix is six edits across two mechanisms rather than one.

More to the point, it only holds while every future caller keeps getting it right. The trap resets every time someone writes 8 instead of 8.0 against a float-declared key, and it fails silently in the worst way: validate_config() reads the raw apps.yaml value against the float schema and passes it, so apps.yaml still says 4.8 while the runtime behaves as 4. Nothing warns, and the symptom surfaces somewhere unrelated — in #4925 as nightly Solcast HTTP 429s. A correctness property that depends on every call site remembering an invisible convention will drift back.

The framing that convinced me: the type is already declared authoritatively in APPS_SCHEMA, and get_arg() currently prefers an incidental literal over it. That reads as the accessor being wrong, not the callers.

There is also direct precedent — #4441 fixed this exact mechanism on the CONFIG_ITEMS route inside get_ha_config(), normalising an int default to float at the source for the same reason. This is the same bug on the apps.yaml/APPS_SCHEMA route, which get_ha_config()s normalisation cannot reach because there is no config_index entry to match. Fixing it in get_arg() keeps both routes consistent.

Two honest costs, so they are on the record rather than buried:

  • it adds an APPS_SCHEMA lookup to a hot accessor (dict .get on a resolved-once global, but it is not free); and
  • it makes get_arg()s return type depend on that global rather than purely on its arguments.

If you would rather keep the accessor dumb, the alternative I would want is all six call sites fixed plus a test that fails when a new int default appears against a float-declared key — the sweep in test_component_arg_specs_resolve_float_declared_keys_as_float already does roughly that for the component specs and could be widened. Without the guard I think it regresses quietly. Happy to switch it round if you prefer that shape — just say which.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

solcast_poll_hours 4.8 is silently truncated to 4h, exhausting the Solcast daily quota

2 participants