Make pyright mostly happy #140
Conversation
WalkthroughAdds extensive static typing and type-safety across modules, removes unused XML/update code, tightens error handling, reorganizes some imports, standardizes ignoring of side-effect return values, and extends CLI/update workflows (including a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant Manager
participant UpdateMgr
participant DeviceMgr
User->>CLI: invoke subcommand (e.g. update/list/install)
CLI->>Manager: call_func(function, args)
Manager->>UpdateMgr: resolve/version selection (latest/toltec/regex)
alt version requires device interaction
Manager->>DeviceMgr: connect (address, password)
DeviceMgr-->>Manager: device status (version, engine type)
UpdateMgr->>DeviceMgr: download/install update files
DeviceMgr-->>UpdateMgr: install result
else purely server-side list
UpdateMgr-->>Manager: list of versions
end
Manager-->>CLI: result/exit
CLI-->>User: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codexctl/__init__.py(12 hunks)codexctl/analysis.py(1 hunks)codexctl/device.py(17 hunks)codexctl/server.py(7 hunks)codexctl/sync.py(7 hunks)codexctl/updates.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- codexctl/analysis.py
- codexctl/init.py
- codexctl/server.py
- codexctl/device.py
🧰 Additional context used
🪛 Ruff (0.12.2)
codexctl/updates.py
109-111: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
113-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
codexctl/sync.py
58-58: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build for windows-latest
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for remarkable
- GitHub Check: Build for macos-latest
🔇 Additional comments (21)
codexctl/sync.py (12)
8-8: LGTM: Type import additions look good.The addition of
IO,Any, andcastfrom the typing module supports the comprehensive type annotation improvements throughout the file.
12-15: LGTM: Constructor type annotations are well implemented.The type annotations for parameters and the logger fallback pattern with casting are correctly implemented. The pyright ignore comment is appropriate here since the cast is necessary for the fallback behavior.
24-32: LGTM: Method signature improvements with proper None handling.The type annotations for the
__POSTmethod are comprehensive and the None check with default empty dict initialization is a good practice.
61-64: LGTM: Proper type checking and casting.The isinstance check followed by explicit casting is a good pattern for ensuring type safety while satisfying the type checker.
74-78: Consider handling the recursive call result.The recursive call result is being discarded with
_, but since the function modifiescurrentDocumentsin place, this is actually correct behavior.
88-90: LGTM: Consistent type checking pattern.The same isinstance check and exception raising pattern is consistently applied, which improves code reliability.
99-107: LGTM: Improved variable extraction and type safety.Storing the identifier in a variable before comparisons and using explicit type casting improves code readability and type safety.
117-119: LGTM: Consistent return type behavior.Returning an empty list instead of an empty dictionary when no folder ID is found is more consistent with the method's return type annotation.
128-135: LGTM: Consistent type checking and casting.The same defensive programming pattern with type checking and explicit casting is well applied here.
170-176: LGTM: Proper error handling and explicit return value discarding.The isinstance check for error responses and explicit discarding of the write return value with
_improves code clarity.
199-199: LGTM: Explicit discarding of setup call.Explicitly discarding the return value of the setup POST call with
_makes the intent clear.
257-270: LGTM: Improved condition logic and explicit return value discarding.Changing from comparison with empty dictionary to falsy check is more appropriate for the list return type. Explicit discarding of download method return values is good practice.
codexctl/updates.py (9)
10-10: LGTM: Type import addition.The addition of
castfrom typing supports the type annotation improvements throughout the file.
14-32: LGTM: Constructor type annotations and initialization.The constructor properly annotates the logger parameter and uses the same casting pattern for logger fallback as seen in sync.py. The version dictionary type annotations are comprehensive.
34-36: LGTM: Improved return type annotation.The return type is now properly specified as a tuple of dictionaries with appropriate type annotations.
50-50: LGTM: Safe environment variable access.Adding a fallback empty string for the APPDATA environment variable prevents potential None issues.
117-135: LGTM: Method signature and explicit None return.The explicit
Nonereturn type annotation and the list conversion for dictionary keys are good improvements for type safety.
168-173: LGTM: Enhanced parameter type flexibility.Allowing both string and Path objects for the download folder parameter improves the API's usability.
245-247: LGTM: Consistent parameter type improvement.The same Path object support is consistently applied to the private method, maintaining API consistency.
268-268: LGTM: Safe header parsing.Providing a default value of 0 when the content-length header is missing prevents potential type errors.
313-313: LGTM: More specific type annotation.The parameter type annotation is now more specific, improving type safety for the static method.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codexctl/sync.py (1)
2-3: Remove unused imports.The imports
currentframefrominspectandcurrencyfromlocaleare unused and should be removed to keep the codebase clean.Apply this diff to remove unused imports:
-from inspect import currentframe -from locale import currency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codexctl/sync.py(7 hunks)codexctl/updates.py(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
codexctl/updates.py (2)
codexctl/__init__.py (2)
Manager(29-332)__init__(34-43)codexctl/device.py (1)
DeviceManager(18-679)
🪛 Ruff (0.12.2)
codexctl/updates.py
89-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
codexctl/sync.py
2-2: inspect.currentframe imported but unused
Remove unused import: inspect.currentframe
(F401)
3-3: locale.currency imported but unused
Remove unused import: locale.currency
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build for macos-latest
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for windows-latest
- GitHub Check: Build for remarkable
🔇 Additional comments (9)
codexctl/updates.py (5)
14-32: LGTM! Excellent type annotations and initialization.The type annotations for the constructor and instance variables significantly improve code clarity and type safety. The logger initialization with
cast()appropriately handles the None case by falling back to the logging module.
34-82: Excellent type safety improvements.The method signature with proper return type annotation and the JSON content validation using
isinstance()checks significantly improve type safety and runtime error prevention.
111-127: Perfect exception chaining implementation!The exception handling now properly uses
from Nonefor Timeout exceptions andfrom errorfor generic exceptions, which preserves error context while avoiding noise from exception handling itself. This addresses the previous review feedback perfectly.
129-147: Good type safety and defensive programming improvements.The explicit
Nonereturn type annotation improves type safety, and converting dictionary keys to a list before sorting is good defensive programming that ensures compatibility across Python versions.
180-323: Excellent API flexibility and robustness improvements.The addition of
Pathobject support makes the API more flexible, robust content-length parsing with a default prevents errors, and explicitly ignoring return values of write operations clarifies intent. The type annotations throughout enhance code clarity.codexctl/sync.py (4)
14-24: Excellent type annotations and initialization pattern.The constructor type annotations and logger initialization pattern are consistent with the
UpdateManagerclass and improve type safety.
26-54: Excellent parameter handling and type annotations.The comprehensive type annotations and proper handling of the optional
dataparameter with a None check improve both type safety and prevent mutable default argument issues.
56-85: Perfect fix for mutable default argument issue!The change from a mutable list default to
Nonewith proper initialization inside the method perfectly addresses the previous review feedback and prevents the shared state issues that can occur with mutable defaults.
87-275: Comprehensive type safety and robustness improvements.The extensive type annotations, explicit type checking for server responses, and clear handling of return values throughout all methods significantly improve code robustness and maintainability. The casting operations and validation prevent runtime errors from unexpected server responses.
Co-authored-by: Jayy001 <56607897+Jayy001@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codexctl/__init__.py (1)
234-235: Verify version_lookup function typingThe function accepts
str | Nonebut immediately casts tostr, which could raise an exception ifNoneis passed.def version_lookup(version: str | None) -> re.Match[str] | None: + if version is None: + return None return re.search(r"\b\d+\.\d+\.\d+\.\d+\b", cast(str, version))
♻️ Duplicate comments (2)
codexctl/updates.py (1)
86-93: Apply consistent exception handling patternSimilar to the previous comment, use
TypeErrorfor type validation and maintain consistent exception chaining.codexctl/device.py (1)
93-93: Apply consistent logger fallback patternSimilar to the UpdateManager, use
logging.getLogger(__name__)instead of casting the module.- self.logger: logging.Logger = logger or cast(logging.Logger, logging) # pyright:ignore [reportInvalidCast] + self.logger: logging.Logger = logger or logging.getLogger(__name__)
🧹 Nitpick comments (2)
codexctl/updates.py (1)
69-76: Use proper exception types for type validationThe static analysis correctly identifies that
TypeErrorwould be more appropriate thanValueErrorfor type validation failures. Also, thefrom Noneexception chaining is correctly applied here.contents = json.load(f) # pyright:ignore [reportAny] if not isinstance(contents, dict): - raise ValueError() + raise TypeError("Expected dict from JSON") - except ValueError: + except (ValueError, TypeError) as e: raise SystemError( f"Version-ids.json @ {file_location} is corrupted! Please delete it and try again. Also, PLEASE open an issue on the repo showing the contents of the file." - ) from None + ) from ecodexctl/device.py (1)
474-475: Security consideration: Using os.system with shell commandsWhile the static analysis flags these as potential security issues, in this context (device management on controlled hardware), the risk is minimal. However, consider using
subprocess.runfor consistency with other parts of the codebase.Example for one instance:
- _ = os.system("chmod +x /tmp/restore.sh") - _ = os.system("/tmp/restore.sh") + _ = subprocess.run(["chmod", "+x", "/tmp/restore.sh"], check=False) + _ = subprocess.run(["/tmp/restore.sh"], check=False)Also applies to: 502-502, 608-608, 723-723
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codexctl/__init__.py(12 hunks)codexctl/device.py(17 hunks)codexctl/updates.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
codexctl/device.py (1)
tests/test.py (1)
write(33-34)
🪛 Ruff (0.12.2)
codexctl/device.py
474-474: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
474-474: Starting a process with a partial executable path
(S607)
475-475: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
475-475: Probable insecure usage of temporary file or directory: "/tmp/restore.sh"
(S108)
502-502: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
502-502: Starting a process with a partial executable path
(S607)
608-608: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
608-608: Starting a process with a partial executable path
(S607)
723-723: Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without shell
(S605)
723-723: Starting a process with a partial executable path
(S607)
codexctl/updates.py
71-71: Prefer TypeError exception for invalid type
(TRY004)
71-71: Abstract raise to an inner function
(TRY301)
74-76: Avoid specifying long messages outside the exception class
(TRY003)
88-88: Prefer TypeError exception for invalid type
(TRY004)
88-88: Abstract raise to an inner function
(TRY301)
91-93: Avoid specifying long messages outside the exception class
(TRY003)
116-116: Probable use of requests call without timeout
(S113)
122-124: Avoid specifying long messages outside the exception class
(TRY003)
127-129: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for remarkable
- GitHub Check: Build for windows-latest
- GitHub Check: Build for ubuntu-latest
🔇 Additional comments (5)
codexctl/updates.py (2)
271-271: Simplify file_length handlingGood defensive programming with the fallback to 0, but the code could be slightly cleaner.
290-297: Good practice: Explicitly discarding return valuesUsing
_to explicitly discard return values from write operations improves code clarity and shows intentional disregard of these values.codexctl/__init__.py (1)
286-287: Good use of explicit discard patternConsistently using
_for discarding return values fromshutil.moveoperations shows intentional handling.Also applies to: 324-325
codexctl/device.py (2)
428-455: Well-structured hardware-specific restore logicGood implementation of hardware-specific restore scripts for different device types. The use of f-strings and proper shell script structure is clean.
278-279: Improved return type with comprehensive device statusGood enhancement to return more device information including the version ID and old update engine status.
| hw = args.get("hardware") | ||
| remarkable_version = HardwareType.parse(hw) if hw else None | ||
| remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None |
There was a problem hiding this comment.
Type casting issue with HardwareType.parse
HardwareType.parse returns a HardwareType enum, not a string. The cast is incorrect.
hw = args.get("hardware")
- remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None
+ remarkable_version = HardwareType.parse(hw) if hw else None📝 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.
| hw = args.get("hardware") | |
| remarkable_version = HardwareType.parse(hw) if hw else None | |
| remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None | |
| hw = args.get("hardware") | |
| remarkable_version = HardwareType.parse(hw) if hw else None |
🤖 Prompt for AI Agents
In codexctl/__init__.py around lines 54-55, the code incorrectly casts the
result of HardwareType.parse to str; HardwareType.parse returns a HardwareType
enum. Replace the cast and assign the enum directly (remarkable_version =
HardwareType.parse(hw) if hw else None). If the downstream code actually
requires a string, explicitly convert the enum to the desired string form (e.g.,
.value or .name) instead of using cast.
There was a problem hiding this comment.
It's being used as a string later in the code.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the HardwareType.parse method signature and return type
ast-grep --pattern 'class HardwareType {
$$$
def parse($$$) {
$$$
}
$$$
}'
# Also search for HardwareType definition and parse method
rg -n "class HardwareType" --type=py
rg -n "def parse" --type=py -A 10 -B 2
Length of output: 831
🏁 Script executed:
#!/bin/bash
# Check what UpdateManager methods expect for hardware type parameter
ast-grep --pattern 'class UpdateManager {
$$$
def get_latest_version($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class UpdateManager {
$$$
def get_toltec_version($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class UpdateManager {
$$$
def download_version($$$) {
$$$
}
$$$
}'
# Also check the method signatures directly
rg -n "def get_latest_version\|def get_toltec_version\|def download_version" --type=py -A 3 -B 1
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for UpdateManager class and its methods more broadly
rg -n "class UpdateManager" --type=py -A 20
# Search for specific method definitions in updates.py
rg -n "get_latest_version\|get_toltec_version\|download_version" codexctl/updates.py -A 5 -B 2
Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Search specifically for the method definitions we need
fd "updates.py" --exec cat {} | grep -n -A 10 "def get_latest_version\|def get_toltec_version\|def download_version"
Length of output: 1321
🏁 Script executed:
#!/bin/bash
# Check how remarkable_version is used later in the code, particularly in device.py
rg -n "remarkable.hardware" codexctl/__init__.py -A 2 -B 2
Length of output: 751
@coderabbit-user Thank you for the clarification! You're absolutely right. After analyzing the code, I can see that the UpdateManager methods (get_latest_version, get_toltec_version, and download_version) all expect a HardwareType enum as their first parameter, not a string.
However, I notice there's an inconsistency in the code. The remarkable_version variable is used in two different contexts:
- For UpdateManager method calls (lines 64, 66, 85) - these expect
HardwareTypeenum - For comparisons (lines 73, 75, 77) - these compare against
HardwareTypeenum values
But I also see that later in the code (around line 205-217), there's a remarkable.hardware property that's used consistently as a HardwareType enum.
The cast to str appears to be incorrect. The variable should remain as HardwareType enum throughout. If there are places where a string representation is actually needed, those should use .value or .name on the enum instead of casting the enum to string.
Could you point me to the specific lines where remarkable_version is used as a string? I may have missed them in my analysis, and it would help determine the best approach for fixing this type inconsistency.
✏️ Learnings added
Learnt from: Eeems
PR: Jayy001/codexctl#140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.136Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| machine_contents = file.read().strip("\n") | ||
|
|
||
| self.hardware = HardwareType.parse(machine_contents) | ||
| self.hardware = HardwareType.parse(machine_contents) |
There was a problem hiding this comment.
Missing attribute assignment
The self.hardware attribute is only set in the non-remote path. This will cause an AttributeError when accessing self.hardware in remote mode.
with ftp.file("/sys/devices/soc0/machine") as file:
machine_contents = file.read().decode("utf-8").strip("\n")
+ self.hardware = HardwareType.parse(machine_contents)
else:
with open("/sys/devices/soc0/machine") as file:
machine_contents = file.read().strip("\n")
self.hardware = HardwareType.parse(machine_contents)📝 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.
| self.hardware = HardwareType.parse(machine_contents) | |
| if self.remote: | |
| with ftp.file("/sys/devices/soc0/machine") as file: | |
| machine_contents = file.read().decode("utf-8").strip("\n") | |
| self.hardware = HardwareType.parse(machine_contents) | |
| else: | |
| with open("/sys/devices/soc0/machine") as file: | |
| machine_contents = file.read().strip("\n") | |
| self.hardware = HardwareType.parse(machine_contents) |
🤖 Prompt for AI Agents
In codexctl/device.py around line 113, self.hardware is only assigned on the
non-remote branch which leads to AttributeError in remote mode; ensure
self.hardware is always defined by either parsing machine_contents in the remote
branch as well (e.g., self.hardware = HardwareType.parse(machine_contents)) or
by initializing self.hardware to a sensible default (e.g., None or
HardwareType.UNKNOWN) before the remote/non-remote conditional so subsequent
code can safely access the attribute.
| echo "new: ${NEWPART}" | ||
| echo "fallback: ${OLDPART}" | ||
| echo $NEWPART > /sys/devices/platform/lpgpr/root_part | ||
| /sbin/fw_setenv "active_partition" "${NEWPART}\" |
There was a problem hiding this comment.
Fix unterminated string in restore script
There's a syntax error in the shell script - missing closing quote on Line 454.
-/sbin/fw_setenv "active_partition" "${NEWPART}\"
+/sbin/fw_setenv "active_partition" "${NEWPART}"📝 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.
| /sbin/fw_setenv "active_partition" "${NEWPART}\" | |
| /sbin/fw_setenv "active_partition" "${NEWPART}" |
🤖 Prompt for AI Agents
In codexctl/device.py around line 454, the shell command string for fw_setenv is
missing a closing quote and has an extra backslash before the newline; fix the
unterminated string by ensuring the command is properly quoted and escaped
(e.g., close the double quote after ${NEWPART} and remove the stray backslash)
so the generated restore script emits a valid line like /sbin/fw_setenv
"active_partition" "${NEWPART}".
|
|
||
| if self.logger is None: | ||
| self.logger = logging | ||
| self.logger: logging.Logger = logger or cast(logging.Logger, logging) # pyright:ignore [reportInvalidCast] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a proper logger fallback pattern
The cast to logging.Logger is incorrect since logging is a module, not a logger instance. Consider using logging.getLogger(__name__) as a proper fallback.
- self.logger: logging.Logger = logger or cast(logging.Logger, logging) # pyright:ignore [reportInvalidCast]
+ self.logger: logging.Logger = logger or logging.getLogger(__name__)📝 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.
| self.logger: logging.Logger = logger or cast(logging.Logger, logging) # pyright:ignore [reportInvalidCast] | |
| self.logger: logging.Logger = logger or logging.getLogger(__name__) |
🤖 Prompt for AI Agents
In codexctl/updates.py around line 23, the current fallback uses an incorrect
cast of the logging module to logging.Logger; replace that with a proper
fallback by assigning self.logger = logger or logging.getLogger(__name__),
remove the invalid cast and the pyright ignore, and ensure the type of the
logger variable is annotated or inferred as logging.Logger so static checkers
accept it.
| contents = requests.get( # pyright:ignore [reportAny] | ||
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | ||
| ).json() | ||
| json.dump(contents, f, indent=4) |
There was a problem hiding this comment.
Add timeout to requests.get call
The static analysis correctly identifies a missing timeout on the HTTP request. This could cause the application to hang indefinitely if the server doesn't respond.
contents = requests.get( # pyright:ignore [reportAny]
- "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json"
+ "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json",
+ timeout=30
).json()📝 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.
| contents = requests.get( # pyright:ignore [reportAny] | |
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | |
| ).json() | |
| json.dump(contents, f, indent=4) | |
| contents = requests.get( # pyright:ignore [reportAny] | |
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json", | |
| timeout=30 | |
| ).json() | |
| json.dump(contents, f, indent=4) |
🧰 Tools
🪛 Ruff (0.12.2)
116-116: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In codexctl/updates.py around lines 116 to 119, the requests.get call lacks a
timeout which can cause indefinite hanging; update the call to include a
reasonable timeout (e.g., timeout=10) and handle network errors by catching
requests.exceptions.RequestException so you can log or propagate a clear error
instead of blocking indefinitely.
|
It would probably be worth implementing the out of range comment |
@Jayy001 I've gone through with a LSP and addressed as much of the warnings/errors it was raising about incorrect/unknown types as possible. This found a couple method signatures that were incorrect, several places where
Nonecould be returned that were not being handled properly, and other issues. There are still some warnings/errors that probably should be addressed, but I'm done for now. I have not done any testing of this, so I would highly recommend doing so as part of reviewing the changes.I do think that some work needs to be done to expand the automated testing to cover more of the possible functions, as well as better test the various branching paths, as there are various open issues about codepaths that have just never worked, so they were never tested.
Summary by CodeRabbit
New Features
Refactor
Style
Documentation