Skip to content

Fix un-tracked issues & add tests#5

Merged
yava-code merged 1 commit into
mainfrom
jules-fix-untracked-issues-5215528783929094644
May 17, 2026
Merged

Fix un-tracked issues & add tests#5
yava-code merged 1 commit into
mainfrom
jules-fix-untracked-issues-5215528783929094644

Conversation

@yava-code

@yava-code yava-code commented May 17, 2026

Copy link
Copy Markdown
Owner
  • Cleaned up unused imports: Optional, annotations, field_validator.
  • Optimized uncached font loading in notify.py by loading the font once and reusing it.
  • Optimized CPU-intensive window handle lookup in notify.py by caching the HWND.
  • Optimized file reading for recent logs and actions in video_recorder.py using collections.deque.
  • Added missing edge case test for generate_ai_status empty API key in tests/test_notify.py.
  • Added tests for untested functions and properties in video_recorder.py (is_idle, status, force_trigger).
  • Added missing test files: test_train.py, test_noita_env.py, test_train_multi.py, test_eval.py, test_offline_analysis.py, test_callbacks.py.
  • Fixed missing tests for setup_bot_menu error path in tests/test_notify.py.

PR created automatically by Jules for task 5215528783929094644 started by @yava-code

Summary by CodeRabbit

  • Tests

    • Added comprehensive test suites across multiple modules including callback initialization, model evaluation, environment operations, notification systems, offline analysis, training setup, checkpoint discovery, and video recording functionality.
  • Performance

    • Optimized efficiency of log and action trace processing through improved streaming algorithms.
  • Chores

    • Internal code structure and import reorganization improvements.

Review Change Stack

@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented May 17, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR performs a systematic maintenance pass removing deprecated future annotations imports, optimizing font and window handle caching in notify.py, improving file reading efficiency in video_recorder.py via deque-based tailing, and adding comprehensive test coverage across eight modules to validate core functionality.

Changes

Maintenance and Optimization Pass

Layer / File(s) Summary
Future annotations import removal across modules
callbacks.py, config.py, eval.py, notify.py, train.py, video_recorder.py
Removes from __future__ import annotations directive and cleans up unused imports (Optional, field_validator) across six modules.
PIL font caching optimization
notify.py
Introduces module-level _cached_font and _get_font() helper to lazily load arial.ttf with fallback, integrated into make_death_postcard and capture_noita_screen to avoid repeated font instantiation.
HWND caching optimization
notify.py
Converts TelegramNotifier._find_noita_hwnd from static to instance method, caching discovered window handles on self._cached_hwnd and reusing them until the window is no longer valid.
Deque-based file tailing
video_recorder.py
Updates _read_recent_logs and _read_recent_actions to stream file lines using deque(..., maxlen=...) instead of loading entire files and slicing, reducing memory overhead.
Test coverage for callbacks and eval
tests/test_callbacks.py, tests/test_eval.py
Adds test_callbacks_init validating NoitaMonitorCallback instantiation, and test_calculate_thinking validating eval.calculate_thinking with mocked PyTorch model and observation.
Test coverage for noita_env
tests/test_noita_env.py
Adds make_env helper mocking external dependencies and TestNoitaEnvMethods validating render() no-op, close() websocket cleanup and lock usage, and _handle() exception resilience.
Test coverage for notify and offline_analysis
tests/test_notify.py, tests/test_offline_analysis.py
Adds tests validating generate_ai_status Groq API key warnings, setup_bot_menu exception handling, and analyze_actions with mocked file I/O and missing file scenarios.
Test coverage for train modules
tests/test_train.py, tests/test_train_multi.py
Adds test_train.py validating setup_logging logger wiring and setup_wandb enabled/disabled/error behaviors; adds test_train_multi.py validating find_latest_checkpoint across empty, non-matching, single-match, and multi-match scenarios.
Test coverage for video_recorder state handling
tests/test_video_recorder.py
Adds parametrized test for trigger_event robustness with empty context, and TestStateProperties validating is_idle and status properties, plus force_trigger correctly bypassing cooldown.

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 Import cleanup hops past old future ways,
Caching fonts and windows now—faster always!
Deques trim the files with elegant grace,
Tests bloom across modules, covering the space.
Performance and safety in one graceful dance! 🎪

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.76% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main objectives: fixing untracked issues and adding tests, which aligns with the changeset's core purpose.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jules-fix-untracked-issues-5215528783929094644

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes __future__ imports across the codebase, introduces caching for font loading and window handles in notify.py, and optimizes log reading in video_recorder.py using collections.deque. It also significantly expands the test suite with several new test files. Feedback focuses on the new mandatory wandb dependency in train.py, thread-safety and redundancy issues in the font caching logic, and the need for AsyncMock in the environment tests to correctly handle async iteration. Additionally, using getattr was suggested for more idiomatic attribute access when checking for cached window handles.

Comment thread train.py
Comment thread notify.py
Comment on lines +26 to +35
_cached_font = None
def _get_font():
global _cached_font
if _cached_font is None:
try:
from PIL import ImageFont
_cached_font = ImageFont.truetype("arial.ttf", 24)
except IOError:
_cached_font = ImageFont.load_default()
return _cached_font

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The font loading logic is not thread-safe and contains a redundant import. Since TelegramNotifier uses a background polling thread, concurrent calls to _get_font could lead to race conditions or multiple font loading attempts. Additionally, ImageFont is already imported at the top of the file (line 22), making the local import unnecessary.

Suggested change
_cached_font = None
def _get_font():
global _cached_font
if _cached_font is None:
try:
from PIL import ImageFont
_cached_font = ImageFont.truetype("arial.ttf", 24)
except IOError:
_cached_font = ImageFont.load_default()
return _cached_font
_font_lock = threading.Lock()
_cached_font = None
def _get_font():
global _cached_font
if _cached_font is None:
with _font_lock:
if _cached_font is None:
try:
_cached_font = ImageFont.truetype("arial.ttf", 24)
except IOError:
_cached_font = ImageFont.load_default()
return _cached_font

Comment thread notify.py
Comment on lines +362 to +365
if hasattr(self, '_cached_hwnd') and self._cached_hwnd is not None:
if user32.IsWindow(self._cached_hwnd):
return self._cached_hwnd
self._cached_hwnd = None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using hasattr repeatedly is less efficient and less idiomatic than using getattr with a default value, especially since _cached_hwnd is not initialized in the constructor. This approach is cleaner and avoids potential AttributeError issues if the attribute is accessed elsewhere.

Suggested change
if hasattr(self, '_cached_hwnd') and self._cached_hwnd is not None:
if user32.IsWindow(self._cached_hwnd):
return self._cached_hwnd
self._cached_hwnd = None
cached_hwnd = getattr(self, '_cached_hwnd', None)
if cached_hwnd is not None:
if user32.IsWindow(cached_hwnd):
return cached_hwnd
self._cached_hwnd = None

Comment thread tests/test_noita_env.py
Comment on lines +42 to +43
mock_ws = MagicMock()
mock_ws.recv.side_effect = Exception("Connection closed")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test uses MagicMock for mock_ws, but env._handle performs an async for iteration over it. MagicMock does not support async iteration by default, which will cause the test to fail with a TypeError. You should use AsyncMock instead to correctly simulate the websocket's behavior.

Suggested change
mock_ws = MagicMock()
mock_ws.recv.side_effect = Exception("Connection closed")
from unittest.mock import AsyncMock
mock_ws = AsyncMock()
mock_ws.__aiter__.return_value = [Exception("Connection closed")] # Simulate failure during iteration

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (3)
tests/test_train_multi.py (1)

21-26: ⚡ Quick win

Make checkpoint ordering deterministic without sleep.

Using time.sleep(0.01) to force file timestamp order can be flaky in CI/filesystems with coarse mtime resolution. Set explicit mtimes instead.

Proposed deterministic update
+import os
+import time
@@
-    import time
     f1.touch()
-    time.sleep(0.01)
+    base = time.time()
+    os.utime(f1, (base + 1, base + 1))
     f3.touch()
-    time.sleep(0.01)
+    os.utime(f3, (base + 2, base + 2))
     f2.touch()
+    os.utime(f2, (base + 3, base + 3))
🤖 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 `@tests/test_train_multi.py` around lines 21 - 26, Replace the flaky
time.sleep-based ordering by setting explicit modification times for the files;
instead of calling f1.touch(); time.sleep(...); f3.touch(); time.sleep(...);
f2.touch(), call os.utime or Path.utime on f1, f3, and f2 with monotonically
increasing timestamps (e.g., now, now+1, now+2) so checkpoint ordering is
deterministic in tests/test_train_multi.py; remove the sleep calls and use the
explicit utime updates targeting the existing f1, f3, f2 symbols.
tests/test_notify.py (1)

203-207: ⚡ Quick win

Assert the error path is actually exercised.

At Line 205, this currently only checks “no exception.” Add an assertion that the mocked requests.post was called so the failure path is truly covered.

Proposed test hardening
 class TestSetupBotMenu:
     def test_setup_bot_menu_error(self, fake):
-        with patch('requests.post', side_effect=Exception('Mock API Error')):
+        with patch('requests.post', side_effect=Exception('Mock API Error')) as mock_post:
             # Should not crash
             fake.setup_bot_menu()
+            assert mock_post.call_count > 0
🤖 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 `@tests/test_notify.py` around lines 203 - 207, The test
TestSetupBotMenu.test_setup_bot_menu_error currently only ensures no exception
is raised; update it to assert the mocked failure path is exercised by assigning
the patch to a mock variable (e.g., mock_post = patch('requests.post',
side_effect=Exception(...))).start() or use with patch(...) as mock_post: and
after calling fake.setup_bot_menu() add an assertion like
mock_post.assert_called_once() (or mock_post.assert_called()) to verify
requests.post was invoked during setup_bot_menu.
tests/test_video_recorder.py (1)

266-277: 💤 Low value

Consider testing force_trigger from additional states.

While testing from cooldown state validates the bypass behavior, testing from idle, recording, and post states would provide more complete coverage and confirm the method works correctly regardless of the recorder's state.

💡 Example expansion
`@pytest.mark.parametrize`("state", ["idle", "recording", "post", "cooldown"])
def test_force_trigger(self, recorder, state):
    recorder._state = state
    recorder.force_trigger("force_event", {"info": "data"})
    
    assert not recorder._event_q.empty()
    item = recorder._event_q.get_nowait()
    assert item["name"] == "force_event"
    assert item["ctx"] == {"info": "data"}
🤖 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 `@tests/test_video_recorder.py` around lines 266 - 277, Extend the
test_force_trigger case to parametrize over recorder states so we validate
force_trigger works from "idle", "recording", "post", as well as "cooldown"; set
recorder._state to each state before calling
recorder.force_trigger("force_event", {"info":"data"}), then assert
recorder._event_q is not empty and verify the queued item via
recorder._event_q.get_nowait() has item["name"] == "force_event" and item["ctx"]
== {"info":"data"} to ensure behavior is consistent across states.
🤖 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 `@tests/test_eval.py`:
- Around line 9-15: The test currently hides a missing implementation by
skipping when calculate_thinking cannot be imported; instead make the test fail
loudly: remove the try/except import wrapper (or keep it but add an explicit
assertion) so that test_calculate_thinking either raises ImportError or asserts
calculate_thinking is not None with a clear failure message. Update references
to the calculate_thinking symbol in test_calculate_thinking so the test will
fail if the function is absent rather than being skipped.

In `@tests/test_noita_env.py`:
- Around line 5-15: The test's make_env() currently writes mocks directly into
sys.modules and never restores them; change it to use unittest.mock.patch.dict
to temporarily inject the mocked modules (keys 'mss', 'pygetwindow', 'pyrect')
into sys.modules so they are automatically restored, perform
importlib.reload(noita_env) inside that patch.dict context, and ensure imports
for patch (e.g., from unittest.mock import patch) are present so the mocking is
scoped and doesn't leak global interpreter state.

In `@tests/test_video_recorder.py`:
- Around line 113-116: The test test_trigger_does_not_crash_on_empty_ctx calls
recorder.trigger_event(event_name, {}) but does not disable cooldown, so the
assertion on recorder._event_q may fail; before calling trigger_event set
recorder.COOLDOWN_SEC = 0 and recorder._last_sent = 0.0 (as done in other tests)
to ensure events are queued regardless of cooldown. Locate this test and add
those two assignments referencing the recorder instance to guarantee
deterministic behavior.
- Around line 252-257: The test test_is_idle is missing coverage for the "post"
state; update the test to set recorder._state = "post" and assert
recorder.is_idle is False so the is_idle property is validated for idle,
recording, post, and cooldown states; locate the test function test_is_idle and
the recorder/is_idle checks to add the new assertion.
- Around line 259-264: The test_status function is missing coverage for the
"post" state; update test_status to set recorder._state = "post" and assert
recorder.status == "post" along with the existing assertions so the test covers
idle → recording → post → idle transitions; modify the test_status method
(referenced by name) to include the "post" state check using recorder._state and
recorder.status.

In `@train.py`:
- Line 16: Remove the top-level "import wandb" so wandb is not imported at
module load; rely on the existing guarded lazy import that checks
cfg.wandb_enabled (the conditional import at/around the code near the guarded
section around line 68). Specifically, delete the top-level import statement and
ensure all references use the lazily imported wandb (or are behind the
cfg.wandb_enabled check) so no import happens when cfg.wandb_enabled is false.

---

Nitpick comments:
In `@tests/test_notify.py`:
- Around line 203-207: The test TestSetupBotMenu.test_setup_bot_menu_error
currently only ensures no exception is raised; update it to assert the mocked
failure path is exercised by assigning the patch to a mock variable (e.g.,
mock_post = patch('requests.post', side_effect=Exception(...))).start() or use
with patch(...) as mock_post: and after calling fake.setup_bot_menu() add an
assertion like mock_post.assert_called_once() (or mock_post.assert_called()) to
verify requests.post was invoked during setup_bot_menu.

In `@tests/test_train_multi.py`:
- Around line 21-26: Replace the flaky time.sleep-based ordering by setting
explicit modification times for the files; instead of calling f1.touch();
time.sleep(...); f3.touch(); time.sleep(...); f2.touch(), call os.utime or
Path.utime on f1, f3, and f2 with monotonically increasing timestamps (e.g.,
now, now+1, now+2) so checkpoint ordering is deterministic in
tests/test_train_multi.py; remove the sleep calls and use the explicit utime
updates targeting the existing f1, f3, f2 symbols.

In `@tests/test_video_recorder.py`:
- Around line 266-277: Extend the test_force_trigger case to parametrize over
recorder states so we validate force_trigger works from "idle", "recording",
"post", as well as "cooldown"; set recorder._state to each state before calling
recorder.force_trigger("force_event", {"info":"data"}), then assert
recorder._event_q is not empty and verify the queued item via
recorder._event_q.get_nowait() has item["name"] == "force_event" and item["ctx"]
== {"info":"data"} to ensure behavior is consistent across states.
🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: d0f7c90e-b34d-44ef-a95e-6d66b205a27a

📥 Commits

Reviewing files that changed from the base of the PR and between 5f420eb and 6a77baa.

📒 Files selected for processing (14)
  • callbacks.py
  • config.py
  • eval.py
  • notify.py
  • tests/test_callbacks.py
  • tests/test_eval.py
  • tests/test_noita_env.py
  • tests/test_notify.py
  • tests/test_offline_analysis.py
  • tests/test_train.py
  • tests/test_train_multi.py
  • tests/test_video_recorder.py
  • train.py
  • video_recorder.py
💤 Files with no reviewable changes (2)
  • callbacks.py
  • eval.py

Comment thread tests/test_eval.py
Comment on lines +9 to +15
try:
from eval import calculate_thinking
except ImportError:
calculate_thinking = None

@pytest.mark.skipif(calculate_thinking is None, reason="calculate_thinking not found")
def test_calculate_thinking():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t skip this test when calculate_thinking is missing.

Current skip logic turns a broken/missing target function into a green test run, which weakens coverage for this path.

Suggested fix
-try:
-    from eval import calculate_thinking
-except ImportError:
-    calculate_thinking = None
-
-@pytest.mark.skipif(calculate_thinking is None, reason="calculate_thinking not found")
+from eval import calculate_thinking
 def test_calculate_thinking():
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try:
from eval import calculate_thinking
except ImportError:
calculate_thinking = None
@pytest.mark.skipif(calculate_thinking is None, reason="calculate_thinking not found")
def test_calculate_thinking():
from eval import calculate_thinking
def test_calculate_thinking():
🤖 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 `@tests/test_eval.py` around lines 9 - 15, The test currently hides a missing
implementation by skipping when calculate_thinking cannot be imported; instead
make the test fail loudly: remove the try/except import wrapper (or keep it but
add an explicit assertion) so that test_calculate_thinking either raises
ImportError or asserts calculate_thinking is not None with a clear failure
message. Update references to the calculate_thinking symbol in
test_calculate_thinking so the test will fail if the function is absent rather
than being skipped.

Comment thread tests/test_noita_env.py
Comment on lines +5 to +15
def make_env():
import importlib, types, sys

# Mock problematic modules
sys.modules['mss'] = MagicMock()
sys.modules['pygetwindow'] = MagicMock()
sys.modules['pyrect'] = MagicMock()

import noita_env
importlib.reload(noita_env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid leaking mocked modules into global interpreter state.

make_env() writes directly into sys.modules and never restores it. That can contaminate later tests and create order-dependent failures. Wrap this in patch.dict(...): so mocks are scoped.

Suggested fix
 def make_env():
     import importlib, types, sys
 
-    # Mock problematic modules
-    sys.modules['mss'] = MagicMock()
-    sys.modules['pygetwindow'] = MagicMock()
-    sys.modules['pyrect'] = MagicMock()
-
-    import noita_env
-    importlib.reload(noita_env)
+    mocked_modules = {
+        'mss': MagicMock(),
+        'pygetwindow': MagicMock(),
+        'pyrect': MagicMock(),
+    }
+    with patch.dict(sys.modules, mocked_modules):
+        import noita_env
+        importlib.reload(noita_env)
 
     env = noita_env.NoitaEnv.__new__(noita_env.NoitaEnv)
     env._lock = MagicMock()
🤖 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 `@tests/test_noita_env.py` around lines 5 - 15, The test's make_env() currently
writes mocks directly into sys.modules and never restores them; change it to use
unittest.mock.patch.dict to temporarily inject the mocked modules (keys 'mss',
'pygetwindow', 'pyrect') into sys.modules so they are automatically restored,
perform importlib.reload(noita_env) inside that patch.dict context, and ensure
imports for patch (e.g., from unittest.mock import patch) are present so the
mocking is scoped and doesn't leak global interpreter state.

Comment on lines +113 to +116
@pytest.mark.parametrize("event_name", ALL_EVENTS)
def test_trigger_does_not_crash_on_empty_ctx(self, recorder, event_name):
recorder.trigger_event(event_name, {})
assert not recorder._event_q.empty()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Test may fail intermittently due to missing cooldown setup.

The assertion assert not recorder._event_q.empty() will fail if the recorder is in cooldown, but this test doesn't disable cooldown like other tests in the file. Compare with lines 167-171 and 226-230, which explicitly set recorder.COOLDOWN_SEC = 0 and recorder._last_sent = 0.0 to ensure events are queued.

🔧 Proposed fix
 `@pytest.mark.parametrize`("event_name", ALL_EVENTS)
 def test_trigger_does_not_crash_on_empty_ctx(self, recorder, event_name):
+    recorder.COOLDOWN_SEC = 0
+    recorder._last_sent = 0.0
     recorder.trigger_event(event_name, {})
     assert not recorder._event_q.empty()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@pytest.mark.parametrize("event_name", ALL_EVENTS)
def test_trigger_does_not_crash_on_empty_ctx(self, recorder, event_name):
recorder.trigger_event(event_name, {})
assert not recorder._event_q.empty()
`@pytest.mark.parametrize`("event_name", ALL_EVENTS)
def test_trigger_does_not_crash_on_empty_ctx(self, recorder, event_name):
recorder.COOLDOWN_SEC = 0
recorder._last_sent = 0.0
recorder.trigger_event(event_name, {})
assert not recorder._event_q.empty()
🤖 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 `@tests/test_video_recorder.py` around lines 113 - 116, The test
test_trigger_does_not_crash_on_empty_ctx calls
recorder.trigger_event(event_name, {}) but does not disable cooldown, so the
assertion on recorder._event_q may fail; before calling trigger_event set
recorder.COOLDOWN_SEC = 0 and recorder._last_sent = 0.0 (as done in other tests)
to ensure events are queued regardless of cooldown. Locate this test and add
those two assignments referencing the recorder instance to guarantee
deterministic behavior.

Comment on lines +252 to +257
def test_is_idle(self, recorder):
assert recorder.is_idle is True
recorder._state = "recording"
assert recorder.is_idle is False
recorder._state = "cooldown"
assert recorder.is_idle is False

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Incomplete state coverage: missing "post" state.

Line 10 documents the state machine as idle → recording → post → idle, but this test only checks idle, recording, and cooldown. The post state should also be tested to ensure is_idle returns the expected value.

📝 Proposed fix to add "post" state coverage
     def test_is_idle(self, recorder):
         assert recorder.is_idle is True
         recorder._state = "recording"
         assert recorder.is_idle is False
+        recorder._state = "post"
+        assert recorder.is_idle is False
         recorder._state = "cooldown"
         assert recorder.is_idle is False
🤖 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 `@tests/test_video_recorder.py` around lines 252 - 257, The test test_is_idle
is missing coverage for the "post" state; update the test to set recorder._state
= "post" and assert recorder.is_idle is False so the is_idle property is
validated for idle, recording, post, and cooldown states; locate the test
function test_is_idle and the recorder/is_idle checks to add the new assertion.

Comment on lines +259 to +264
def test_status(self, recorder):
assert recorder.status == "idle"
recorder._state = "recording"
assert recorder.status == "recording"
recorder._state = "cooldown"
assert recorder.status == "cooldown"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Incomplete state coverage: missing "post" state.

This test should include the post state to match the documented state machine (line 10: idle → recording → post → idle).

📝 Proposed fix to add "post" state coverage
     def test_status(self, recorder):
         assert recorder.status == "idle"
         recorder._state = "recording"
         assert recorder.status == "recording"
+        recorder._state = "post"
+        assert recorder.status == "post"
         recorder._state = "cooldown"
         assert recorder.status == "cooldown"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_status(self, recorder):
assert recorder.status == "idle"
recorder._state = "recording"
assert recorder.status == "recording"
recorder._state = "cooldown"
assert recorder.status == "cooldown"
def test_status(self, recorder):
assert recorder.status == "idle"
recorder._state = "recording"
assert recorder.status == "recording"
recorder._state = "post"
assert recorder.status == "post"
recorder._state = "cooldown"
assert recorder.status == "cooldown"
🤖 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 `@tests/test_video_recorder.py` around lines 259 - 264, The test_status
function is missing coverage for the "post" state; update test_status to set
recorder._state = "post" and assert recorder.status == "post" along with the
existing assertions so the test covers idle → recording → post → idle
transitions; modify the test_status method (referenced by name) to include the
"post" state check using recorder._state and recorder.status.

Comment thread train.py
import argparse
import os
import sys
import wandb

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that wandb is currently imported at module load time.
rg -nP '^\s*import wandb\b' train.py
rg -nP '\bif\s+not\s+cfg\.wandb_enabled\b|\bimport wandb\b' train.py

Repository: yava-code/noitarl

Length of output: 240


🏁 Script executed:

#!/bin/bash
# Get context around the imports and cfg.wandb_enabled check
head -80 train.py | cat -n

Repository: yava-code/noitarl

Length of output: 2663


Keep wandb optional by removing the top-level import.

The import at line 16 executes at module load time, before cfg.wandb_enabled is checked. This causes startup failure when W&B is disabled and not installed. Remove the top-level import and rely on the guarded lazy import at line 68.

Suggested fix
-import wandb
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import wandb
🤖 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 `@train.py` at line 16, Remove the top-level "import wandb" so wandb is not
imported at module load; rely on the existing guarded lazy import that checks
cfg.wandb_enabled (the conditional import at/around the code near the guarded
section around line 68). Specifically, delete the top-level import statement and
ensure all references use the lazily imported wandb (or are behind the
cfg.wandb_enabled check) so no import happens when cfg.wandb_enabled is false.

@yava-code yava-code merged commit 697e041 into main May 17, 2026
3 checks passed
@yava-code yava-code deleted the jules-fix-untracked-issues-5215528783929094644 branch May 17, 2026 01:48
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.

1 participant