feat: pre-commit refactor, allow cli arguments #5
Conversation
matiasdaloia
added
bug
|
Warning Rate limit exceeded@matiasdaloia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughMigrates the CLI from argparse to Click, consolidates utility logic into the main module, adds logging and subprocess sanitization, adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Click as Click CLI
participant Main as check_undeclared_software
participant Git as Git subprocess
participant ScanOSS as scanoss-py subprocess
participant Logger as Logging
User->>Click: run command with options
Click->>Main: invoke main(ctx, api_url, api_key, ...)
Main->>Logger: configure_logging(debug)
Main->>Git: get_staged_files()
Git-->>Main: list of staged files or []
alt no staged files
Main-->>User: exit 0 (no files staged)
else staged files present
Main->>Main: build scan command (sanitize api_key)
Main->>ScanOSS: run_subcommand(scan_cmd)
ScanOSS-->>Main: CompletedProcess (scan writes results)
Main->>ScanOSS: run_subcommand(results_cmd)
ScanOSS-->>Main: JSON results
Main->>Main: parse results
alt pending results found
Main->>Main: present_results_table(results, output_path)
Main-->>User: exit 1 (failure)
else no pending results
Main-->>User: exit 0 (success)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
.gitignore (1)
32-33: Good add; consider common env patterns.Recommend also ignoring variant files while keeping an example tracked.
.scanoss +.env +.env.* +!.env.examplesetup.cfg (1)
31-31: Use a compatible range and keep pins DRY with requirements.txt.
- Recommend click>=8.1,<9 to allow patch updates.
- Ensure requirements.txt matches to avoid resolver conflicts.
- click==8.1.8 + click>=8.1,<9src/hooks/check_undeclared_software.py (8)
58-78: Improve error handling in get_staged_files (use logging.exception, avoid bare except).Replace
logging.error(...)in except blocks withlogging.exception(...)and avoid a blindexcept Exception.- except subprocess.CalledProcessError as e: - logging.error(f"Git command failed: {e}") - except Exception as e: - logging.error(f"{e}") + except subprocess.CalledProcessError: + logging.exception("Git command failed while listing staged files") + except OSError: + logging.exception("OS error while listing staged files") return []Based on static analysis hints.
80-90: Add a timeout to subprocess to avoid hanging pre-commit.Long-running or wedged subprocesses will block commits.
-def run_subcommand( - command: list[str], check: bool = True -) -> subprocess.CompletedProcess[str]: +def run_subcommand( + command: list[str], check: bool = True, timeout: int | None = 300 +) -> subprocess.CompletedProcess[str]: - return subprocess.run(command, check=check, capture_output=True, text=True) + return subprocess.run( + command, check=check, capture_output=True, text=True, timeout=timeout + )And keep callers defaulted. Optionally catch
subprocess.TimeoutExpiredwhere invoked to log and exit cleanly.
118-160: Resolve output path before comparison to pick the correct hint.Equality against DEFAULT path can fail if one is absolute. Resolve both.
- if output_path == DEFAULT_SCANOSS_SCAN_RESULTS_FILE: + if output_path.resolve() == DEFAULT_SCANOSS_SCAN_RESULTS_FILE.resolve(): command_msg = "Run [green]'scanoss-cc'[/green] in the terminal to view the results in more detail." else: command_msg = f"Run [green]'scanoss-cc --input {output_path}'[/green] in the terminal to view the results in more detail."
197-205: Make output option path safer.Resolve the path and ensure it’s writable to fail fast on bad values.
-@click.option( +@click.option( "-o", "--output", - type=click.Path(path_type=Path), + type=click.Path(path_type=Path, resolve_path=True, writable=True), default=DEFAULT_SCANOSS_SCAN_RESULTS_FILE, help=f"Output file for scan results (default: {DEFAULT_SCANOSS_SCAN_RESULTS_FILE})", )
273-285: Stop the spinner and log full traceback on failures.One error path exits without stopping the status spinner and uses logging.error.
- process_status = console.status("[bold green] Running SCANOSS scan...") + process_status = console.status("[bold green] Running SCANOSS scan...") logging.debug(f"Executing command: {sanitize_scan_command(scanoss_scan_cmd)}") process_status.start() try: scan_result = run_subcommand(scanoss_scan_cmd) - except subprocess.CalledProcessError as e: - logging.error( - f"Error running scanoss command with return code {e.returncode}: {e.stderr}" - ) - ctx.exit(EXIT_FAILURE) + except subprocess.CalledProcessError: + process_status.stop() + logging.exception("scanoss-py scan failed") + ctx.exit(EXIT_FAILURE) + except (FileNotFoundError, OSError): + process_status.stop() + logging.exception("scanoss-py not found or OS error during execution") + ctx.exit(EXIT_FAILURE)Based on static analysis hints.
286-301: Prefer logging.exception in exception handlers.Switch logging.error(...) to logging.exception(...) to include tracebacks.
- except OSError as e: - logging.error(f"Failed to create output directory {output.parent}: {e}") + except OSError: + logging.exception(f"Failed to create output directory {output.parent}") process_status.stop() ctx.exit(EXIT_FAILURE) @@ - except OSError as e: - logging.error(f"Failed to write scan results to {output}: {e}") + except OSError: + logging.exception(f"Failed to write scan results to {output}") process_status.stop() ctx.exit(EXIT_FAILURE)Based on static analysis hints.
314-324: Also log traceback when JSON parsing fails.- except json.JSONDecodeError: - logging.error("Failed to parse JSON response from SCANOSS") + except json.JSONDecodeError: + logging.exception("Failed to parse JSON response from SCANOSS") ctx.exit(EXIT_FAILURE)Based on static analysis hints.
48-56: Optional: hook RichHandler into logging for consistent console output.This keeps logs styled via Rich while preserving your format.
import logging @@ -logging.basicConfig(level=log_level, format=LOG_FORMAT, force=True) +from rich.logging import RichHandler +logging.basicConfig( + level=log_level, + format=LOG_FORMAT, + handlers=[RichHandler(rich_tracebacks=True)], + force=True, +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks).pre-commit-hooks.yaml(1 hunks)requirements.txt(1 hunks)scanoss.json(2 hunks)setup.cfg(1 hunks)src/hooks/__init__.py(1 hunks)src/hooks/check_undeclared_software.py(3 hunks)src/hooks/utils.py(0 hunks)
💤 Files with no reviewable changes (1)
- src/hooks/utils.py
🧰 Additional context used
🪛 Ruff (0.14.1)
src/hooks/check_undeclared_software.py
66-66: Starting a process with a partial executable path
(S607)
74-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
75-75: Do not catch blind exception: Exception
(BLE001)
76-76: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
89-89: subprocess call: check for execution of untrusted input
(S603)
281-283: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
290-290: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
299-299: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
322-322: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (6)
scanoss.json (1)
5-5: LGTM.Formatting-only change; behavior identical.
src/hooks/__init__.py (2)
1-25: License header addition is fine.SPDX and MIT text properly included in module docstring.
25-25: Confirm version alignment.Ensure version ("0.2.0") matches the intended next release and setup.cfg’s
version = attr: hooks.__version__.requirements.txt (1)
2-3: Resolve duplication of click and rich dependencies between setup.cfg and requirements.txt.Dependencies are pinned identically in both files (setup.cfg lines 30–31 and requirements.txt lines 2–3). Verify whether this duplication is intentional for your project workflow. If requirements.txt should rely on setup.cfg, remove it from requirements.txt. If both files are necessary, consider relaxing the hard pin on click from
==8.1.8to>=8.1,<9to allow security patches while maintaining stability..pre-commit-hooks.yaml (1)
8-9: The review comment's core assertion is incorrect. pre-commit supports a verbose key as an optional boolean available in the hook repository manifest (.pre-commit-hooks.yaml). The original comment falsely claims the key is unsupported and will break manifest validation. Conversely,verbose: trueandrequire_serial: trueare both valid configuration options and do not require removal.Likely an incorrect or invalid review comment.
src/hooks/check_undeclared_software.py (1)
312-333: Exit-code contract for 'scanoss-py results --has-pending' is correct.I verified the scanoss-py v1.38.0 source code (cli.py and results.py). The
--has-pendingflag filters results for pending items and exits with code 1 if any are found, or 0 if none exist. Your code's assumptions align with this contract:
- returncode 1 → pending results exist
- returncode 0 → clean (no pending)
- other codes → errors
The implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
38-38: Mirrors the--versionto--helpchange in release.yml (line 32).This change is identical to the one in
.github/workflows/release.yml. See the verification comment in that file for concerns about the Click migration's--versionsupport.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
8-8: Cosmetic quote style updates in YAML.The changes from single to double quotes are stylistic and have no functional impact. These may reflect a formatting preference or linter convention being applied consistently. No action needed unless this conflicts with project style guidelines.
Also applies to: 19-19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(3 hunks).github/workflows/test.yml(1 hunks)CHANGELOG.md(2 hunks)src/hooks/__init__.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/init.py
matiasdaloia
deleted the
fix/mdaloia/SP-3538-Pre-commit-hook-failing-when-committing-a-no-match-file
branch
Summary by CodeRabbit
New Features
Chores