Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends the codexctl tool to support a new ReMarkable hardware variant (RMPPURE) by updating the hardware type enumeration, integrating version data, refactoring the bootloader downgrade flow with improved validation, and normalizing code formatting across multiple modules. Dependency versions are bumped to support the new device family. ChangesRMPPURE Hardware Support
Dependency Updates and Code Formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
codexctl/analysis.py (1)
23-32: ⚡ Quick winSet
stacklevel=2on these warnings.Without it, the warning points inside
get_update_image()instead of the caller, which makes these validation failures harder to trace.♻️ Suggested fix
- warnings.warn("Signature doesn't match contents", RuntimeWarning) + warnings.warn("Signature doesn't match contents", RuntimeWarning, stacklevel=2) ... - warnings.warn("Public key missing", RuntimeWarning) + warnings.warn("Public key missing", RuntimeWarning, stacklevel=2) ... - warnings.warn("Unable to open public key", RuntimeWarning) + warnings.warn("Unable to open public key", RuntimeWarning, stacklevel=2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codexctl/analysis.py` around lines 23 - 32, The warnings in the exception handlers for UpdateImageSignatureException, FileNotFoundError, and the OSError branch should include stacklevel=2 so the warning points at the caller of get_update_image() instead of inside it; update the three warnings.warn(...) calls inside the except blocks (the one for UpdateImageSignatureException, the one for FileNotFoundError, and the one in the OSError errno==ENOTDIR branch) to pass stacklevel=2 as an argument while keeping the existing messages and behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@codexctl/__init__.py`:
- Around line 81-100: The code builds version table labels using
remarkable_version.formatted_name which is None when no --hardware is passed,
causing an AttributeError; update the label construction in the version_blocks
appends to use the explicit HardwareType enum members (e.g.,
HardwareType.RMPPURE.formatted_name, HardwareType.RMPP.formatted_name,
HardwareType.RMPPM.formatted_name, HardwareType.RM2.formatted_name,
HardwareType.RM1.formatted_name) instead of remarkable_version.formatted_name,
keeping the same version variables (remarkable_ppure_versions,
remarkable_pp_versions, remarkable_ppm_versions, remarkable_2_versions,
remarkable_1_versions) and preserving the existing conditional checks that use
remarkable_version to filter when a specific device is selected.
In `@codexctl/device.py`:
- Around line 266-286: The branch in connect_to_device that handles credentialed
SSH (the if authentication: block using client.connect) currently catches
paramiko.ssh_exception.AuthenticationException and only prints an error,
allowing execution to continue to the "Success: Connected to device" and return
client; change the except block to fail fast by either re-raising the caught
AuthenticationException (use raise or raise from to preserve context) or by
returning None/raising a custom exception after logging so callers never reach
the success path; update the except handling for
paramiko.ssh_exception.AuthenticationException in connect_to_device (the block
around client.connect) to log the error and then raise.
- Around line 263-264: Replace the insecure AutoAddPolicy usage in
DeviceManager.connect_to_device by using a safe host-key policy: load known
hosts (client.load_system_host_keys() and/or
client.load_host_keys(<repo-known_hosts>)) and set a RejectPolicy or implement a
TOFU check instead of
client.set_missing_host_key_policy(paramiko.AutoAddPolicy()); ensure the code
validates the server key against known keys and fails explicitly on mismatch.
Also fix the authentication branch: when catching
paramiko.ssh_exception.AuthenticationException in connect_to_device do not print
and continue—return an error or raise the exception (or return None) and do not
print a misleading "success" or return the SSH client on auth failure so callers
won’t operate on an unauthenticated/invalid client.
In `@codexctl/sync.py`:
- Around line 23-43: The __POST method in RmWebInterfaceAPI is masking real
failures and can hang because it calls requests.post without a timeout and
catches all Exceptions returning None; change __POST to pass a reasonable
timeout (e.g. timeout=10) to requests.post and stop swallowing errors—catch only
requests.exceptions.RequestException to log error details via self.logger.error
(include the exception and endpoint) and re-raise or return a clear error object
instead of None; also fix __get_documents_recursive's mutable default by
changing currentDocuments=[] to currentDocuments=None and initializing
currentDocuments = [] inside the function to avoid cross-call state leakage.
- Around line 45-66: The method __get_documents_recursive uses a mutable default
argument currentDocuments=[], which causes document lists to persist across
top-level calls; change the signature to accept currentDocuments=None and inside
the function set currentDocuments = [] when None, keeping all existing logic
intact (references: __get_documents_recursive, __get_docs which calls it without
currentDocuments). Ensure callers remain unchanged and return the newly created
list so repeated sync() calls on the same RmWebInterfaceAPI instance do not
accumulate stale documents.
In `@codexctl/updates.py`:
- Around line 119-121: The three outbound requests in update_version_ids,
get_toltec_version, and __download_version_file must include bounded timeouts:
add a timeout argument to the requests.get call in update_version_ids (the call
returning .json()), to the requests.get in get_toltec_version, and to the
streaming requests.get in __download_version_file; use a sensible connect/read
timeout (e.g. timeout=(5, 30)) for the streaming call and a shorter single or
tuple timeout for simple JSON/HTML fetches, and keep the existing exception
handling by allowing requests.exceptions.Timeout to be raised/caught as before.
Ensure you apply the timeout parameter to the exact calls referenced by the
function names update_version_ids, get_toltec_version, and
__download_version_file.
- Around line 116-123: The current code opens location for writing before
fetching, which can corrupt the cache on network/parse failure; change the flow
in the function containing the "with open(location, "w", newline="\n") as f:"
block so you first perform the HTTP GET (requests.get(...)) and call .json() to
validate the payload, then write the validated JSON to a temporary file (use
tempfile.NamedTemporaryFile(delete=False) or tempfile.mkstemp), flush and close
it, and finally atomically replace the original file with os.replace(temp_path,
location); keep the existing logger.debug("Downloading version-ids.json") and
json.dump(contents, f, indent=4) logic but move them to operate on the temp file
and ensure exceptions from requests or json parsing are raised before any
replace occurs.
In `@requirements.txt`:
- Around line 3-4: requirements.txt updated remarkable-update-image to 1.4.1
(non-linux) and remarkable-update-fuse to 1.4.2 (linux), but pyproject.toml
still pins both to 1.3; open pyproject.toml and update the dependency entries
for remarkable-update-image and remarkable-update-fuse to match the versions and
platform markers used in requirements.txt, then regenerate the lockfile
(poetry.lock or pip-tools output) so the lockfile reflects these new versions.
---
Nitpick comments:
In `@codexctl/analysis.py`:
- Around line 23-32: The warnings in the exception handlers for
UpdateImageSignatureException, FileNotFoundError, and the OSError branch should
include stacklevel=2 so the warning points at the caller of get_update_image()
instead of inside it; update the three warnings.warn(...) calls inside the
except blocks (the one for UpdateImageSignatureException, the one for
FileNotFoundError, and the one in the OSError errno==ENOTDIR branch) to pass
stacklevel=2 as an argument while keeping the existing messages and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4625349c-bc8a-4c87-8418-ec4134354724
📒 Files selected for processing (10)
codexctl/__init__.pycodexctl/__main__.pycodexctl/analysis.pycodexctl/device.pycodexctl/server.pycodexctl/sync.pycodexctl/updates.pydata/version-ids.jsonrequirements.txttests/test.py
Comment on lines
+266
to
+286
| if authentication: | ||
| self.logger.debug(f"Using authentication: {authentication}") | ||
| try: | ||
| if os.path.isfile(authentication): | ||
| self.logger.debug( | ||
| f"Attempting to connect to {remote_address} with key file {authentication}" | ||
| ) | ||
| client.connect( | ||
| remote_address, username="root", key_filename=authentication | ||
| ) | ||
| else: | ||
| self.logger.debug( | ||
| f"Attempting to connect to {remote_address} with password {authentication}" | ||
| ) | ||
| client.connect( | ||
| remote_address, username="root", password=authentication | ||
| ) | ||
|
|
||
| except paramiko.ssh_exception.AuthenticationException: | ||
| print("Incorrect password or ssh path given in arguments!") | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections with line numbers
python3 - <<'PY'
import itertools,sys,os
path="codexctl/device.py"
start=240; end=340
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}: {line.rstrip()}")
if i>end:
break
PY
# Also locate success log/return and SFTP/exec_command occurrences
rg -n "Success: Connected|open_sftp|exec_command|client\.connect|AuthenticationException|Incorrect password" codexctl/device.pyRepository: Jayy001/codexctl
Length of output: 7062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
path="codexctl/device.py"
start=240; end=340
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}: {line.rstrip()}")
if i>end:
break
PY
rg -n "Success: Connected|open_sftp|exec_command|client\.connect|AuthenticationException|Incorrect password" codexctl/device.pyRepository: Jayy001/codexctl
Length of output: 7062
Fail fast when credentialed SSH login is rejected.
In connect_to_device(), the if authentication: branch catches paramiko.ssh_exception.AuthenticationException and only prints, but then still executes print("Success: Connected to device") and return client (lines 320-322). Re-raise (or otherwise abort) inside the except so callers never reach the success path when client.connect() fails.
Suggested fix
- except paramiko.ssh_exception.AuthenticationException:
- print("Incorrect password or ssh path given in arguments!")
+ except paramiko.ssh_exception.AuthenticationException as error:
+ raise SystemExit(
+ "Incorrect password or ssh path given in arguments!"
+ ) from error📝 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
| if authentication: | |
| self.logger.debug(f"Using authentication: {authentication}") | |
| try: | |
| if os.path.isfile(authentication): | |
| self.logger.debug( | |
| f"Attempting to connect to {remote_address} with key file {authentication}" | |
| ) | |
| client.connect( | |
| remote_address, username="root", key_filename=authentication | |
| ) | |
| else: | |
| self.logger.debug( | |
| f"Attempting to connect to {remote_address} with password {authentication}" | |
| ) | |
| client.connect( | |
| remote_address, username="root", password=authentication | |
| ) | |
| except paramiko.ssh_exception.AuthenticationException: | |
| print("Incorrect password or ssh path given in arguments!") | |
| if authentication: | |
| self.logger.debug(f"Using authentication: {authentication}") | |
| try: | |
| if os.path.isfile(authentication): | |
| self.logger.debug( | |
| f"Attempting to connect to {remote_address} with key file {authentication}" | |
| ) | |
| client.connect( | |
| remote_address, username="root", key_filename=authentication | |
| ) | |
| else: | |
| self.logger.debug( | |
| f"Attempting to connect to {remote_address} with password {authentication}" | |
| ) | |
| client.connect( | |
| remote_address, username="root", password=authentication | |
| ) | |
| except paramiko.ssh_exception.AuthenticationException as error: | |
| raise SystemExit( | |
| "Incorrect password or ssh path given in arguments!" | |
| ) from error |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codexctl/device.py` around lines 266 - 286, The branch in connect_to_device
that handles credentialed SSH (the if authentication: block using
client.connect) currently catches paramiko.ssh_exception.AuthenticationException
and only prints an error, allowing execution to continue to the "Success:
Connected to device" and return client; change the except block to fail fast by
either re-raising the caught AuthenticationException (use raise or raise from to
preserve context) or by returning None/raising a custom exception after logging
so callers never reach the success path; update the except handling for
paramiko.ssh_exception.AuthenticationException in connect_to_device (the block
around client.connect) to log the error and then raise.
Comment on lines
+23
to
+43
| def __POST(self, endpoint, data={}, fileUpload=False): | ||
| try: | ||
| logging.debug( | ||
| f"Sending POST request to {self.BASE + endpoint} with data {data}" | ||
| ) | ||
|
|
||
| if fileUpload: | ||
| result = requests.post(self.BASE + endpoint, files=data) | ||
| else: | ||
| result = requests.post(self.BASE + endpoint, data=data) | ||
|
|
||
| if result.status_code == 408: | ||
| self.logger.error("Request timed out!") | ||
|
|
||
| logging.debug(f"Result headers: {result.headers}") | ||
| if "application/json" in result.headers["Content-Type"]: | ||
| return result.json() | ||
| return result.content | ||
|
|
||
| except Exception: | ||
| return None |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
fd -t f "sync.py" -a . | head
sed -n '1,200p' codexctl/sync.py | nl -ba | sed -n '1,200p'
# show around lines 1-120 with line numbers
sed -n '1,160p' codexctl/sync.py | nl -ba
# find definition of __get_documents_recursive
rg -n "__get_documents_recursive" -S codexctl/sync.py
# print around it
start=$(rg -n "__get_documents_recursive" -S codexctl/sync.py | cut -d: -f1 | head -n1)
echo "start=$start"
sed -n "$((start-20)),$((start+120))p" codexctl/sync.py | nl -ba
# extract __POST definition
rg -n "def __POST" -S codexctl/sync.py
postline=$(rg -n "def __POST" -S codexctl/sync.py | head -n1 | cut -d: -f1)
sed -n "$((postline-20)),$((postline+80))p" codexctl/sync.py | nl -baRepository: Jayy001/codexctl
Length of output: 1299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' codexctl/sync.py | nl -ba
echo "----"
rg -n "__get_documents_recursive" -S codexctl/sync.py
echo "----"
start=$(rg -n "__get_documents_recursive" -S codexctl/sync.py | head -n1 | cut -d: -f1)
sed -n "$((start-30)),$((start+140))p" codexctl/sync.py | nl -ba
echo "----"
rg -n "def __POST" -S codexctl/sync.py
echo "----"
postline=$(rg -n "def __POST" -S codexctl/sync.py | head -n1 | cut -d: -f1)
sed -n "$((postline-30)),$((postline+90))p" codexctl/sync.py | nl -baRepository: Jayy001/codexctl
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Basic file existence
test -f codexctl/sync.py && echo "Found codexctl/sync.py" || (echo "Missing"; exit 1)
echo "=== File head (first 220 lines) ==="
sed -n '1,220p' codexctl/sync.py | nl -ba
echo "=== Locate __get_documents_recursive ==="
rg -n "__get_documents_recursive" -S codexctl/sync.py || true
start=$(rg -n "__get_documents_recursive" -S codexctl/sync.py | head -n1 | cut -d: -f1 || true)
if [[ -n "${start:-}" ]]; then
echo "=== Around __get_documents_recursive (start=$start) ==="
sed -n "$((start-30)),$((start+180))p" codexctl/sync.py | nl -ba
fi
echo "=== Locate __POST ==="
rg -n "def __POST" -S codexctl/sync.py || true
postline=$(rg -n "def __POST" -S codexctl/sync.py | head -n1 | cut -d: -f1 || true)
if [[ -n "${postline:-}" ]]; then
echo "=== Around __POST (postline=$postline) ==="
sed -n "$((postline-30)),$((postline+120))p" codexctl/sync.py | nl -ba
fiRepository: Jayy001/codexctl
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test -f codexctl/sync.py && echo "Found codexctl/sync.py" || (echo "Missing"; exit 1)
echo "=== File head (first 220 lines) ==="
sed -n '1,220p' codexctl/sync.py | nl -ba
echo "=== Locate __get_documents_recursive ==="
rg -n "__get_documents_recursive" -S codexctl/sync.py || true
start=$(rg -n "__get_documents_recursive" -S codexctl/sync.py | head -n1 | cut -d: -f1 || true)
if [[ -n "${start:-}" ]]; then
echo "=== Around __get_documents_recursive (start=$start) ==="
sed -n "$((start-30)),$((start+180))p" codexctl/sync.py | nl -ba
fi
echo "=== Locate __POST ==="
rg -n "def __POST" -S codexctl/sync.py || true
postline=$(rg -n "def __POST" -S codexctl/sync.py | head -n1 | cut -d: -f1 || true)
if [[ -n "${postline:-}" ]]; then
echo "=== Around __POST (postline=$postline) ==="
sed -n "$((postline-30)),$((postline+120))p" codexctl/sync.py | nl -ba
fiRepository: Jayy001/codexctl
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="codexctl/sync.py"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "=== __POST context ==="
postline=$(rg -n "def __POST" -S "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${postline:-}" ]]; then
start=$((postline-25)); end=$((postline+90))
sed -n "${start},${end}p" "$FILE" | awk '{printf "%5d %s\n", NR+'$start'-1, $0}'
else
echo "No __POST found"
fi
echo "=== __get_documents_recursive context ==="
gline=$(rg -n "__get_documents_recursive" -S "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${gline:-}" ]]; then
start=$((gline-25)); end=$((gline+180))
sed -n "${start},${end}p" "$FILE" | awk '{printf "%5d %s\n", NR+'$start'-1, $0}'
else
echo "No __get_documents_recursive found"
fi
echo "=== Any requests.post occurrences in sync.py (with nearby lines) ==="
rg -n "requests\.post" -S "$FILE" || trueRepository: Jayy001/codexctl
Length of output: 1924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="codexctl/sync.py"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "== rg locations =="
rg -n "def __POST" -S "$FILE" || true
rg -n "__get_documents_recursive" -S "$FILE" || true
echo "== context __POST =="
postline=$(rg -n "def __POST" -S "$FILE" | head -n1 | cut -d: -f1 || true)
echo "postline=$postline"
if [[ -n "${postline:-}" ]]; then
start=$((postline-25))
if (( start < 1 )); then start=1; fi
end=$((postline+90))
sed -n "${start},${end}p" "$FILE" | head -n 200
fi
echo "== context __get_documents_recursive =="
gline=$(rg -n "__get_documents_recursive" -S "$FILE" | head -n1 | cut -d: -f1 || true)
echo "gline=$gline"
if [[ -n "${gline:-}" ]]; then
start=$((gline-25))
if (( start < 1 )); then start=1; fi
end=$((gline+200))
sed -n "${start},${end}p" "$FILE" | head -n 250
fi
echo "== any requests.post occurrences =="
rg -n "requests\.post" -S "$FILE" || trueRepository: Jayy001/codexctl
Length of output: 11643
🏁 Script executed:
python3 - <<'PY'
import requests
print("has requests.RequestException:", hasattr(requests, "RequestException"))
if hasattr(requests, "RequestException"):
print("requests.RequestException =", requests.RequestException)
print("has requests.exceptions.RequestException:", hasattr(requests.exceptions, "RequestException"))
print("requests.exceptions.RequestException =", requests.exceptions.RequestException)
PYRepository: Jayy001/codexctl
Length of output: 304
Add a request timeout and stop masking failures in RmWebInterfaceAPI.__POST
__POST(self, endpoint, data={}, fileUpload=False)callsrequests.post(...)without a timeout and catchesException, returningNoneon real request failures (making diagnosis hard and risking indefinite hangs on dead devices).__get_documents_recursive(..., currentDocuments=[])uses a mutable default list, so document lists can leak across calls.
🔧 Possible fix
- def __POST(self, endpoint, data={}, fileUpload=False):
+ def __POST(self, endpoint, data=None, fileUpload=False, timeout=30):
+ if data is None:
+ data = {}
try:
@@
- result = requests.post(self.BASE + endpoint, files=data)
+ result = requests.post(self.BASE + endpoint, files=data, timeout=timeout)
else:
- result = requests.post(self.BASE + endpoint, data=data)
+ result = requests.post(self.BASE + endpoint, data=data, timeout=timeout)
@@
- except Exception:
+ except requests.RequestException:
return None🧰 Tools
🪛 ast-grep (0.43.0)
[info] 29-29: no timeout was given on call to external resource
Context: requests.post(self.BASE + endpoint, files=data)
Note: [CWE-1088].
(requests-timeout)
[info] 31-31: no timeout was given on call to external resource
Context: requests.post(self.BASE + endpoint, data=data)
Note: [CWE-1088].
(requests-timeout)
🪛 Ruff (0.15.15)
[warning] 23-23: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[error] 30-30: Probable use of requests call without timeout
(S113)
[error] 32-32: Probable use of requests call without timeout
(S113)
[warning] 42-42: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codexctl/sync.py` around lines 23 - 43, The __POST method in
RmWebInterfaceAPI is masking real failures and can hang because it calls
requests.post without a timeout and catches all Exceptions returning None;
change __POST to pass a reasonable timeout (e.g. timeout=10) to requests.post
and stop swallowing errors—catch only requests.exceptions.RequestException to
log error details via self.logger.error (include the exception and endpoint) and
re-raise or return a clear error object instead of None; also fix
__get_documents_recursive's mutable default by changing currentDocuments=[] to
currentDocuments=None and initializing currentDocuments = [] inside the function
to avoid cross-call state leakage.
Source: Linters/SAST tools
Comment on lines
+116
to
+123
| with open(location, "w", newline="\n") as f: | ||
| try: | ||
| self.logger.debug("Downloading version-ids.json") | ||
| contents = requests.get( | ||
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | ||
| ).json() | ||
| json.dump(contents, f, indent=4) | ||
| f.write("\n") |
Contributor
There was a problem hiding this comment.
Refresh version-ids.json atomically.
This opens location for write before the download succeeds, so a timeout/HTTP error/JSON parse failure destroys the last known-good cache and leaves the user with a corrupted file on the next run. Fetch and validate first, write to a temporary file, then os.replace() it into place.
Suggested fix
- with open(location, "w", newline="\n") as f:
- try:
- self.logger.debug("Downloading version-ids.json")
- contents = requests.get(
- "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json"
- ).json()
- json.dump(contents, f, indent=4)
- f.write("\n")
- except requests.exceptions.Timeout:
- raise SystemExit(
- "Connection timed out while downloading version-ids.json! Do you have an internet connection?"
- )
- except Exception as error:
- raise SystemExit(
- f"Unknown error while downloading version-ids.json! {error}"
- )
+ try:
+ self.logger.debug("Downloading version-ids.json")
+ contents = requests.get(
+ "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json"
+ ).json()
+ except requests.exceptions.Timeout:
+ raise SystemExit(
+ "Connection timed out while downloading version-ids.json! Do you have an internet connection?"
+ )
+ except Exception as error:
+ raise SystemExit(
+ f"Unknown error while downloading version-ids.json! {error}"
+ )
+
+ tmp_location = f"{location}.tmp"
+ with open(tmp_location, "w", newline="\n") as f:
+ json.dump(contents, f, indent=4)
+ f.write("\n")
+ os.replace(tmp_location, location)📝 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
| with open(location, "w", newline="\n") as f: | |
| try: | |
| self.logger.debug("Downloading version-ids.json") | |
| contents = requests.get( | |
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | |
| ).json() | |
| json.dump(contents, f, indent=4) | |
| f.write("\n") | |
| try: | |
| self.logger.debug("Downloading version-ids.json") | |
| contents = requests.get( | |
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | |
| ).json() | |
| except requests.exceptions.Timeout: | |
| raise SystemExit( | |
| "Connection timed out while downloading version-ids.json! Do you have an internet connection?" | |
| ) | |
| except Exception as error: | |
| raise SystemExit( | |
| f"Unknown error while downloading version-ids.json! {error}" | |
| ) | |
| tmp_location = f"{location}.tmp" | |
| with open(tmp_location, "w", newline="\n") as f: | |
| json.dump(contents, f, indent=4) | |
| f.write("\n") | |
| os.replace(tmp_location, location) |
🧰 Tools
🪛 ast-grep (0.43.0)
[info] 118-120: no timeout was given on call to external resource
Context: requests.get(
"https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json"
)
Note: [CWE-1088].
(requests-timeout)
🪛 Ruff (0.15.15)
[error] 119-119: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codexctl/updates.py` around lines 116 - 123, The current code opens location
for writing before fetching, which can corrupt the cache on network/parse
failure; change the flow in the function containing the "with open(location,
"w", newline="\n") as f:" block so you first perform the HTTP GET
(requests.get(...)) and call .json() to validate the payload, then write the
validated JSON to a temporary file (use
tempfile.NamedTemporaryFile(delete=False) or tempfile.mkstemp), flush and close
it, and finally atomically replace the original file with os.replace(temp_path,
location); keep the existing logger.debug("Downloading version-ids.json") and
json.dump(contents, f, indent=4) logic but move them to operate on the temp file
and ensure exceptions from requests or json parsing are raised before any
replace occurs.
Comment on lines
+119
to
+121
| contents = requests.get( | ||
| "https://raw.githubusercontent.com/Jayy001/codexctl/main/data/version-ids.json" | ||
| ).json() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections around the referenced lines
sed -n '90,150p' codexctl/updates.py | nl -ba | sed -n '1,120p'
echo "----"
sed -n '150,220p' codexctl/updates.py | nl -ba | sed -n '1,120p'
echo "----"
sed -n '320,380p' codexctl/updates.py | nl -ba | sed -n '1,120p'
# Find all outbound HTTP calls and whether a timeout is specified
echo "==== requests.* usage ===="
rg -n --hidden --no-ignore -S "requests\.(get|post|put|delete|head)\(" codexctl/updates.py || true
echo "==== timeout= usage ===="
rg -n --hidden --no-ignore -S "timeout\s*=" codexctl/updates.py || true
# Look for a shared session or helper applying timeouts
echo "==== requests.Session usage ===="
rg -n --hidden --no-ignore -S "requests\.Session\(" codexctl/updates.py || true
echo "==== any 'timeout' keyword near HTTP calls ===="
rg -n --hidden --no-ignore -S "requests\.(get|post|put|delete|head)\([^\\n]*timeout" codexctl/updates.py || trueRepository: Jayy001/codexctl
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print relevant sections with line numbers (without nl)
echo "==== codexctl/updates.py:90-150 ===="
sed -n '90,150p' codexctl/updates.py | awk '{print NR+89 "\t" $0}'
echo "----"
echo "==== codexctl/updates.py:150-220 ===="
sed -n '150,220p' codexctl/updates.py | awk '{print NR+149 "\t" $0}'
echo "----"
echo "==== codexctl/updates.py:320-380 ===="
sed -n '320,380p' codexctl/updates.py | awk '{print NR+319 "\t" $0}'
# Find all outbound HTTP calls and whether a timeout is specified
echo "==== requests.* usage (calls) ===="
rg -n --hidden --no-ignore -S "requests\.(get|post|put|delete|head)\(" codexctl/updates.py || true
echo "==== timeout= usage ===="
rg -n --hidden --no-ignore -S "timeout\s*=" codexctl/updates.py || true
echo "==== requests.Session usage ===="
rg -n --hidden --no-ignore -S "requests\.Session\(" codexctl/updates.py || true
echo "==== any 'timeout' keyword near HTTP calls ===="
rg -n --hidden --no-ignore -S "requests\.(get|post|put|delete|head)\([^\n]*timeout" codexctl/updates.py || trueRepository: Jayy001/codexctl
Length of output: 8078
Add bounded timeouts to all outbound HTTP calls in codexctl/updates.py.
update_version_ids(requests.get(.../version-ids.json).json()at 119-121) has no timeout (yet it only catchesrequests.exceptions.Timeout).get_toltec_version(requests.get("https://toltec-dev.org/stable/Compatibility")at 175) has no timeout.__download_version_file(requests.get(uri, stream=True)at 351) has no timeout (streaming can hang without connect/read bounds).
🧰 Tools
🪛 Ruff (0.15.15)
[error] 119-119: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codexctl/updates.py` around lines 119 - 121, The three outbound requests in
update_version_ids, get_toltec_version, and __download_version_file must include
bounded timeouts: add a timeout argument to the requests.get call in
update_version_ids (the call returning .json()), to the requests.get in
get_toltec_version, and to the streaming requests.get in
__download_version_file; use a sensible connect/read timeout (e.g. timeout=(5,
30)) for the streaming call and a shorter single or tuple timeout for simple
JSON/HTML fetches, and keep the existing exception handling by allowing
requests.exceptions.Timeout to be raised/caught as before. Ensure you apply the
timeout parameter to the exact calls referenced by the function names
update_version_ids, get_toltec_version, and __download_version_file.
Source: Linters/SAST tools
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 by CodeRabbit
Release Notes
New Features
Improvements
Chores