Upgrade Pyright from basic to strict mode (closes #291)#316
Open
nitrobass24 wants to merge 2 commits intodevelopfrom
Open
Upgrade Pyright from basic to strict mode (closes #291)#316nitrobass24 wants to merge 2 commits intodevelopfrom
nitrobass24 wants to merge 2 commits intodevelopfrom
Conversation
Switch pyrightconfig.json to strict mode with 0 errors. Changes: - Add missing parameter type annotations across 30+ files - Add type arguments to generic classes (Queue, list, dict, Callable, etc.) - Convert namedtuple to NamedTuple with typed fields (job_status.py) - Add `from __future__ import annotations` where runtime subscript of multiprocessing.Queue would fail - Add type: ignore comments for intentional patterns: - reportPrivateUsage: cross-class access in controller - reportUnusedFunction: Bottle plugin decorators in security.py - reportUnnecessaryComparison: defensive None checks - Disable reportUnknown* family (cascading noise from untyped deps: bottle, pexpect, tblib) and reportMissingTypeStubs — to be re-enabled incrementally as type stubs are added - Disable reportUnusedImport (conflicts with __init__.py re-exports) Closes #291 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR comprehensively strengthens type annotations across the Python codebase to enable Pyright strict mode. Changes include adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/controller/extract/dispatch.py`:
- Around line 62-63: The __eq__ method on ExtractStatus blindly accesses
other.__dict__, which raises AttributeError for primitives or objects without
__dict__; add a type guard similar to ModelDiff.__eq__: check isinstance(other,
ExtractStatus) (or return NotImplemented when other is not an ExtractStatus) and
only then compare self.__dict__ == other.__dict__; this preserves correct
equality semantics and lets Python fallback to other comparisons when
appropriate.
In `@src/python/controller/validate/validate_process.py`:
- Around line 61-62: The __eq__ implementation on ValidateStatus should first
check that other is an instance of ValidateStatus and return NotImplemented for
non-matching types to follow Python's equality protocol; replace the direct
other.__dict__ access in ValidateStatus.__eq__ with an isinstance(other,
ValidateStatus) guard and only then compare the relevant attributes (e.g.,
self.__dict__ == other.__dict__) so comparing to ints/strings returns
NotImplemented instead of raising AttributeError.
In `@src/python/lftp/job_status.py`:
- Around line 85-86: The __eq__ implementation in LftpJobStatus should guard
against non-LftpJobStatus inputs to avoid AttributeError; update
LftpJobStatus.__eq__ to first check isinstance(other, LftpJobStatus) and if not,
return NotImplemented, otherwise compare the two objects (e.g., via
self.__dict__ == other.__dict__); apply the same pattern to other vulnerable
__eq__ implementations you saw (in file.py, validate_process.py, dispatch.py,
diff.py) to follow PEP 207.
In `@src/python/model/diff.py`:
- Around line 25-26: The ModelDiff.__eq__ method currently accesses
other.__dict__ directly which raises AttributeError for non-ModelDiff values;
update ModelDiff.__eq__ to first check isinstance(other, ModelDiff) (mirroring
the pattern used in AutoQueuePattern.__eq__) and return NotImplemented when the
other object is not a ModelDiff, otherwise compare the dicts as before.
In `@src/python/system/file.py`:
- Around line 29-30: The __eq__ implementation on SystemFile should guard
against non-SystemFile comparisons to avoid AttributeError; modify
SystemFile.__eq__ to first check the type (e.g., isinstance(other, SystemFile))
and if it is not the same type return NotImplemented (or False), otherwise
compare the internal state (self.__dict__) as before; reference the
SystemFile.__eq__ method and ensure the function returns NotImplemented for
unsupported types so Python can handle symmetric comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10a0b4e6-6f93-4bba-93f1-d31a6670eb8f
📒 Files selected for processing (32)
src/python/common/app_process.pysrc/python/common/config.pysrc/python/common/context.pysrc/python/common/multiprocessing_logger.pysrc/python/common/path_pairs_config.pysrc/python/common/types.pysrc/python/controller/auto_queue.pysrc/python/controller/controller.pysrc/python/controller/controller_persist.pysrc/python/controller/extract/dispatch.pysrc/python/controller/extract/extract.pysrc/python/controller/extract/extract_process.pysrc/python/controller/model_builder.pysrc/python/controller/notifier.pysrc/python/controller/validate/validate_process.pysrc/python/lftp/job_status.pysrc/python/lftp/lftp.pysrc/python/model/diff.pysrc/python/model/file.pysrc/python/pyrightconfig.jsonsrc/python/scan_fs.pysrc/python/seedsync.pysrc/python/ssh/sshcp.pysrc/python/system/file.pysrc/python/system/scanner.pysrc/python/web/handler/logs.pysrc/python/web/handler/path_pairs.pysrc/python/web/handler/stream_log.pysrc/python/web/security.pysrc/python/web/serialize/serialize_model.pysrc/python/web/web_app.pysrc/python/web/web_app_job.py
Comment on lines
+62
to
63
| def __eq__(self, other: object) -> bool: | ||
| return self.__dict__ == other.__dict__ |
There was a problem hiding this comment.
Missing type guard causes AttributeError for non-ExtractStatus comparisons.
Same issue as ModelDiff.__eq__—accessing other.__dict__ without a type check will fail for primitives or __slots__-based objects.
🛡️ Proposed fix
def __eq__(self, other: object) -> bool:
+ if not isinstance(other, ExtractStatus):
+ return NotImplemented
return self.__dict__ == other.__dict__📝 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 __eq__(self, other: object) -> bool: | |
| return self.__dict__ == other.__dict__ | |
| def __eq__(self, other: object) -> bool: | |
| if not isinstance(other, ExtractStatus): | |
| return NotImplemented | |
| return self.__dict__ == other.__dict__ |
🤖 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 62 - 63, The __eq__
method on ExtractStatus blindly accesses other.__dict__, which raises
AttributeError for primitives or objects without __dict__; add a type guard
similar to ModelDiff.__eq__: check isinstance(other, ExtractStatus) (or return
NotImplemented when other is not an ExtractStatus) and only then compare
self.__dict__ == other.__dict__; this preserves correct equality semantics and
lets Python fallback to other comparisons when appropriate.
Comment on lines
+61
to
62
| def __eq__(self, other: object) -> bool: | ||
| return self.__dict__ == other.__dict__ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/python/controller/validate/validate_process.py | head -100Repository: nitrobass24/seedsync
Length of output: 3368
ValidateStatus.__eq__ must return NotImplemented for non-matching types instead of raising.
Direct other.__dict__ dereference without type checking violates Python's equality contract. Comparing ValidateStatus objects with unrelated types (e.g., int, str) raises AttributeError instead of gracefully handling the type mismatch.
Proposed fix
def __eq__(self, other: object) -> bool:
- return self.__dict__ == other.__dict__
+ if not isinstance(other, ValidateStatus):
+ return NotImplemented
+ return self.__dict__ == other.__dict__📝 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 __eq__(self, other: object) -> bool: | |
| return self.__dict__ == other.__dict__ | |
| def __eq__(self, other: object) -> bool: | |
| if not isinstance(other, ValidateStatus): | |
| return NotImplemented | |
| return self.__dict__ == other.__dict__ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/controller/validate/validate_process.py` around lines 61 - 62, The
__eq__ implementation on ValidateStatus should first check that other is an
instance of ValidateStatus and return NotImplemented for non-matching types to
follow Python's equality protocol; replace the direct other.__dict__ access in
ValidateStatus.__eq__ with an isinstance(other, ValidateStatus) guard and only
then compare the relevant attributes (e.g., self.__dict__ == other.__dict__) so
comparing to ints/strings returns NotImplemented instead of raising
AttributeError.
Comment on lines
+85
to
86
| def __eq__(self, other: object) -> bool: | ||
| return self.__dict__ == other.__dict__ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether similar fragile equality implementations exist.
rg -nP --type=py -C2 'def __eq__\(self,\s*other:\s*object\)\s*->\s*bool:\n\s*return\s+self\.__dict__\s*==\s*other\.__dict__'Repository: nitrobass24/seedsync
Length of output: 46
🏁 Script executed:
cat -n src/python/lftp/job_status.py | sed -n '80,95p'Repository: nitrobass24/seedsync
Length of output: 545
🏁 Script executed:
# Also check the full context of the class to understand the structure
rg -n "class LftpJobStatus" src/python/lftp/job_status.py -A 20Repository: nitrobass24/seedsync
Length of output: 774
🏁 Script executed:
# Test whether AttributeError is actually raised when comparing with None or int
python3 << 'EOFPYTHON'
class TestEq:
def __eq__(self, other: object) -> bool:
return self.__dict__ == other.__dict__
obj = TestEq()
try:
result = obj == None
print(f"obj == None returned: {result}")
except AttributeError as e:
print(f"AttributeError raised: {e}")
try:
result = obj == 5
print(f"obj == 5 returned: {result}")
except AttributeError as e:
print(f"AttributeError raised: {e}")
try:
result = obj == "string"
print(f"obj == 'string' returned: {result}")
except AttributeError as e:
print(f"AttributeError raised: {e}")
EOFPYTHONRepository: nitrobass24/seedsync
Length of output: 260
🏁 Script executed:
# Search for other __eq__ implementations in the codebase
rg -n "def __eq__" src/python/ -B 2 -A 3Repository: nitrobass24/seedsync
Length of output: 3232
Guard __eq__ against non-LftpJobStatus inputs.
Line 86 raises AttributeError when other lacks __dict__ (e.g., None, int, str). Return NotImplemented for unsupported types instead, following PEP 207. Note: similar vulnerable patterns exist in src/python/system/file.py, src/python/controller/validate/validate_process.py, src/python/controller/extract/dispatch.py, and src/python/model/diff.py.
Proposed fix
def __eq__(self, other: object) -> bool:
- return self.__dict__ == other.__dict__
+ if not isinstance(other, LftpJobStatus):
+ return NotImplemented
+ return self.__dict__ == other.__dict__🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/lftp/job_status.py` around lines 85 - 86, The __eq__
implementation in LftpJobStatus should guard against non-LftpJobStatus inputs to
avoid AttributeError; update LftpJobStatus.__eq__ to first check
isinstance(other, LftpJobStatus) and if not, return NotImplemented, otherwise
compare the two objects (e.g., via self.__dict__ == other.__dict__); apply the
same pattern to other vulnerable __eq__ implementations you saw (in file.py,
validate_process.py, dispatch.py, diff.py) to follow PEP 207.
Comment on lines
+25
to
26
| def __eq__(self, other: object) -> bool: | ||
| return self.__dict__ == other.__dict__ |
There was a problem hiding this comment.
Missing type guard causes AttributeError for non-ModelDiff comparisons.
Accessing other.__dict__ without an isinstance check will raise AttributeError when comparing against primitives or objects using __slots__ (e.g., ModelDiff(...) == None or ModelDiff(...) == 5).
The correct pattern (already used in AutoQueuePattern.__eq__ in this PR) is to return NotImplemented for incompatible types:
🛡️ Proposed fix
def __eq__(self, other: object) -> bool:
+ if not isinstance(other, ModelDiff):
+ return NotImplemented
return self.__dict__ == other.__dict__🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/model/diff.py` around lines 25 - 26, The ModelDiff.__eq__ method
currently accesses other.__dict__ directly which raises AttributeError for
non-ModelDiff values; update ModelDiff.__eq__ to first check isinstance(other,
ModelDiff) (mirroring the pattern used in AutoQueuePattern.__eq__) and return
NotImplemented when the other object is not a ModelDiff, otherwise compare the
dicts as before.
Comment on lines
+29
to
30
| def __eq__(self, other: object) -> bool: | ||
| return self.__dict__ == other.__dict__ |
There was a problem hiding this comment.
__eq__ may raise AttributeError for non-SystemFile comparisons.
The type annotation other: object is correct, but the implementation accesses other.__dict__ without checking if other is a SystemFile. Comparing with incompatible types (e.g., None, int) will raise AttributeError instead of returning False.
Proposed fix
def __eq__(self, other: object) -> bool:
+ if not isinstance(other, SystemFile):
+ return NotImplemented
return self.__dict__ == other.__dict__📝 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 __eq__(self, other: object) -> bool: | |
| return self.__dict__ == other.__dict__ | |
| def __eq__(self, other: object) -> bool: | |
| if not isinstance(other, SystemFile): | |
| return NotImplemented | |
| return self.__dict__ == other.__dict__ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/system/file.py` around lines 29 - 30, The __eq__ implementation on
SystemFile should guard against non-SystemFile comparisons to avoid
AttributeError; modify SystemFile.__eq__ to first check the type (e.g.,
isinstance(other, SystemFile)) and if it is not the same type return
NotImplemented (or False), otherwise compare the internal state (self.__dict__)
as before; reference the SystemFile.__eq__ method and ensure the function
returns NotImplemented for unsupported types so Python can handle symmetric
comparisons.
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
reportUnknown*family rules — these produce cascading noise from untyped third-party dependencies (bottle, pexpect, tblib), not from project codereportMissingTypeStubsandreportUnusedImport(conflicts with__init__.pyre-export pattern)What strict mode catches that basic didn't
reportMissingParameterTypereportMissingTypeArgumentlist[str], notlist)reportPrivateUsage_protectedaccess (ignored where intentional)reportUnnecessaryComparisonreportIncompatibleMethodOverrideDeferred rules (to enable later with type stubs)
The
reportUnknown*family (900+ errors) cascades from three untyped deps. These will be re-enabled as stubs become available:bottle— no official type stubspexpect— no official type stubstblib— no official type stubsTest plan
pyright— 0 errors in strict moderuff check— all checks passedruff format --check— all formattedCloses #291
🤖 Generated with Claude Code
Summary by CodeRabbit