Conversation
WalkthroughAdds a startup update check and a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as rogue.__main__
participant Checker as rogue.common.update_checker
participant Cache as Local Cache
participant PyPI as PyPI Registry
participant TUI as RogueTuiInstaller
User->>CLI: Run CLI (or --version)
alt --version
CLI-->>User: Print __version__ and exit
else Startup
CLI->>Checker: check_for_updates(__version__)
Checker->>Cache: read last_check / cached latest
alt cache missing or >24h
Checker->>PyPI: fetch latest package info (HTTP, timeout)
PyPI-->>Checker: latest version metadata
Checker->>Cache: persist last_check & latest_version
end
Checker->>Checker: compare latest vs current
alt update available
Checker-->>User: Rich prompt (show diff, actions)
User->>Checker: confirm / decline
alt confirm
Checker->>Checker: run uv/uvx upgrade (with timeout)
Checker->>TUI: install_rogue_tui(upgrade=True)
TUI-->>Checker: install result (downloads use verify=False)
Checker-->>User: notify success/failure
else decline
Checker-->>User: skip update
end
else up-to-date
Checker-->>CLI: continue startup
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 4
🧹 Nitpick comments (4)
rogue/common/tui_installer.py (1)
95-104: Stream the download to avoid large in-memory buffers.requests.get(...).content loads the whole binary in memory. Stream to file in chunks to reduce memory spikes and support large assets.
- response = requests.get( - download_url, - timeout=60, - headers={ - "Accept": "application/octet-stream", - **self._headers, - }, - ) - response.raise_for_status() - # Create a temporary file - with tempfile.NamedTemporaryFile( - delete=False, - suffix="-rogue-tui", - ) as tmp_file: - tmp_file.write(response.content) + with requests.get( + download_url, + timeout=60, + headers={"Accept": "application/octet-stream", **self._headers}, + stream=True, + ) as response: + response.raise_for_status() + with tempfile.NamedTemporaryFile(delete=False, suffix="-rogue-tui") as tmp_file: + for chunk in response.iter_content(chunk_size=1024 * 64): + if chunk: # keep-alive chunks + tmp_file.write(chunk)rogue/__main__.py (3)
255-267: Use robust version parsing (packaging) instead of naive tuple split.The current split int logic breaks for pre-releases/post/dev versions. packaging.version handles PEP 440 semantics.
-from typing import Any, Dict, Optional +from typing import Any, Dict, Optional +from packaging.version import Version, InvalidVersion @@ -def _is_newer_version(latest: str, current: str) -> bool: - """Compare version strings to determine if latest is newer than current.""" - def version_tuple(v: str) -> tuple: - """Convert version string to tuple for comparison.""" - try: - return tuple(map(int, v.split("."))) - except ValueError: - # Handle non-standard version formats gracefully - return (0, 0, 0) - return version_tuple(latest) > version_tuple(current) +def _is_newer_version(latest: str, current: str) -> bool: + """PEP 440-aware comparison.""" + try: + return Version(latest) > Version(current) + except InvalidVersion: + # Fallback: if strings differ, assume latest is newer + return latest != current
320-349: Prefer ‘uv tool upgrade’ and improve fallback.Use the canonical upgrade command and don’t rely on matching stderr text.
- result = subprocess.run( - ["uv", "tool", "install", "-U", "rogue-ai"], + result = subprocess.run( + ["uv", "tool", "upgrade", "rogue-ai"], capture_output=True, text=True, timeout=600, # 10 minute timeout for the update ) # nosec: B607 B603 @@ - if result.returncode != 0 and "is not installed" in result.stderr: + if result.returncode != 0: console.print("[dim]Trying alternative update method...[/dim]") # For uvx installations, we need to reinstall result = subprocess.run( # nosec: B607 B603 ["uvx", "--refresh", "rogue-ai", "--version"], capture_output=True, text=True, timeout=600, # 10 minute timeout for the update )Also applies to: 351-356
221-235: Centralize the PyPI distribution name and add UA header constant.Hardcoding "rogue-ai" in multiple places invites drift if the dist name changes. Extract to a constant and reuse.
-from . import __version__ +from . import __version__ +PYPI_DIST_NAME = "rogue-ai" @@ - response = requests.get( - "https://pypi.org/pypi/rogue-ai/json", + response = requests.get( + f"https://pypi.org/pypi/{PYPI_DIST_NAME}/json", timeout=5, headers={"User-Agent": f"rogue-ai/{__version__}"}, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rogue/__init__.py(1 hunks)rogue/__main__.py(4 hunks)rogue/common/tui_installer.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/__init__.pyrogue/common/tui_installer.pyrogue/__main__.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/__init__.pyrogue/common/tui_installer.pyrogue/__main__.py
🧬 Code graph analysis (1)
rogue/__main__.py (2)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(16-188)install_rogue_tui(156-188)rogue/run_tui.py (1)
run_rogue_tui(7-24)
⏰ 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). (1)
- GitHub Check: codestyle
🔇 Additional comments (1)
rogue/__main__.py (1)
45-51: --version flag looks good; ensure it bypasses update checks.The early exit is correct. After moving the update check into main(), confirm that --version skips any network work as above.
Also applies to: 100-103
| def install_rogue_tui( | ||
| self, | ||
| upgrade: bool = False, | ||
| ) -> bool: | ||
| """Install rogue-tui from GitHub releases if not already installed.""" | ||
| # Check if rogue-tui is already available | ||
| if self._is_rogue_tui_installed(): | ||
| if self._is_rogue_tui_installed() and not upgrade: | ||
| logger.info("rogue-tui is already installed.") | ||
| return True | ||
|
|
There was a problem hiding this comment.
Upgrade path can fail on Windows: use atomic replace instead of shutil.move.
When upgrade=True and the destination exists, shutil.move can fail on Windows if the file exists/in use. Use Path.replace (os.replace) for atomic overwrite.
Apply:
- install_path = self._get_install_path()
- shutil.move(tmp_path, install_path)
+ install_path = self._get_install_path()
+ # Atomic overwrite (works on Windows and POSIX)
+ Path(tmp_path).replace(install_path)Also applies to: 177-184
There was a problem hiding this comment.
@drorIvry we should test this but it sounds good
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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 (3)
rogue/server/services/qualifire_service.py (1)
26-32: Re-enable TLS verification for Qualifire reportingAdding
verify=Falsedisables HTTPS certificate validation, so any on-path attacker can intercept responses or stealX-qualifire-key. Given this endpoint transmits authenticated evaluation data, we can't ship without TLS verification. Please drop the override (or plug in a proper CA bundle when talking to an internal PKI) so we stay compliant with Bandit B501 and keep traffic secure.response = requests.post( f"{request.qualifire_url}/api/rogue/v1/report", headers={"X-qualifire-key": request.qualifire_api_key}, json=api_evaluation_result.model_dump(mode="json"), timeout=300, - verify=False, # nosec: B501 )rogue/common/tui_installer.py (2)
47-53: Don’t bypass TLS when fetching release metadata
verify=Falsecompletely skips certificate validation for the GitHub API call, so a hijacked network could hand us a forged release payload. Please restore default verification (or allow an injected CA path) instead of suppressing Bandit B501.response = requests.get( url, timeout=10, headers=self._headers, - verify=False, # nosec: B501 )
96-104: Validate TLS certificates for binary downloadsThe binary download is now performed without TLS verification, meaning an attacker could swap in a malicious
rogue-tui. We must keep certificate checks enabled (or supply a trusted CA bundle) and remove thenosecexception.response = requests.get( download_url, timeout=60, headers={ "Accept": "application/octet-stream", **self._headers, }, - verify=False, # nosec: B501 )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rogue/__main__.py(4 hunks)rogue/common/tui_installer.py(3 hunks)rogue/common/update_checker.py(1 hunks)rogue/server/services/qualifire_service.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/main.py
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/tui_installer.pyrogue/server/services/qualifire_service.pyrogue/common/update_checker.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/tui_installer.pyrogue/server/services/qualifire_service.pyrogue/common/update_checker.py
🧬 Code graph analysis (1)
rogue/common/update_checker.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(16-190)install_rogue_tui(158-190)
⏰ 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). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
rogue/common/tui_installer.py (3)
156-160: Consider keeping the PATH addition log at debug level (not commented).Commented logs tend to rot; emitting at debug keeps output clean while preserving observability.
Apply:
- # logger.debug(f"Adding {install_dir} to PATH environment variable.") + logger.debug("Adding {} to PATH environment variable.", install_dir)
52-52: Reuse a single Console instance.Instantiate Console once (e.g., self._console in init) and reuse; avoids repeated setup and eases testing via injection.
Example:
# in __init__ from rich.console import Console self._console: Console = Console() # replace local Console() with: console = self._consoleAlso applies to: 93-93, 181-181
191-193: Remove commented debug or guard with env flag.Dead comments drift. If useful, gate with an env flag (e.g., ROGUE_DEBUG) and log at debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rogue/common/tui_installer.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/tui_installer.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/tui_installer.py
⏰ 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). (2)
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
🔇 Additional comments (6)
rogue/common/tui_installer.py (6)
14-15: Rich Console import — looks good.No issues; aligns with the new TUI messaging.
93-99: Console usage — OK.Local Console for per‑operation status is fine; see consolidation note below.
176-179: Public API change — looks good.install_rogue_tui(upgrade: bool = False) -> bool is clear and typed.
183-185: Early-exit UX message — OK.Message is clear; function returns fast when already installed and not upgrading.
214-215: Success message — OK.Clear confirmation; keep the debug path log if you re-enable debug logging.
205-207: Use atomic replace for robustness on Windows and POSIX.shutil.move can fail on Windows when the target exists/in use; atomic replace avoids partial updates. Ensure temp is on the same volume (addressed in download diff above).
Apply:
- with console.status("[bold yellow]Installing rogue-tui...", spinner="dots"): - shutil.move(tmp_path, install_path) + with console.status("[bold yellow]Installing rogue-tui...", spinner="dots"): + # Atomic overwrite (Windows/POSIX) — requires tmp on same volume + Path(tmp_path).replace(install_path)
| # Show spinner during download | ||
| with console.status( | ||
| "[bold green]Downloading rogue-tui binary...", | ||
| spinner="dots", | ||
| ): | ||
| response = requests.get( | ||
| download_url, | ||
| timeout=60, | ||
| headers={ | ||
| "Accept": "application/octet-stream", | ||
| **self._headers, | ||
| }, | ||
| verify=False, # nosec: B501 | ||
| ) | ||
| response.raise_for_status() | ||
|
|
There was a problem hiding this comment.
Stream download and write to same-volume temp file; avoid verify=False and large memory spike.
- Avoid reading the whole asset into memory.
- Write into a temp file in the target directory to enable atomic replace later.
- Keep TLS verification enabled.
As per coding guidelines
Apply:
- with console.status(
+ with console.status(
"[bold green]Downloading rogue-tui binary...",
spinner="dots",
):
- response = requests.get(
- download_url,
- timeout=60,
- headers={
- "Accept": "application/octet-stream",
- **self._headers,
- },
- verify=False, # nosec: B501
- )
- response.raise_for_status()
-
- # Create a temporary file
- with tempfile.NamedTemporaryFile(
- delete=False,
- suffix="-rogue-tui",
- ) as tmp_file:
- tmp_file.write(response.content)
- tmp_path = tmp_file.name
+ with requests.get(
+ download_url,
+ timeout=60,
+ headers={"Accept": "application/octet-stream", **self._headers, "User-Agent": "rogue-updater"},
+ stream=True,
+ ) as response:
+ response.raise_for_status()
+ install_dir = self._get_install_path().parent
+ Path(install_dir).mkdir(parents=True, exist_ok=True)
+ with tempfile.NamedTemporaryFile(
+ delete=False,
+ dir=install_dir,
+ prefix=".rogue-tui.",
+ suffix=".tmp",
+ ) as tmp_file:
+ for chunk in response.iter_content(chunk_size=1_048_576):
+ if chunk:
+ tmp_file.write(chunk)
+ tmp_path = tmp_file.nameOptional: prefer asset["browser_download_url"] when available to avoid the API indirection requirement for Accept header.
Also applies to: 125-132
🤖 Prompt for AI Agents
In rogue/common/tui_installer.py around lines 109-124 (and similarly 125-132),
the code downloads the entire binary into memory and disables TLS verification;
change to stream the response (requests.get(..., stream=True)) with verify left
enabled (remove verify=False), call response.raise_for_status(), and write
chunks to a same-volume temporary file in the target directory (use
tempfile.NamedTemporaryFile(dir=target_dir, delete=False) or mkstemp) iterating
response.iter_content(chunk_size=8192) to avoid memory spikes; after fully
writing and flushing/fsync the temp file, atomically replace the final target
with os.replace(temp_path, final_path) and ensure the temp file is removed on
exceptions to avoid leaks; optionally use asset["browser_download_url"] when
present to skip the API Accept header.
There was a problem hiding this comment.
@drorIvry streaming the file instead of keeping it all in memory sounds like a good idea
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| logger.info(f"rogue-tui installed successfully to {install_path}") | ||
| console.print("[green]✅ rogue-tui installed successfully![/green]") | ||
| # logger.debug(f"rogue-tui installed to {install_path}") |
There was a problem hiding this comment.
| # logger.debug(f"rogue-tui installed to {install_path}") |
|
|
||
| try: | ||
| with open(cache_file, "r") as f: | ||
| return json.load(f) |
| cache_data = { | ||
| "last_check": datetime.now().isoformat(), | ||
| "latest_version": latest_version, | ||
| "current_version": current_version, | ||
| } |
There was a problem hiding this comment.
a bit overkill no?
There was a problem hiding this comment.
Maybe, but since this file can be changed manually by the user, why not make sure it is exactly as you expect it?
rogue/common/update_checker.py
Outdated
| # First, try to upgrade using uv tool | ||
| # (for users who installed with uv tool install) | ||
| result = subprocess.run( # nosec: B607 B603 | ||
| ["uv", "tool", "install", "-U", "rogue-ai"], |
There was a problem hiding this comment.
In all the prints, you specify uv tool upgrade rogue-ai, but when you update, you run uv tool install -U rogue-ai
I think we should add a comment here why we use install -U here
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
rogue/common/tui_installer.py (3)
56-63: Critical: TLS verification disabled (already flagged).The
verify=Falseparameter on line 60 bypasses TLS certificate validation, exposing users to MITM attacks. This was flagged in previous reviews and must be addressed before merge.As per coding guidelines
109-127: Critical: TLS bypass and memory spike (already flagged).Two critical issues previously flagged remain unaddressed:
- Line 116:
verify=Falsedisables TLS verification, creating MITM vulnerability- Line 125:
response.contentloads entire binary into memory instead of streaming, causing memory spikes on large downloadsPrevious review recommended streaming with
stream=Trueand writing chunks viaiter_content(), plus creating the temp file in the target directory withtempfile.NamedTemporaryFile(dir=install_dir, ...)for atomic replace.As per coding guidelines
199-201: Critical: Non-atomic move on Windows (already flagged).Using
shutil.move()on line 201 can fail on Windows during upgrades when the destination file exists or is in use. Previous review recommended usingPath.replace()for atomic overwrite on both Windows and POSIX.Recommended fix from previous review:
- shutil.move(tmp_path, install_path) + Path(tmp_path).replace(install_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(2 hunks)rogue/__init__.py(1 hunks)rogue/common/tui_installer.py(6 hunks)sdks/python/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdks/python/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/init.py
🧰 Additional context used
📓 Path-based instructions (3)
pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
Add new dependencies to pyproject.toml
Files:
pyproject.toml
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/tui_installer.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/tui_installer.py
🔇 Additional comments (5)
pyproject.toml (1)
3-3: LGTM! Version bumps are consistent.The project version and SDK dependency are correctly updated from 0.1.3 to 0.1.4, aligning with the version bump in
rogue/__init__.pyandsdks/python/pyproject.tomlmentioned in the AI summary.Also applies to: 23-23
rogue/common/tui_installer.py (4)
14-14: Good addition of Console for better UX.The import of
Consolefrom rich.console enables the improved user experience with status spinners and colored output throughout the installation flow.
47-55: Good UX improvement with status spinner.The Console status spinner provides clear feedback during the GitHub API call, improving the user experience.
88-88: Good UX improvement with Console and status spinner.The Console instance and download status spinner provide clear feedback to users during the binary download process.
Also applies to: 105-108
171-173: Good additions: upgrade parameter and Console UI.The
upgradeparameter enables the update flow introduced in this PR, and the Console-based output (installation status, success/failure messages) significantly improves user experience.Also applies to: 176-184
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
rogue/common/tui_installer.py (3)
88-131: Remove verify=False and stream the download to avoid memory spike.This segment has two critical issues already flagged in past reviews:
- Line 116: TLS verification is disabled (
verify=False)- Lines 109-126: The entire binary is loaded into memory rather than streamed
As per coding guidelines
Apply this diff to enable TLS verification and stream the download:
# Show spinner during download with console.status( "[bold green]Downloading rogue-tui binary...", spinner="dots", ): - response = requests.get( + with requests.get( download_url, timeout=60, headers={ "Accept": "application/octet-stream", **self._headers, }, - verify=False, # nosec: B501 - ) - response.raise_for_status() - - # Create a temporary file - with tempfile.NamedTemporaryFile( - delete=False, - suffix="-rogue-tui", - ) as tmp_file: - tmp_file.write(response.content) - tmp_path = tmp_file.name + stream=True, + ) as response: + response.raise_for_status() + install_dir = self._get_install_path().parent + Path(install_dir).mkdir(parents=True, exist_ok=True) + with tempfile.NamedTemporaryFile( + delete=False, + dir=install_dir, + prefix=".rogue-tui.", + suffix=".tmp", + ) as tmp_file: + for chunk in response.iter_content(chunk_size=1_048_576): + if chunk: + tmp_file.write(chunk) + tmp_path = tmp_file.name
47-66: Remove verify=False to enable TLS verification.This segment still bypasses TLS verification (line 60), which has been extensively flagged in past reviews as a critical security issue that enables MITM attacks.
As per coding guidelines
Apply this diff to enable TLS verification:
with console.status( "[bold blue]Fetching latest release information...", spinner="dots", ): response = requests.get( url, timeout=10, headers=self._headers, - verify=False, # nosec: B501 ) response.raise_for_status() return response.json()
170-207: Use atomic file replacement instead of shutil.move for Windows compatibility.Line 197 uses
shutil.movewhich can fail on Windows when the destination exists or is in use. This was flagged in past reviews.Apply this diff to use atomic replacement:
try: # Move to final location install_path = self._get_install_path() with console.status("[bold yellow]Installing rogue-tui...", spinner="dots"): - shutil.move(tmp_path, install_path) + Path(tmp_path).replace(install_path)rogue/common/update_checker.py (1)
85-100: Remove verify=False to enable TLS verification for PyPI requests.Line 91 disables TLS verification for the PyPI API call, allowing potential MITM attacks where an attacker could spoof version information and trick users into malicious upgrades. This was flagged in past reviews as critical.
As per coding guidelines
Apply this diff to enable TLS verification:
try: response = requests.get( "https://pypi.org/pypi/rogue-ai/json", timeout=5, - verify=False, # nosec: B501 ) response.raise_for_status()
🧹 Nitpick comments (2)
rogue/common/tui_installer.py (1)
206-206: Commented debug log line should be removed.The commented logger line at line 206 creates maintenance debt. Past reviewers noted this should either be uncommented or removed. Based on the commit markers showing this was addressed, this may be an oversight.
Remove the commented line:
console.print("[green]✅ rogue-tui installed successfully![/green]") - # logger.debug(f"rogue-tui installed to {install_path}") return Truerogue/common/update_checker.py (1)
206-206: Clarify whyinstall -Uis used instead ofupgradecommand.A past reviewer noted the documentation mentions
uv tool upgrade rogue-aibut the code usesuv tool install -U rogue-ai. Adding a comment explaining this choice would improve maintainability.Add a clarifying comment:
# First, try to upgrade using uv tool # (for users who installed with uv tool install) + # Note: Using "install -U" instead of "upgrade" for broader compatibility result = subprocess.run( # nosec: B607 B603 ["uv", "tool", "install", "-U", "rogue-ai"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rogue/common/tui_installer.py(5 hunks)rogue/common/update_checker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/tui_installer.pyrogue/common/update_checker.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/tui_installer.pyrogue/common/update_checker.py
🧬 Code graph analysis (1)
rogue/common/update_checker.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-207)install_rogue_tui(170-207)
🔇 Additional comments (8)
rogue/common/tui_installer.py (1)
14-14: Import added for Rich Console UI enhancements.The Rich Console import supports the new status spinners and colored output throughout the installer. This is a positive improvement for user experience.
rogue/common/update_checker.py (7)
1-24: Module structure and imports look good.The module follows proper structure with comprehensive docstring, appropriate imports, and correct use of nosec markers for subprocess (which is reviewed in context below).
26-55: Update check function properly implements silent failure pattern.The top-level exception handler (lines 52-54) correctly ensures update checks never break the application. Cache check logic and version comparison flow are well-structured.
57-83: Cache management functions are well-implemented.Both
_get_update_cacheand_should_skip_update_checkhandle errors gracefully (returning safe defaults) and implement reasonable 24-hour check frequency.
102-118: Cache save function properly handles write errors.The function correctly creates parent directories and silently handles IOError on write failures, which is appropriate for non-critical cache operations.
120-127: Version comparison correctly handles invalid version formats.Using
packaging.versionfor comparison is the right approach, and the exception handler prevents crashes on non-standard versions.
129-178: Update prompt provides excellent UX with Rich formatting.The prompt is well-designed with clear visual hierarchy, helpful manual update instructions, and proper handling of keyboard interrupts (Ctrl+C). The interactive confirmation pattern matches the oh-my-zsh style mentioned in the module docstring.
180-257: Update command has robust subprocess handling and fallback logic.The function properly:
- Checks for
uvavailability before attempting update- Sets appropriate 10-minute timeouts to prevent hangs
- Handles multiple installation methods (uv tool vs uvx)
- Provides clear user feedback for all outcomes
- Calls
RogueTuiInstallerafter successful package update- Uses nosec markers appropriately for controlled subprocess calls
The subprocess usage is justified and well-controlled with timeouts and explicit command arrays.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rogue/common/update_checker.py (2)
206-206: Document whyuv tool install -Uis used instead ofuv tool upgrade.As noted in previous comments, the code uses
uv tool install -Ubut all user-facing messages referenceuv tool upgrade. This inconsistency is confusing. Please add a comment explaining whyinstall -Uis preferred, or change the command to match the documentation.Based on past review comments.
85-100: Critical security issue already flagged: Remove verify=False.This is a duplicate of the existing critical issue. Line 91 disables TLS certificate verification, which allows an attacker to perform man-in-the-middle attacks and serve malicious update metadata. Remove
verify=Falseand thenoseccomment to use default certificate validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rogue/common/update_checker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/update_checker.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/update_checker.py
🧬 Code graph analysis (1)
rogue/common/update_checker.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-207)install_rogue_tui(170-207)
⏰ 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). (2)
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
🔇 Additional comments (6)
rogue/common/update_checker.py (6)
26-55: LGTM! Well-structured update check flow.The function properly handles the update check lifecycle with appropriate error handling. Silent failures are acceptable for this non-critical feature.
57-83: LGTM! Proper cache handling with platformdirs.The cache helpers correctly use platformdirs for cross-platform cache directory location and handle I/O errors gracefully. The 24-hour check interval is reasonable.
102-118: LGTM! Safe cache persistence.The cache writer correctly creates the cache directory structure and handles write failures gracefully. Silent failure is appropriate for non-critical cache writes.
120-127: LGTM! Robust version comparison.The function correctly uses the
packaginglibrary for semantic version comparison and handles non-standard version formats gracefully.
129-178: LGTM! Excellent user experience with Rich.The update prompt uses Rich effectively to create a clear, informative UI. The function properly handles user interrupts and provides helpful manual update instructions.
242-256: LGTM! Comprehensive error handling.The exception handling covers the main failure modes (timeout, missing command, general errors) and provides helpful user guidance for manual recovery in each case.
| console.print( | ||
| "[dim]uv not found. please update manually using[/dim]" | ||
| "[dim]- uv tool upgrade rogue-ai[/dim]" | ||
| "[dim]or[/dim]" | ||
| "[dim]- pip install -e . (if you cloned the repo)[/dim]", | ||
| ) |
There was a problem hiding this comment.
Fix error message formatting.
The multiple [dim] tags should be in separate console.print() calls or combined into a single formatted string with newlines for proper display.
Apply this diff:
- console.print(
- "[dim]uv not found. please update manually using[/dim]"
- "[dim]- uv tool upgrade rogue-ai[/dim]"
- "[dim]or[/dim]"
- "[dim]- pip install -e . (if you cloned the repo)[/dim]",
- )
+ console.print("[yellow]⚠️ uv not found.[/yellow]")
+ console.print("[dim]Please update manually using:[/dim]")
+ console.print("[dim] • uv tool upgrade rogue-ai[/dim]")
+ console.print("[dim] • pip install --upgrade rogue-ai[/dim]")
+ console.print("[dim] • pip install -e . (if you cloned the repo)[/dim]")📝 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.
| console.print( | |
| "[dim]uv not found. please update manually using[/dim]" | |
| "[dim]- uv tool upgrade rogue-ai[/dim]" | |
| "[dim]or[/dim]" | |
| "[dim]- pip install -e . (if you cloned the repo)[/dim]", | |
| ) | |
| console.print("[yellow]⚠️ uv not found.[/yellow]") | |
| console.print("[dim]Please update manually using:[/dim]") | |
| console.print("[dim] • uv tool upgrade rogue-ai[/dim]") | |
| console.print("[dim] • pip install --upgrade rogue-ai[/dim]") | |
| console.print("[dim] • pip install -e . (if you cloned the repo)[/dim]") |
🤖 Prompt for AI Agents
In rogue/common/update_checker.py around lines 195 to 200, the console.print
call uses multiple adjacent "[dim]" tags which misformat output; change it to
either multiple console.print() calls (one per dim block) or combine into a
single formatted string with explicit newlines so the dim formatting applies
correctly to each line (e.g., build a single f-string with "\n" between lines or
call console.print three times), preserving the same messages and order.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
no idea, but it works lol
| @@ -1,5 +1,5 @@ | |||
| version = 1 | |||
| revision = 3 | |||
| revision = 2 | |||
There was a problem hiding this comment.
I think this might be incorrect.
run uv lock to make sure
There was a problem hiding this comment.
the file was auto generated
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
Left a few comments, (also replied to rabbit comments that might be useful)
The rest LGTM
Co-authored-by: yuval-qf <yuval@qualifire.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rogue/common/update_checker.py (1)
85-99: CRITICAL: Restore HTTPS verification for the PyPI update check.Line 91 disables TLS certificate verification with
verify=False, allowing an attacker to perform a man-in-the-middle attack, spoof PyPI metadata, and trick users into installing malicious upgrades. This is a serious security vulnerability for an auto-update mechanism.Apply this diff to restore certificate validation:
response = requests.get( "https://pypi.org/pypi/rogue-ai/json", timeout=5, - verify=False, # nosec: B501 )If you have specific certificate issues in your environment, use a custom CA bundle via the
verifyparameter instead of disabling verification entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rogue/common/update_checker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rogue,tests,examples}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
{rogue,tests,examples}/**/*.py: Format Python code with Black
Organize imports according to isort conventions
Use type hints for all function signatures
Follow PEP 8 naming (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Ensure code passes flake8 linting
Ensure code passes mypy type checks per .mypy.ini
Address Bandit-reported security issues in Python code
Files:
rogue/common/update_checker.py
rogue/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place application/package code under the rogue/ directory
Files:
rogue/common/update_checker.py
🧬 Code graph analysis (1)
rogue/common/update_checker.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-207)install_rogue_tui(170-207)
⏰ 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). (1)
- GitHub Check: codestyle
🔇 Additional comments (6)
rogue/common/update_checker.py (6)
1-24: LGTM! Module structure and imports are well-organized.The module docstring clearly describes the purpose, and imports are properly organized with appropriate type hints.
26-54: LGTM! Main entry point is well-structured.The function properly orchestrates the update check workflow with appropriate error handling that prevents update checks from disrupting the main application.
102-118: LGTM! Cache persistence is handled correctly.The function safely creates the cache directory and handles write errors gracefully without disrupting the application flow.
120-126: LGTM! Version comparison is robust.The function correctly uses the
packaginglibrary for semantic version comparison and gracefully handles non-standard version formats.
129-177: LGTM! Update prompt provides clear UX.The Rich-formatted panel clearly presents version information, and the interactive prompt gracefully handles user responses including keyboard interrupts.
180-253: No dynamic subprocess arguments detected
Bothsubprocess.runcalls pass only hardcoded list literals (no f-strings or user input); suppressions for B607/B603 remain justified.
| def _should_skip_update_check(cache_info: Dict[str, Any]) -> bool: | ||
| """Check if we should skip the update check based on cache.""" | ||
| if not cache_info: | ||
| return False | ||
|
|
||
| last_check = cache_info.get("last_check") | ||
| if not last_check: | ||
| return False | ||
|
|
||
| # Skip if we've checked in the last 24 hours | ||
| last_check_time = datetime.fromisoformat(last_check) | ||
| return datetime.now() - last_check_time < timedelta(hours=24) |
There was a problem hiding this comment.
Add error handling for datetime parsing.
Line 81 calls datetime.fromisoformat(last_check) without error handling. If the cache file is manually edited with an invalid date format, this will raise a ValueError and propagate up, potentially breaking the update check.
Apply this diff to handle malformed dates gracefully:
def _should_skip_update_check(cache_info: Dict[str, Any]) -> bool:
"""Check if we should skip the update check based on cache."""
if not cache_info:
return False
last_check = cache_info.get("last_check")
if not last_check:
return False
# Skip if we've checked in the last 24 hours
- last_check_time = datetime.fromisoformat(last_check)
- return datetime.now() - last_check_time < timedelta(hours=24)
+ try:
+ last_check_time = datetime.fromisoformat(last_check)
+ return datetime.now() - last_check_time < timedelta(hours=24)
+ except (ValueError, TypeError):
+ # Invalid date format in cache - treat as if we should check
+ return False📝 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.
| def _should_skip_update_check(cache_info: Dict[str, Any]) -> bool: | |
| """Check if we should skip the update check based on cache.""" | |
| if not cache_info: | |
| return False | |
| last_check = cache_info.get("last_check") | |
| if not last_check: | |
| return False | |
| # Skip if we've checked in the last 24 hours | |
| last_check_time = datetime.fromisoformat(last_check) | |
| return datetime.now() - last_check_time < timedelta(hours=24) | |
| def _should_skip_update_check(cache_info: Dict[str, Any]) -> bool: | |
| """Check if we should skip the update check based on cache.""" | |
| if not cache_info: | |
| return False | |
| last_check = cache_info.get("last_check") | |
| if not last_check: | |
| return False | |
| # Skip if we've checked in the last 24 hours | |
| try: | |
| last_check_time = datetime.fromisoformat(last_check) | |
| return datetime.now() - last_check_time < timedelta(hours=24) | |
| except (ValueError, TypeError): | |
| # Invalid date format in cache - treat as if we should check | |
| return False |
🤖 Prompt for AI Agents
In rogue/common/update_checker.py around lines 71 to 82, the call to
datetime.fromisoformat(last_check) can raise a ValueError (or TypeError) if the
cache contains a malformed date; wrap the parsing in a try/except that catches
ValueError and TypeError, optionally log or warn about the malformed cache
value, and return False (skip the update check) when parsing fails so the
exception does not propagate.
2 participants
Summary by CodeRabbit
New Features
Chores
Behavior