From 0d2b250837dc69d13715eab289057340343fd1ee Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 13 Oct 2026 02:10:30 +0000 Subject: [PATCH] fix(review): apply Claude review fixes (turn 1) - openc3/lib/openc3/topics/topic.rb:74 - SonarQube rubydre:S7865: replaced ENV['OPENC3_STREAM_MAX_AGE_SECONDS'] with ENV.fetch('OPENC3_STREAM_MAX_AGE_SECONDS', nil). Behaviour is unchanged: nil still selects the default. - openc3/python/openc3/topics/topic.py:121 - SonarQube python:S5806: stopped reassigning the builtin-shadowing `id` and decode into a local `entry_id` instead. The parameter name stays `id`, so positional callers and parity with Ruby are unaffected. - openc3/python/test/topics/test_topic.py:36,41,60,64,87 - SonarQube python:S8997: replaced direct os.environ assignments with scoped `with patch.dict(os.environ, {...})` blocks. pytest's monkeypatch can't be used in a unittest.TestCase. Every assertion is kept. AI-Review-Bot: true AI-Review-Run: 38335348590 --- openc3/lib/openc3/topics/topic.rb | 2 +- openc3/python/openc3/topics/topic.py | 5 ++--- openc3/python/test/topics/test_topic.py | 28 ++++++++++++------------- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/openc3/lib/openc3/topics/topic.rb b/openc3/lib/openc3/topics/topic.rb index fc0070f80c..7e395e7fce 100644 --- a/openc3/lib/openc3/topics/topic.rb +++ b/openc3/lib/openc3/topics/topic.rb @@ -71,7 +71,7 @@ def self.write_topic(topic, msg_hash, id = '*', maxlen = nil, approximate = 'tru # @return [Float] Configured safety max age in seconds (<= 0 means disabled) def self.stream_max_age_seconds - value = ENV['OPENC3_STREAM_MAX_AGE_SECONDS'] + value = ENV.fetch('OPENC3_STREAM_MAX_AGE_SECONDS', nil) return STREAM_MAX_AGE_DEFAULT_SECONDS if value.nil? return value.to_f end diff --git a/openc3/python/openc3/topics/topic.py b/openc3/python/openc3/topics/topic.py index 1bfe2cdd8a..33661c53d0 100644 --- a/openc3/python/openc3/topics/topic.py +++ b/openc3/python/openc3/topics/topic.py @@ -117,9 +117,8 @@ def stream_safety_minid(cls, id="*", max_age_ms=None): if not id or id == "*": base_ms = int(time.time() * 1000) else: - if isinstance(id, bytes): - id = id.decode() - base_ms = int(str(id).partition("-")[0]) + entry_id = id.decode() if isinstance(id, bytes) else str(id) + base_ms = int(entry_id.partition("-")[0]) minid_ms = base_ms - max_age_ms if minid_ms <= 0: return None diff --git a/openc3/python/test/topics/test_topic.py b/openc3/python/test/topics/test_topic.py index 17462ed36a..558b13e10c 100644 --- a/openc3/python/test/topics/test_topic.py +++ b/openc3/python/test/topics/test_topic.py @@ -33,15 +33,15 @@ def test_max_age_defaults_to_600(self): self.assertEqual(Topic.stream_max_age_seconds(), 600) def test_max_age_reads_env(self): - os.environ["OPENC3_STREAM_MAX_AGE_SECONDS"] = "120" - self.assertEqual(Topic.stream_max_age_seconds(), 120.0) + with patch.dict(os.environ, {"OPENC3_STREAM_MAX_AGE_SECONDS": "120"}): + self.assertEqual(Topic.stream_max_age_seconds(), 120.0) def test_max_age_ms_is_cached(self): self.assertEqual(Topic.stream_max_age_ms(), 600000) - os.environ["OPENC3_STREAM_MAX_AGE_SECONDS"] = "120" - self.assertEqual(Topic.stream_max_age_ms(), 600000) - Topic.clear_stream_max_age_cache() - self.assertEqual(Topic.stream_max_age_ms(), 120000) + with patch.dict(os.environ, {"OPENC3_STREAM_MAX_AGE_SECONDS": "120"}): + self.assertEqual(Topic.stream_max_age_ms(), 600000) + Topic.clear_stream_max_age_cache() + self.assertEqual(Topic.stream_max_age_ms(), 120000) @patch("openc3.topics.topic.time.time", return_value=1700000000.0) def test_minid_relative_to_now_for_auto_ids(self, _time): @@ -57,13 +57,13 @@ def test_minid_explicit_max_age(self): self.assertIsNone(Topic.stream_safety_minid("1700000000000-0", max_age_ms=0)) def test_minid_disabled(self): - os.environ["OPENC3_STREAM_MAX_AGE_SECONDS"] = "0" - self.assertIsNone(Topic.stream_safety_minid()) - self.assertIsNone(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT")) + with patch.dict(os.environ, {"OPENC3_STREAM_MAX_AGE_SECONDS": "0"}): + self.assertIsNone(Topic.stream_safety_minid()) + self.assertIsNone(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT")) Topic.clear_stream_max_age_cache() - os.environ["OPENC3_STREAM_MAX_AGE_SECONDS"] = "" - self.assertIsNone(Topic.stream_safety_minid()) - self.assertIsNone(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT")) + with patch.dict(os.environ, {"OPENC3_STREAM_MAX_AGE_SECONDS": ""}): + self.assertIsNone(Topic.stream_safety_minid()) + self.assertIsNone(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT")) def test_minid_none_for_small_ids(self): self.assertIsNone(Topic.stream_safety_minid("1000-0")) @@ -84,8 +84,8 @@ def test_log_stream_minid_uses_min_age(self, _time): @patch("openc3.topics.topic.time.time", return_value=1700000000.0) def test_log_stream_minid_uses_larger_max_age(self, _time): - os.environ["OPENC3_STREAM_MAX_AGE_SECONDS"] = "3600" - self.assertEqual(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT"), "1699996400000") + with patch.dict(os.environ, {"OPENC3_STREAM_MAX_AGE_SECONDS": "3600"}): + self.assertEqual(Topic.log_stream_safety_minid("INST", "TLM", "DEFAULT"), "1699996400000") def test_log_stream_minid_is_cached_per_stream(self): with patch.object(TargetModel, "get", return_value=None) as get: