Conversation
|
Review these changes at https://app.gitnotebooks.com/AlphaSphereDotAI/visualizr/pull/387 |
Reviewer's GuideThis PR refactors the pre-push git tag validation from an inline Bash script to a dedicated Python utility by updating trunk.yaml to invoke git_tag.py with its own runtime and dependencies, adds the new Python implementation under .trunk/utils, and introduces a requirements file for its dependencies. Class diagram for the new git tag validation utility (git_tag.py)classDiagram
class git_tag {
+read_version_from_pyproject(path: Path) str
+get_exact_tag_for_head(repo: Repo) str | None
+main() int
}
class Logger {
+info(msg)
+error(msg)
}
class Repo {
+head
+active_branch
+git
}
class Path {
+cwd()
+exists()
+read_text()
}
class tomli_loads
class rich_console_Console
class rich_logging_RichHandler
git_tag --> Logger : uses
git_tag --> Repo : uses
git_tag --> Path : uses
git_tag --> tomli_loads : uses
git_tag --> rich_console_Console : uses
git_tag --> rich_logging_RichHandler : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe hook Changes
Sequence DiagramsequenceDiagram
participant main as main()
participant repo as GitPython<br/>(Repo)
participant toml as pyproject.toml
participant logger as Logger
main->>repo: Open repository at ../../
logger->>logger: Log repo location
main->>repo: Check if HEAD is detached<br/>and on main branch
alt Non-main or detached
main->>logger: Exit with 0
end
main->>repo: Get exact tag for HEAD<br/>(git describe --exact-match)
alt No exact tag found
main->>logger: Exit with 0
end
main->>toml: read_version_from_pyproject()
alt pyproject.toml missing/invalid
logger->>logger: Log error
main->>logger: Exit with 1
end
main->>main: Compare tag vs version
alt Mismatch
logger->>logger: Log error
main->>logger: Exit with 1
else Match
logger->>logger: Log success
main->>logger: Exit with 0
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
MH0386
changed the title
Summary of ChangesHello @MH0386, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the pre-push git hook responsible for validating that the current git tag matches the version defined in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider detecting the repo root dynamically (e.g., using Repo(search_parent_directories=True)) rather than hardcoding a "../../" path.
- Update the branch check to include both "main" and "master" (or detect the default branch dynamically) to match the original script’s behavior.
- Use the repository root (repo.working_tree_dir) when resolving the pyproject.toml path instead of relying on the current working directory in .trunk/utils.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider detecting the repo root dynamically (e.g., using Repo(search_parent_directories=True)) rather than hardcoding a "../../" path.
- Update the branch check to include both "main" and "master" (or detect the default branch dynamically) to match the original script’s behavior.
- Use the repository root (repo.working_tree_dir) when resolving the pyproject.toml path instead of relying on the current working directory in .trunk/utils.
## Individual Comments
### Comment 1
<location> `.trunk/utils/git_tag.py:65` </location>
<code_context>
+ if not tag:
+ logger.info("No tag found for the current commit.")
+ return 0
+ version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml")
+ if not version:
+ logger.error("pyproject.toml not found or version not set.")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Assumes pyproject.toml is in the current working directory.
This approach may fail if the script is run outside the project root. Consider resolving the path from the repository root or allowing it to be configurable.
Suggested implementation:
```python
# Resolve pyproject.toml from the repository root
pyproject_path = Path(repo.working_tree_dir) / "pyproject.toml"
version: str = read_version_from_pyproject(pyproject_path)
```
If you want to make the path to `pyproject.toml` configurable, you could add an optional parameter to the function signature (e.g., `pyproject_path: Path | None = None`) and use it if provided, otherwise default to the repository root as above.
</issue_to_address>
### Comment 2
<location> `.trunk/utils/git_tag.py:38-41` </location>
<code_context>
+ """Return exact tag for HEAD commit, or None."""
+ try:
+ commit_hex: str = repo.head.commit.hexsha
+ except Exception:
+ return None
+ try:
+ return repo.git.describe("--tags", "--exact-match", commit_hex)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching broad Exception may obscure errors.
Consider catching only specific exceptions relevant to this code, such as AttributeError or ValueError, to prevent masking other errors.
```suggestion
try:
commit_hex: str = repo.head.commit.hexsha
except (AttributeError, ValueError):
return None
```
</issue_to_address>
### Comment 3
<location> `.trunk/utils/git_tag.py:26-33` </location>
<code_context>
+ if not path.exists():
+ msg: str = f"{path} does not exist."
+ raise FileNotFoundError(msg)
+ data: dict = loads(path.read_text(encoding="utf-8"))
+ project: dict = data["project"]
+ return project["version"]
</code_context>
<issue_to_address>
**suggestion:** No error handling for invalid TOML or missing keys.
Add error handling for malformed TOML and missing 'project' or 'version' keys to avoid uncaught exceptions and improve error messages.
```suggestion
def read_version_from_pyproject(path: Path) -> str:
"""Return the package version from pyproject.toml (PEP 621 style for uv)."""
if not path.exists():
msg: str = f"{path} does not exist."
raise FileNotFoundError(msg)
try:
data: dict = loads(path.read_text(encoding="utf-8"))
except Exception as e:
msg: str = f"Failed to parse TOML from {path}: {e}"
raise ValueError(msg) from e
try:
project: dict = data["project"]
except KeyError:
msg: str = f"'project' section missing in {path}"
raise KeyError(msg)
try:
return project["version"]
except KeyError:
msg: str = f"'version' key missing in 'project' section of {path}"
raise KeyError(msg)
```
</issue_to_address>
### Comment 4
<location> `.trunk/utils/git_tag.py:36` </location>
<code_context>
+ return project["version"]
+
+
+def get_exact_tag_for_head(repo: Repo) -> str | None:
+ """Return exact tag for HEAD commit, or None."""
+ try:
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring nested logic and splitting repeated code into helpers to improve clarity and reduce boilerplate.
```suggestion
The overall flow is fine, but you can trim down some of the nested logic and repeated boilerplate:
1. Merge the two `try/except` in `get_exact_tag_for_head`, and catch both errors at once.
2. Use `tomli.load` directly on a file handle instead of `loads(path.read_text())`.
3. Extract logging setup and branch‐checking into small helpers.
For example:
```python
# 1) Simplify get_exact_tag_for_head
from git import GitCommandError
def get_exact_tag_for_head(repo: Repo) -> str | None:
try:
commit = repo.head.commit.hexsha
return repo.git.describe("--tags", "--exact-match", commit)
except (GitCommandError, AttributeError):
return None
```
```python
# 2) Read toml via a file handle
import tomli
def read_version_from_pyproject(path: Path) -> str:
if not path.exists():
raise FileNotFoundError(f"{path} does not exist.")
with path.open("rb") as f:
data = tomli.load(f)
return data["project"]["version"]
```
```python
# 3) Break up main() and move logging setup
from logging import basicConfig, INFO, getLogger
from rich.logging import RichHandler
from rich.console import Console
logger = getLogger(__name__)
def setup_logging() -> None:
basicConfig(
level=INFO,
handlers=[RichHandler(console=Console(), rich_tracebacks=True)],
format="%(message)s",
)
def is_on_main_branch(repo: Repo) -> bool:
try:
return repo.active_branch.name == "main"
except Exception:
logger.info("Detached HEAD or unknown branch.")
return False
def main() -> int:
setup_logging()
repo = Repo("../../")
logger.info("Repository at %s", repo.working_tree_dir)
if not is_on_main_branch(repo):
return 0
tag = get_exact_tag_for_head(repo)
if not tag:
logger.info("No tag found for the current commit.")
return 0
version = read_version_from_pyproject(Path.cwd() / "pyproject.toml")
if tag != version:
logger.error("Tag '%s' does not match version '%s'.", tag, version)
return 1
logger.info("Tag '%s' matches version '%s'.", tag, version)
return 0
if __name__ == "__main__":
sys_exit(main())
```
These changes keep all functionality intact but reduce indentation, remove redundant error blocks, and make each helper single‐purpose.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if not tag: | ||
| logger.info("No tag found for the current commit.") | ||
| return 0 | ||
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") |
There was a problem hiding this comment.
suggestion (bug_risk): Assumes pyproject.toml is in the current working directory.
This approach may fail if the script is run outside the project root. Consider resolving the path from the repository root or allowing it to be configurable.
Suggested implementation:
# Resolve pyproject.toml from the repository root
pyproject_path = Path(repo.working_tree_dir) / "pyproject.toml"
version: str = read_version_from_pyproject(pyproject_path)If you want to make the path to pyproject.toml configurable, you could add an optional parameter to the function signature (e.g., pyproject_path: Path | None = None) and use it if provided, otherwise default to the repository root as above.
| try: | ||
| commit_hex: str = repo.head.commit.hexsha | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
suggestion (bug_risk): Catching broad Exception may obscure errors.
Consider catching only specific exceptions relevant to this code, such as AttributeError or ValueError, to prevent masking other errors.
| try: | |
| commit_hex: str = repo.head.commit.hexsha | |
| except Exception: | |
| return None | |
| try: | |
| commit_hex: str = repo.head.commit.hexsha | |
| except (AttributeError, ValueError): | |
| return None |
| return project["version"] | ||
|
|
||
|
|
||
| def get_exact_tag_for_head(repo: Repo) -> str | None: |
There was a problem hiding this comment.
issue (complexity): Consider refactoring nested logic and splitting repeated code into helpers to improve clarity and reduce boilerplate.
| def get_exact_tag_for_head(repo: Repo) -> str | None: | |
| The overall flow is fine, but you can trim down some of the nested logic and repeated boilerplate: | |
| 1. Merge the two `try/except` in `get_exact_tag_for_head`, and catch both errors at once. | |
| 2. Use `tomli.load` directly on a file handle instead of `loads(path.read_text())`. | |
| 3. Extract logging setup and branch‐checking into small helpers. | |
| For example: | |
| ```python | |
| # 1) Simplify get_exact_tag_for_head | |
| from git import GitCommandError | |
| def get_exact_tag_for_head(repo: Repo) -> str | None: | |
| try: | |
| commit = repo.head.commit.hexsha | |
| return repo.git.describe("--tags", "--exact-match", commit) | |
| except (GitCommandError, AttributeError): | |
| return None |
# 2) Read toml via a file handle
import tomli
def read_version_from_pyproject(path: Path) -> str:
if not path.exists():
raise FileNotFoundError(f"{path} does not exist.")
with path.open("rb") as f:
data = tomli.load(f)
return data["project"]["version"]# 3) Break up main() and move logging setup
from logging import basicConfig, INFO, getLogger
from rich.logging import RichHandler
from rich.console import Console
logger = getLogger(__name__)
def setup_logging() -> None:
basicConfig(
level=INFO,
handlers=[RichHandler(console=Console(), rich_tracebacks=True)],
format="%(message)s",
)
def is_on_main_branch(repo: Repo) -> bool:
try:
return repo.active_branch.name == "main"
except Exception:
logger.info("Detached HEAD or unknown branch.")
return False
def main() -> int:
setup_logging()
repo = Repo("../../")
logger.info("Repository at %s", repo.working_tree_dir)
if not is_on_main_branch(repo):
return 0
tag = get_exact_tag_for_head(repo)
if not tag:
logger.info("No tag found for the current commit.")
return 0
version = read_version_from_pyproject(Path.cwd() / "pyproject.toml")
if tag != version:
logger.error("Tag '%s' does not match version '%s'.", tag, version)
return 1
logger.info("Tag '%s' matches version '%s'.", tag, version)
return 0
if __name__ == "__main__":
sys_exit(main())These changes keep all functionality intact but reduce indentation, remove redundant error blocks, and make each helper single‐purpose.
|
|
Thank you for your contribution @MH0386! Your pull request has been merged. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively replaces the inline bash script for git tag validation with a more maintainable and feature-rich Python utility. The use of libraries like GitPython, tomli, and rich is a great choice. I've identified a critical issue regarding the path resolution for pyproject.toml that would cause the script to fail, along with several other high and medium severity issues related to error handling, hardcoded values, and a potential regression in functionality. My review includes specific suggestions to improve the script's robustness and correctness. Please take a look at the detailed comments.
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | ||
| if not version: | ||
| logger.error("pyproject.toml not found or version not set.") | ||
| return 1 |
There was a problem hiding this comment.
This block has a few issues that will prevent the script from working correctly:
- Incorrect Path: The script is executed from
.trunk/utils, soPath.cwd() / "pyproject.toml"incorrectly looks forpyproject.tomlin that subdirectory instead of the project root. This will cause aFileNotFoundError. - Unhandled Exceptions:
read_version_from_pyprojectcan raise aKeyErrorif theversionfield is missing, which would crash the script. This should be handled gracefully. - Dead Code: The
if not version:check is unreachable becauseread_version_from_pyprojecteither returns a string or raises an exception.
I suggest replacing this block to fix the path and add proper error handling.
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | |
| if not version: | |
| logger.error("pyproject.toml not found or version not set.") | |
| return 1 | |
| try: | |
| version: str = read_version_from_pyproject( | |
| Path(repo.working_tree_dir) / "pyproject.toml" | |
| ) | |
| except FileNotFoundError: | |
| logger.error("pyproject.toml not found in repository root.") | |
| return 1 | |
| except KeyError: | |
| logger.error("Could not find 'version' in [project] table of pyproject.toml.") | |
| return 1 |
| # detached HEAD or no branch | ||
| logger.info("Detached HEAD or unknown branch.") | ||
| return 0 | ||
| if branch != "main": |
There was a problem hiding this comment.
The previous bash script checked for both main and master branches. This new implementation only checks for main. To maintain compatibility and avoid breaking workflows on repositories that still use master, it's better to check for both.
| if branch != "main": | |
| if branch not in ("main", "master"): |
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Using except Exception: is too broad and can hide unexpected bugs. It's better to catch a more specific exception. repo.head.commit.hexsha can raise a ValueError if HEAD is unresolvable (e.g., in a new repository with no commits), so catching that is more precise.
| except Exception: | |
| return None | |
| except ValueError: | |
| return None |
|
|
||
| def main() -> int: | ||
| """Validate git tag against pyproject.toml version.""" | ||
| repo: Repo = Repo("../../") |
There was a problem hiding this comment.
The hardcoded relative path ../../ to the repository root is brittle. If this script is moved or its execution context changes, this path will break. A more robust approach is to let GitPython find the repository root by searching parent directories from the script's location.
| repo: Repo = Repo("../../") | |
| repo: Repo = Repo(Path(__file__).parent, search_parent_directories=True) |
| except Exception: | ||
| # detached HEAD or no branch | ||
| logger.info("Detached HEAD or unknown branch.") | ||
| return 0 |
There was a problem hiding this comment.
Using except Exception: is too broad. When the repository is in a detached HEAD state, repo.active_branch.name raises a TypeError. Catching this specific exception is safer and makes the code's intent clearer.
| except Exception: | |
| # detached HEAD or no branch | |
| logger.info("Detached HEAD or unknown branch.") | |
| return 0 | |
| except TypeError: | |
| # detached HEAD or no branch | |
| logger.info("Detached HEAD or unknown branch.") | |
| return 0 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the git tag validation logic from a bash script embedded in trunk.yaml to a standalone Python utility. The changes improve maintainability, error handling, and logging.
Key Changes:
- Replaces inline bash script with a structured Python module (
git_tag.py) - Adds proper dependency management via
requirements.txt - Improves logging with rich console output and better error messages
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.trunk/utils/requirements.txt |
Adds Python dependencies (GitPython, tomli, rich) for the new utility |
.trunk/utils/git_tag.py |
New Python module that validates git tags against pyproject.toml version with improved error handling and logging |
.trunk/trunk.yaml |
Updates action definition to run the Python script instead of inline bash, configuring runtime and dependencies |
|
|
||
| def main() -> int: | ||
| """Validate git tag against pyproject.toml version.""" | ||
| repo: Repo = Repo("../../") |
There was a problem hiding this comment.
The hardcoded relative path '../../' is fragile and assumes the script is always run from .trunk/utils. Consider using Path(__file__).parent.parent.parent to compute the repository root dynamically, or better yet, use Repo(search_parent_directories=True) which will search upward for the git repository automatically.
| repo: Repo = Repo("../../") | |
| repo: Repo = Repo(search_parent_directories=True) |
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | ||
| if not version: | ||
| logger.error("pyproject.toml not found or version not set.") |
There was a problem hiding this comment.
The check if not version: on line 66 will never be true because read_version_from_pyproject() raises a FileNotFoundError or KeyError rather than returning an empty string. The function should either be wrapped in a try-except block, or this conditional should be removed.
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | |
| if not version: | |
| logger.error("pyproject.toml not found or version not set.") | |
| try: | |
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | |
| except (FileNotFoundError, KeyError) as e: | |
| logger.error("pyproject.toml not found or version not set: %s", e) |
| try: | ||
| commit_hex: str = repo.head.commit.hexsha | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Catching bare Exception is too broad. Consider catching specific exceptions like ValueError or checking if repo.head.is_detached instead of relying on exception handling for control flow.
| try: | |
| commit_hex: str = repo.head.commit.hexsha | |
| except Exception: | |
| return None | |
| if repo.head.is_detached: | |
| return None | |
| commit_hex: str = repo.head.commit.hexsha |
| try: | ||
| branch: str = repo.active_branch.name | ||
| except Exception: | ||
| # detached HEAD or no branch | ||
| logger.info("Detached HEAD or unknown branch.") | ||
| return 0 |
There was a problem hiding this comment.
Catching bare Exception is too broad. Consider catching TypeError specifically, which is raised when HEAD is detached, or check repo.head.is_detached before accessing active_branch.
| try: | |
| branch: str = repo.active_branch.name | |
| except Exception: | |
| # detached HEAD or no branch | |
| logger.info("Detached HEAD or unknown branch.") | |
| return 0 | |
| if repo.head.is_detached: | |
| # detached HEAD or no branch | |
| logger.info("Detached HEAD or unknown branch.") | |
| return 0 | |
| branch: str = repo.active_branch.name |
| if not tag: | ||
| logger.info("No tag found for the current commit.") | ||
| return 0 | ||
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") |
There was a problem hiding this comment.
Using Path.cwd() assumes the current working directory is the repository root, but the script is configured to run_from: .trunk/utils in trunk.yaml. This will fail to find pyproject.toml. Use repo.working_tree_dir or compute the path relative to the repository root discovered on line 50.
| version: str = read_version_from_pyproject(Path.cwd() / "pyproject.toml") | |
| version: str = read_version_from_pyproject(Path(repo.working_tree_dir) / "pyproject.toml") |
🧪 CI InsightsHere's what we observed from your CI run for 8c8e847. 🟢 All jobs passed!But CI Insights is watching 👀 |
Summary by Sourcery
Replace the inline bash git-tag validation hook with a dedicated Python utility and update the Trunk CI configuration to run it
New Features: