From 99d0ee2208434d1f5ca0195a48f7e9b78a70c990 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sat, 1 Aug 2026 10:21:19 -0400 Subject: [PATCH] Address review findings on target_fps validation and config resolution - Reject floats and bools before int() in the target_fps save path. A JSON body can carry them, where int(90.5) silently stored 90 and true stored 1. Form posts send strings, so '90.5' already failed in int(). - Assert the template's target_fps is 100, not merely an int, so the documented default is actually pinned. - Empty-config precedence: keeping the `and config` check deliberately, now spelled out in the comment and covered by a test. Both managers default to the same config/config.json, so falling through cannot pick up a different file's settings; treating {} as an answer would instead return {} when the first manager simply hasn't loaded yet, silently disabling every setting read through the property -- the failure this property exists to fix. Suite 1446 passed. The float-rejection test was checked to fail without the guard. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --- src/plugin_system/base_plugin.py | 8 ++++++++ test/test_plugin_system.py | 17 ++++++++++++++++- test/test_web_api.py | 15 +++++++++++++++ web_interface/blueprints/api_v3.py | 12 +++++++++++- 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index c735e024..8ec4414d 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -199,6 +199,14 @@ class BasePlugin(ABC): # Only a real mapping is usable: callers do .get() on this and feed # the result to numeric code, so handing back whatever a stub or a # half-built manager returned would fail later and further away. + # + # An empty dict is treated as "nothing here yet" rather than a + # valid answer, so resolution continues to the next source. Both + # managers default to the same config/config.json, so falling + # through cannot pick up a different file's settings -- but it does + # rescue the case where the first manager simply hasn't loaded yet, + # which would otherwise return {} and silently disable every + # setting read through this property. if isinstance(config, dict) and config: return config return {} diff --git a/test/test_plugin_system.py b/test/test_plugin_system.py index 57d3d48d..f9b539d7 100644 --- a/test/test_plugin_system.py +++ b/test/test_plugin_system.py @@ -303,6 +303,21 @@ class TestBasePluginGlobalConfig: ) assert plugin.global_config["target_fps"] == 100 + def test_empty_plugin_manager_config_falls_through(self, mock_display_manager): + """An empty first source means "not loaded yet", not "the answer". + + Both managers default to the same config/config.json, so falling + through cannot pick up a different file. Returning {} here instead + would silently disable every setting read through this property -- + the exact failure this property exists to fix. + """ + plugin = self._plugin( + mock_display_manager, + self._manager_with({"target_fps": 100}), # cache_manager + self._manager_with({}), # plugin_manager: empty + ) + assert plugin.global_config["target_fps"] == 100 + def test_returns_empty_dict_when_no_config_manager(self, mock_display_manager): # Plain objects: no config_manager attribute at all. plugin = self._plugin(mock_display_manager, object(), object()) @@ -356,4 +371,4 @@ class TestBasePluginGlobalConfig: import json with open("config/config.template.json") as fh: template = json.load(fh) - assert isinstance(template.get("target_fps"), int) + assert template.get("target_fps") == 100 diff --git a/test/test_web_api.py b/test/test_web_api.py index 48524158..418bb68c 100644 --- a/test/test_web_api.py +++ b/test/test_web_api.py @@ -201,6 +201,21 @@ class TestConfigAPI: saved = mock_config_manager.save_config_atomic.call_args[0][0] assert saved['web_display_autostart'] is True + @pytest.mark.parametrize('value', [90.5, 90.0, True]) + def test_save_target_fps_rejects_non_integer_json(self, client, mock_config_manager, value): + """int() would truncate silently: 90.5 -> 90, True -> 1. + + Only JSON can carry these; a form post sends '90.5', which int() + already rejects. + """ + response = client.post( + '/api/v3/config/main', + data=json.dumps({'target_fps': value}), + content_type='application/json', + ) + + assert response.status_code == 400 + @pytest.mark.parametrize('value', ['20', '250', 'fast']) def test_save_target_fps_rejects_out_of_range(self, client, mock_config_manager, value): """Values ScrollHelper would silently clamp are reported instead.""" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index af2b6d1f..9b4fec57 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -753,8 +753,18 @@ def save_main_config(): # would have been quietly altered is reported rather than appearing to # save and then behaving differently. if 'target_fps' in data and data['target_fps'] not in ('', None): + raw_target_fps = data['target_fps'] + # A JSON body can carry real floats and bools, where int() would + # silently truncate: 90.5 would save as 90, and true as 1. Reject + # them rather than storing a value the user did not ask for. Form + # posts arrive as strings, so '90.5' still fails in int() below. + if isinstance(raw_target_fps, (bool, float)): + return jsonify({ + 'status': 'error', + 'message': "Invalid value for target_fps: must be an integer" + }), 400 try: - target_fps = int(data['target_fps']) + target_fps = int(raw_target_fps) except (ValueError, TypeError): return jsonify({ 'status': 'error',