refactor: scripts #1108
refactor: scripts #1108
Conversation
…d validation - Introduced a new command group for configuration management, including `generate` and `validate` commands. - The `generate` command creates configuration example files in various formats (env, toml, yaml, json, markdown). - The `validate` command checks the validity of configuration files and provides a summary of settings and their sources. - Enhanced user feedback with console output for successful operations and error handling.
- Added a new command group for database operations, encapsulating various commands such as `init`, `check`, `dev`, `downgrade`, `health`, `history`, `new`, `nuke`, `push`, `reset`, `schema`, `show`, `status`, `tables`, and `version`. - Each command provides specific functionalities for managing database migrations, health checks, and schema validation. - Enhanced user feedback with informative console outputs for successful operations and error handling.
- Added a new command group for development tools, encapsulating commands such as `lint`, `lint-fix`, `format`, `type-check`, `docstring-coverage`, `pre-commit`, `clean`, and `all`. - Each command provides specific functionalities for code quality checks, formatting, and cleaning up project artifacts. - Enhanced user feedback with informative console outputs for successful operations and error handling.
- Added a new command group for documentation operations, encapsulating commands such as `build`, `lint`, `serve`, and various Wrangler commands (`deploy`, `deployments`, `dev`, `rollback`, `tail`, `versions`). - Each command provides specific functionalities for building, serving, and managing documentation, as well as deploying to Cloudflare Workers. - Enhanced user feedback with informative console outputs for successful operations and error handling.
- Added a new command group for testing operations, encapsulating commands such as `all`, `benchmark`, `coverage`, `file`, `html`, `parallel`, `plain`, and `quick`. - Each command provides specific functionalities for running tests, generating reports, and managing coverage. - Enhanced user feedback with informative console outputs for test execution and results.
- Added a new command group for Tux bot operations, encapsulating `start` and `version` commands. - The `start` command initiates the Tux Discord bot with options for debug mode and provides user feedback on the bot's status. - The `version` command displays the current version of the Tux bot, enhancing user experience with informative console outputs.
- Refactored the CLI infrastructure to create a unified entry point for all commands, enhancing organization and usability. - Removed the BaseCLI, CLI, and Registry modules, consolidating functionality into a new core structure. - Introduced a new `create_app` function for standardized Typer application creation and command group registration. - Added process management utilities for running shell commands and managing subprocesses. - Enhanced user feedback with consistent console output across all command groups.
- Deleted multiple CLI scripts including `config.py`, `db.py`, `dev.py`, `docs.py`, `test.py`, and `tux.py` to simplify the project structure. - This cleanup enhances maintainability and reduces complexity by consolidating command functionalities into a unified CLI framework. - Improved overall organization and usability of the CLI commands.
- Changed the CLI entry point in `pyproject.toml` from `scripts.cli:main` to `scripts:main` to reflect the new structure of the CLI scripts. - This adjustment aligns with recent refactoring efforts to streamline the command organization and improve maintainability.
Reviewer's GuideRefactors the CLI scripts into a modern, modular Typer-based architecture with a unified root entry point and dedicated subcommand modules for config, db, dev, docs, test, and tux, plus shared utilities for UI, process execution, and core bootstrapping. Class diagram for core CLI infrastructure and command groups after refactorclassDiagram
class CoreFactory {
+Path ROOT
+Path SRC
+create_app(name, help_text, no_args_is_help, kwargs) Typer
}
class UIUtilities {
+Console console
+print_success(message)
+print_error(message)
+print_info(message)
+print_warning(message)
+print_section(title, color)
+rich_print(message)
+print_table(title, columns, data)
+create_progress_bar(description, total) Progress
}
class ProcRunner {
+run_command(command, capture_output, check, env) CompletedProcess
}
class RootCLI {
+Typer app
+main()
}
class DbCommands {
+Typer app
+init()
+dev(create_only, name)
+push()
+status()
+new(message, auto_generate)
+history()
+check()
+show(revision)
+tables()
+health()
+schema()
+queries()
+reset()
+downgrade(revision, force)
+nuke(force, fresh, yes)
+version()
+main()
}
class DevCommands {
+Typer app
+lint(fix)
+lint_fix()
+format_code()
+type_check()
+lint_docstring()
+docstring_coverage()
+pre_commit()
+clean()
+run_all_checks(fix)
+main()
}
class DocsCommands {
+Typer app
+serve(dev_addr, open_browser, strict)
+build(clean, strict)
+lint()
+wrangler_dev(port, remote)
+wrangler_deploy(env, dry_run)
+wrangler_deployments(limit)
+wrangler_versions(action, version_id, alias)
+wrangler_tail(format_output, status)
+wrangler_rollback(version_id, message)
+main()
}
class ConfigCommands {
+Typer app
+generate(format_, output)
+validate()
+main()
}
class TuxCommands {
+Typer app
+start(debug)
+show_version()
+main()
}
class TestCommands {
+Typer app
+main()
}
CoreFactory <.. RootCLI : uses
CoreFactory <.. DbCommands : uses
CoreFactory <.. DevCommands : uses
CoreFactory <.. DocsCommands : uses
CoreFactory <.. ConfigCommands : uses
CoreFactory <.. TuxCommands : uses
CoreFactory <.. TestCommands : uses
UIUtilities <.. DbCommands : uses
UIUtilities <.. DevCommands : uses
UIUtilities <.. DocsCommands : uses
UIUtilities <.. ConfigCommands : uses
UIUtilities <.. TuxCommands : uses
ProcRunner <.. DbCommands : uses
ProcRunner <.. DevCommands : uses
ProcRunner <.. DocsCommands : uses
RootCLI o-- DbCommands : add_typer(db.app)
RootCLI o-- DevCommands : add_typer(dev.app)
RootCLI o-- DocsCommands : add_typer(docs.app)
RootCLI o-- ConfigCommands : add_typer(config.app)
RootCLI o-- TestCommands : add_typer(test.app)
RootCLI o-- TuxCommands : add_typer(tux.app)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the CLI into a root Typer app with per-feature command-group packages under scripts/*, adds an app factory and bootstrap, provides shared UI and subprocess utilities, splits monoliths into focused submodules, updates pyproject script entry points and basedpyright settings, introduces new exception classes and a model rich_repr hook, and adjusts a DB-related integration test. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant RootApp as scripts:main (root app)
participant Group as scripts.db.init (subcommand)
participant Proc as scripts.proc.run_command
participant DB as alembic / DatabaseService
User->>RootApp: run "db init"
RootApp->>Group: dispatch "init" command
Group->>Proc: run_command(["uv","run","alembic",...])
Proc->>DB: spawn subprocess / connect to DB
DB-->>Proc: exit status & output
Proc-->>Group: stdout/stderr or raise CalledProcessError
Group-->>RootApp: return result (success/failure)
RootApp-->>User: render output via scripts.ui (print_section/print_success/print_error)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
📚 Documentation Preview
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 security issues, 6 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
scripts/test/html.py, the--htmloutput is written toreports/test_report.htmlbutopen_browserattempts to openhtmlcov/index.html; consider pointing the browser to the actual pytest HTML report path to avoid confusing or broken behavior. - The logic to locate
zensical.toml/wrangler.tomland print the same error messages is duplicated across several docs commands (e.g.,build,serve,wrangler_*); extracting these checks into shared helpers in a common module would reduce repetition and keep behavior consistent. - Several commands swallow broad exceptions without surfacing any diagnostic information (e.g.,
test/coverage.py,test/html.py,dev/docstring_coverage.py); consider at least logging the exception or returning a non-zero exit to avoid silently masking failures during CI or local runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scripts/test/html.py`, the `--html` output is written to `reports/test_report.html` but `open_browser` attempts to open `htmlcov/index.html`; consider pointing the browser to the actual pytest HTML report path to avoid confusing or broken behavior.
- The logic to locate `zensical.toml` / `wrangler.toml` and print the same error messages is duplicated across several docs commands (e.g., `build`, `serve`, `wrangler_*`); extracting these checks into shared helpers in a common module would reduce repetition and keep behavior consistent.
- Several commands swallow broad exceptions without surfacing any diagnostic information (e.g., `test/coverage.py`, `test/html.py`, `dev/docstring_coverage.py`); consider at least logging the exception or returning a non-zero exit to avoid silently masking failures during CI or local runs.
## Individual Comments
### Comment 1
<location> `scripts/db/tables.py:31` </location>
<code_context>
- service = DatabaseService(echo=False)
- await service.connect(CONFIG.database_url)
-
- async def _get_tables(session: Any) -> list[tuple[str, int]]:
- """Get list of tables with their column counts.
-
</code_context>
<issue_to_address>
**issue (bug_risk):** Column count subquery doesn't filter by schema and may over-count columns for tables with the same name in other schemas.
The `column_count` subquery only filters on `table_name`, while the outer query restricts `table_schema` to `public`. If a table name exists in multiple schemas, the count will be inflated. Filter by both table name and schema in the subquery, e.g.:
```sql
(SELECT COUNT(*)
FROM information_schema.columns c
WHERE c.table_name = t.table_name
AND c.table_schema = t.table_schema) AS column_count
```
so the count matches the outer query’s table set.
</issue_to_address>
### Comment 2
<location> `scripts/dev/clean.py:114-118` </location>
<code_context>
- ]
-
- # Directories to never clean
- protected_dirs = {".venv", "venv"}
-
- for pattern, description in patterns_to_clean:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Protected directory check uses substring matching, which can unintentionally skip unrelated paths.
This condition:
```python
if all(protected not in str(m) for protected in protected_dirs)
```
will treat any path containing the substrings `"venv"` or `".venv"` as protected (e.g. `"convenience"`, usernames, etc.), so unrelated files/dirs may be skipped from cleaning.
To avoid this, match on path components instead, for example:
```python
protected_dirs = {".venv", "venv"}
matches = [
m
for m in matches
if not any(part in protected_dirs for part in m.parts)
]
```
or restrict protection to specific known paths (e.g. `project_root / '.venv'`).
```suggestion
matches = [
m
for m in matches
if not any(part in protected_dirs for part in m.parts)
]
```
</issue_to_address>
### Comment 3
<location> `scripts/docs/wrangler_versions.py:21-29` </location>
<code_context>
-
- def wrangler_versions(
- self,
- action: Annotated[
- str,
- Option("--action", "-a", help="Action to perform: list, view, or upload"),
- ] = "list",
- version_id: Annotated[
- str,
- Option("--version-id", help="Version ID to view"),
- ] = "",
- alias: Annotated[
- str,
- Option("--alias", help="Preview alias name"),
</code_context>
<issue_to_address>
**suggestion:** Unconstrained `action` argument and loosely validated options can lead to confusing Wrangler invocations.
`action` is currently a free-form `str` passed straight to `wrangler`, and related options are only loosely checked. This allows invalid combinations (e.g. `--action view` without `--version-id`, or a misspelled action) that only fail later with unclear Wrangler errors.
To improve this:
* Restrict `action` to a `Literal["list", "view", "upload"]` so Typer validates values and surfaces them in help.
* Add explicit checks for required options per action (e.g. require `--version-id` for `view`, `--alias` for `upload`) and fail early with a clear error message before invoking `wrangler`.
Suggested implementation:
```python
from typing import Literal
from scripts.core import create_app
from scripts.proc import run_command
from scripts.ui import print_error, print_section, print_success
```
```python
action: Annotated[
Literal["list", "view", "upload"],
Option(
"--action",
"-a",
help="Action to perform",
),
] = "list",
```
```python
if not Path("wrangler.toml").exists():
print_error("wrangler.toml not found. Please run from the project root.")
return
if action == "view" and not version_id:
print_error("The --version-id option is required when --action view is used.")
return
if action == "upload" and not alias:
print_error("The --alias option is required when --action upload is used.")
return
cmd = ["wrangler", "versions", action]
```
</issue_to_address>
### Comment 4
<location> `scripts/dev/all.py:41` </location>
<code_context>
- ) -> None:
- """Run all development checks including linting, type checking, and documentation."""
- self.rich.print_section("Running All Development Checks", "blue")
- checks: list[tuple[str, Callable[[], None]]] = [
- ("Linting", self.lint_fix if fix else self.lint),
- ("Code Formatting", self.format_code),
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the per-check execution and result handling into a helper with a small `Check` dataclass to make `run_all_checks`’s loop simpler and more self-documenting.
You can simplify the control flow and make the orchestration more self‑documenting without changing behavior by:
1. Normalizing the `SystemExit` handling into a small helper.
2. Using a lightweight data structure instead of raw tuples.
That keeps `run_all_checks` focused on orchestration, while still working with the existing “commands that call `sys.exit`” design.
Example refactor:
```python
from dataclasses import dataclass
from collections.abc import Callable
from typer import Exit
# ...
@dataclass
class Check:
name: str
func: Callable[[], None]
def run_check(check: Check) -> bool:
"""Run a single check, normalizing SystemExit/Exception to a bool result."""
try:
check.func()
return True
except SystemExit as e:
return e.code == 0
except Exception:
return False
```
Then `run_all_checks` becomes simpler:
```python
@app.command(name="all")
def run_all_checks(
fix: Annotated[bool, Option("--fix", help="Automatically fix issues where possible")] = False,
) -> None:
print_section("Running All Development Checks", "blue")
checks: list[Check] = [
Check("Linting", lint_fix if fix else lint),
Check("Code Formatting", format_code),
Check("Type Checking", type_check),
Check("Docstring Linting", lint_docstring),
Check("Pre-commit Checks", pre_commit),
]
results: list[tuple[str, bool]] = []
with create_progress_bar("Running Development Checks", len(checks)) as progress:
task = progress.add_task("Running Development Checks", total=len(checks))
for check in checks:
progress.update(task, description=f"Running {check.name}...")
progress.refresh()
success = run_check(check)
results.append((check.name, success))
progress.advance(task)
progress.refresh()
# ... rest of summary/Exit logic unchanged ...
```
This:
- Removes the inlined `try/except` and comment noise from the main loop.
- Makes each check self‑documenting (`Check(name, func)` instead of raw tuples).
- Preserves all existing behavior, including handling `SystemExit` and other exceptions.
</issue_to_address>
### Comment 5
<location> `scripts/db/init.py:28` </location>
<code_context>
- self.rich.rich_print("")
-
- # Check if tables exist
- async def _check_tables():
- """Check if any tables exist in the database.
-
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the database inspection logic into a single async helper invoked once from `init` to flatten control flow and remove duplication.
You can simplify the control flow and remove duplication by:
1. Using a single async helper that:
- Creates `DatabaseService` once
- Connects once
- Runs both queries
- Disconnects once
2. Calling `asyncio.run` only once from `init`.
For example:
```python
import asyncio
import pathlib
from sqlalchemy import text
from scripts.core import create_app
from scripts.proc import run_command
from scripts.ui import print_error, print_section, print_success, rich_print
from tux.database.service import DatabaseService
from tux.shared.config import CONFIG
app = create_app()
async def _inspect_db_state() -> tuple[int, int]:
service = DatabaseService(echo=False)
try:
await service.connect(CONFIG.database_url)
async with service.session() as session:
table_result = await session.execute(
text(
"""
SELECT COUNT(*) FROM information_schema.tables
WHERE table_schema = 'public'
AND table_type = 'BASE TABLE'
AND table_name != 'alembic_version'
""",
),
)
migration_result = await session.execute(
text("SELECT COUNT(*) FROM alembic_version"),
)
table_count = table_result.scalar() or 0
migration_count = migration_result.scalar() or 0
return table_count, migration_count
except Exception:
# Preserve current behavior: treat errors as "0"
return 0, 0
finally:
try:
await service.disconnect()
except Exception:
# swallow to match current semantics
pass
@app.command(name="init")
def init() -> None:
"""Initialize database with proper migration from empty state."""
print_section("Initialize Database", "green")
rich_print("[bold green]Initializing database with migrations...[/bold green]")
rich_print("[yellow]This will create an initial migration file.[/yellow]\n")
table_count, migration_count = asyncio.run(_inspect_db_state())
migration_dir = pathlib.Path("src/tux/database/migrations/versions")
migration_files = list(migration_dir.glob("*.py")) if migration_dir.exists() else []
migration_file_count = len([f for f in migration_files if f.name != "__init__.py"])
if table_count > 0 or migration_count > 0 or migration_file_count > 0:
rich_print(
f"[red]Database already has {table_count} tables, "
f"{migration_count} migrations in DB, and "
f"{migration_file_count} migration files![/red]"
)
rich_print(
"[yellow]'db init' only works on completely empty databases with no migration files.[/yellow]"
)
return
try:
rich_print("[blue]Generating initial migration...[/blue]")
run_command(
[
"uv",
"run",
"alembic",
"revision",
"--autogenerate",
"-m",
"initial schema",
],
)
rich_print("[blue]Applying initial migration...[/blue]")
run_command(["uv", "run", "alembic", "upgrade", "head"])
print_success("Database initialized with migrations")
rich_print("[green]Ready for development[/green]")
except Exception:
print_error("Failed to initialize database")
if __name__ == "__main__":
app()
```
This keeps all behavior intact while:
- Flattening the control flow (no nested inner async functions).
- Avoiding duplicated `DatabaseService` / `connect` / `disconnect` / `try/except` logic.
- Using a single event loop via one `asyncio.run` call.
</issue_to_address>
### Comment 6
<location> `scripts/tux/start.py:19` </location>
<code_context>
+app = create_app()
+
+
+@app.command(name="start")
+def start(
+ debug: Annotated[bool, Option("--debug", help="Enable debug mode")] = False,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the `run()` invocation and its exception handling into a helper that returns an exit code plus a post-run behavior flag so `start()` only handles shared messaging and a single exit path.
You can reduce the branching/duplication in `start()` by:
1. Moving the `run()` + exception handling into a helper that always returns an exit code and a flag indicating whether to run the “generic post-run” messaging.
2. Letting `start()` just handle the generic messages and a single `sys.exit()`.
This keeps all current behavior (including when you *don’t* want “Bot started successfully” printed, e.g. on `"Event loop stopped..."`).
Example refactor:
```python
from enum import Enum, auto
class PostRunBehavior(Enum):
NORMAL = auto() # run generic status messages
SKIP = auto() # messages already printed in helper
def _run_bot(debug: bool) -> tuple[int, PostRunBehavior]:
try:
if debug:
print_info("Debug mode enabled")
exit_code = run()
return exit_code, PostRunBehavior.NORMAL
except RuntimeError as e:
msg = str(e)
if "setup failed" in msg.lower():
print_error("Bot setup failed")
return 1, PostRunBehavior.SKIP
elif "Event loop stopped before Future completed" in msg:
print_info("Bot shutdown completed")
return 0, PostRunBehavior.SKIP
else:
print_error(f"Runtime error: {e}")
return 1, PostRunBehavior.SKIP
except KeyboardInterrupt:
print_info("Bot shutdown requested by user (Ctrl+C)")
return 130, PostRunBehavior.SKIP
except SystemExit as e:
# Preserve current behavior
sys.exit(e.code)
except Exception as e:
print_error(f"Failed to start bot: {e}")
return 1, PostRunBehavior.SKIP
```
Then `start()` becomes much simpler:
```python
@app.command(name="start")
def start(
debug: Annotated[bool, Option("--debug", help="Enable debug mode")] = False,
) -> None:
print_section("Starting Tux Bot", "blue")
rich_print("[bold blue]Starting Tux Discord bot...[/bold blue]")
exit_code, behavior = _run_bot(debug)
if behavior is PostRunBehavior.NORMAL:
if exit_code == 0:
print_success("Bot started successfully")
elif exit_code == 130:
print_info("Bot shutdown requested by user (Ctrl+C)")
else:
print_error(f"Bot exited with code {exit_code}")
sys.exit(exit_code)
```
Optional follow-up (to remove brittle string checks over time):
Define specific exception types in the bot layer and raise them instead of `RuntimeError` with magic messages:
```python
# tux/main.py or a shared errors module
class SetupError(RuntimeError):
pass
class GracefulShutdown(RuntimeError):
pass
```
Then the helper becomes clearer:
```python
from tux.main import SetupError, GracefulShutdown
def _run_bot(debug: bool) -> tuple[int, PostRunBehavior]:
try:
...
return run(), PostRunBehavior.NORMAL
except SetupError:
print_error("Bot setup failed")
return 1, PostRunBehavior.SKIP
except GracefulShutdown:
print_info("Bot shutdown completed")
return 0, PostRunBehavior.SKIP
...
```
</issue_to_address>
### Comment 7
<location> `scripts/config/generate.py:88` </location>
<code_context>
result = subprocess.run(cmd, capture_output=True, text=True, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `scripts/docs/build.py:53` </location>
<code_context>
subprocess.run(cmd, check=True, env=os.environ.copy())
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 9
<location> `scripts/docs/serve.py:61` </location>
<code_context>
subprocess.run(cmd, check=True, env=os.environ.copy())
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 10
<location> `scripts/docs/wrangler_tail.py:47` </location>
<code_context>
subprocess.run(cmd, check=True, env=os.environ.copy())
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 11
<location> `scripts/proc.py:46-52` </location>
<code_context>
result = subprocess.run(
command,
check=check,
capture_output=capture_output,
text=True,
env=run_env,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1108 +/- ##
==========================================
- Coverage 39.92% 39.88% -0.04%
==========================================
Files 203 203
Lines 14529 14558 +29
Branches 1709 1709
==========================================
+ Hits 5800 5807 +7
- Misses 8729 8751 +22 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
scripts/db/version.py-20-37 (1)
20-37: Improve error handling specificity and exit code.The current implementation has two issues:
- The bare
except Exception:on line 36 is too broad and will mask unexpected errors. If arun_commandcall fails, it raisesCalledProcessError(per scripts/proc.py), but the generic catch-all swallows the error details and exits successfully (exit code 0).- The command should exit with a non-zero code on failure to signal the error to the caller.
🔎 Proposed fix
+ import sys + from subprocess import CalledProcessError + try: rich_print("[cyan]Current migration:[/cyan]") run_command(["uv", "run", "alembic", "current"]) rich_print("[cyan]Database driver:[/cyan]") run_command( [ "uv", "run", "python", "-c", "import psycopg; print(f'psycopg version: {psycopg.__version__}')", ], ) print_success("Version information displayed") - except Exception: + except CalledProcessError: print_error("Failed to get version information") + sys.exit(1)scripts/db/health.py-29-56 (1)
29-56: Improve error handling and exit code.Similar to the version command, this implementation:
- Uses a bare
except Exception:that swallows all errors without specific handling- Prints success message inside the try block (line 52) before any exception handling
- Doesn't exit with a non-zero code on failure
🔎 Proposed fix
+ import sys + try: with create_progress_bar("Connecting to database...") as progress: progress.add_task("Checking database health...", total=None) service = DatabaseService(echo=False) await service.connect(CONFIG.database_url) health_data = await service.health_check() await service.disconnect() if health_data["status"] == "healthy": rich_print("[green]Database is healthy![/green]") rich_print( f"[green]Connection: {health_data.get('connection', 'OK')}[/green]", ) rich_print( f"[green]Response time: {health_data.get('response_time', 'N/A')}[/green]", ) else: rich_print("[red]Database is unhealthy![/red]") rich_print( f"[red]Error: {health_data.get('error', 'Unknown error')}[/red]", ) print_success("Health check completed") except Exception as e: print_error(f"Failed to check database health: {e}") + sys.exit(1)scripts/config/validate.py-45-45 (1)
45-45: Potential IndexError if database URL is shorter than 50 characters.Line 45 assumes
config.database_urlhas at least 50 characters. If the URL is shorter, the slice[:50]is safe, but adding"..."makes it misleading. Consider checking the length first.🔎 Proposed fix
- table.add_row("Database URL", f"{config.database_url[:50]}...", "✓") + db_url = config.database_url + db_url_display = f"{db_url[:50]}..." if len(db_url) > 50 else db_url + table.add_row("Database URL", db_url_display, "✓")scripts/docs/wrangler_dev.py-45-49 (1)
45-49: Success message will never be reached during normal operation.The
wrangler devcommand runs a blocking development server. Withcapture_output=False,run_commandwon't return until the server terminates. The success message on line 47 will only be reached if the server is stopped, which means it's misleading—it suggests the server "started" when it actually has already stopped.Additionally, catching generic
Exceptionis too broad.🔎 Proposed fix
+from subprocess import CalledProcessError + cmd = ["wrangler", "dev", f"--port={port}"] if remote: cmd.append("--remote") print_info(f"Starting Wrangler dev server on port {port}...") + print_info("Press Ctrl+C to stop the server") try: run_command(cmd, capture_output=False) - print_success(f"Wrangler dev server started at http://localhost:{port}") - except Exception as e: + except (CalledProcessError, KeyboardInterrupt): + print_info("\nWrangler dev server stopped") + except Exception as e: print_error(f"Error: {e}")scripts/dev/lint.py-32-37 (1)
32-37: Overly broad exception handling.Catching generic
Exceptionwill suppress all exceptions, includingKeyboardInterruptandSystemExit. Sincerun_commandraisessubprocess.CalledProcessErroron failure (per its signature in scripts/proc.py), catch that specifically instead.🔎 Proposed fix
+from subprocess import CalledProcessError + @app.command(name="lint") def lint( fix: Annotated[bool, Option("--fix", help="Automatically apply fixes")] = False, ) -> None: """Run linting checks with Ruff to ensure code quality.""" print_section("Running Linting", "blue") print_info("Checking code quality with Ruff...") cmd = ["uv", "run", "ruff", "check"] if fix: cmd.append("--fix") cmd.append(".") try: run_command(cmd) print_success("Linting completed successfully") - except Exception: + except CalledProcessError: print_error("Linting did not pass - see issues above") sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
scripts/docs/wrangler_versions.py-41-46 (1)
41-46: Missing parameter validation for action-specific requirements.The command construction doesn't validate that required parameters are provided:
action="view"requires--version-id, but if it's empty, the command will failaction="upload"requires--alias, but if it's empty, the flag is silently omittedConsider validating parameters before building the command.
🔎 Proposed fix
cmd = ["wrangler", "versions", action] - if action == "view" and version_id: + if action == "view": + if not version_id: + print_error("--version-id is required for 'view' action") + return cmd.append(version_id) - elif action == "upload" and alias: + elif action == "upload": + if not alias: + print_error("--alias is required for 'upload' action") + return cmd.extend(["--preview-alias", alias])Committable suggestion skipped: line range outside the PR's diff.
scripts/dev/lint_fix.py-21-26 (1)
21-26: Overly broad exception handling.Catching generic
Exceptionwill suppress all exceptions, includingKeyboardInterruptandSystemExit. Sincerun_commandraisessubprocess.CalledProcessErroron failure, catch that specifically instead.🔎 Proposed fix
+from subprocess import CalledProcessError + @app.command(name="lint-fix") def lint_fix() -> None: """Run linting checks with Ruff and automatically apply fixes.""" print_section("Running Linting with Fixes", "blue") try: run_command(["uv", "run", "ruff", "check", "--fix", "."]) print_success("Linting with fixes completed successfully") - except Exception: + except CalledProcessError: print_error("Linting with fixes did not complete - see issues above") sys.exit(1)scripts/docs/wrangler_versions.py-48-52 (1)
48-52: Overly broad exception handling.Catching generic
Exceptionwill suppress all exceptions includingKeyboardInterrupt. Sincerun_commandraisessubprocess.CalledProcessErroron failure, catch that specifically.🔎 Proposed fix
+from subprocess import CalledProcessError + try: run_command(cmd, capture_output=False) print_success(f"Version {action} completed") - except Exception as e: + except CalledProcessError as e: print_error(f"Error: {e}") + raiseCommittable suggestion skipped: line range outside the PR's diff.
scripts/docs/wrangler_rollback.py-49-53 (1)
49-53: Exception handler doesn't exit with error code.The exception is caught and printed, but the command exits with code 0. This masks failures from calling scripts or CI pipelines.
🔎 Proposed fix
try: run_command(cmd, capture_output=False) print_success(f"Successfully rolled back to version {version_id}") except Exception as e: print_error(f"Error: {e}") + raise Exit(1) from NoneCommittable suggestion skipped: line range outside the PR's diff.
scripts/docs/wrangler_rollback.py-33-41 (1)
33-41: Validation failures return without exit code, appearing as success.When
wrangler.tomlis missing orversion_idis empty, the function returns normally, causing the CLI to exit with code 0 (success). Useraise Exit(1)for consistency with other commands.🔎 Proposed fix
+from typer import Exit, Option -from typer import Option ... if not Path("wrangler.toml").exists(): print_error("wrangler.toml not found. Please run from the project root.") - return + raise Exit(1) - if not version_id: + if not version_id: print_error( "Version ID is required. Use wrangler-deployments to find version IDs.", ) - return + raise Exit(1)scripts/tux/__init__.py-12-13 (1)
12-13: Pass explicitnameparameter toadd_typer()calls or set names on sub-apps.The
start.appandversion.appinstances are created withcreate_app()without anameparameter, resulting inname=None. Whenadd_typer()is called without an explicitnameargument, Typer uses the sub-app'snameattribute as the command group name. Withname=None, the subcommands will lack proper group names in the CLI.Either:
- Pass
name="start"andname="version"tocreate_app()calls in their respective files, or- Pass explicit
nameparameters:app.add_typer(start.app, name="start")andapp.add_typer(version.app, name="version")scripts/db/queries.py-67-69 (1)
67-69: Handle potentialNonequery text.The
querycolumn frompg_stat_activitycan beNULLif query tracking is disabled. SlicingNonewould raise aTypeError.🔎 Proposed fix
for pid, duration, query_text, state in long_queries: rich_print(f"[red]PID {pid}[/red]: {state} for {duration}") - rich_print(f" Query: {query_text[:100]}...") + query_preview = (query_text[:100] + "...") if query_text else "<no query text>" + rich_print(f" Query: {query_preview}")scripts/db/tables.py-31-44 (1)
31-44: SQL subquery may count columns from wrong schema.The subquery counting columns doesn't filter by
table_schema, so if a table with the same name exists in multiple schemas, the column count will include columns from all of them.🔎 Proposed fix
result = await session.execute( text(""" SELECT table_name, - (SELECT COUNT(*) FROM information_schema.columns WHERE table_name = t.table_name) as column_count + (SELECT COUNT(*) FROM information_schema.columns c + WHERE c.table_name = t.table_name + AND c.table_schema = t.table_schema) as column_count FROM information_schema.tables t WHERE table_schema = 'public' AND table_type = 'BASE TABLE' AND table_name != 'alembic_version' ORDER BY table_name """), )scripts/docs/serve.py-42-57 (1)
42-57: Strict mode passed despite being unsupported.The
--strictoption help text states it's "currently unsupported", yet it's still appended to the command at lines 56-57. This could cause errors ifzensical servedoesn't recognize the flag.🔎 Proposed fix: either remove the option or add a warning
- strict: Annotated[ - bool, - Option("--strict", "-s", help="Strict mode (currently unsupported)"), - ] = False,Or, if you want to keep it for future use:
if strict: + print_info("Warning: --strict mode is currently unsupported by zensical") cmd.append("--strict")Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (38)
scripts/db/history.py (1)
20-24: Consider more specific exception handling.The bare
except Exception:clause catches all exceptions broadly. While functional, catchingsubprocess.CalledProcessErrorspecifically (whichrun_commandraises when a command fails) would provide clearer intent.🔎 Proposed refinement
+ import subprocess + try: run_command(["uv", "run", "alembic", "history", "--verbose"]) print_success("History displayed") - except Exception: + except subprocess.CalledProcessError: print_error("Failed to get migration history")scripts/db/check.py (1)
20-24: Consider more specific exception handling.The bare
except Exception:clause is overly broad. Catchingsubprocess.CalledProcessErrorspecifically would make the error handling more explicit and maintainable.🔎 Proposed refinement
+ import subprocess + try: run_command(["uv", "run", "alembic", "check"]) print_success("All migrations validated successfully!") - except Exception: + except subprocess.CalledProcessError: print_error("Migration validation failed - check your migration files")scripts/test/quick.py (1)
15-21: Consider adding error handling for process execution.The
os.execvpcall can raiseOSErrorif the command is not found or cannot be executed. While this is rare withuv, adding a try-except block would provide a better user experience.🔎 Proposed refinement
+ from scripts.ui import print_error + print_section("Quick Tests", "blue") cmd = ["uv", "run", "pytest", "--no-cov"] print_info(f"Running: {' '.join(cmd)}") - os.execvp(cmd[0], cmd) + try: + os.execvp(cmd[0], cmd) + except OSError as e: + print_error(f"Failed to execute command: {e}") + raisescripts/db/push.py (1)
20-24: Consider more specific exception handling.The bare
except Exception:clause catches all exceptions broadly. Catchingsubprocess.CalledProcessErrorspecifically would provide clearer intent and better maintainability.🔎 Proposed refinement
+ import subprocess + try: run_command(["uv", "run", "alembic", "upgrade", "head"]) print_success("All migrations applied!") - except Exception: + except subprocess.CalledProcessError: print_error("Failed to apply migrations")scripts/test/plain.py (1)
15-21: Consider adding error handling for process execution.The
os.execvpcall can raiseOSErrorif the command cannot be executed. Adding error handling would improve the user experience if the command fails.🔎 Proposed refinement
+ from scripts.ui import print_error + print_section("Plain Tests", "blue") cmd = ["uv", "run", "pytest", "-p", "no:sugar"] print_info(f"Running: {' '.join(cmd)}") - os.execvp(cmd[0], cmd) + try: + os.execvp(cmd[0], cmd) + except OSError as e: + print_error(f"Failed to execute command: {e}") + raisescripts/test/benchmark.py (1)
15-21: Consider adding error handling for process execution.The
os.execvpcall can raiseOSErrorif the command fails to execute. Adding error handling would provide better feedback if the command cannot be found or fails to start.🔎 Proposed refinement
+ from scripts.ui import print_error + print_section("Benchmark Tests", "blue") cmd = ["uv", "run", "pytest", "--benchmark-only", "--benchmark-sort=mean"] print_info(f"Running: {' '.join(cmd)}") - os.execvp(cmd[0], cmd) + try: + os.execvp(cmd[0], cmd) + except OSError as e: + print_error(f"Failed to execute command: {e}") + raisescripts/dev/pre_commit.py (1)
21-26: Consider more specific exception handling.The bare
except Exception:clause is overly broad. Catchingsubprocess.CalledProcessErrorspecifically would make the error handling more explicit.🔎 Proposed refinement
+ import subprocess + try: run_command(["uv", "run", "pre-commit", "run", "--all-files"]) print_success("Pre-commit checks completed successfully") - except Exception: + except subprocess.CalledProcessError: print_error("Pre-commit checks did not pass - see issues above") sys.exit(1)scripts/test/parallel.py (1)
38-49: Consider error handling for process replacement.The
os.execvpcall on line 49 will replace the current process. If it fails (e.g.,uvis not found in PATH), it raisesOSError, which will propagate uncaught and may produce a confusing stack trace. Consider wrapping it in a try-except block to provide a cleaner error message.🔎 Suggested enhancement
+ import sys + print_info(f"Running: {' '.join(cmd)}") - os.execvp(cmd[0], cmd) + try: + os.execvp(cmd[0], cmd) + except OSError as e: + from scripts.ui import print_error + print_error(f"Failed to execute command: {e}") + sys.exit(1)scripts/dev/type_check.py (1)
21-26: Consider more specific exception handling.The bare
except Exception:on line 24 catches all exceptions. While the exit code handling is correct, catchingsubprocess.CalledProcessErrorspecifically would be more precise and avoid catching unexpected errors likeKeyboardInterruptorSystemExit.🔎 Suggested refinement
+ from subprocess import CalledProcessError + try: run_command(["uv", "run", "basedpyright"]) print_success("Type checking completed successfully") - except Exception: + except CalledProcessError: print_error("Type checking did not pass - see issues above") sys.exit(1)scripts/db/new.py (1)
38-44: Consider more specific exception handling.The bare
except Exception:on line 42 is overly broad. Catchingsubprocess.CalledProcessErrorspecifically would be more precise. The use oftyper.Exitis appropriate for Typer applications.🔎 Suggested refinement
+ from subprocess import CalledProcessError + try: run_command(cmd) print_success(f"Migration generated: {message}") rich_print("[yellow]Review the migration file before applying[/yellow]") - except Exception: + except CalledProcessError: print_error("Failed to generate migration") raise Exit(1) from Nonescripts/dev/lint_docstring.py (1)
21-26: Consider more specific exception handling (pattern repeated across dev commands).The bare
except Exception:on line 24 is overly broad. This pattern appears in multiple dev command files (type_check.py, lint_docstring.py). Consider catchingsubprocess.CalledProcessErrorspecifically for consistency and precision.🔎 Suggested refinement
+ from subprocess import CalledProcessError + try: run_command(["uv", "run", "pydoclint", "--config=pyproject.toml", "."]) print_success("Docstring linting completed successfully") - except Exception: + except CalledProcessError: print_error("Docstring linting did not pass - see issues above") sys.exit(1)scripts/dev/clean.py (2)
23-38: Potential performance concern with directory size calculation.Line 32 computes the directory size by traversing all files with
item.rglob("*")before deletion. For large cache directories (e.g.,.mypy_cache,node_modulesif present), this could be slow. Consider:
- Skipping size calculation for directories (just track count)
- Using a size estimation or limit
However, given the cleanup targets (mainly Python caches), this is likely acceptable.
74-74: Fragile project root path calculation.Line 74 uses
Path(__file__).parent.parent.parentto determine the project root. This is fragile and assumes the file remains atscripts/dev/clean.py. Consider using a more robust approach.🔎 Alternative approaches
Option 1: Use the ROOT constant from scripts/core.py (if available):
+ from scripts.core import ROOT + - project_root = Path(__file__).parent.parent.parent + project_root = ROOTOption 2: Search for a marker file (e.g., pyproject.toml):
project_root = Path(__file__).parent.parent.parent + # Verify we found the project root + if not (project_root / "pyproject.toml").exists(): + print_error("Could not locate project root") + sys.exit(1)scripts/db/schema.py (1)
38-43: Consider ensuring database cleanup on exception.If an exception occurs during
validate_schema(),disconnect()won't be called. Consider wrapping database operations in a try-finally block or checking ifDatabaseServicesupports context manager protocol.🔎 Suggested refactor using try-finally
with create_progress_bar("Validating schema...") as progress: progress.add_task("Validating schema against models...", total=None) service = DatabaseService(echo=False) - await service.connect(CONFIG.database_url) - schema_result = await service.validate_schema() - await service.disconnect() + try: + await service.connect(CONFIG.database_url) + schema_result = await service.validate_schema() + finally: + await service.disconnect()scripts/tux/version.py (1)
21-32: Consider usingtyper.Exitfor consistency.The exception handling correctly exits with code 1, but uses
sys.exit(1)while other commands in this PR usetyper.Exit(1). For consistency across the CLI codebase, consider using Typer's exit mechanism.🔎 Suggested refactor
+from typer import Exit + - except ImportError as e: + except ImportError as e: print_error(f"Failed to import version: {e}") - sys.exit(1) + raise Exit(1) from e except Exception as e: print_error(f"Failed to show version: {e}") - sys.exit(1) + raise Exit(1) from escripts/dev/format.py (1)
21-26: Consider catching more specific exceptions.The bare
except Exceptioncatches all exceptions, which may hide unexpected errors. Consider catchingsubprocess.CalledProcessErrorspecifically, or letrun_commandhandle the error propagation since it already raises on failure withcheck=Trueby default.🔎 Alternative: Remove try/except and let run_command handle errors
Since
run_commandalready raisesCalledProcessErroron failure and prints error details, you could simplify:- try: - run_command(["uv", "run", "ruff", "format", "."]) - print_success("Code formatting completed successfully") - except Exception: - print_error("Code formatting did not pass - see issues above") - sys.exit(1) + run_command(["uv", "run", "ruff", "format", "."]) + print_success("Code formatting completed successfully")Note: This changes the error message behavior, so keep the current approach if the custom error message is preferred.
scripts/proc.py (1)
45-63: Consider simplifying the try/except/else structure.The
elseclause afterexcept(Lines 62-63) is uncommon. Typically, you'd return the result after the try block completes. While functionally correct, the current structure may be less readable.🔎 Suggested refactoring for clarity
try: result = subprocess.run( command, check=check, capture_output=capture_output, text=True, env=run_env, ) if capture_output and result.stdout: console.print(result.stdout.strip()) + + return result except subprocess.CalledProcessError as e: print_error(f"Command failed: {' '.join(command)}") if e.stderr: console.print(f"[red]{e.stderr.strip()}[/red]") raise - else: - return resultscripts/docs/wrangler_deployments.py (2)
29-31: Consider usingsys.exit(1)for consistency.The early return on missing
wrangler.toml(Line 31) is inconsistent with error handling in other commands (e.g.,scripts/dev/format.pyusessys.exit(1)). Whilereturnworks, usingsys.exit(1)would signal a non-zero exit code to the shell, which is more conventional for CLI error conditions.if not Path("wrangler.toml").exists(): print_error("wrangler.toml not found. Please run from the project root.") - return + sys.exit(1)Note: This would require importing
sys.
37-41: Improve exception handling specificity.The bare
except Exception(Line 37) and direct exception printing (Line 41) could be improved:
- Catch specific exceptions like
subprocess.CalledProcessErrororFileNotFoundError- Avoid printing raw exception objects, which may expose internal details
🔎 More specific error handling
try: run_command(cmd, capture_output=False) print_success("Deployment history retrieved") - except Exception as e: - print_error(f"Error: {e}") + except subprocess.CalledProcessError: + print_error("Failed to retrieve deployment history") + sys.exit(1)scripts/test/__init__.py (1)
10-22: Redundant import ofquick_testsfunction.Line 22 imports
quick_testsdirectly fromscripts.test.quick, but Line 20 already imports thequickmodule. You could usequick.quick_tests()instead of the direct import, reducing redundancy.🔎 Remove redundant import
from scripts.test import ( all as all_tests, ) from scripts.test import ( benchmark, coverage, file, html, parallel, plain, quick, ) -from scripts.test.quick import quick_tests app = create_app(name="test", help_text="Testing operations", no_args_is_help=False)Then update line 40:
@app.callback(invoke_without_command=True) def default_callback(ctx: Context) -> None: """Run quick tests if no command specified.""" if ctx.invoked_subcommand is None: - quick_tests() + quick.quick_tests()scripts/docs/lint.py (1)
44-49: Lint checks may have false positives/negatives.The current checks have some limitations:
- Line 46:
content.startswith("#")assumes the title is the first line, but many Markdown files have YAML frontmatter before the title.- Line 48: Simple string matching for
TODOorFIXMEmay flag occurrences in code blocks or quoted text where they're intentional.These are reasonable heuristics for a linting tool, but consider documenting these limitations or refining the checks if needed.
🔎 Enhanced check for frontmatter
To handle YAML frontmatter:
content = md_file.read_text() + # Skip YAML frontmatter if present + if content.startswith("---"): + parts = content.split("---", 2) + if len(parts) >= 3: + content = parts[2].strip() + if content.strip() == "": issues.append(f"Empty file: {md_file}") elif not content.startswith("#"): issues.append(f"Missing title: {md_file}")scripts/dev/lint.py (1)
19-37: Consider consolidatinglintandlint-fixcommands.Both
scripts/dev/lint.pyandscripts/dev/lint_fix.pyimplement essentially the same functionality. Sincelintalready has a--fixflag,lint-fixis redundant. Consider either:
- Removing
lint-fixand documenting the--fixflag- Or keeping both for UX convenience if users prefer explicit command names
scripts/dev/docstring_coverage.py (1)
19-25: Improve exception handling to distinguish between low coverage and actual errors.While the comment explains that
docstr-coveragereturns non-zero for low coverage, the current implementation also swallows legitimate errors (e.g., command not found, permission errors). Consider checking the exit code or catching specific exceptions.🔎 Proposed improvement
+from subprocess import CalledProcessError + @app.command(name="docstring-coverage") def docstring_coverage() -> None: """Check docstring coverage across the codebase.""" print_section("Docstring Coverage", "blue") try: run_command(["uv", "run", "docstr-coverage", "--verbose", "2", "."]) print_success("Docstring coverage report generated") - except Exception: - # docstr-coverage might return non-zero if coverage is below threshold - # but we still want to show the report - pass + except CalledProcessError as e: + # docstr-coverage returns non-zero if coverage is below threshold + # Exit codes 1-99 typically indicate coverage issues (show report) + # Exit codes 100+ indicate actual errors + if e.returncode < 100: + print_success("Docstring coverage report generated (coverage below threshold)") + else: + print_error(f"Error running docstring coverage: {e}") + raisescripts/db/dev.py (1)
34-66: Consider extracting duplicate command and refining exception handling.
- The Alembic revision command (lines 37-47 and 51-61) is duplicated. Extract it to reduce repetition.
- Catching bare
Exceptionand immediately discarding the chain withfrom Noneloses valuable debugging context. Sincerun_commandalready handlesCalledProcessErrorwith detailed output, consider catching that specifically or re-raising with context.🔎 Proposed refactor
try: + # Create migration + run_command( + [ + "uv", + "run", + "alembic", + "revision", + "--autogenerate", + "-m", + migration_name, + ], + ) + if create_only: - rich_print("[bold blue]Creating migration only...[/bold blue]") - run_command( - [ - "uv", - "run", - "alembic", - "revision", - "--autogenerate", - "-m", - migration_name, - ], - ) print_success("Migration created - review and apply with 'db push'") else: - rich_print("[bold blue]Creating and applying migration...[/bold blue]") - run_command( - [ - "uv", - "run", - "alembic", - "revision", - "--autogenerate", - "-m", - migration_name, - ], - ) run_command(["uv", "run", "alembic", "upgrade", "head"]) print_success("Migration created and applied!") - except Exception: + except subprocess.CalledProcessError: print_error("Failed to create migration") - raise Exit(1) from None + raise Exit(1)scripts/docs/wrangler_rollback.py (1)
21-24: Required parameter declared as optional with empty default.
version_idis validated as required (line 37-41), but declared with= ""default. This is non-idiomatic for Typer—consider usingArgumentfor positional required input or setting a proper required Option pattern.🔎 Proposed fix using Argument
+from typer import Argument, Option -from typer import Option ... def wrangler_rollback( version_id: Annotated[ - str, - Option("--version-id", help="Version ID to rollback to"), - ] = "", + str | None, + Argument(help="Version ID to rollback to"), + ] = None,scripts/config/generate.py (3)
7-7: Inconsistent subprocess usage—consider usingrun_commandfromscripts/proc.Other CLI commands in this PR use the shared
run_commandhelper fromscripts/proc.py, which provides consistent error handling and output formatting. This file usessubprocess.rundirectly, creating inconsistency.Also applies to: 87-88
30-51: Consider removing the unsupportedoutputparameter.The
outputparameter is declared but immediately rejected with an error. This creates confusing UX—users see the option in help but can't use it. Either remove the parameter entirely or add a TODO comment explaining the planned implementation.🔎 Option 1: Remove the parameter
def generate( format_: Annotated[ Literal["env", "toml", "yaml", "json", "markdown", "all"], Option( "--format", "-f", help="Format to generate (env, toml, yaml, json, markdown, all)", ), ] = "all", - output: Annotated[ - Path | None, - Option( - "--output", - "-o", - help="Output file path (not supported with CLI approach - uses pyproject.toml paths)", - ), - ] = None, ) -> None: """Generate configuration example files in various formats.""" console.print(Panel.fit("Configuration Generator", style="bold blue")) - - if output is not None: - console.print( - "Custom output paths are not supported when using CLI approach", - style="red", - ) - console.print( - "Use pyproject.toml configuration to specify custom paths", - style="yellow", - ) - raise Exit(code=1)
81-84: Silent no-op for invalid format values.Although the
Literaltype annotation should prevent invalid values at the type level,format_map.get(format_, [])returns an empty list for unrecognized formats, causing the command to silently succeed without generating anything. Consider adding a fallback error.🔎 Proposed fix
- formats_to_generate = format_map.get(format_, []) + formats_to_generate = format_map.get(format_) + if formats_to_generate is None: + console.print(f"Unknown format: {format_}", style="red") + raise Exit(code=1)scripts/tux/start.py (1)
40-46: String matching on exception messages is fragile.Checking
"setup failed"and"Event loop stopped before Future completed"in exception messages is brittle—these strings may change between library versions. Consider catching more specific exception types or using constants if the messages are under your control.scripts/docs/serve.py (1)
20-25: Consider simplifying return type.The function returns a string that's only used for its truthiness. Returning
boolwould be clearer, or rename to_has_zensical_config().🔎 Proposed simplification
-def _find_zensical_config() -> str | None: +def _has_zensical_config() -> bool: current_dir = Path.cwd() if (current_dir / "zensical.toml").exists(): - return "zensical.toml" + return True print_error("Can't find zensical.toml file. Please run from the project root.") - return None + return Falsescripts/db/downgrade.py (1)
44-48: Consider catching the specific exception type.
run_commandraisessubprocess.CalledProcessErroron failure (perscripts/proc.py). CatchingExceptionis overly broad. Additionally,run_commandalready prints error details before re-raising, so you may see duplicate error output.🔎 Proposed fix
+import subprocess + try: run_command(["uv", "run", "alembic", "downgrade", revision]) print_success(f"Successfully downgraded to revision: {revision}") - except Exception: + except subprocess.CalledProcessError: print_error(f"Failed to downgrade to revision: {revision}")scripts/test/coverage.py (1)
34-37: Consider usinginttype forfail_under.Using
intinstead ofstrwould let Typer validate the input and provide better error messages for invalid values.🔎 Proposed fix
fail_under: Annotated[ - str | None, + int | None, Option("--fail-under", help="Minimum coverage percentage required"), ] = None,Then at line 68:
if fail_under: - cmd.extend(["--cov-fail-under", fail_under]) + cmd.extend(["--cov-fail-under", str(fail_under)])scripts/db/init.py (2)
28-61: Combine async operations and fix potential resource leak.Two issues:
- Two separate
asyncio.run()calls are inefficient—combine both checks into a single async function.- If an exception occurs after
connect()but beforedisconnect(), the connection leaks. Use try/finally or an async context manager.🔎 Proposed refactor
- async def _check_tables(): - try: - service = DatabaseService(echo=False) - await service.connect(CONFIG.database_url) - async with service.session() as session: - result = await session.execute( - text( - "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema = 'public' AND table_type = 'BASE TABLE' AND table_name != 'alembic_version'", - ), - ) - table_count = result.scalar() or 0 - await service.disconnect() - except Exception: - return 0 - else: - return table_count - - async def _check_migrations(): - try: - service = DatabaseService(echo=False) - await service.connect(CONFIG.database_url) - async with service.session() as session: - result = await session.execute( - text("SELECT COUNT(*) FROM alembic_version"), - ) - migration_count = result.scalar() or 0 - await service.disconnect() - except Exception: - return 0 - else: - return migration_count - - table_count = asyncio.run(_check_tables()) - migration_count = asyncio.run(_check_migrations()) + async def _check_database_state() -> tuple[int, int]: + """Return (table_count, migration_count).""" + service = DatabaseService(echo=False) + try: + await service.connect(CONFIG.database_url) + async with service.session() as session: + tables_result = await session.execute( + text( + "SELECT COUNT(*) FROM information_schema.tables " + "WHERE table_schema = 'public' AND table_type = 'BASE TABLE' " + "AND table_name != 'alembic_version'" + ), + ) + table_count = tables_result.scalar() or 0 + + try: + migrations_result = await session.execute( + text("SELECT COUNT(*) FROM alembic_version"), + ) + migration_count = migrations_result.scalar() or 0 + except Exception: + migration_count = 0 + + return table_count, migration_count + except Exception: + return 0, 0 + finally: + await service.disconnect() + + table_count, migration_count = asyncio.run(_check_database_state())
96-97: Bare exception discards useful diagnostic information.The
except Exceptionblock loses the actual error. Consider logging or printing the exception details to aid debugging.🔎 Proposed fix
- except Exception: - print_error("Failed to initialize database") + except Exception as e: + print_error(f"Failed to initialize database: {e}")scripts/docs/wrangler_tail.py (1)
38-42: Redundant condition forformat_output.Since
format_outputdefaults to"pretty", the conditionif format_output:is always true. Consider removing the condition or changing the default to an empty string if you want the flag to be truly optional.🔎 Proposed fix (if format should be optional)
- format_output: Annotated[ - str, - Option("--format", help="Output format: json or pretty"), - ] = "pretty", + format_output: Annotated[ + str, + Option("--format", help="Output format: json or pretty"), + ] = "", cmd = ["wrangler", "tail"] if format_output: cmd.extend(["--format", format_output])scripts/test/file.py (1)
36-37: Consider handlingos.execvpfailure.If
uvis not installed or not in PATH,os.execvpwill raiseOSError. While acceptable for a dev tool, adding a try/except would provide a clearer error message.🔎 Proposed fix
+from scripts.ui import print_error, print_info, print_section print_info(f"Running: {' '.join(cmd)}") - os.execvp(cmd[0], cmd) + try: + os.execvp(cmd[0], cmd) + except OSError as e: + print_error(f"Failed to execute command: {e}")scripts/docs/build.py (1)
20-25: Duplicate helper function — extract to shared module.
_find_zensical_config()is duplicated inscripts/docs/serve.py(lines 19-24). Extract this to a shared utility (e.g.,scripts/docs/utils.py) to eliminate duplication.scripts/dev/all.py (1)
41-47: Consider usinglint(fix=True)instead oflint_fix.The
lintfunction already accepts afixparameter (perscripts/dev/lint.pylines 19-36). Usinglintwith the parameter would be more consistent and reduce the need for a separatelint_fixfunction.🔎 Proposed change
-from scripts.dev.lint_fix import lint_fix checks: list[tuple[str, Callable[[], None]]] = [ - ("Linting", lint_fix if fix else lint), + ("Linting", lambda: lint(fix=fix)), ("Code Formatting", format_code), ("Type Checking", type_check), ("Docstring Linting", lint_docstring), ("Pre-commit Checks", pre_commit), ]
- Introduced a new asynchronous function `_inspect_db_state` to consolidate the logic for checking the database's table and migration counts. - Replaced the previous separate table and migration check functions with a single call to `_inspect_db_state`, improving code clarity and maintainability. - Enhanced error handling during database initialization to provide more informative feedback on failures.
…ection - Moved the instantiation of the DatabaseService to the beginning of the `_check_queries` function for better clarity. - Simplified the execution of the long query retrieval by combining parameters into a single line. - Enhanced the query preview output to handle cases with no query text. - Ensured the service disconnects in a `finally` block to guarantee proper resource management.
…oved query structure - Refactored the `tables` function to include a progress bar during the fetching of database tables, improving user experience. - Moved the instantiation of the `DatabaseService` to the beginning of the `_list_tables` function for better clarity. - Updated the SQL query to ensure accurate column counting by including the schema in the condition. - Ensured the service disconnects in a `finally` block to guarantee proper resource management.
- Introduced a `Check` dataclass to encapsulate individual development checks, improving code organization and readability. - Refactored the `run_all_checks` function to utilize the new `Check` class, enhancing the clarity of check execution and result handling. - Simplified error handling during check execution by centralizing it in the `run_check` function, ensuring consistent behavior across all checks.
…irectory checks - Replaced the hardcoded project root path with the ROOT constant for better maintainability. - Enhanced the directory protection check to utilize a more efficient component-based approach. - Cleaned up code formatting for improved readability.
…nfiguration checks - Replaced the custom zensical configuration check with a utility function for better clarity and maintainability. - Updated the help text for the strict mode option to remove the unsupported note. - Enhanced error handling during the documentation build process to ensure exceptions are raised appropriately.
- Updated the documentation serving script to utilize a utility function for zensical configuration checks, improving clarity and maintainability. - Revised the help text for the strict mode option to remove the unsupported note while adding a warning message for its current state. - Enhanced error handling to ensure proper feedback when the zensical configuration is not found.
…angler_deploy - Replaced the direct file existence check for 'wrangler.toml' with a utility function to enhance clarity and maintainability. - Updated error handling to raise exceptions with informative messages during the documentation build and deployment processes, ensuring better feedback on failures. - Ensured strict mode is enforced during the build process to catch issues early.
…ID and improve error handling - Replaced the option for version ID with a required argument for clearer usage. - Enhanced error handling by utilizing a utility function to check for wrangler configuration, improving maintainability and user feedback. - Updated exception handling to ensure consistent error reporting during rollback operations.
…ent handling and error checks - Updated the action parameter to use a Literal type for better clarity and restricted options. - Replaced the direct file existence check with a utility function to verify wrangler configuration. - Added error handling to ensure required options are provided for 'view' and 'upload' actions, improving user feedback.
- Introduced `has_zensical_config` and `has_wrangler_config` functions to verify the presence of configuration files, improving maintainability and user feedback. - Enhanced error messaging to guide users when configuration files are missing, ensuring they run scripts from the project root.
- Updated the coverage report command to accept an integer for the `--fail-under` option, improving type safety. - Simplified the command construction for coverage report formats, ensuring only valid formats are accepted. - Improved error handling during report generation, providing clearer feedback for failures and unexpected errors.
- Modified the init command test to assert failure when the database already exists, ensuring proper error handling. - Added setup method in recovery scenarios to ensure a clean and up-to-date database state before tests.
… parameters - Introduced new reporting options in the pyproject.toml for handling unknown variable, member, argument, and parameter types, enhancing type checking capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/db/schema.py (1)
36-76: Missingfinallyblock causes connection leak on failure.The
service.disconnect()call on line 42 is inside thetryblock. Ifservice.connect()orservice.validate_schema()raises an exception,disconnect()is never called, causing a connection pool leak. This was previously identified but the current code still has the issue.Compare with
scripts/db/health.pywhich correctly uses afinallyblock (lines 63-64).🔎 Proposed fix using finally block
async def _schema_check(): + service = DatabaseService(echo=False) try: with create_status("Validating schema against models...") as status: - service = DatabaseService(echo=False) await service.connect(CONFIG.database_url) schema_result = await service.validate_schema() - await service.disconnect() status.update("[bold green]Validation complete![/bold green]") if schema_result["status"] == "valid": rich_print("[green]Database schema validation passed![/green]") rich_print( "[green]All tables and columns match model definitions.[/green]", ) else: error_msg = schema_result.get( "error", "Unknown schema validation error", ) rich_print("[red]Database schema validation failed![/red]") rich_print(f"[red]Error: {error_msg}[/red]\n") rich_print("[yellow]Suggested fixes:[/yellow]") rich_print(" • Run 'uv run db reset' to reset and reapply migrations") rich_print( " • Run 'uv run db nuke --force' for complete database reset", ) rich_print( " • Check that your models match the latest migration files", ) _fail() print_success("Schema validation completed") except Exception as e: if not isinstance(e, Exit): print_error(f"Failed to validate database schema: {e}") raise Exit(1) from e raise + finally: + await service.disconnect()
🧹 Nitpick comments (9)
scripts/config/validate.py (1)
54-74: Hardcoded paths assume command is run from repository root.The config file checks (lines 54-62) and config directory check (line 65) use relative paths that assume the working directory is the repository root. This is a common pattern for CLI tools, but consider documenting this requirement in the command's docstring or help text.
🔎 Optional enhancement to improve docstring
@app.command(name="validate") def validate() -> None: - """Validate the current configuration.""" + """Validate the current configuration. + + Note: Must be run from the repository root directory. + """ console.print(Panel.fit("Configuration Validator", style="bold blue"))scripts/db/init.py (1)
71-80: Redundant.pysuffix check in migration file filter.The
f.suffix == ".py"check on line 78 is redundant sinceglob("*.py")on line 69 already ensures all matched files have a.pyextension.🔎 Simplified filter
migration_file_count = len( [ f for f in migration_files if f.name != "__init__.py" and not f.name.startswith("_") - and f.suffix == ".py" ], )src/tux/main.py (1)
66-67: Potential ValueError if SystemExit.code is a non-numeric string.
SystemExit.codecan be any object (int, str, None). If it's a non-numeric string (e.g.,SystemExit("error message")),int(e.code)will raiseValueError, which would then be caught by the genericExceptionhandler on line 71.This is unlikely in practice since Typer uses integer exit codes, but could be made more defensive.
🔎 More defensive handling
except SystemExit as e: - return int(e.code) if e.code is not None else 1 + if e.code is None: + return 1 + if isinstance(e.code, int): + return e.code + return 1pyproject.toml (1)
321-324: Consider the trade-offs of suppressing all unknown type reports for scripts.Setting all four unknown-type reports to
"none"for the scripts execution environment (UnknownVariableType, UnknownMemberType, UnknownArgumentType, UnknownParameterType) disables significant type-checking coverage. While this is pragmatic for tooling code that interacts with dynamic libraries like Typer and Rich, it may hide legitimate type issues in script logic.Consider a more targeted approach: suppress only the most problematic categories (e.g., UnknownMemberType for Rich's dynamic attributes) while keeping others enabled, or add
# type: ignorecomments to specific problematic lines rather than blanket suppression.scripts/db/tables.py (1)
65-66: Consider formatting table data for better readability.The
print_pretty(tables_data)call displays raw tuples(table_name, column_count), which works but could be more user-friendly. Consider formatting as a table or structured output for improved UX, especially since this is a user-facing CLI command.Example: Format as a simple table
- rich_print(f"[green]Found {len(tables_data)} tables:[/green]") - print_pretty(tables_data) + rich_print(f"[green]Found {len(tables_data)} tables:[/green]\n") + for table_name, column_count in tables_data: + rich_print(f" [cyan]{table_name:40}[/cyan] {column_count:3} columns")scripts/docs/lint.py (1)
58-59: Consider case-insensitive detection for TODO/FIXME markers.The check
"TODO" in content or "FIXME" in contentonly catches uppercase markers and will miss common lowercase variants like "todo", "fixme", or mixed case like "Todo". This could lead to incomplete linting results.Proposed fix for case-insensitive detection
- elif "TODO" in content or "FIXME" in content: + elif "TODO" in content.upper() or "FIXME" in content.upper(): issues.append(f"Contains TODO/FIXME: {md_file}")Or use regex for more precise word-boundary matching:
import re # ... elif re.search(r'\b(TODO|FIXME)\b', content, re.IGNORECASE):scripts/db/nuke.py (1)
71-74: Consider making production URL keywords configurable.The hardcoded production keywords
["prod", "live", "allthingslinux.org"]work for the current setup but might miss organization-specific production indicators or trigger false positives. Consider making these configurable via environment variables or config to improve flexibility across different deployment environments.Example: Load from environment
# Allow override via env var prod_keywords = os.getenv("PROD_DB_KEYWORDS", "prod,live,allthingslinux.org").split(",") is_prod_db = any(kw.strip() in db_url.lower() for kw in prod_keywords if kw.strip())scripts/dev/all.py (1)
38-47: Consider logging exceptions for easier debugging.The broad
Exceptioncatch on line 44 normalizes all failures toFalse, which is appropriate for the check aggregator pattern. However, for debugging, consider logging the exception details before returningFalse.🔎 Optional enhancement to log exceptions
+from scripts.ui import print_error + def run_check(check: Check) -> bool: """Run a single check, normalizing SystemExit/Exception to a bool result.""" try: check.func() except SystemExit as e: return e.code == 0 - except Exception: + except Exception as e: + print_error(f"Unexpected error in {check.name}: {e}") return False else: return Truescripts/ui.py (1)
157-195: Remove or utilize the unuseddescriptionparameter.The
descriptionparameter (line 158) is never used in the function body. TheTextColumnon line 178 displays{task.description}, which comes from theadd_task()call, not from this parameter. This creates a misleading API where callers might assume the parameter sets the task description.🔎 Proposed fix: Remove the unused parameter
def create_progress_bar( - description: str = "Processing...", total: int | None = None, ) -> Progress: """ Create a functional Rich progress bar. Parameters ---------- - description : str, optional - Text to show next to the progress bar. total : int | None, optional Total number of steps. If provided, shows percentage and bar. Returns ------- Progress A configured Progress instance. """Callers should provide the description when calling
add_task()instead, which already happens in the current usage pattern (seescripts/dev/all.pyline 74).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
pyproject.toml(2 hunks)scripts/config/validate.py(1 hunks)scripts/db/downgrade.py(1 hunks)scripts/db/health.py(1 hunks)scripts/db/init.py(1 hunks)scripts/db/nuke.py(1 hunks)scripts/db/queries.py(1 hunks)scripts/db/schema.py(1 hunks)scripts/db/tables.py(1 hunks)scripts/dev/all.py(1 hunks)scripts/dev/lint.py(1 hunks)scripts/dev/pre_commit.py(1 hunks)scripts/docs/lint.py(1 hunks)scripts/docs/wrangler_dev.py(1 hunks)scripts/proc.py(1 hunks)scripts/ui.py(1 hunks)src/tux/database/models/base.py(2 hunks)src/tux/main.py(1 hunks)tests/integration/test_database_cli_commands.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/dev/pre_commit.py
- scripts/docs/wrangler_dev.py
- scripts/proc.py
🧰 Additional context used
🧬 Code graph analysis (7)
scripts/db/init.py (4)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (3)
print_pretty(57-74)print_section(46-49)rich_print(52-54)src/tux/database/service.py (3)
DatabaseService(42-506)session(173-200)disconnect(107-113)
scripts/dev/lint.py (3)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
scripts/db/health.py (7)
scripts/core.py (1)
create_app(35-66)scripts/ui.py (4)
create_status(111-127)print_pretty(57-74)print_section(46-49)rich_print(52-54)src/tux/database/service.py (2)
health_check(385-416)disconnect(107-113)src/tux/modules/config/config.py (1)
config(48-52)scripts/db/schema.py (1)
_fail(25-27)typings/typer/main.pyi (1)
command(28-29)src/tux/shared/config/settings.py (1)
database_url(269-295)
scripts/docs/lint.py (2)
scripts/core.py (1)
create_app(35-66)scripts/ui.py (3)
create_progress_bar(157-195)print_pretty(57-74)print_section(46-49)
scripts/db/schema.py (5)
scripts/core.py (1)
create_app(35-66)scripts/ui.py (3)
create_status(111-127)print_section(46-49)rich_print(52-54)src/tux/database/service.py (2)
DatabaseService(42-506)validate_schema(418-506)scripts/db/health.py (1)
_fail(26-28)src/tux/shared/config/settings.py (1)
database_url(269-295)
scripts/dev/all.py (7)
scripts/core.py (1)
create_app(35-66)scripts/dev/format.py (1)
format_code(18-27)scripts/dev/lint.py (1)
lint(21-41)scripts/dev/lint_docstring.py (1)
lint_docstring(18-27)scripts/dev/pre_commit.py (1)
pre_commit(18-30)scripts/dev/type_check.py (1)
type_check(18-27)scripts/ui.py (4)
create_progress_bar(157-195)print_section(46-49)print_table(130-154)rich_print(52-54)
scripts/db/tables.py (6)
scripts/core.py (1)
create_app(35-66)scripts/ui.py (5)
create_progress_bar(157-195)print_info(36-38)print_pretty(57-74)print_section(46-49)rich_print(52-54)src/tux/core/logging.py (1)
database(496-519)src/tux/database/service.py (3)
DatabaseService(42-506)session(173-200)disconnect(107-113)src/tux/modules/config/config.py (1)
config(48-52)typings/typer/main.pyi (1)
command(28-29)
⏰ 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: Run All Tests (3.13.8)
- GitHub Check: Seer Code Review
🔇 Additional comments (24)
src/tux/database/models/base.py (2)
16-16: LGTM!The Rich import is correctly added to support the new
__rich_repr__method for improved debugging output.
177-189: LGTM! Clean Rich integration for improved debugging.The
__rich_repr__implementation correctly follows Rich's repr protocol and makes sound design choices:
- Uses
__dict__to access only loaded instance data, avoiding lazy-loaded relationship traversal.- Skips private attributes, consistent with the existing
to_dictmethod.- Yields raw values rather than serialized forms, which is appropriate since Rich handles datetime, UUID, and Enum objects well for debugging output.
The comment clearly documents the intentional design decision regarding relationship traversal.
scripts/config/validate.py (3)
1-17: LGTM!Clean imports and standard app creation pattern consistent with the new CLI structure.
20-49: LGTM! Configuration summary display is well-structured.The table provides useful visibility into the loaded configuration with proper masking of sensitive values (BOT_TOKEN) and truncation of long URLs.
78-80: Exception handling appropriately exits with code 1.The broad
Exceptioncatch is acceptable here since configuration validation can fail in many ways, and the error message is displayed to the user.scripts/db/health.py (2)
37-65: Past review issues have been addressed.The
finallyblock (lines 63-64) ensuresservice.disconnect()is always called, fixing the previously identified connection leak. The health check now correctly accesses themodeanderrorkeys returned byhealth_check(), and exits with code 1 when the database is unhealthy.
26-28: Good pattern for Ruff TRY301 compliance.The
_fail()helper cleanly separates the exit logic to satisfy the linter rule about raising exceptions withintryblocks.scripts/db/init.py (3)
28-56: Past review issues addressed; _inspect_db_state consolidates DB inspection.The async helper now performs both queries in a single session context and disconnects in a
finallyblock. Thecontextlib.suppress(Exception)around disconnect is appropriate for cleanup.One note: silently returning
(0, 0)on exceptions (lines 50-52) means a database connection failure will cause init to proceed and fail later at the Alembic step. This is documented as intentional ("Preserve current behavior"), but could result in confusing error messages when the database is unreachable.
82-94: Good: Exits with code 1 when database is not empty.This addresses the past review feedback about silent success when initialization is blocked.
116-118: Good: Exits with code 1 on failure.The exception handler now properly exits with a non-zero code, addressing the past review feedback.
scripts/db/schema.py (1)
25-27: Good pattern for Ruff TRY301 compliance.Consistent with
scripts/db/health.py.src/tux/main.py (2)
50-73: Exception handling is well-structured with appropriate exit codes.The refactored exception handling provides granular error responses:
- Database, setup, and general errors exit with code 1
- Graceful shutdown exits with code 0
- KeyboardInterrupt now correctly returns 130 (Unix convention: 128 + SIGINT), addressing the past review feedback
- Stack traces are logged only for unexpected exceptions
42-45: Good: Debug mode is now clearly indicated in startup logs.The conditional log message helps with troubleshooting.
tests/integration/test_database_cli_commands.py (2)
398-403: LGTM - Clean database setup for recovery tests.The setup_method ensures recovery tests start with a clean, up-to-date database state by performing a nuclear reset followed by applying migrations. This is appropriate for testing recovery scenarios.
160-161: LGTM - Flexible assertion accommodates refactored error messages.The updated assertion accepts either "Database initialization blocked" or "Database already has", which appropriately handles variations in error messaging after the CLI refactoring.
scripts/dev/lint.py (1)
33-41: LGTM - Proper error handling and exit codes.The exception handling correctly catches
CalledProcessErrorfor command failures and general exceptions for unexpected errors, with appropriate exit codes (1) for CI/CD integration.scripts/db/downgrade.py (2)
47-51: LGTM - Consistent confirmation behavior.The confirmation prompt is now required for all revisions unless
--forceis used, which is consistent with the data-loss warning and provides a uniform safety mechanism.
56-58: LGTM - Proper error handling with exit code.The exception handler correctly prints the error and exits with code 1, ensuring failures are detected by CI/CD pipelines.
scripts/db/queries.py (1)
46-78: LGTM - Proper resource management with try/finally.The database connection is correctly managed with a try/finally block ensuring
service.disconnect()is called even when exceptions occur. The use ofcreate_statusfor user feedback is also appropriate.scripts/db/nuke.py (2)
64-87: LGTM - Comprehensive production safeguards.The production detection checks multiple environment variables (ENVIRONMENT, APP_ENV, CONFIG.PRODUCTION) and database URL keywords ("prod", "live", "allthingslinux.org") with a FORCE_NUKE override. This provides strong protection against accidental production data loss.
122-127: LGTM - Proper error handling and resource cleanup.The try/except/finally structure correctly handles exceptions by printing the error with traceback, raising Exit(1), and ensuring database disconnection in the finally block.
scripts/dev/all.py (2)
30-36: LGTM! Clean dataclass design.The
Checkdataclass provides a clear abstraction for representing development checks with a name and callable function.
50-106: LGTM! Well-structured check orchestration.The command correctly:
- Captures the
fixflag in a lambda closure for the lint check (line 61)- Uses Rich's Progress API properly with task tracking (lines 70-80)
- Aggregates results and presents a clear summary table (lines 82-97)
- Exits with appropriate status code based on check outcomes (lines 102-106)
scripts/ui.py (1)
1-54: LGTM! Clean and consistent UI helper design.The shared console instance (line 23) prevents terminal state conflicts, and the basic print helpers provide a consistent, icon-based interface for different message types.
- Replaced manual input prompt with a confirmation dialog in the downgrade script to enhance user experience and prevent accidental downgrades. - Streamlined the filtering of migration files in the init script for better readability. - Allowed environment variable override for production database keywords in the nuke script, improving flexibility. - Ensured proper disconnection of the database service in the schema script after validation. - Adjusted progress bar settings in the tables script for clearer output formatting.
…lopment scripts - Enhanced error handling in all.py to print specific error messages for unexpected exceptions during checks. - Updated progress bar messages in all.py and clean.py for better clarity during execution.
- Removed redundant progress message in lint.py for cleaner output. - Updated TODO and FIXME checks to be case-insensitive, enhancing linting accuracy.
- Added an optional parameter `indent_guides` to the `print_pretty` function for improved visual representation of nested structures. - Updated the function call to include the new parameter, ensuring consistent behavior with the default value set to True. - Removed the redundant `description` parameter from the `create_progress_bar` function for cleaner code.
- Improved the error handling logic in the run function to ensure consistent exit codes for SystemExit exceptions. - Added checks for None and non-integer exit codes, returning 1 as a default for unexpected cases, enhancing robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
scripts/dev/all.py (1)
39-49: Consider defensive handling of SystemExit.code.SystemExit.code can be None or a non-integer, though in practice the called functions use
sys.exit(1). For robustness, explicitly handle edge cases.🔎 Safer comparison
def run_check(check: Check) -> bool: """Run a single check, normalizing SystemExit/Exception to a bool result.""" try: check.func() except SystemExit as e: - return e.code == 0 + return e.code == 0 if isinstance(e.code, int) else False except Exception as e: print_error(f"Unexpected error in {check.name}: {e}") return False else: return Truescripts/db/schema.py (1)
25-27: Unnecessary helper function adds indirection.The
_fail()helper exists solely to raiseExit(1), but it could be inlined at line 65. The comment suggests this is a workaround for TRY301, but raisingExit(1)after validation failure is legitimate and doesn't require extraction.🔎 Simplified approach
Remove the helper and inline the raise:
-def _fail(): - """Raise Exit(1) to satisfy Ruff's TRY301 rule.""" - raise Exit(1) - @app.command(name="schema") def schema() -> None: """Validate that database schema matches model definitions.""" ... rich_print( " • Check that your models match the latest migration files", ) - _fail() + raise Exit(1) print_success("Schema validation completed")If Ruff still complains, suppress the rule with
# noqa: TRY301since this is a valid use case.src/tux/main.py (1)
50-77: Strong error handling with appropriate exit codes.The exception handling is comprehensive and well-structured:
- Exception ordering is correct (specific before general)
- Exit codes follow Unix conventions (0=success, 1=error, 130=interrupt)
- User-friendly error messages with actionable guidance (e.g., docker compose hint)
- Proper use of log levels (info for graceful shutdown, error/critical for failures)
💡 Optional: Log non-int SystemExit codes for debugging
When
SystemExit.codeis neitherNonenor anint(e.g., a string), it's silently converted to exit code1. Consider logging this case for debugging purposes:except SystemExit as e: if e.code is None: return 1 if isinstance(e.code, int): return e.code + logger.warning(f"SystemExit with non-int code: {e.code!r}, returning 1") return 1scripts/db/tables.py (1)
66-66: Consider dynamic column width for long table names.The fixed 40-character width (
:40) may truncate unusually long table names. While acceptable for most cases, consider using dynamic width based on the longest table name in the result set for better readability.🔎 Example dynamic width implementation
# Before the loop max_name_len = max(len(name) for name, _ in tables_data) if tables_data else 0 width = max(max_name_len, 10) # minimum 10 chars # In the loop rich_print(f" [cyan]{table_name:{width}}[/cyan] {column_count:3} columns")scripts/docs/lint.py (1)
48-58: Inconsistent whitespace handling between files with and without frontmatter.The frontmatter stripping (line 51) applies
.strip()to the content, but for files without frontmatter, the content remains unstripped. This creates inconsistent validation:
- With frontmatter: Content is stripped, so leading whitespace before
#is tolerated- Without frontmatter: Content is not stripped, so leading whitespace causes "Missing title" error
A file with
# Titlewould fail if it has no frontmatter, but pass if the same content appears after frontmatter.🔎 Proposed fix for consistent whitespace handling
if len(parts) >= 3: content = parts[2].strip() + else: + content = content.strip() - if content.strip() == "": + if content == "": issues.append(f"Empty file: {md_file}") elif not content.startswith("#"): issues.append(f"Missing title: {md_file}")This strips content unconditionally and simplifies the empty check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
scripts/db/downgrade.py(1 hunks)scripts/db/init.py(1 hunks)scripts/db/nuke.py(1 hunks)scripts/db/schema.py(1 hunks)scripts/db/tables.py(1 hunks)scripts/dev/all.py(1 hunks)scripts/dev/clean.py(1 hunks)scripts/docs/lint.py(1 hunks)scripts/ui.py(1 hunks)src/tux/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/dev/clean.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/tux/main.py (2)
src/tux/shared/exceptions.py (4)
TuxDatabaseError(73-74)TuxError(50-51)TuxGracefulShutdown(66-67)TuxSetupError(62-63)src/tux/core/app.py (1)
run(111-156)
scripts/docs/lint.py (3)
scripts/core.py (1)
create_app(35-66)scripts/ui.py (3)
create_progress_bar(166-201)print_pretty(57-83)print_section(46-49)typings/typer/main.pyi (1)
command(28-29)
scripts/dev/all.py (8)
scripts/core.py (1)
create_app(35-66)scripts/dev/format.py (1)
format_code(18-27)scripts/dev/lint.py (1)
lint(21-41)scripts/dev/lint_docstring.py (1)
lint_docstring(18-27)scripts/dev/pre_commit.py (1)
pre_commit(18-30)scripts/dev/type_check.py (1)
type_check(18-27)scripts/ui.py (4)
create_progress_bar(166-201)print_section(46-49)print_table(139-163)rich_print(52-54)typings/typer/main.pyi (1)
command(28-29)
scripts/db/nuke.py (3)
scripts/ui.py (4)
print_info(36-38)print_section(46-49)prompt(101-117)rich_print(52-54)src/tux/database/service.py (3)
DatabaseService(42-506)session(173-200)disconnect(107-113)src/tux/shared/config/settings.py (1)
database_url(269-295)
scripts/db/init.py (3)
scripts/proc.py (1)
run_command(14-73)scripts/ui.py (3)
print_pretty(57-83)print_section(46-49)rich_print(52-54)src/tux/database/service.py (2)
DatabaseService(42-506)session(173-200)
⏰ 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: Run All Tests (3.13.8)
- GitHub Check: Seer Code Review
🔇 Additional comments (8)
src/tux/main.py (3)
12-17: LGTM: Clean exception imports.All imported exceptions are used in the error handlers below, and the imports are specific rather than using wildcards.
20-48: LGTM: Clean function signature and initialization.The
debugparameter provides useful user feedback, and thenoqa: PLR0911suppression is justified given the comprehensive error handling with multiple distinct exception types. Per the docstring, logging configuration is handled by the CLI layer, so usingdebugonly for status messages here is appropriate.
63-65: LGTM: KeyboardInterrupt correctly fixed.The exit code
130now correctly follows Unix convention (128 + SIGINT) and is consistent withTuxApp.run()behavior. This resolves the inconsistency flagged in the previous review.scripts/db/tables.py (1)
34-74: LGTM! Previous critical issues successfully resolved.The resource leak and column count accuracy issues from previous reviews have been properly addressed:
- Resource cleanup (lines 73-74): The
finallyblock now guaranteesservice.disconnect()is called regardless of early returns or exceptions.- Column count accuracy (lines 45-46): The subquery now filters by both
table_nameandtable_schema, preventing over-counting when table names exist in multiple schemas.The implementation follows async/await best practices, includes comprehensive error handling, and properly manages database resources.
scripts/docs/lint.py (4)
1-22: LGTM! Imports and app setup follow established patterns.The imports are clean and the app creation using the factory function follows the codebase conventions.
29-32: LGTM! Exit behavior correctly implemented.The missing directory check now properly exits with code 1, consistent with other commands in this PR.
64-68: LGTM! Exit code handling properly implemented.The command now correctly exits with code 1 when issues are found, enabling proper CI/CD integration. The use of
print_prettyfor displaying issues is a nice touch for readability.
71-72: LGTM! Standard entry point.The main guard allows the script to be executed standalone, which is useful for testing.
- Introduced a warning log for cases where a SystemExit exception is raised with a non-integer exit code, improving error visibility and debugging capabilities in the run function.
- Removed the redundant _fail function and directly raised Exit(1) for schema validation errors, streamlining the error handling process. - This change enhances code clarity and maintains compliance with Ruff's TRY301 rule.
- Updated the run_check function to return False for non-integer SystemExit codes, enhancing error handling consistency. - This change ensures that unexpected exit codes are managed appropriately, improving robustness in the development scripts.
- Changed the error message for the unsupported strict mode in serve.py to clarify that it is not yet supported by zensical, improving user understanding of the feature's status.
- Enhanced the content validation logic in the lint function to ensure that empty content is correctly identified and handled. - Added a condition to strip whitespace from content before checking for emptiness, improving linting accuracy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/docs/serve.py (1)
55-59: Consider reordering exception handlers for idiomatic Python.While the code functions correctly (KeyboardInterrupt inherits from BaseException, not Exception), Python convention places more specific exception handlers before general ones. Reordering improves readability and maintainability.
🔎 Suggested reordering
try: print_info(f"Starting documentation server at {dev_addr}") # Using run_command for consistency and logging run_command(cmd, capture_output=False) + except KeyboardInterrupt: + print_info("\nDocumentation server stopped") except Exception as e: print_error(f"Failed to start documentation server: {e}") raise Exit(1) from e - except KeyboardInterrupt: - print_info("\nDocumentation server stopped")scripts/dev/all.py (1)
62-81: Solid orchestration logic with minor output consideration.The check sequencing and progress tracking are well-implemented. The lambda correctly captures the
fixparameter, and the progress bar properly tracks execution.Note: Each check function prints its own section header (e.g.,
print_section("Running Linting", "blue")), which may create visual redundancy with the progress bar's "Running {check.name}..." description. This is acceptable for maintaining consistency when checks are run individually vs. collectively, but consider whether a quieter mode might improve UX when running all checks together.src/tux/main.py (1)
50-53: Helpful database error guidance.The docker compose suggestion is user-friendly. Optionally, consider mentioning the
-dflag for detached mode:docker compose up -d.🔎 Optional enhancement for detached mode
- logger.info("To start the database, run: docker compose up") + logger.info("To start the database, run: docker compose up -d")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
scripts/db/schema.py(1 hunks)scripts/dev/all.py(1 hunks)scripts/docs/lint.py(1 hunks)scripts/docs/serve.py(1 hunks)src/tux/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/db/schema.py
- scripts/docs/lint.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/tux/main.py (2)
src/tux/shared/exceptions.py (4)
TuxDatabaseError(73-74)TuxError(50-51)TuxGracefulShutdown(66-67)TuxSetupError(62-63)src/tux/core/app.py (2)
run(111-156)TuxApp(69-536)
scripts/dev/all.py (7)
scripts/core.py (1)
create_app(35-66)scripts/dev/format.py (1)
format_code(18-27)scripts/dev/lint.py (1)
lint(21-41)scripts/dev/lint_docstring.py (1)
lint_docstring(18-27)scripts/dev/pre_commit.py (1)
pre_commit(18-30)scripts/dev/type_check.py (1)
type_check(18-27)scripts/ui.py (4)
create_progress_bar(166-201)print_section(46-49)print_table(139-163)rich_print(52-54)
scripts/docs/serve.py (4)
scripts/core.py (1)
create_app(35-66)scripts/docs/utils.py (1)
has_zensical_config(8-13)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
⏰ 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: Run All Tests (3.13.8)
🔇 Additional comments (8)
scripts/dev/all.py (3)
28-28: Inconsistency between AI summary and actual code.The AI-generated summary states that
appis created withname="dev"andhelp_text="Development tools", but the actual code callscreate_app()with no parameters. If this subcommand app doesn't require these parameters, the summary should be corrected.
31-49: Well-designed check orchestration and error handling.The
Checkdataclass andrun_checkfunction provide clean abstraction for running checks with normalized boolean results. The SystemExit handling correctly returnsFalsefor non-integer exit codes, aligning with the commit message improvements. This design allows all checks to run even if some fail, which is valuable for comprehensive CI/CD feedback.
83-107: Clear summary presentation and appropriate exit handling.The summary table provides a clean visual overview of check results, and the exit logic correctly returns a non-zero code when any check fails. The design appropriately relies on individual checks to print detailed error messages during execution, keeping the summary concise.
src/tux/main.py (5)
12-20: Well-structured exception handling setup.The expanded exception imports and function signature are well-organized. The
noqa: PLR0911is justified here—explicit exception-to-exit-code mapping improves clarity over consolidating returns.
41-48: Clean initialization logic.The debug mode logging and app initialization are straightforward and correct.
63-65: Exit code 130 now follows Unix conventions.The KeyboardInterrupt handler correctly returns 130 (128 + SIGINT) and uses a message consistent with
TuxApp.run(). This addresses the previous review concern.
76-78: Comprehensive catch-all exception handler.The generic exception handler with full traceback logging provides good diagnostic information for unexpected failures.
66-72: Remove unnecessary defensive handling for non-int SystemExit codes.The codebase contains
sys.exit()calls with variable arguments (e.g.,sys.exit(exit_code),sys.exit(main())), which are not guaranteed to be integers. The defensive checks forNoneand non-int codes in this handler are justified and should be retained to handle edge cases where these values may not be integer types.
- Improved the table listing output by dynamically adjusting the column width based on the longest table name, ensuring better readability. - Enhanced error handling in the version function to provide more specific error messages for exceptions, improving user feedback and robustness.
- Moved the KeyboardInterrupt exception handling to the correct position to ensure the documentation server stops gracefully, improving user experience during server operation.
- Extracted the coverage command building logic into a separate function `_build_coverage_cmd` for better readability and maintainability. - Created a new function `_handle_browser_opening` to manage the opening of the coverage report in the browser, improving code organization and clarity. - Enhanced user feedback for browser opening scenarios based on the report format.
- Added exception handling for KeyboardInterrupt to provide user feedback when tests are interrupted. - Updated command printing to use shlex.join for better command formatting.
- Changed the database startup command in the error log from "docker compose up" to "docker compose up -d" for clarity and to ensure the database runs in detached mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/db/version.py (1)
39-41: Consider simplifying the error message.Since
run_commandalready prints detailed error information (stdout, stderr, and the command that failed), appending{e}here adds a less user-friendly CalledProcessError representation. Consider simplifying to just"Failed to get version information"for cleaner output.🔎 Proposed simplification
except CalledProcessError as e: - print_error(f"Failed to get version information: {e}") + print_error("Failed to get version information") sys.exit(1)src/tux/main.py (2)
20-20: Consider expanding debug parameter usage or renaming for clarity.The
debugparameter currently only affects the startup log message but doesn't alter behavior or logging configuration (which happens beforerun()is called, per the docstring note). If this parameter is intended for future expansion, the current usage is fine. Otherwise, consider whether a parameter specifically for this log message variation is necessary, or document that the parameter is primarily for caller visibility rather than functional changes.Also applies to: 42-45
66-72: SystemExit handler is defensively comprehensive.The handling of
SystemExitwith non-int codes is thorough but potentially over-defensive. In typical Python code,SystemExit.codeis eitherNoneor anint. The warning for non-int codes handles an edge case that rarely occurs in practice.If this defensive approach is intentional (e.g., for robustness against third-party code), the current implementation is sound. Otherwise, consider simplifying to only handle the common cases (
Noneandint).scripts/test/coverage.py (1)
32-36: Optional: Consider clarifying --quick and --format interaction.When both
--quickand--formatare specified,--quicktakes precedence and silently ignores the format option. While this behavior is logical (--quick explicitly skips report generation), users might find it confusing.Consider adding a validation check or warning message when both are specified, or use Typer's mutual exclusivity features to prevent the combination.
🔎 Example: Add a validation message in coverage_report
After line 93, add:
if quick and format_type: print_info("Note: --quick takes precedence; --format will be ignored")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
scripts/db/tables.py(1 hunks)scripts/db/version.py(1 hunks)scripts/docs/serve.py(1 hunks)scripts/test/coverage.py(1 hunks)scripts/test/html.py(1 hunks)src/tux/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/db/tables.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/tux/main.py (1)
src/tux/shared/exceptions.py (4)
TuxDatabaseError(73-74)TuxError(50-51)TuxGracefulShutdown(66-67)TuxSetupError(62-63)
scripts/docs/serve.py (4)
scripts/core.py (1)
create_app(35-66)scripts/docs/utils.py (1)
has_zensical_config(8-13)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
scripts/test/html.py (3)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
scripts/db/version.py (4)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_section(46-49)rich_print(52-54)typings/typer/main.pyi (1)
command(28-29)
scripts/test/coverage.py (3)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
⏰ 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: Run All Tests (3.13.8)
- GitHub Check: Seer Code Review
🔇 Additional comments (16)
scripts/db/version.py (4)
1-12: LGTM!The imports are well-organized and all are used. The module docstring clearly describes the command's purpose.
14-14: LGTM!App creation follows the standard pattern using the centralized factory.
17-38: LGTM!The command structure is sound and correctly executes both version checks. The use of run_command with appropriate labels provides good user feedback.
47-48: LGTM!The main guard follows the standard pattern for CLI modules.
src/tux/main.py (3)
50-53: LGTM! Clear database error handling with helpful user guidance.The suggestion to run
docker compose up -dprovides actionable guidance for users encountering database connection issues, and the detached mode flag is appropriate for the use case.
63-65: KeyboardInterrupt exit code now follows Unix convention.The exit code of 130 (128 + SIGINT) correctly follows Unix conventions and addresses the previous review comment about inconsistent handling. The message "Application interrupted by user" is also consistent with the TuxApp layer.
73-75: Follow up: RuntimeError duplicate logging still present.The previous review comment questioned whether logging
RuntimeErrorat the critical level here creates duplicate log entries, sinceTuxApp.run()already logs the sameRuntimeErrorat the error level before re-raising.The current implementation:
TuxApp.run()logs at error level and re-raisesmain.run()catches and logs at critical levelQuestion: Is the severity escalation from error → critical intentional to distinguish initialization failures? If not, consider one of these approaches:
- Remove logging here and return 1 silently (since it's already logged in TuxApp)
- Log only at debug/trace level to avoid duplicate user-visible entries
- Add a comment explaining why both layers log the same error
This aligns with the previous review comment's verification request.
Based on past review comments, this duplicate logging concern was previously raised and remains unresolved.
scripts/docs/serve.py (1)
1-63: LGTM! All previous issues properly addressed.The implementation is solid and all concerns from previous reviews have been correctly resolved:
- ✅ Exit codes are now properly set (Exit(1) on lines 42, 49, and 59)
- ✅ Secure subprocess usage with list-based command construction
- ✅ Comprehensive error handling including KeyboardInterrupt for graceful shutdown
- ✅ Appropriate use of
capture_output=Falsefor the long-running server process, allowing live output to stream to the consoleThe command signature is well-structured with clear type annotations and help text, and the strict mode handling explicitly communicates the lack of support rather than failing silently.
scripts/test/coverage.py (5)
1-20: LGTM: Clean imports and app setup.The imports are well-organized, and the Typer app is properly initialized using the standardized factory function.
23-41: LGTM: Default coverage target properly implemented.The function correctly addresses the previous review concern by providing a default coverage target (
'src/tux') whenspecificis not provided. The command building logic properly handles all coverage options.
44-63: LGTM: Comprehensive browser opening logic with excellent UX.The helper function provides clear feedback for all scenarios (HTML exists, file missing, wrong format, no format). The exception handling at lines 50-53 is appropriate for
webbrowser.open, which can raise various platform-specific exceptions.
66-112: LGTM: Exception handling properly addresses all previous concerns.The error handling is comprehensive and correctly addresses the past review comments about silent exception swallowing:
- KeyboardInterrupt is handled gracefully
- CalledProcessError provides specific feedback about coverage failures
- Unexpected exceptions are caught and logged with context
All exceptions result in appropriate user feedback and exit codes.
115-116: LGTM: Standard entry point.The main block follows the standard Typer pattern for direct script execution.
scripts/test/html.py (3)
1-20: LGTM!The imports and app creation follow the new unified CLI pattern established in this refactor, using the factory-driven approach with
create_appand shared utilities fromscripts.uiandscripts.proc.
47-56: Good defensive handling of browser opening.The nested try-except for
webbrowser.open()correctly prevents browser failures from breaking the test workflow, with appropriate warnings for both browser failures and missing reports.
33-40: Coverage is already configured inpyproject.tomlvia[tool.pytest.ini_options] addopts = ["--cov=src/tux", ...], so the command will collect coverage data correctly. No changes needed.Likely an incorrect or invalid review comment.
- Simplified the RuntimeError exception handling by removing redundant logging, as it is already managed in TuxApp.run(). This change enhances code clarity and maintains consistent error management.
- Added a note indicating that when --quick is used alongside --format, the format option will be ignored. This improves user understanding of command behavior and ensures clarity in reporting.
- Updated the error handling in the version function to remove the exception details from the error message, providing a cleaner output while maintaining user feedback on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/tux/main.py (1)
73-75: RuntimeError comment may be inaccurate for initialization failures.The comment states "RuntimeError is already logged in TuxApp.run()" but this assumes all
RuntimeErrorexceptions originate fromapp.run()(line 48). If aRuntimeErroris raised duringTuxApp()instantiation (line 47), it would not be logged byTuxApp.run()and would be silently caught here without any logging.Consider either:
- Adding debug logging here for initialization-phase errors, or
- Refining the comment to note it applies only to runtime errors, not initialization errors
🔎 Option 1: Add debug logging for completeness
except RuntimeError: - # RuntimeError is already logged in TuxApp.run() + # RuntimeError from app.run() is already logged; log others at debug for visibility + logger.debug("Runtime error during application lifecycle") return 1scripts/test/coverage.py (2)
97-98: Consider making the precedence warning more prominent or restructuring the logic.When both
--quickand--formatare specified, the current implementation silently ignores--format. While the info message at lines 97-98 alerts the user, it may be missed. Consider one of these approaches:
- Elevate to a warning level for better visibility
- Treat
--quickwith--formatas mutually exclusive arguments using Typer's parameter validation- Exit early with an error if both are specified
This would prevent user confusion and make the behavior more explicit.
🔎 Option 1: Elevate to warning (minimal change)
Add a
print_warninghelper toscripts/ui.py(if not already present) and use it:if quick and format_type: - print_info("Note: --quick takes precedence; --format will be ignored") + print_warning("--quick takes precedence; --format will be ignored")🔎 Option 2: Mutually exclusive parameters (better UX)
Use Typer's callback to validate:
from typer import Context, BadParameter def validate_quick_and_format(ctx: Context, param: Any, value: Any) -> Any: """Ensure --quick and --format are not used together.""" if value and ctx.params.get("format_type"): raise BadParameter("--quick and --format cannot be used together") return value # Then in coverage_report: quick: Annotated[ bool, Option("--quick", help="...", callback=validate_quick_and_format), ] = False,
105-106: Consider auto-enabling HTML format when--openis specified.Currently, if a user runs
coverage --openwithout--format html, no HTML report is generated and they receive a message. For better UX, automatically enable HTML format when--openis used.🔎 Proposed enhancement
@app.command(name="coverage") def coverage_report( specific: Annotated[...] = None, format_type: Annotated[...] = None, quick: Annotated[...] = False, fail_under: Annotated[...] = None, open_browser: Annotated[...] = False, ) -> None: """Generate coverage reports.""" print_section("Coverage Report", "blue") + + # Auto-enable HTML format when --open is specified + if open_browser and not format_type and not quick: + format_type = "html" + print_info("Auto-enabling HTML format for browser opening") cmd = _build_coverage_cmd(specific, format_type, quick, fail_under)This eliminates the need for users to remember to specify both flags and provides a more intuitive experience.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/db/version.py(1 hunks)scripts/test/coverage.py(1 hunks)src/tux/main.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/tux/main.py (2)
src/tux/shared/exceptions.py (4)
TuxDatabaseError(73-74)TuxError(50-51)TuxGracefulShutdown(66-67)TuxSetupError(62-63)src/tux/core/app.py (1)
run(111-156)
scripts/test/coverage.py (3)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_info(36-38)print_section(46-49)
scripts/db/version.py (3)
scripts/core.py (1)
create_app(35-66)scripts/proc.py (1)
run_command(14-73)scripts/ui.py (2)
print_section(46-49)rich_print(52-54)
⏰ 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: Run All Tests (3.13.8)
🔇 Additional comments (7)
src/tux/main.py (3)
63-65: Previous review feedback addressed: KeyboardInterrupt exit code now consistent.The
KeyboardInterrupthandler now returns exit code130, aligning with the Unix convention (128 + SIGINT) and matching the behavior inTuxApp.run(). This resolves the inconsistency flagged in the previous review.
73-75: Previous review feedback addressed: RuntimeError logging duplication eliminated.The comment clarifies that
RuntimeErroris already logged inTuxApp.run(), and this handler now simply returns the exit code without additional logging. This eliminates the duplicate log entries flagged in the previous review.
50-78: Exception handling is comprehensive and well-structured.The expanded exception handling provides appropriate responses for each error type with clear logging and correct exit codes. The granular approach improves debuggability and user experience, particularly the helpful Docker Compose hint for database errors (line 52).
scripts/test/coverage.py (1)
108-115: Excellent exception handling implementation.The comprehensive exception handling addresses all the concerns from previous reviews:
- KeyboardInterrupt (line 108-109): Gracefully handles user cancellation with clear messaging
- CalledProcessError (lines 110-112): Specifically catches subprocess failures with informative error messages and proper exit code
- Catch-all Exception (lines 113-115): Prevents silent failures while logging unexpected errors
This is a significant improvement over the previous bare
except: passpattern and ensures users receive clear feedback when something goes wrong.scripts/db/version.py (3)
1-21: Well-structured command initialization.The imports, app creation, and command declaration follow the refactored CLI patterns correctly. The use of the
create_app()factory and Rich-based UI helpers provides consistent output formatting.
23-38: Command execution logic is correct.Both
run_commandcalls properly check database component versions, and the success message is appropriately placed to only execute when both commands complete successfully.
47-48: Standard main guard implementation.The main guard correctly enables direct script execution while supporting integration with the unified CLI architecture.
- Added handling for KeyboardInterrupt in coverage and HTML report generation to ensure graceful exits with a specific exit code. - Simplified the error message in the version function to provide a cleaner output while maintaining user feedback on unexpected errors.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
Modern Modular CLI Refactoring
This refactor transforms the
scripts/directory into a clean, functional architecture. We are moving away from class-based registries to a factory-driven "One File Per Command" system.Summary by Sourcery
Refactor the CLI scripts into a modular Typer-based architecture with a single unified entry point and dedicated subcommands for config, database, development, documentation, testing, and bot operations.
New Features:
Enhancements: