Add Ruff linting and formatting with CI enforcement#288
Merged
nitrobass24 merged 4 commits intodevelopfrom Mar 17, 2026
Merged
Conversation
PR 1 of 2: Auto-fixes and configuration. - Add [tool.ruff] config to pyproject.toml with E, W, F, I, B, UP rules - Set line-length=120, target Python 3.12 - Apply ruff check --fix: import sorting, type annotation modernization, redundant open modes, invalid escape sequences, deprecated typing imports - Apply ruff format: consistent formatting across 99 files - Fix bare except in e2e script, unused loop vars, import ordering in persist.py, undefined WebApp forward reference - Add ruff to dev dependency group - Add python-lint CI job (ruff check + ruff format --check) - Gate publish on python-lint passing Deferred to PR 2: E501 (line length), UP032 (f-string migration), B904 (raise-without-from), S rules (bandit security) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Ruff linting and formatting (pyproject.toml) and a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
Prevents indefinite hangs if a test spawns a blocking process. The full suite runs in ~36 seconds locally, so 5 minutes is generous. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Run ruff check/format from src/python working directory so per-file-ignores paths resolve correctly - Exclude test_multiprocessing_logger.py which uses timeout_decorator and is flaky on CI runners (multiprocessing timing sensitive) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/python/controller/notifier.py (1)
64-67:⚠️ Potential issue | 🟠 MajorAvoid tracking a thread before it is started; shutdown can join an unstarted thread.
shutdown()can snapshotself._active_threadsand calljoin()(Line 66) beforethread.start()runs, which can raiseRuntimeErrorand break shutdown reliability.🔧 Proposed fix
- thread = threading.Thread(target=self._thread_wrapper, args=(url, payload), daemon=True) + thread = threading.Thread(target=self._thread_wrapper, args=(url, payload), daemon=True) with self._lock: if self._shutdown_flag: self._logger.debug("Webhook suppressed during shutdown: %s %s", event_type, filename) return self._active_threads.add(thread) - thread.start() + try: + thread.start() + except Exception: + self._active_threads.discard(thread) + raiseAlso applies to: 92-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/notifier.py` around lines 64 - 67, The shutdown() implementation may join threads that haven’t been started (from self._active_threads), causing RuntimeError; modify shutdown() to skip joining threads that are not started by checking thread.is_alive() (or thread.ident) before calling thread.join(timeout=...), so only live threads are joined; apply the same fix to the second similar block handling threads at lines 92-98; keep snapshotting of self._active_threads but filter it to threads where thread.is_alive() is True before calling join.src/python/common/persist.py (1)
75-81:⚠️ Potential issue | 🟠 MajorPreserve the existing file mode before
os.replace().
mkstemp()creates a fresh temp file, andos.replace()swaps that inode into place. On updates, the first successful save can therefore silently change the persisted file's permissions. Copy the current mode ontotmp_pathbefore the replace so existing readers keep working.🔧 Minimal fix
fd, tmp_path = tempfile.mkstemp(dir=dir_name, prefix=".tmp_persist_") try: + if os.path.isfile(file_path): + os.chmod(tmp_path, os.stat(file_path).st_mode & 0o7777) with os.fdopen(fd, "w") as f: f.write(self.to_str()) f.flush() os.fsync(f.fileno()) os.replace(tmp_path, file_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/common/persist.py` around lines 75 - 81, The temporary file created via mkstemp (variables fd and tmp_path) can replace the target via os.replace and unintentionally change file permissions; before calling os.replace(tmp_path, file_path) in the save routine (the block that opens fd and writes self.to_str()), read the current mode of file_path (via os.stat(file_path).st_mode) if file_path exists and apply it to tmp_path with os.chmod(tmp_path, mode) so the inode swap preserves the original permissions; if file_path does not exist, skip the chmod step. Ensure any OSError from stat/chmod is handled (e.g., ignore and proceed) so the replace still happens.src/python/model/file.py (1)
207-238:⚠️ Potential issue | 🟠 MajorAdd union type annotations for timestamp properties that can be
None.These getters are initialized to
None(lines 44–47) and tests assert this initial state, but their return type annotations declaredatetimewithout theNoneoption. This creates a type contract mismatch that misleads callers and static checkers.Proposed fix
- def local_created_timestamp(self) -> datetime: + def local_created_timestamp(self) -> datetime | None: return self.__local_created_timestamp - def local_modified_timestamp(self) -> datetime: + def local_modified_timestamp(self) -> datetime | None: return self.__local_modified_timestamp - def remote_created_timestamp(self) -> datetime: + def remote_created_timestamp(self) -> datetime | None: return self.__remote_created_timestamp - def remote_modified_timestamp(self) -> datetime: + def remote_modified_timestamp(self) -> datetime | None: return self.__remote_modified_timestamp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/model/file.py` around lines 207 - 238, The timestamp property annotations (local_created_timestamp, local_modified_timestamp, remote_created_timestamp, remote_modified_timestamp and their setters) currently declare return/param types as datetime while they are initialized to None; update their type annotations to Optional[datetime] (or Union[datetime, None]) and add `from typing import Optional` import, and adjust the setters' runtime checks to accept None (use isinstance(value, datetime) when value is not None) instead of strict type(...) comparison to ensure both None and datetime values are valid.src/python/controller/scan/local_scanner.py (1)
32-34:⚠️ Potential issue | 🟠 MajorDon’t suppress invalid path-type errors as empty scan results.
Line 32 returns
[]for any non-directory path. If the path exists but is a file (misconfiguration), this now silently hides an error.Suggested fix
- if not os.path.isdir(self.__local_path): + if not os.path.exists(self.__local_path): self.logger.warning("Scan path does not exist: {}".format(self.__local_path)) return []This keeps the “missing directory” fallback while still allowing
SystemScannerError("Path is not a directory")to surface for invalid configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/scan/local_scanner.py` around lines 32 - 34, The current check in the LocalScanner that returns [] whenever not os.path.isdir(self.__local_path) should be split: if the path does not exist, keep the logger.warning("Scan path does not exist: {}".format(self.__local_path)) and return []; but if the path exists and is not a directory, raise SystemScannerError("Path is not a directory") (or equivalent) so invalid file-configs are surfaced. Update the method where self.__local_path is evaluated (the local scanner class/method containing that os.path.isdir check) to perform os.path.exists first, then os.path.isdir, using self.logger and SystemScannerError as shown in the diff.src/python/controller/scan/remote_scanner.py (1)
45-47:⚠️ Potential issue | 🟠 MajorUse
posixpath.join()for remote SSH paths.Line 47 constructs a remote path with
os.path.join(), which uses the local OS path separator. On Windows hosts this produces\instead of/, breaking remote path resolution and execution on the POSIX SSH server.Add
import posixpathand replaceos.path.join()withposixpath.join()to ensure POSIX path separators are used consistently regardless of the host OS.Suggested fix
import hashlib import json import logging import os +import posixpath import time @@ script_name = os.path.basename(self.__local_path_to_scan_script) if os.path.basename(self.__remote_path_to_scan_script) != script_name: - self.__remote_path_to_scan_script = os.path.join(self.__remote_path_to_scan_script, script_name) + self.__remote_path_to_scan_script = posixpath.join(self.__remote_path_to_scan_script, script_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/scan/remote_scanner.py` around lines 45 - 47, The remote path is being built with os.path.join which uses the host OS separator; update remote path construction in remote_scanner.py so POSIX separators are always used: add "import posixpath" at the top and replace the os.path.join call that assigns __remote_path_to_scan_script (the branch that compares os.path.basename(self.__remote_path_to_scan_script) and script_name) with posixpath.join(self.__remote_path_to_scan_script, script_name) so remote SSH paths use '/' regardless of the host OS.src/python/controller/controller.py (1)
285-299:⚠️ Potential issue | 🟠 MajorDisabled-pairs path still creates an active default pair.
When all configured pairs are disabled, this branch still returns a fallback default pair and can continue syncing unexpectedly. It should return no pair contexts in that case, and only use fallback when the pair list is actually empty.
💡 Proposed fix
if not enabled_pairs: if pairs: # All configured pairs are disabled — signal idle state self.__context.status.controller.no_enabled_pairs = True self.logger.warning("All path pairs are disabled. Enable a pair in Settings to start syncing.") - # Backward compatibility: no path pairs configured, use config.lftp - return [ - self._create_pair_context( - pair_id=None, - name="Default", - remote_path=self.__context.config.lftp.remote_path, - local_path=self.__context.config.lftp.local_path, - ) - ] + return [] + # Backward compatibility: no path pairs configured, use config.lftp + self.__context.status.controller.no_enabled_pairs = False + return [ + self._create_pair_context( + pair_id=None, + name="Default", + remote_path=self.__context.config.lftp.remote_path, + local_path=self.__context.config.lftp.local_path, + ) + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/controller.py` around lines 285 - 299, The current branch treats the "no enabled pairs" case the same as "no pairs configured" and always returns a fallback default pair; change the logic in the method that checks enabled_pairs and pairs so that when enabled_pairs is empty but pairs is non-empty you set self.__context.status.controller.no_enabled_pairs = True, emit the warning, and return an empty list (no pair contexts), and only construct the fallback via self._create_pair_context(...) using self.__context.config.lftp.* when pairs is actually empty; update the if/else flow accordingly to differentiate the two cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/common/app_process.py`:
- Around line 114-115: The terminate loop in AppProcess uses a CPU-burning
busy-wait: while self.is_alive() and elapsed_ms(timestamp_start) <
AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS: pass; replace this with a bounded
wait to yield CPU — e.g., compute remaining_time = max(0,
(AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS - elapsed_ms(timestamp_start)) /
1000.0) and call self.join(timeout=min(0.1, remaining_time)) or alternatively
sleep a short interval (time.sleep(0.05)) inside the loop; keep the existing
is_alive(), elapsed_ms, timestamp_start and timeout logic but ensure the loop
uses join/sleep to avoid spinning.
In `@src/python/controller/controller_persist.py`:
- Line 71: The error message raised by PersistError incorrectly references
AutoQueuePersist; update the message to reference the correct class name
ControllerPersist where the exception is raised (the raise PersistError(...)
call in controller_persist.py) so it reads something like "Error parsing
ControllerPersist - ..." and preserve the existing inclusion of the exception
type and message (type(e).__name__ and str(e)) in the formatted string.
In `@src/python/controller/extract/dispatch.py`:
- Around line 104-112: The code reads self.__task_queue.queue directly in
status(), extract(), and __worker(), which can race with concurrent put()/get();
fix by acquiring the queue's lock and taking a safe snapshot inside the critical
section: wrap accesses to self.__task_queue.queue with "with
self.__task_queue.mutex:" and inside that block copy the deque into a local list
(e.g., tasks = list(self.__task_queue.queue)) then operate on that local
snapshot outside the lock; apply this pattern in status(), extract(), and the
queue-peeking portion of __worker().
In `@src/python/controller/notifier.py`:
- Around line 114-115: Wrap the urllib.request.urlopen call in a context manager
to ensure the response is closed; replace the standalone
urllib.request.urlopen(req, timeout=5) call with a with-statement (using the
existing req Request object) so the response returned by urllib.request.urlopen
is explicitly closed even on exceptions, preventing socket/resource leaks in
notifier.py.
In `@src/python/seedsync.py`:
- Around line 478-480: The except block that calls
Seedsync.logger.exception("Caught exception") must guard against Seedsync.logger
being None to avoid masking the original error; update the exception handler
(the except Exception block around the top-level startup) to check if
Seedsync.logger is not None before calling Seedsync.logger.exception, and if it
is None fall back to a safe alternative such as printing the exception or using
the standard logging/traceback modules to emit the traceback, then re-raise the
original exception as before.
In `@src/python/ssh/sshcp.py`:
- Around line 31-38: The constructor declares user optional but callers build
targets as "{user}@{host}", causing "None@host" when unset; update either
__init__ to require user (remove default None) or (preferable) add a single
helper like _remote_target(self) that returns f"{self.__user}@{self.__host}" if
self.__user else self.__host, then change all call sites (_run_shell_command,
_sftp_stat, shell, copy) to use that helper instead of formatting
"{user}@{host}" directly so host-only usage works correctly.
In `@src/python/tests/integration/test_controller/test_controller.py`:
- Around line 86-87: The code in create_archive opens os.devnull into fnull but
never closes it; change the subprocess invocation in create_archive to ensure
fnull is closed (e.g., use a with open(os.devnull, "w") as fnull: block around
subprocess.Popen/communicate or explicitly call fnull.close() after
communicate()) so the file descriptor is released after the subprocess runs.
In `@src/python/tests/integration/test_controller/test_extract/test_extract.py`:
- Around line 49-64: The subprocesses that create archives (invocations using
subprocess.Popen in the setup that set TestExtract.ar_rar,
TestExtract.ar_rar_split_p1/TestExtract.ar_rar_split_p2, and
TestExtract.ar_tar_gz) must be awaited and the os.devnull handle closed: replace
or wrap those subprocess.Popen calls with subprocess.run (or call .wait() on the
returned Popen) so the test setup blocks until the archive creation finishes,
check the process returncode and raise/log on non-zero to surface failures, and
ensure the opened fnull file descriptor is closed after use.
In `@src/python/tests/unittests/test_controller/test_auto_queue.py`:
- Around line 183-184: Remove the debug prints that leak into CI by deleting the
print(content) and print(AutoQueuePattern(pattern='fi"ve').to_str()) calls in
the test_auto_queue unit test (and the other intermediate payload print around
line 204); instead, replace them with appropriate assertions on the payload or
on AutoQueuePattern.to_str() using the AutoQueuePattern class and any existing
test helpers so the test verifies behavior without printing.
In `@src/python/tests/unittests/test_controller/test_extract/test_dispatch.py`:
- Around line 704-705: The loop waiting for callbacks uses an incorrect boolean:
change the while condition that currently reads "while
self.mock_extract_archive.call_count < 1 and
self.listener.extract_completed.call_count < 1:" to use "or" so it continues
until both self.mock_extract_archive.call_count and
self.listener.extract_completed.call_count have reached 1; update the predicate
in the test_dispatch waiting loop to "while ... < 1 or ... < 1" to prevent early
exit and flakiness.
---
Outside diff comments:
In `@src/python/common/persist.py`:
- Around line 75-81: The temporary file created via mkstemp (variables fd and
tmp_path) can replace the target via os.replace and unintentionally change file
permissions; before calling os.replace(tmp_path, file_path) in the save routine
(the block that opens fd and writes self.to_str()), read the current mode of
file_path (via os.stat(file_path).st_mode) if file_path exists and apply it to
tmp_path with os.chmod(tmp_path, mode) so the inode swap preserves the original
permissions; if file_path does not exist, skip the chmod step. Ensure any
OSError from stat/chmod is handled (e.g., ignore and proceed) so the replace
still happens.
In `@src/python/controller/controller.py`:
- Around line 285-299: The current branch treats the "no enabled pairs" case the
same as "no pairs configured" and always returns a fallback default pair; change
the logic in the method that checks enabled_pairs and pairs so that when
enabled_pairs is empty but pairs is non-empty you set
self.__context.status.controller.no_enabled_pairs = True, emit the warning, and
return an empty list (no pair contexts), and only construct the fallback via
self._create_pair_context(...) using self.__context.config.lftp.* when pairs is
actually empty; update the if/else flow accordingly to differentiate the two
cases.
In `@src/python/controller/notifier.py`:
- Around line 64-67: The shutdown() implementation may join threads that haven’t
been started (from self._active_threads), causing RuntimeError; modify
shutdown() to skip joining threads that are not started by checking
thread.is_alive() (or thread.ident) before calling thread.join(timeout=...), so
only live threads are joined; apply the same fix to the second similar block
handling threads at lines 92-98; keep snapshotting of self._active_threads but
filter it to threads where thread.is_alive() is True before calling join.
In `@src/python/controller/scan/local_scanner.py`:
- Around line 32-34: The current check in the LocalScanner that returns []
whenever not os.path.isdir(self.__local_path) should be split: if the path does
not exist, keep the logger.warning("Scan path does not exist:
{}".format(self.__local_path)) and return []; but if the path exists and is not
a directory, raise SystemScannerError("Path is not a directory") (or equivalent)
so invalid file-configs are surfaced. Update the method where self.__local_path
is evaluated (the local scanner class/method containing that os.path.isdir
check) to perform os.path.exists first, then os.path.isdir, using self.logger
and SystemScannerError as shown in the diff.
In `@src/python/controller/scan/remote_scanner.py`:
- Around line 45-47: The remote path is being built with os.path.join which uses
the host OS separator; update remote path construction in remote_scanner.py so
POSIX separators are always used: add "import posixpath" at the top and replace
the os.path.join call that assigns __remote_path_to_scan_script (the branch that
compares os.path.basename(self.__remote_path_to_scan_script) and script_name)
with posixpath.join(self.__remote_path_to_scan_script, script_name) so remote
SSH paths use '/' regardless of the host OS.
In `@src/python/model/file.py`:
- Around line 207-238: The timestamp property annotations
(local_created_timestamp, local_modified_timestamp, remote_created_timestamp,
remote_modified_timestamp and their setters) currently declare return/param
types as datetime while they are initialized to None; update their type
annotations to Optional[datetime] (or Union[datetime, None]) and add `from
typing import Optional` import, and adjust the setters' runtime checks to accept
None (use isinstance(value, datetime) when value is not None) instead of strict
type(...) comparison to ensure both None and datetime values are valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1991ae82-e2f3-4571-bdf5-6173921c393d
📒 Files selected for processing (114)
.github/workflows/ci.ymlsrc/docker/test/e2e/parse_seedsync_status.pysrc/pyinstaller_hooks/hook-patoolib.pysrc/python/common/app_process.pysrc/python/common/config.pysrc/python/common/constants.pysrc/python/common/context.pysrc/python/common/error.pysrc/python/common/job.pysrc/python/common/multiprocessing_logger.pysrc/python/common/path_pairs_config.pysrc/python/common/persist.pysrc/python/common/status.pysrc/python/common/types.pysrc/python/controller/auto_queue.pysrc/python/controller/controller.pysrc/python/controller/controller_job.pysrc/python/controller/controller_persist.pysrc/python/controller/delete/delete_process.pysrc/python/controller/extract/dispatch.pysrc/python/controller/extract/extract.pysrc/python/controller/extract/extract_process.pysrc/python/controller/extract/extract_request.pysrc/python/controller/model_builder.pysrc/python/controller/move/move_process.pysrc/python/controller/notifier.pysrc/python/controller/scan/active_scanner.pysrc/python/controller/scan/local_scanner.pysrc/python/controller/scan/remote_scanner.pysrc/python/controller/scan/scanner_process.pysrc/python/controller/validate/validate_process.pysrc/python/lftp/job_status.pysrc/python/lftp/job_status_parser.pysrc/python/lftp/lftp.pysrc/python/model/diff.pysrc/python/model/file.pysrc/python/model/model.pysrc/python/pyproject.tomlsrc/python/scan_fs.pysrc/python/seedsync.pysrc/python/ssh/sshcp.pysrc/python/system/file.pysrc/python/system/scanner.pysrc/python/tests/conftest.pysrc/python/tests/integration/test_controller/test_controller.pysrc/python/tests/integration/test_controller/test_extract/test_extract.pysrc/python/tests/integration/test_lftp/test_lftp.pysrc/python/tests/integration/test_web/test_handler/test_auto_queue.pysrc/python/tests/integration/test_web/test_handler/test_controller.pysrc/python/tests/integration/test_web/test_handler/test_path_pairs.pysrc/python/tests/integration/test_web/test_handler/test_stream_log.pysrc/python/tests/integration/test_web/test_handler/test_stream_model.pysrc/python/tests/integration/test_web/test_handler/test_stream_status.pysrc/python/tests/integration/test_web/test_web_app.pysrc/python/tests/unittests/test_common/test_app_process.pysrc/python/tests/unittests/test_common/test_config.pysrc/python/tests/unittests/test_common/test_job.pysrc/python/tests/unittests/test_common/test_multiprocessing_logger.pysrc/python/tests/unittests/test_common/test_persist.pysrc/python/tests/unittests/test_common/test_remote_path_utils.pysrc/python/tests/unittests/test_common/test_status.pysrc/python/tests/unittests/test_controller/test_auto_queue.pysrc/python/tests/unittests/test_controller/test_controller_persist.pysrc/python/tests/unittests/test_controller/test_exclude_patterns.pysrc/python/tests/unittests/test_controller/test_extract/test_dispatch.pysrc/python/tests/unittests/test_controller/test_extract/test_extract.pysrc/python/tests/unittests/test_controller/test_extract/test_extract_process.pysrc/python/tests/unittests/test_controller/test_model_builder.pysrc/python/tests/unittests/test_controller/test_move_process.pysrc/python/tests/unittests/test_controller/test_notifier.pysrc/python/tests/unittests/test_controller/test_scan/test_remote_scanner.pysrc/python/tests/unittests/test_controller/test_scan/test_scanner_process.pysrc/python/tests/unittests/test_controller/test_validate/test_validate_process.pysrc/python/tests/unittests/test_lftp/test_job_status.pysrc/python/tests/unittests/test_lftp/test_job_status_parser.pysrc/python/tests/unittests/test_lftp/test_lftp.pysrc/python/tests/unittests/test_lftp/test_lftp_queue_command.pysrc/python/tests/unittests/test_model/test_diff.pysrc/python/tests/unittests/test_model/test_file.pysrc/python/tests/unittests/test_model/test_model.pysrc/python/tests/unittests/test_seedsync.pysrc/python/tests/unittests/test_ssh/test_shell_detect.pysrc/python/tests/unittests/test_ssh/test_sshcp.pysrc/python/tests/unittests/test_system/test_file.pysrc/python/tests/unittests/test_system/test_scanner.pysrc/python/tests/unittests/test_web/test_handler/test_stream_log.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_auto_queue.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_config.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_log_record.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_model.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_status.pysrc/python/tests/unittests/test_web/test_web_app_stream.pysrc/python/web/handler/auto_queue.pysrc/python/web/handler/config.pysrc/python/web/handler/controller.pysrc/python/web/handler/logs.pysrc/python/web/handler/path_pairs.pysrc/python/web/handler/server.pysrc/python/web/handler/status.pysrc/python/web/handler/stream_log.pysrc/python/web/handler/stream_model.pysrc/python/web/handler/stream_status.pysrc/python/web/security.pysrc/python/web/serialize/serialize.pysrc/python/web/serialize/serialize_auto_queue.pysrc/python/web/serialize/serialize_config.pysrc/python/web/serialize/serialize_log_record.pysrc/python/web/serialize/serialize_model.pysrc/python/web/serialize/serialize_status.pysrc/python/web/utils.pysrc/python/web/web_app.pysrc/python/web/web_app_builder.pysrc/python/web/web_app_job.py
💤 Files with no reviewable changes (1)
- src/python/tests/unittests/test_web/test_serialize/test_serialize.py
Comment on lines
+114
to
115
| while self.is_alive() and elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS: | ||
| pass |
There was a problem hiding this comment.
Avoid CPU-burning busy-wait in terminate loop.
Line 114-Line 115 currently spin with pass, which can peg CPU during shutdown. Use a bounded join sleep interval instead.
⚙️ Proposed fix
- while self.is_alive() and elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS:
- pass
+ while self.is_alive() and elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS:
+ self.join(timeout=0.01)📝 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
| while self.is_alive() and elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS: | |
| pass | |
| while self.is_alive() and elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS: | |
| self.join(timeout=0.01) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/common/app_process.py` around lines 114 - 115, The terminate loop
in AppProcess uses a CPU-burning busy-wait: while self.is_alive() and
elapsed_ms(timestamp_start) < AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS: pass;
replace this with a bounded wait to yield CPU — e.g., compute remaining_time =
max(0, (AppProcess.__DEFAULT_TERMINATE_TIMEOUT_MS - elapsed_ms(timestamp_start))
/ 1000.0) and call self.join(timeout=min(0.1, remaining_time)) or alternatively
sleep a short interval (time.sleep(0.05)) inside the loop; keep the existing
is_alive(), elapsed_ms, timestamp_start and timeout logic but ensure the loop
uses join/sleep to avoid spinning.
Comment on lines
+104
to
112
| def status(self) -> list[ExtractStatus]: | ||
| tasks = list(self.__task_queue.queue) | ||
| statuses = [] | ||
| for task in tasks: | ||
| status = ExtractStatus(name=task.root_name, | ||
| is_dir=task.root_is_dir, | ||
| state=ExtractStatus.State.EXTRACTING, | ||
| pair_id=task.pair_id) | ||
| status = ExtractStatus( | ||
| name=task.root_name, is_dir=task.root_is_dir, state=ExtractStatus.State.EXTRACTING, pair_id=task.pair_id | ||
| ) | ||
| statuses.append(status) | ||
| return statuses |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show every direct access to the queue internals in this file.
rg -n -C2 'self\.__task_queue\.(queue|mutex)' src/python/controller/extract/dispatch.pyRepository: nitrobass24/seedsync
Length of output: 900
🏁 Script executed:
#!/bin/bash
# Check the class definition and queue initialization
cat -n src/python/controller/extract/dispatch.py | head -100Repository: nitrobass24/seedsync
Length of output: 3298
🏁 Script executed:
#!/bin/bash
# Look for where __task_queue is initialized
rg -n '__task_queue\s*=' src/python/controller/extract/dispatch.pyRepository: nitrobass24/seedsync
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Look for imports to determine Queue type
head -50 src/python/controller/extract/dispatch.pyRepository: nitrobass24/seedsync
Length of output: 1106
🏁 Script executed:
#!/bin/bash
# Check if queue.Queue has a mutex attribute in Python 3.12
python3 << 'EOF'
import queue
q = queue.Queue()
# Check what attributes are available
attrs = [a for a in dir(q) if not a.startswith('__')]
print("Public attributes:", attrs)
# Check for private lock
if hasattr(q, '_lock'):
print("Has _lock:", type(q._lock))
if hasattr(q, 'mutex'):
print("Has mutex:", type(q.mutex))
EOFRepository: nitrobass24/seedsync
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Show all three direct .queue accesses to understand the pattern
cat -n src/python/controller/extract/dispatch.py | sed -n '103,107p; 130,136p; 184,191p'Repository: nitrobass24/seedsync
Length of output: 1058
🏁 Script executed:
#!/bin/bash
# Check which method contains each access and what thread context
# Lines 133 and 188 should be in the worker thread
cat -n src/python/controller/extract/dispatch.py | sed -n '125,195p'Repository: nitrobass24/seedsync
Length of output: 3976
🏁 Script executed:
#!/bin/bash
# Check context of extract() - is it called from main thread or worker?
# Also check if there's documentation about why direct .queue access is used
grep -n "def extract" src/python/controller/extract/dispatch.py
grep -n "\.extract(" src/python/controller/extract/dispatch.py | head -5Repository: nitrobass24/seedsync
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Verify: lines 186-188 are ONLY in __worker(), confirming single-threaded access there
# Check what line 180 starts (should be __worker method definition)
cat -n src/python/controller/extract/dispatch.py | sed -n '180p'Repository: nitrobass24/seedsync
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Confirm: extract() is public (called externally), so line 133 is unsafe
# Check for external calls to extract()
find src/python -name "*.py" -type f | xargs grep -l "\.extract(" | head -5Repository: nitrobass24/seedsync
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Show the full __worker method to confirm lines 186-188 context
cat -n src/python/controller/extract/dispatch.py | sed -n '180,230p'Repository: nitrobass24/seedsync
Length of output: 2467
🏁 Script executed:
#!/bin/bash
# Verify: Does queue.Queue.mutex actually protect access to .queue?
# Check Python docs/source to confirm this is the expected usage
python3 << 'EOF'
import queue
import threading
q = queue.Queue()
q.put("item1")
# Simulate checking the deque directly
with q.mutex:
items = list(q.queue)
print("Items with mutex:", items)
# This is the expected pattern
print("Test: mutex successfully protects .queue access")
EOFRepository: nitrobass24/seedsync
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Confirm: all three accesses are in different methods with different thread contexts
# Lines 105 (status - public), 133 (extract - public), 186-188 (__worker - worker thread only)
echo "=== Method contexts ==="
echo "Line 105 in:"
grep -B 20 "tasks = list" src/python/controller/extract/dispatch.py | grep "def " | tail -1
echo "Line 133 in:"
grep -B 5 "for task in self.__task_queue.queue:" src/python/controller/extract/dispatch.py | head -1 | grep "def"
echo "Lines 186-188 in __worker (worker thread only)"
echo ""
echo "=== Thread safety analysis ==="
echo "Line 105: status() is public → called from main/external thread"
echo "Line 133: extract() is public → called from main/external thread"
echo "Lines 186-188: __worker() → only called as worker thread target"
echo ""
echo "Potential races:"
echo "- Main thread: status() reads .queue while worker thread may put()/get()"
echo "- Main thread: extract() reads .queue while worker thread may put()/get()"
echo "- Worker thread: __worker reads .queue; main thread calls put() from extract() or extract_completed callback"Repository: nitrobass24/seedsync
Length of output: 704
Protect all direct .queue accesses with the mutex to prevent data races.
Directly reading self.__task_queue.queue without the queue lock risks RuntimeError (deque mutated during iteration) or torn snapshots. This affects three locations:
- Line 105:
status()snapshots.queuewhile worker thread callsput()/get() - Line 133:
extract()iterates.queuewhile worker thread callsput()/get() - Lines 186–188:
__worker()peeks.queuewhile main thread callsput()
Wrap each access with self.__task_queue.mutex:
🔧 Minimal fix
def status(self) -> list[ExtractStatus]:
- tasks = list(self.__task_queue.queue)
+ with self.__task_queue.mutex:
+ tasks = list(self.__task_queue.queue)
statuses = []Apply the same pattern to extract() (line 133) and __worker() (lines 186–188).
📝 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 status(self) -> list[ExtractStatus]: | |
| tasks = list(self.__task_queue.queue) | |
| statuses = [] | |
| for task in tasks: | |
| status = ExtractStatus(name=task.root_name, | |
| is_dir=task.root_is_dir, | |
| state=ExtractStatus.State.EXTRACTING, | |
| pair_id=task.pair_id) | |
| status = ExtractStatus( | |
| name=task.root_name, is_dir=task.root_is_dir, state=ExtractStatus.State.EXTRACTING, pair_id=task.pair_id | |
| ) | |
| statuses.append(status) | |
| return statuses | |
| def status(self) -> list[ExtractStatus]: | |
| with self.__task_queue.mutex: | |
| tasks = list(self.__task_queue.queue) | |
| statuses = [] | |
| for task in tasks: | |
| status = ExtractStatus( | |
| name=task.root_name, is_dir=task.root_is_dir, state=ExtractStatus.State.EXTRACTING, pair_id=task.pair_id | |
| ) | |
| statuses.append(status) | |
| return statuses |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/controller/extract/dispatch.py` around lines 104 - 112, The code
reads self.__task_queue.queue directly in status(), extract(), and __worker(),
which can race with concurrent put()/get(); fix by acquiring the queue's lock
and taking a safe snapshot inside the critical section: wrap accesses to
self.__task_queue.queue with "with self.__task_queue.mutex:" and inside that
block copy the deque into a local list (e.g., tasks =
list(self.__task_queue.queue)) then operate on that local snapshot outside the
lock; apply this pattern in status(), extract(), and the queue-peeking portion
of __worker().
Comment on lines
+114
to
115
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}, method="POST") | ||
| urllib.request.urlopen(req, timeout=5) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all urlopen call sites and inspect whether they are context-managed.
rg -n -C2 'urllib\.request\.urlopen\s*\(' --type=pyRepository: nitrobass24/seedsync
Length of output: 606
🏁 Script executed:
# Get the full function context around line 115
cat -n src/python/controller/notifier.py | sed -n '100,130p'Repository: nitrobass24/seedsync
Length of output: 1123
🌐 Web query:
urllib.request.urlopen context manager Python 3.12 best practice
💡 Result:
In Python 3.12, urllib.request.urlopen() returns a response object that already works as a context manager, so the best practice is to use with ... as ... to ensure the underlying network resource is closed even if an exception occurs. [1]
import urllib.request
url = "https://example.com/"
req = urllib.request.Request(
url,
headers={"User-Agent": "my-app/1.0"},
)
with urllib.request.urlopen(req, timeout=10) as resp:
body = resp.read() # read/stream *inside* the block
status = resp.status
headers = resp.headersKey practices:
- Prefer
with urllib.request.urlopen(...) as resp:rather than callingresp.close()manually. [1][2] - Set an explicit
timeoutso a stalled connection doesn’t block indefinitely. [1] - Don’t return/keep
resp(or iterators reading from it) outside thewithblock—it will be closed on exit, so reads after the block may fail. [1] contextlib.closing(urlopen(...))is usually unnecessary now, because the response object itself is a context manager. [1][3]
Sources: Python 3.12 urllib.request.urlopen docs [1], Python “HOWTO Fetch Internet Resources Using urllib” (3.12) [2], discussion of closing() vs urlopen() context manager behavior [3].
Close the urlopen response explicitly to prevent socket/resource leaks.
urllib.request.urlopen() returns a response object that should be used as a context manager in Python 3.12. Without it, the underlying network connection may not be promptly released, especially if exceptions occur.
🔧 Proposed fix
- urllib.request.urlopen(req, timeout=5)
+ with urllib.request.urlopen(req, timeout=5) as response:
+ response.read()📝 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
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}, method="POST") | |
| urllib.request.urlopen(req, timeout=5) | |
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}, method="POST") | |
| with urllib.request.urlopen(req, timeout=5) as response: | |
| response.read() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/controller/notifier.py` around lines 114 - 115, Wrap the
urllib.request.urlopen call in a context manager to ensure the response is
closed; replace the standalone urllib.request.urlopen(req, timeout=5) call with
a with-statement (using the existing req Request object) so the response
returned by urllib.request.urlopen is explicitly closed even on exceptions,
preventing socket/resource leaks in notifier.py.
Comment on lines
+478
to
480
| except Exception: | ||
| Seedsync.logger.exception("Caught exception") | ||
| raise |
There was a problem hiding this comment.
Guard top-level exception logging when logger is uninitialized.
At Line 479, Seedsync.logger can still be None if startup fails early, which can mask the original exception with a secondary AttributeError.
Proposed fix
except ServiceRestart:
Seedsync.logger.info("Restarting...")
continue
except Exception:
- Seedsync.logger.exception("Caught exception")
+ if Seedsync.logger is not None:
+ Seedsync.logger.exception("Caught exception")
+ else:
+ logging.exception("Caught exception before Seedsync logger initialization")
raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/seedsync.py` around lines 478 - 480, The except block that calls
Seedsync.logger.exception("Caught exception") must guard against Seedsync.logger
being None to avoid masking the original error; update the exception handler
(the except Exception block around the top-level startup) to check if
Seedsync.logger is not None before calling Seedsync.logger.exception, and if it
is None fall back to a safe alternative such as printing the exception or using
the standard logging/traceback modules to emit the traceback, then re-raise the
original exception as before.
Comment on lines
+31
to
+38
| def __init__(self, host: str, port: int, user: str = None, password: str = None): | ||
| if host is None: | ||
| raise ValueError("Hostname not specified.") | ||
| self.__host = host | ||
| self.__port = port | ||
| self.__user = user | ||
| self.__password = password | ||
| self.__detected_shell: Optional[str] = None | ||
| self.__detected_shell: str | None = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any Sshcp call sites omit `user=` or pass a None-like value.
rg -nP -C2 '\bSshcp\s*\(' src/pythonRepository: nitrobass24/seedsync
Length of output: 9370
🏁 Script executed:
#!/bin/bash
# Check the test at line 64 in test_sshcp.py
sed -n '63,68p' src/python/tests/unittests/test_ssh/test_sshcp.pyRepository: nitrobass24/seedsync
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check validate_process.py context around line 309
sed -n '307,315p' src/python/controller/validate/validate_process.pyRepository: nitrobass24/seedsync
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Check the methods in sshcp.py at the mentioned line ranges
sed -n '128,129p; 161,165p; 403,404p; 423,424p' src/python/ssh/sshcp.pyRepository: nitrobass24/seedsync
Length of output: 698
API contract violation: user parameter optional in signature but required at runtime.
__init__() declares user=None as optional, yet all command methods (_run_shell_command(), _sftp_stat(), shell(), copy()) format the remote target as "{user}@{host}" without checking for None. If any caller uses the default, SSH/SCP/SFTP operations fail with None@host format errors. Either make user required or handle the host-only case when self.__user is unset.
🔧 Suggested fix: centralize target construction
+ def _remote_target(self) -> str:
+ return "{}@{}".format(self.__user, self.__host) if self.__user else self.__host
+
def _run_shell_command(self, command: str) -> bytes:
...
- args = ["{}@{}".format(self.__user, self.__host), quoted]
+ args = [self._remote_target(), quoted]
return self.__run_command(command="ssh", flags=" ".join(flags), args=" ".join(args))
def _sftp_stat(self, remote_path: str):
...
args = [
"-b",
"-",
- "{}@{}".format(self.__user, self.__host),
+ self._remote_target(),
]
...
def shell(self, command: str) -> bytes:
...
- args = ["{}@{}".format(self.__user, self.__host), command]
+ args = [self._remote_target(), command]
return self.__run_command(command="ssh", flags=" ".join(flags), args=" ".join(args))
def copy(self, local_path: str, remote_path: str):
...
- args = [local_path, "{}@{}:{}".format(self.__user, self.__host, remote_path)]
+ args = [local_path, "{}:{}".format(self._remote_target(), remote_path)]
self.__run_command(command="scp", flags=" ".join(flags), args=" ".join(args))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/ssh/sshcp.py` around lines 31 - 38, The constructor declares user
optional but callers build targets as "{user}@{host}", causing "None@host" when
unset; update either __init__ to require user (remove default None) or
(preferable) add a single helper like _remote_target(self) that returns
f"{self.__user}@{self.__host}" if self.__user else self.__host, then change all
call sites (_run_shell_command, _sftp_stat, shell, copy) to use that helper
instead of formatting "{user}@{host}" directly so host-only usage works
correctly.
Comment on lines
+86
to
+87
| fnull = open(os.devnull, "w") | ||
| subprocess.Popen(["rar", "a", "-ep", path, temp_file_path], stdout=fnull).communicate() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'open\(os\.devnull' src/python/tests/integration/test_controller/test_controller.pyRepository: nitrobass24/seedsync
Length of output: 284
Close the os.devnull file handle in create_archive.
Line 86 opens a file descriptor with open(os.devnull, "w") that is never explicitly closed. Since this runs during setUp(), repeated test execution can accumulate open descriptors and cause test flakiness or resource exhaustion.
♻️ Proposed fix
elif ext == "rar":
- fnull = open(os.devnull, "w")
- subprocess.Popen(["rar", "a", "-ep", path, temp_file_path], stdout=fnull).communicate()
+ with open(os.devnull, "w") as fnull:
+ proc = subprocess.Popen(["rar", "a", "-ep", path, temp_file_path], stdout=fnull)
+ proc.communicate()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/integration/test_controller/test_controller.py` around lines
86 - 87, The code in create_archive opens os.devnull into fnull but never closes
it; change the subprocess invocation in create_archive to ensure fnull is closed
(e.g., use a with open(os.devnull, "w") as fnull: block around
subprocess.Popen/communicate or explicitly call fnull.close() after
communicate()) so the file descriptor is released after the subprocess runs.
Comment on lines
+49
to
+64
| fnull = open(os.devnull, "w") | ||
| TestExtract.ar_rar = os.path.join(archive_dir, "file.rar") | ||
| subprocess.Popen(["rar", | ||
| "a", | ||
| "-ep", | ||
| TestExtract.ar_rar, | ||
| temp_file], | ||
| stdout=fnull) | ||
| subprocess.Popen(["rar", "a", "-ep", TestExtract.ar_rar, temp_file], stdout=fnull) | ||
|
|
||
| # rar split | ||
| subprocess.Popen(["rar", | ||
| "a", | ||
| "-ep", "-m0", "-v50k", | ||
| os.path.join(archive_dir, "file.split.rar"), | ||
| temp_file], | ||
| stdout=fnull) | ||
| subprocess.Popen( | ||
| ["rar", "a", "-ep", "-m0", "-v50k", os.path.join(archive_dir, "file.split.rar"), temp_file], stdout=fnull | ||
| ) | ||
| TestExtract.ar_rar_split_p1 = os.path.join(archive_dir, "file.split.part1.rar") | ||
| TestExtract.ar_rar_split_p2 = os.path.join(archive_dir, "file.split.part2.rar") | ||
|
|
||
| # tar.gz | ||
| TestExtract.ar_tar_gz = os.path.join(archive_dir, "file.tar.gz") | ||
| subprocess.Popen(["tar", | ||
| "czvf", | ||
| TestExtract.ar_tar_gz, | ||
| "-C", os.path.dirname(temp_file), | ||
| os.path.basename(temp_file)]) | ||
| subprocess.Popen( | ||
| ["tar", "czvf", TestExtract.ar_tar_gz, "-C", os.path.dirname(temp_file), os.path.basename(temp_file)] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="$(fd 'test_extract\.py$' | head -n1)"
echo "Inspecting: $TARGET"
echo "---- subprocess calls ----"
rg -n -C2 'subprocess\.(Popen|run)\(' "$TARGET"
echo "---- explicit waits/communicates ----"
rg -n -C2 '\.(wait|communicate)\(' "$TARGET" || trueRepository: nitrobass24/seedsync
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Get the file and examine full setUpClass method
TARGET="$(fd 'test_extract\.py$' | head -n1)"
echo "=== Full setUpClass method ==="
sed -n '40,100p' "$TARGET"
echo ""
echo "=== Check imports ==="
head -n 20 "$TARGET"
echo ""
echo "=== Full subprocess context in setUpClass ==="
sed -n '45,75p' "$TARGET"Repository: nitrobass24/seedsync
Length of output: 4202
🏁 Script executed:
#!/bin/bash
TARGET="$(fd 'test_extract\.py$' | head -n1)"
# Show a few test methods that use these archive files
echo "=== Sample test methods using archives ==="
sed -n '120,160p' "$TARGET"
echo ""
echo "=== Check for any waits before archive usage ==="
rg -n 'def test_' "$TARGET" | head -5Repository: nitrobass24/seedsync
Length of output: 2261
Wait for archive subprocesses and close os.devnull handle.
Lines 51, 54, and 62 spawn subprocesses with subprocess.Popen(...) without waiting for completion. Test methods immediately access the archive files (e.g., test_is_archive_rar() at line 128), creating a race condition where tests can fail if archives aren't fully created. Additionally, the file handle opened at line 49 is never closed.
Proposed fix
- fnull = open(os.devnull, "w")
- TestExtract.ar_rar = os.path.join(archive_dir, "file.rar")
- subprocess.Popen(["rar", "a", "-ep", TestExtract.ar_rar, temp_file], stdout=fnull)
-
- # rar split
- subprocess.Popen(
- ["rar", "a", "-ep", "-m0", "-v50k", os.path.join(archive_dir, "file.split.rar"), temp_file], stdout=fnull
- )
+ with open(os.devnull, "w") as fnull:
+ TestExtract.ar_rar = os.path.join(archive_dir, "file.rar")
+ subprocess.run(["rar", "a", "-ep", TestExtract.ar_rar, temp_file], stdout=fnull, stderr=fnull, check=True)
+
+ # rar split
+ subprocess.run(
+ ["rar", "a", "-ep", "-m0", "-v50k", os.path.join(archive_dir, "file.split.rar"), temp_file],
+ stdout=fnull,
+ stderr=fnull,
+ check=True,
+ )
@@
- subprocess.Popen(
- ["tar", "czvf", TestExtract.ar_tar_gz, "-C", os.path.dirname(temp_file), os.path.basename(temp_file)]
- )
+ subprocess.run(
+ ["tar", "czvf", TestExtract.ar_tar_gz, "-C", os.path.dirname(temp_file), os.path.basename(temp_file)],
+ check=True,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/integration/test_controller/test_extract/test_extract.py`
around lines 49 - 64, The subprocesses that create archives (invocations using
subprocess.Popen in the setup that set TestExtract.ar_rar,
TestExtract.ar_rar_split_p1/TestExtract.ar_rar_split_p2, and
TestExtract.ar_tar_gz) must be awaited and the os.devnull handle closed: replace
or wrap those subprocess.Popen calls with subprocess.run (or call .wait() on the
returned Popen) so the test setup blocks until the archive creation finishes,
check the process returncode and raise/log on non-zero to surface failures, and
ensure the opened fnull file descriptor is closed after use.
Comment on lines
+704
to
705
| while self.mock_extract_archive.call_count < 1 and self.listener.extract_completed.call_count < 1: | ||
| pass |
There was a problem hiding this comment.
Fix wait predicate to avoid flaky test exit.
Line 704 should use or, not and; current logic can exit before both call counts reach 1.
✅ Proposed fix
- while self.mock_extract_archive.call_count < 1 and self.listener.extract_completed.call_count < 1:
+ while self.mock_extract_archive.call_count < 1 or self.listener.extract_completed.call_count < 1:
pass📝 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
| while self.mock_extract_archive.call_count < 1 and self.listener.extract_completed.call_count < 1: | |
| pass | |
| while self.mock_extract_archive.call_count < 1 or self.listener.extract_completed.call_count < 1: | |
| pass |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/unittests/test_controller/test_extract/test_dispatch.py`
around lines 704 - 705, The loop waiting for callbacks uses an incorrect
boolean: change the while condition that currently reads "while
self.mock_extract_archive.call_count < 1 and
self.listener.extract_completed.call_count < 1:" to use "or" so it continues
until both self.mock_extract_archive.call_count and
self.listener.extract_completed.call_count have reached 1; update the predicate
in the test_dispatch waiting loop to "while ... < 1 or ... < 1" to prevent early
exit and flakiness.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 59-61: The CI Ruff check step ("Ruff check" in the workflow,
working-directory src/python) is not enforcing Bandit security rules because the
pyproject.toml `select` list omits "S"; open src/python/pyproject.toml and add
"S" to the `select` array (the same list that currently contains E,W,F,I,B,UP)
so Ruff will include security checks during the `ruff check` job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 120758b1-d9a6-4b4d-9db6-4bdd274ac683
📒 Files selected for processing (1)
.github/workflows/ci.yml
- Fix copy-paste error in controller_persist.py: error message referenced "AutoQueuePersist" instead of "ControllerPersist" - Remove debug print() statements from test_auto_queue.py - Fix ModelFile timestamp property return types: datetime -> datetime | None (fields are initialized to None but annotations said datetime) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/python/model/file.py (2)
33-33:⚠️ Potential issue | 🟡 MinorType hint mismatch:
pair_idparameter defaults toNonebut is typed asstr.The default value
Noneis incompatible with the declared typestr. Since the property getter at line 92 returnsstr | None, the parameter should match.Proposed fix
- def __init__(self, name: str, is_dir: bool, pair_id: str = None): + def __init__(self, name: str, is_dir: bool, pair_id: str | None = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/model/file.py` at line 33, The constructor parameter pair_id in __init__ is annotated as str but defaults to None, causing a type mismatch with the pair_id property (which returns str | None); change the annotation to Optional[str] (or str | None) for the pair_id parameter in __init__ so the default None is valid and consistent with the pair_id getter and internal storage.
266-268: 🧹 Nitpick | 🔵 TrivialInconsistent type hint style:
Optional["ModelFile"]should use union syntax.The
parentproperty still uses the legacyOptionalsyntax while all other optional types in this file have been modernized toX | None. This would also allow removing theOptionalimport at line 7.Proposed fix
`@property` - def parent(self) -> Optional["ModelFile"]: + def parent(self) -> "ModelFile" | None: return self.__parentAnd remove the unused import:
-from typing import Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/model/file.py` around lines 266 - 268, The parent property uses legacy Optional[...]; update its return annotation from Optional["ModelFile"] to the modern union syntax "ModelFile | None" (keeping the forward reference quotes if needed) and remove the now-unused Optional import from the top of the file; locate the property named parent on class ModelFile (returns self.__parent) and adjust the type hint and imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/tests/unittests/test_controller/test_auto_queue.py`:
- Line 329: Replace the verbose set construction
set([Controller.Command.Action.QUEUE] * 3) with the simpler literal singleton
set {Controller.Command.Action.QUEUE}; update occurrences that use the
repeated-list-to-set pattern (e.g., the assertion comparing {c.action for c in
commands}) to use the concise set literal to improve readability without
changing behavior.
---
Outside diff comments:
In `@src/python/model/file.py`:
- Line 33: The constructor parameter pair_id in __init__ is annotated as str but
defaults to None, causing a type mismatch with the pair_id property (which
returns str | None); change the annotation to Optional[str] (or str | None) for
the pair_id parameter in __init__ so the default None is valid and consistent
with the pair_id getter and internal storage.
- Around line 266-268: The parent property uses legacy Optional[...]; update its
return annotation from Optional["ModelFile"] to the modern union syntax
"ModelFile | None" (keeping the forward reference quotes if needed) and remove
the now-unused Optional import from the top of the file; locate the property
named parent on class ModelFile (returns self.__parent) and adjust the type hint
and imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6da79b68-fa9c-4e9c-865e-da5450835780
📒 Files selected for processing (3)
src/python/controller/controller_persist.pysrc/python/model/file.pysrc/python/tests/unittests/test_controller/test_auto_queue.py
| calls = self.controller.queue_command.call_args_list[-3:] | ||
| commands = [calls[i][0][0] for i in range(3)] | ||
| self.assertEqual(set([Controller.Command.Action.QUEUE]*3), {c.action for c in commands}) | ||
| self.assertEqual(set([Controller.Command.Action.QUEUE] * 3), {c.action for c in commands}) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style note (optional).
The pattern set([Controller.Command.Action.QUEUE] * 3) works correctly but could be simplified to {Controller.Command.Action.QUEUE} since multiplying a value creates identical elements that collapse to a singleton set.
This is purely stylistic and doesn't affect test correctness.
♻️ Simplified form (optional)
- self.assertEqual(set([Controller.Command.Action.QUEUE] * 3), {c.action for c in commands})
+ self.assertEqual({Controller.Command.Action.QUEUE}, {c.action for c in commands})Similar simplification could apply to lines 359, 908, 937, 989, 1029, 1241, 1280.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/unittests/test_controller/test_auto_queue.py` at line 329,
Replace the verbose set construction set([Controller.Command.Action.QUEUE] * 3)
with the simpler literal singleton set {Controller.Command.Action.QUEUE}; update
occurrences that use the repeated-list-to-set pattern (e.g., the assertion
comparing {c.action for c in commands}) to use the concise set literal to
improve readability without changing behavior.
5 tasks
3 tasks
5 tasks
nitrobass24
added a commit
that referenced
this pull request
Mar 18, 2026
scan_fs.py runs on the remote seedbox which may have Python 3.8+. The Ruff modernization (PR #288) introduced 3.10+ syntax (X | None, list[X]) that would cause SyntaxError on older Pythons. Add `from __future__ import annotations` to defer annotation evaluation, making the modern syntax valid on Python 3.8+. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
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
1 participant
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.
Summary
PR 1 of 2 for #251. Adds Ruff as the Python linter/formatter with auto-fixes applied and CI enforcement.
What Ruff does
Ruff replaces flake8, isort, black, and pyupgrade in a single Rust-based tool. This PR enables:
Changes
pyproject.tomlwith line-length=120, target Python 3.12Optional[X]→X | None,List→list, etc. (UP006/UP045/UP035)ruff formatexcept Exception, unused loop vars, E402 import ordering, F821 undefined forward referencepython-lintjob runsruff check+ruff format --check, gates publishDeferred to PR 2
.format()calls, large churnPer-file ignores
__init__.py— F401 (re-exports) and I001 (import order is load-order sensitive due to circular deps)tests/**— W291 (trailing whitespace in lftp output test data)Closes #251
Test plan
python-lintjob passes (ruff check + format check)python-testjob passes (480 tests)unit-testjob passes (287 Angular tests)ruff checkandruff format --checkpass locally🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features
Bug Fixes