Follow-ups from #441: secret-helper migration, ten more bug fixes, and coverage for every remaining untested module - #444
Merged
Conversation
…ager secret strip/merge array-aware
api_v3.py carried three inline nested copies of find_secret_fields/
separate_secrets (main-config save, plugin-config save, plugin-config
reset). They drifted from each other (one lacked isinstance guards) and
none supported the canonical module's array-item secrets
(accounts[].token). All three endpoints now import from
src/web_interface/secret_helpers.
Adopting the canonical behavior makes array-item secrets reachable, and
their parallel-placeholder shape ([{'token': ...}, {}] alongside the
regular list) was not survivable by ConfigManager's round-trip:
_strip_secrets_recursive dropped the whole key (losing the regular
fields from config.json) and _deep_merge replaced the regular list
wholesale on load. Both are now array-aware:
- strip removes the secret fields from each item and ALWAYS keeps the
list so indices survive for merge-on-load; whole-key secrets (scalar
lists, shape mismatches) still drop the key entirely — never leak.
- merge folds each secrets item into the config item at the same index,
skipping {} placeholders. The regular list's length is authoritative
in both directions: a user deleting an array item never has it
resurrected from a stale secrets entry (extras warn and are ignored).
api_v3's own deep_merge intentionally still replaces lists wholesale —
form posts carry complete arrays and index-merging would resurrect
deleted items; a comment now documents that.
Tests: the parity guard flips from 'exactly 3 inline copies' to 'zero,
and the canonical import must exist'; TestArraySecretStripAndMerge
covers the new strip/merge semantics incl. length-mismatch contracts;
new test_api_v3_secret_roundtrip.py drives all three endpoints through
a Flask client with a REAL ConfigManager+SchemaManager over tmp_path,
proving secrets land in config_secrets.json, config.json stays clean,
and a fresh load merges them back into the right array items.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
… resolver, repos, config, validator Nine fixes for bugs surfaced while writing coverage for previously untested modules (plus the bool-duration quirk pinned in PR #441): - base_plugin.get_display_duration: exclude bools from both numeric branches — display_duration=True no longer reads as a 1-second slot; it falls through to config, then the 15.0 default. - display_helper: draw_error_message/draw_no_data_message called _draw_centered_text with the wrong arguments and crashed with AttributeError — both now delegate to draw_centered_text. draw_scorebug_layout drew status and clock at the same y, overprinting each other — they now share one combined top line. draw_ticker_layout drew its text starting at x=display_width (fully off-canvas), returning a blank frame every time — now draws at x=0; scroll_speed stays accepted-but-unused and is documented as such. - api_helper.clear_cache guarded on a nonexistent CacheManager.clear() method, silently never clearing anything; it now uses the real surface (clear_cache/delete/list_cache_files) and no-ops safely otherwise. - base_odds_manager._extract_espn_data raised AttributeError when ESPN sent explicit JSON nulls ("homeTeamOdds": null) — every level now null-safes with 'or {}'. format_odds_summary gated on is_odds_available, which deliberately ignores money lines, so ML-only odds formatted as "No odds available" — it now gates only on empty/no_odds data and formats money lines. - logging_config.ContextualFormatter mutated record.msg in place, so a second handler prepended the context prefix twice; it now formats a copy. log_error hardcoded exc_info=True and raised TypeError when the caller passed exc_info — now kwargs.setdefault. - dynamic_team_resolver wrote its "shared" class cache through self, creating instance shadows — the cache was per-instance and every scoreboard refetched rankings. Writes now go through the class. - saved_repositories cleaned URLs with an unanchored .replace('.git','') that mangled URLs merely containing '.git' (my.github.io -> myhub.io); now strips only a trailing suffix. add/remove also roll back the in-memory list when the save fails, so memory always matches disk. - config_helper.merge_configs shallow-copied the base, aliasing every un-overridden nested dict into the result — now deep-copies. - startup_validator.validate_all accumulated errors/warnings across calls — now resets both lists per run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Nine new suites plus an extension, asserting the Phase-1b fixed behavior
and pinning the quirks deliberately left alone:
- test_logging_config.py: formatters (JSON shape, no record mutation,
single prefix through two handlers), PluginLoggerAdapter precedence,
setup_logging handler hygiene and LEDMATRIX_DEBUG, log_error exc_info.
- test_startup_validator.py: exact messages, error-vs-warning split,
accessor split (load_config vs get_config), cache-dir branches with
os.access monkeypatched (root can write anything in CI), idempotence,
raise_on_errors classification precedence.
- test_config_helper.py (full): load/save round trips, dot-notation
get/set incl. silent-failure contract, post-fix no-aliasing merge,
schema validation branches, the '{id}_config' key pin, default-enabled
pin.
- test_saved_repositories.py: three load shapes, bare-list rewrite pin,
trailing-only .git strip (my.github.io regression), save-failure
rollback, type-classification case-sensitivity pin.
- test_api_helper.py: rate-limit math, cache-hit short circuit, ESPN
URL/key formats, exact User-Agent guard, retry adapter, post-fix
clear_cache against the real CacheManager surface, ttl-dropped pin.
- test_base_odds_manager.py: cache-key/URL construction, no_odds
sentinel round trip, stale-cache fallback, null-safe extraction,
ML-only formatting, is_odds_available truth table (ML-blind by
contract), config key/attr mismatch pin.
- test_dynamic_team_resolver.py: expansion/dedup/slicing, dropped
unknown-dynamic names (TOP_ substring hazard pinned), genuinely
shared class cache (second instance: zero HTTP), TTL expiry,
failure degradation without raising.
- test_display_helper.py (full): the fixed error/no-data renders,
combined scorebug top line, non-blank ticker with scroll_speed
no-op pin, composite upconversion, logo bleed positions, square
orientation pin.
- test_skin_runtime_cache.py: discovery-cache hit/invalidation
semantics (manifest mtime, .py edits pinned as non-invalidating),
sys.modules namespacing contract incl. bare-name restore and stdlib
shadowing, entry-module execute-once, API minor-version tolerance,
skin_matches_target table.
- test_sports_capabilities.py (extended): _draw_celebration_layout
executed for real (flash window, matrix-dims fallback, highlight
alternation, logo-failure isolation), _should_celebrate_for direct,
strict duration boundary, score_to_int edges, both-teams-score
precedence, expired-coalesce refire, disabled-win baseline
preservation, id-less prune.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
…us schedule tests New test_display_controller_schedule.py drives _check_schedule and _check_dim_schedule on a bare controller stub: same-day and midnight-crossing windows with inclusive boundaries, global vs per-day vs legacy-inferred modes (and dim's global-only default — no legacy inference), per-day disabled days, invalid %H:%M fallbacks, unknown timezone -> UTC, dim_brightness default 30, inactive-display short circuit, and the _was_display_active/_was_dimmed transition flags. test_display_controller.py's test_schedule_disabled and test_active_hours patched config_service.get_config — which _check_schedule never reads — so both asserted the init-default value and could not fail. Rewritten on the test_inactive_hours pattern (inject controller.config['schedule'], reset the minute gate, flip the flag to the opposite state first so the assertion has teeth). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Measured 50% with the new suites in place (was 47% baseline when the gate was introduced at 45); floor stays two points under measured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR expands regression coverage and corrects behavior for secret arrays, cache clearing, configuration merging, rendering, odds extraction, plugin state, repository persistence, logging, validation, schedules, skin loading, and sports celebrations. ChangesBehavior corrections and regression coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 35 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/blueprints/api_v3.py (1)
5349-5367: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winOrphaned array-item secrets stay in
config_secrets.jsonafter the user removes items.
separate_secretsomits theaccountskey from the secrets output when no item carries a secret value, for example after the user clears the array.deep_mergethen leaves the previous parallel list incurrent_secrets[plugin_id].ConfigManager._deep_mergelater ignores the extra entries and logs a warning, so no item is resurrected, but the secret values remain on disk indefinitely.Consider deleting the stored secrets entry for a key when the newly separated config contains that key with no secrets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web_interface/blueprints/api_v3.py` around lines 5349 - 5367, Update the secret merge flow around separate_secrets and current_secrets so keys present in the newly separated configuration but containing no secrets remove the corresponding stored entry before or instead of deep_merge. Ensure cleared array-item secrets, such as accounts, are deleted from current_secrets[plugin_id] while preserving unrelated secret keys and existing merge behavior for keys that still contain values.
🧹 Nitpick comments (5)
test/test_skin_runtime_cache.py (1)
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the implicit
Optionalannotations flagged by Ruff (RUF013).
body,extra_files,entry_point,manifest_id, andmanifest_extraare annotated as non-optional but default toNone. Ruff reports RUF013 on lines 55-59. Use explicit optional types.🔧 Proposed fix
- body: str = None, - extra_files: dict = None, - entry_point: str = None, - manifest_id: str = None, - manifest_extra: dict = None, + body: Optional[str] = None, + extra_files: Optional[dict] = None, + entry_point: Optional[str] = None, + manifest_id: Optional[str] = None, + manifest_extra: Optional[dict] = None,Add the import:
from typing import Optional🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_skin_runtime_cache.py` around lines 52 - 60, Update the make_skin parameters body, extra_files, entry_point, manifest_id, and manifest_extra to use explicit Optional annotations, adding the typing.Optional import if needed, while preserving their existing None defaults and behavior.Source: Linters/SAST tools
test/test_sports_capabilities.py (1)
1021-1034: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEncode the expected value for
"-4"in the table instead of a branch in the test body.The table declares
expectedasNonefor"-4", but the body overrides it and asserts-4. The declared value is dead data and contradicts the assertion. The trailing comment on line 1026 also leaves an open question in the test source.♻️ Proposed cleanup
`@pytest.mark.parametrize`("value,expected", [ ({"value": None}, None), # int(float(None)) TypeError -> caught ({"value": "abc"}, None), ({"other": 1}, 0), # neither key -> default 0 ([3], None), # list -> TypeError -> caught - ("-4", None), # regex fallback finds digits -> 4? No: + ("-4", -4), # int(float("-4")) parses directly ]) def test_score_to_int_edges(self, value, expected): - result = CelebrationMixin._score_to_int(value) - if value == "-4": - # int(float("-4")) parses directly: -4. - assert result == -4 - else: - assert result == expected + assert CelebrationMixin._score_to_int(value) == expected🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_sports_capabilities.py` around lines 1021 - 1034, Update test_score_to_int_edges so the "-4" parameter row declares expected as -4, remove the special if value == "-4" branch, and assert result against expected uniformly. Clean up the trailing comment on that row so the table no longer contains contradictory or unresolved information.test/test_display_controller_schedule.py (1)
56-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
clockfixture and the unusedcheck_atparameter.No test requests the
clockfixture.check_atalso acceptsclock=Noneand never uses it. Both helpers start and stop their own patcher throughat(). Dead scaffolding in a new file invites incorrect use later.♻️ Proposed cleanup
-@pytest.fixture -def clock(): - patchers = [] - - def _at(time_str, day="monday"): - patchers.append(p := at(time_str, day)) - return p - - yield _at - for p in patchers: - p.stop() - - -def check_at(dc, time_str, day="monday", clock=None): +def check_at(dc, time_str, day="monday"): """Run _check_schedule at a mocked wall time, resetting the minute gate."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_display_controller_schedule.py` around lines 56 - 77, Remove the unused clock fixture and delete the unused clock parameter from check_at, keeping check_at’s existing at() patching and schedule-reset behavior unchanged.test/test_logging_config.py (1)
132-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the capture handlers after each test.
_captureattaches a handler to a named logger and changes its level, but never restores either. The conftestreset_loggingfixture restores the root logger only, so these handlers stay attached totest.adapter*andtest.lwc*for the rest of the session.TestLogWithContext._captureat Line 231 has the same problem.Use a fixture or
try/finallyto calllogger.removeHandler(handler)and restore the previous level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_logging_config.py` around lines 132 - 138, Update both _capture helpers, including TestLogWithContext._capture, to clean up the attached handler and restore the logger’s previous level after each test. Use a fixture or try/finally so cleanup runs even when assertions fail, and preserve each logger’s original configuration.test/web_interface/test_api_v3_secret_roundtrip.py (1)
84-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
api_v3blueprint attributes after each test.The fixture assigns manager attributes directly on the shared
api_v3blueprint object and never restores them. The objects persist aftertmp_pathis removed. Other test modules that rely on the blueprint state, or that run after this module, can then observe a realConfigManagerpointing at a deleted directory. The failure depends on test order, which makes it hard to diagnose.Convert the fixture to a
yieldfixture and restore the previous values.♻️ Proposed teardown
+ _saved = {name: getattr(api_v3, name, None) for name in ( + "config_manager", "schema_manager", "plugin_manager", + "plugin_store_manager", "saved_repositories_manager", + "operation_queue", "plugin_state_manager", "operation_history", + "cache_manager")} api_v3.config_manager = config_manager @@ e.fresh_load = fresh_load - return e + yield e + for name, value in _saved.items(): + setattr(api_v3, name, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/web_interface/test_api_v3_secret_roundtrip.py` around lines 84 - 122, Convert the fixture that configures the shared api_v3 blueprint into a yield-based fixture, saving each blueprint manager attribute’s original value before assignment and restoring all of them after yield. Ensure teardown restores the complete set of attributes modified in the fixture, including config_manager, schema_manager, plugin_manager, plugin_store_manager, saved_repositories_manager, operation_queue, plugin_state_manager, operation_history, and cache_manager.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/config_helper.py`:
- Around line 164-169: Update merge_configs in
src/common/config_helper.py:164-169 to deep-copy value in the non-recursive
override branch, preserving full independence from override_config. Add a
regression test in test/test_config_helper.py:126-140 that mutates an overridden
nested list or dictionary and verifies override_config remains unchanged.
In `@src/config_manager.py`:
- Around line 296-312: Update _is_parallel_secrets_list and its use in
_strip_secrets_recursive to recognize parallel lists only when each secret item
is a placeholder (known-empty dicts or dictionaries whose values are empty
placeholders such as {"id": ""}); do not classify nested secret-field data as
parallel. For non-placeholder lists, preserve the whole-key secret behavior so
the key is dropped and cannot be merged back by _deep_merge.
In `@src/plugin_system/base_plugin.py`:
- Around line 408-410: Update the numeric validation in validate_config() to
exclude bool values alongside the existing int/float type check, matching
get_display_duration() behavior. Ensure boolean durations are rejected before
display_duration is read while valid numeric durations remain accepted.
In `@src/plugin_system/saved_repositories.py`:
- Around line 110-114: Make repository persistence atomic in
saved_repositories.py: at lines 110-114, retain the prior repositories list
until _save_repositories() completes successfully, and at lines 134-137, retain
the removed entry until the save succeeds; update _save_repositories() to write
and fsync a temporary file in the target directory, then atomically replace
config_path, ensuring failed writes cannot truncate or partially overwrite the
existing file.
---
Outside diff comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 5349-5367: Update the secret merge flow around separate_secrets
and current_secrets so keys present in the newly separated configuration but
containing no secrets remove the corresponding stored entry before or instead of
deep_merge. Ensure cleared array-item secrets, such as accounts, are deleted
from current_secrets[plugin_id] while preserving unrelated secret keys and
existing merge behavior for keys that still contain values.
---
Nitpick comments:
In `@test/test_display_controller_schedule.py`:
- Around line 56-77: Remove the unused clock fixture and delete the unused clock
parameter from check_at, keeping check_at’s existing at() patching and
schedule-reset behavior unchanged.
In `@test/test_logging_config.py`:
- Around line 132-138: Update both _capture helpers, including
TestLogWithContext._capture, to clean up the attached handler and restore the
logger’s previous level after each test. Use a fixture or try/finally so cleanup
runs even when assertions fail, and preserve each logger’s original
configuration.
In `@test/test_skin_runtime_cache.py`:
- Around line 52-60: Update the make_skin parameters body, extra_files,
entry_point, manifest_id, and manifest_extra to use explicit Optional
annotations, adding the typing.Optional import if needed, while preserving their
existing None defaults and behavior.
In `@test/test_sports_capabilities.py`:
- Around line 1021-1034: Update test_score_to_int_edges so the "-4" parameter
row declares expected as -4, remove the special if value == "-4" branch, and
assert result against expected uniformly. Clean up the trailing comment on that
row so the table no longer contains contradictory or unresolved information.
In `@test/web_interface/test_api_v3_secret_roundtrip.py`:
- Around line 84-122: Convert the fixture that configures the shared api_v3
blueprint into a yield-based fixture, saving each blueprint manager attribute’s
original value before assignment and restoring all of them after yield. Ensure
teardown restores the complete set of attributes modified in the fixture,
including config_manager, schema_manager, plugin_manager, plugin_store_manager,
saved_repositories_manager, operation_queue, plugin_state_manager,
operation_history, and cache_manager.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef111d06-130e-447d-834a-bcce50fbc893
📒 Files selected for processing (28)
.github/workflows/test.ymlsrc/base_odds_manager.pysrc/common/api_helper.pysrc/common/config_helper.pysrc/common/display_helper.pysrc/config_manager.pysrc/dynamic_team_resolver.pysrc/logging_config.pysrc/plugin_system/base_plugin.pysrc/plugin_system/saved_repositories.pysrc/startup_validator.pytest/test_api_helper.pytest/test_base_odds_manager.pytest/test_base_plugin_duration.pytest/test_config_helper.pytest/test_config_manager_secrets.pytest/test_display_controller.pytest/test_display_controller_schedule.pytest/test_display_helper.pytest/test_dynamic_team_resolver.pytest/test_logging_config.pytest/test_saved_repositories.pytest/test_skin_runtime_cache.pytest/test_sports_capabilities.pytest/test_startup_validator.pytest/web_interface/test_api_v3_secret_roundtrip.pytest/web_interface/test_secret_separation_parity.pyweb_interface/blueprints/api_v3.py
- config_manager: the "secrets list longer than config list" warning now
interpolates only config-side data (no key name or secrets-derived
values), resolving the CodeQL clear-text-logging alert.
- base_plugin: validate_config rejects bool display_duration, matching
get_display_duration (bool is an int subclass and would otherwise pass
as a positive number).
- config_helper: merge_configs deep-copies override values in the
non-recursive branch so mutating the merged result cannot reach back
into override_config.
- saved_repositories: saves are atomic (temp file + fsync + os.replace),
so a failed write can no longer truncate saved_repositories.json.
- tests: regression cases for each fix, plus a pin that whole-item
array secrets (key[] + key[].field both marked) strip to empty {}
skeletons — no secret values can reach config.json.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Follow-up to #441, delivering its deferred work list — and fixing what that work uncovered. Writing coverage for the ten remaining untested modules surfaced ten more live bugs (two crashing methods, an always-blank ticker render, a silent no-op cache clear, an
AttributeErroron ESPN nulls, a double-prefixing log formatter, a broken "shared" rankings cache, a URL-mangling cleaner, a config-merge aliasing leak, and a non-idempotent validator). All are fixed here, with tests asserting the corrected behavior. The PR also completes the api_v3 secret-logic migration onto the canonicalsecret_helpersmodule — which required makingConfigManager's secret strip/merge round-trip array-aware, since the canonical array-item secret format was not survivable by the old code — plus theget_display_durationbool fix, real schedule/dim coverage forDisplayController, and a coverage ratchet to 48%.Type of change
Related issues
Follow-up work list from #441. Bugs fixed (each was pinned by a new test before the fix, then the test flipped):
find_secret_fields/separate_secretsnow import fromsrc/web_interface/secret_helpers;ConfigManager._strip_secrets_recursive/_deep_mergeare array-aware soaccounts[].token-style secrets round-trip (strip keeps list indices; merge realigns by index; the regular list's length is authoritative in both directions, so deleted items are never resurrected).get_display_durationno longer readsTrueas a 1-second slot (bool excluded from both numeric branches).display_helper:draw_error_message/draw_no_data_messagecrashed withAttributeError; the scorebug overprinted status and clock at the same y;draw_ticker_layoutdrew fully off-canvas (every frame blank).api_helper.clear_cacheguarded on a methodCacheManagerdoesn't have — it never cleared anything; now uses the real surface.base_odds_manager: explicit ESPN nulls ("homeTeamOdds": null) raisedAttributeError; moneyline-only odds formatted as "No odds available".logging_config:ContextualFormattermutated the record, double-prefixing with two handlers;log_error(exc_info=...)raisedTypeError.dynamic_team_resolver: the "shared" class cache was instance-shadowed — every scoreboard refetched AP rankings; writes now go through the class.saved_repositories: unanchored.replace('.git','')mangled URLs (e.g.my.github.io); failed saves left phantom in-memory entries.config_helper.merge_configsaliased un-overridden nested dicts into its result.startup_validator.validate_allaccumulated errors across calls.Test plan
EMULATOR=true python3 run.py) — via the plugin-safety harness (fixture plugin at all 8 panel sizes vs goldens): 65 passedscripts/dev_server.py)pytest) — full CI mirror: 2,574 passed, 0 failed (pytest -m "not hardware" test/ --ignore=test/plugins --cov=src --cov=web_interface), coverage 50%. ~330 new tests across 12 new/extended suites, including endpoint-level secret round-trips through a real ConfigManager+SchemaManager on tmp files.Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changedSavedRepositoriesManager._clean_url,ConfigManager._is_parallel_secrets_list, and every new test module documents the contract it pinsPlugin compatibility
Plugin-facing changes are strictly enabling: array-item secrets (
x-secretinside array items) now actually work end to end;display_duration: truein a hand-edited config no longer produces 1-second slots; the shared rankings cache stops per-scoreboard refetching.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdNotes for reviewer
deep_mergestill replaces lists wholesale (form posts are complete arrays — index-merging would resurrect deleted items; now documented in-code);is_odds_availablestays moneyline-blind (its render-gating contract) whileformat_odds_summarynow formats ML-only data;config_helper's{id}_configkey convention and default-enabled-True divergence.test_secret_separation_parity.pynow asserts zero inline copies and that the canonical import exists.🤖 Generated with Claude Code
https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements