fix(compare): skip malformed compare_list entries instead of crashing, with one shared check

[chalfontchubby]

Fixes #5472

Summary

Compare.run_all() indexed tariff["id"] directly, so a compare_list entry in apps.yaml with no id: line raised KeyError: 'id'. That crash also skipped the compare_active reset in update_pred(), so the comparison, and the crash, re-triggered every 5-minute cycle until the config was fixed.

Review showed the missing id was one of several malformed entries that crash or misbehave in the same way. The editor's own validator in web.py already knew most of the rules, but nothing else used it. This PR puts the rules in one place:

• compare.py: the new module-level check_compare_list() returns the entries that can be compared, plus a message for each entry it skips. Each kept entry is a copy whose id and name are stripped strings and whose config is either a mapping or absent. An entry is skipped when:

  • it isn't a dictionary;
  • it has no id, or only whitespace for one;
  • its id duplicates an earlier entry's (7 and "7" count as the same, and so do agile and " agile ");
  • it has no name;
  • its config: is neither a mapping nor empty (an empty config: (None, "", [] or {}) still means no overrides).

  A compare_list that isn't a list at all gives one problem and no entries. run_all(), publish_only() and load_yaml() all read the list through valid_compare_list(), which logs each skipped entry as Warn: Compare, compare_list entry N ..., skipping it.

• Numeric ids and names: an unquoted id: 2024 or name: 2024 is read by YAML as a number. That number then crashed publish_data() when it built the sensor name, every cycle via publish_only(). Because save_yaml() runs first, the number was also saved to comparisons.yaml and crashed Compare.__init__ on every restart after that. Ids and names now become strings on the way in, and load_yaml()/publish_data() convert anything already saved.

• predbat.py: setting and clearing compare_active around run_all() now lives in a small run_comparison() helper. The flag is cleared in a finally, so a crash in the comparison from any cause can't leave the switch on and re-trigger itself every cycle.

• web.py:

  • The apps.yaml editor's _validate_compare_list() now calls check_compare_list(), so it refuses exactly what a comparison would skip, and it lists every problem at once.
  • The Compare page (html_compare) reads the same checked list, so a numeric id finds its result and a malformed entry no longer crashes the page.
  • The page now shows a "Skipped:" line for each entry it can't use, rather than dropping the entry silently or saying no tariffs are configured.

Behaviour changes for hand-edited configs

These apply only to an apps.yaml that already has one of these mistakes:

• Duplicate ids: the first entry now wins and later ones are skipped with a warning. Before, every entry ran and the last one overwrote the result. The editor already refused duplicates.
• A non-empty config: that isn't a mapping (e.g. config: "best_soc_min: 2.0") now skips that tariff with a warning. Before, it either crashed the comparison (a list, false, 0) or ran with no overrides, logging one "not found" warning per character (a string).
• The editor validates the whole compare_list on every save, as it did before. Because the rules are now stricter, an existing entry with one of these mistakes, a blank id or name, or a 7/"7" clash will block unrelated saves until it is fixed. The error names each entry by its position.

Testing

In apps/predbat/tests/test_compare.py:

• T27 runs valid_compare_list() and check_compare_list() over every malformed shape listed above. It checks the warning for each skipped entry, by position and by reason. It also checks:
  • that ids and names come back as stripped strings;
  • that the user's own entries are copied, not modified;
  • that id: 0 is accepted;
  • which config: values are dropped, which are kept as overrides, and which are skipped.
• T28 runs run_all() against the real fixture, with run_single(), save_yaml() and the other expensive steps stubbed. The compare_list mixes valid and malformed entries, including a list config: that reaches run_all()'s override snapshot. The test checks that only the valid tariffs are compared and saved, keyed by string id, and that each skipped entry is logged.
• T29 makes run_all() raise inside run_comparison(). It checks that the exception propagates, and that compare_active is set and then cleared, and afterwards it puts the shared fixture back as it was.
• T30 / T31 check that load_yaml() turns a numeric key saved in comparisons.yaml into a string, and that publish_data() publishes a numeric name saved there.

In test_web_functions.py, the Compare page:

• shows a numeric id's result;
• survives malformed entries and explains why each was skipped;
• doesn't show "not configured" or "Loading chart" when every entry is malformed.

In test_web_apps_edit.py, the editor refuses a blank id, a numeric/quoted id clash and a duplicate id, and names every problem at once.

Each new test was checked against the code with its fix reverted, or a plausible wrong version substituted, and fails as expected. Examples:

• the old tariff["id"] loop fails T28 with KeyError('id');
• removing the finally leaves compare_active on in T29;
• skipping the string conversion makes T30 and T31 raise TypeError;
• a check that skips or drops every config: fails T27.

./run_pre_commit (all hooks pass) and ./run_all --quick (all tests pass) were run on every commit. Before pushing, each round was reviewed locally with /code-review high plus an independent reviewer, until neither found a significant issue.

Notes / not in this PR

• impact():

  • update_pred comes back HIGH, because it is the main loop. The only change to it moves the compare_active block into run_comparison().
  • _validate_compare_list and html_compare are LOW, with one caller each.

• An exception from run_all() still propagates out of update_pred(). That is unchanged from before, so the rest of that cycle's housekeeping is skipped and the run is recorded as an error. Catching it inside run_comparison() or per tariff would contain it, but that changes how failures are reported, so it is left for a separate decision.

• Not changed here:

  • ids are not checked to be valid Home Assistant entity names (spaces, capitals, 1.5);
  • html_compare still puts the id and name into the page unescaped;
  • results for ids no longer in compare_list are still republished;
  • tariff_catalogue.py (the annual page's dropdowns) keeps its own compare_list rules.

  All of these were here before this PR.

• Related, already tracked separately: Log warnings of invalid compare items #5133/PR fix(compare): publish a valid entity id per tariff and stop republishing removed tariffs (#5133) #5143 (stale or invalid entity-id publishing) and PR fix(compare): stop the midnight tariff comparison changing the live plan's next decision #5465 (the midnight comparison overwriting the live plan's plan_preclip).

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The key skip and exception-reset paths need behavioral regression tests rather than source inspection or no coverage.

2 open findings

• Test compare_active clears when run_all raises · New
• Add behavioral test verifying run_all skips id-less entries · New

What changed in this PR

Prevents malformed tariff comparisons from crashing repeatedly and ensures the activation flag resets after failures.

Changes:

• Skips comparison entries without an ID and logs a warning.
• Resets compare_active using finally.
• Adds helper and source-inspection tests.

File	Description
apps/​predbat/​compare.py	Validates tariff IDs before comparison.
apps/​predbat/​predbat.py	Reliably resets the comparison trigger.
apps/​predbat/​tests/​test_compare.py	Tests tariff-ID handling and wiring.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.