[agent] cleanup: Standardize TODO metadata and restore unused deps script - #383
Open
google-labs-jules[bot] wants to merge 2 commits into
Open
[agent] cleanup: Standardize TODO metadata and restore unused deps script#383google-labs-jules[bot] wants to merge 2 commits into
google-labs-jules[bot] wants to merge 2 commits into
Conversation
…f my changes: - Standardized TODO comments across the codebase to include owner=agent - Restored and fixed the find_stale_unused_deps.py script - Fixed hardcoded command paths for git and other command-line tools for better cross-platform compatibility
Author
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
|
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideStandardizes structured TODO comments to include an owner field across backend, docs, and notebook files, and restores a cross-language dependency staleness detector script that uses git blame and grep to identify stale/unused JS and Python dependencies. Flow diagram for dependency staleness detection scriptflowchart TD
A[start_main] --> B[set_project_root]
B --> C[check_js_deps]
C --> C1[open_frontend_package_json]
C1 --> C2[collect_js_dependencies]
C2 --> C3[for_each_js_dep]
C3 --> C4[get_git_blame_date_frontend_package_json]
C4 --> C5{dep_stale_older_than_90_days}
C5 -->|yes| C6[grep_frontend_src_for_dep]
C6 --> C7{grep_finds_usage}
C7 -->|no| C8[print_stale_unused_js_dep]
C7 -->|yes| C9[next_js_dep]
C8 --> C9
C9 --> C10[end_js_deps]
B --> D[check_py_deps]
D --> D1[open_backend_pyproject_toml]
D1 --> D2[collect_py_dependencies]
D2 --> D3[for_each_py_dep]
D3 --> D4[get_git_blame_date_backend_pyproject]
D4 --> D5{dep_stale_older_than_90_days}
D5 -->|yes| D6[grep_backend_src_for_import_name]
D6 --> D7{grep_finds_usage}
D7 -->|no| D8[print_stale_unused_py_dep]
D7 -->|yes| D9[next_py_dep]
D8 --> D9
D9 --> D10[end_py_deps]
C10 --> E[end_main]
D10 --> E
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- In
find_stale_unused_deps.py, the hardcodedfrontend/andbackend/paths and directgrepinvocations assume a specific repo layout and POSIX tools; consider usingpathlibto derive these paths from the detected project root and Python’sreon file contents instead of shelling out togrepfor better portability (e.g., on Windows). - The Python dependency parsing logic in
check_py_deps()relies on ad‑hoc string splitting, which may fail on more complex version specifiers; using a TOML parser (e.g.,tomllib) to parsepyproject.tomland extracting dependency names structurally would make this more robust to format changes. - In both
check_js_deps()andcheck_py_deps(), the script treats any failure of thegrepsubprocess as ‘unused’; to avoid false positives, you might differentiate between ‘no matches’ and other subprocess errors (e.g., missing command, permission issues) and only classify as unused when the command succeeds but returns no matches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `find_stale_unused_deps.py`, the hardcoded `frontend/` and `backend/` paths and direct `grep` invocations assume a specific repo layout and POSIX tools; consider using `pathlib` to derive these paths from the detected project root and Python’s `re` on file contents instead of shelling out to `grep` for better portability (e.g., on Windows).
- The Python dependency parsing logic in `check_py_deps()` relies on ad‑hoc string splitting, which may fail on more complex version specifiers; using a TOML parser (e.g., `tomllib`) to parse `pyproject.toml` and extracting dependency names structurally would make this more robust to format changes.
- In both `check_js_deps()` and `check_py_deps()`, the script treats any failure of the `grep` subprocess as ‘unused’; to avoid false positives, you might differentiate between ‘no matches’ and other subprocess errors (e.g., missing command, permission issues) and only classify as unused when the command succeeds but returns no matches.
## Individual Comments
### Comment 1
<location path="scripts/find_stale_unused_deps.py" line_range="8-17" />
<code_context>
+from datetime import datetime, timedelta
+
+
+def get_git_blame_date(file_path, line_number):
+ try:
+ result = subprocess.run( # noqa: S603
+ [
+ "git",
+ "blame",
+ "--line-porcelain",
+ "-L",
+ f"{line_number},{line_number}",
+ file_path,
+ ],
+ capture_output=True,
+ text=True,
+ check=True,
+ )
+ for line in result.stdout.splitlines():
+ if line.startswith("author-time "):
+ timestamp = int(line.split(" ")[1])
+ return datetime.fromtimestamp(timestamp)
+ except subprocess.CalledProcessError:
+ pass
+ return datetime.now()
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid using `datetime.now()` as a fallback for blame failures to prevent hiding truly stale deps.
When `git blame` fails (untracked file, no repo, or running outside Git), this returns `datetime.now()`, making every such dep appear fresh and hiding stale ones. Instead, return `None` and let callers either skip these entries or emit a warning so missing blame data doesn’t under-report stale/unused deps.
Suggested implementation:
```python
def get_git_blame_date(file_path, line_number):
"""
Return the datetime of the last change to a given file/line according to git blame.
If blame information cannot be retrieved (e.g., non-git directory, untracked file,
or no author-time line), return None so callers can decide how to handle missing data.
"""
try:
result = subprocess.run( # noqa: S603
[
"git",
"blame",
"--line-porcelain",
"-L",
f"{line_number},{line_number}",
file_path,
],
capture_output=True,
text=True,
check=True,
)
except subprocess.CalledProcessError:
# Git blame failed (e.g. not a repo, file not tracked); report missing data.
return None
for line in result.stdout.splitlines():
if line.startswith("author-time "):
timestamp = int(line.split(" ")[1])
return datetime.fromtimestamp(timestamp)
# No author-time found; treat as missing blame data.
return None
```
Callers of `get_git_blame_date` will need to be updated to handle a `None` return value. Specifically:
1. Any code that currently assumes a `datetime` return (e.g., comparing with `datetime.now()` or subtracting `timedelta`) must first check for `None` and either:
- Skip those entries when computing staleness, or
- Emit a warning/log message indicating that blame information was unavailable for that file/line.
2. If type hints are used in this file, the return type of `get_git_blame_date` should be updated to `Optional[datetime]`, and the callers should reflect that in their annotations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+8
to
+17
| def get_git_blame_date(file_path, line_number): | ||
| try: | ||
| result = subprocess.run( # noqa: S603 | ||
| [ | ||
| "git", | ||
| "blame", | ||
| "--line-porcelain", | ||
| "-L", | ||
| f"{line_number},{line_number}", | ||
| file_path, |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid using datetime.now() as a fallback for blame failures to prevent hiding truly stale deps.
When git blame fails (untracked file, no repo, or running outside Git), this returns datetime.now(), making every such dep appear fresh and hiding stale ones. Instead, return None and let callers either skip these entries or emit a warning so missing blame data doesn’t under-report stale/unused deps.
Suggested implementation:
def get_git_blame_date(file_path, line_number):
"""
Return the datetime of the last change to a given file/line according to git blame.
If blame information cannot be retrieved (e.g., non-git directory, untracked file,
or no author-time line), return None so callers can decide how to handle missing data.
"""
try:
result = subprocess.run( # noqa: S603
[
"git",
"blame",
"--line-porcelain",
"-L",
f"{line_number},{line_number}",
file_path,
],
capture_output=True,
text=True,
check=True,
)
except subprocess.CalledProcessError:
# Git blame failed (e.g. not a repo, file not tracked); report missing data.
return None
for line in result.stdout.splitlines():
if line.startswith("author-time "):
timestamp = int(line.split(" ")[1])
return datetime.fromtimestamp(timestamp)
# No author-time found; treat as missing blame data.
return NoneCallers of get_git_blame_date will need to be updated to handle a None return value. Specifically:
- Any code that currently assumes a
datetimereturn (e.g., comparing withdatetime.now()or subtractingtimedelta) must first check forNoneand either:- Skip those entries when computing staleness, or
- Emit a warning/log message indicating that blame information was unavailable for that file/line.
- If type hints are used in this file, the return type of
get_git_blame_dateshould be updated toOptional[datetime], and the callers should reflect that in their annotations.
…updates I made: - Standardized TODO comments across the codebase to include owner=agent - Restored and fixed the find_stale_unused_deps.py script - Fixed hardcoded command paths for git and other search utilities for better cross-platform compatibility
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Agent Report Summary
find_stale_unused_deps.pyscript. Modified system binaries to use standard path variables.Scan Results
TODOs
owner=agent.Convention Enforcement
find_stale_unused_deps.pyindentation fixes, and cross-platform portability fixes.TODO(priority=..., complexity=..., owner=...)Verification
python scripts/extract_todos_structured.py,uv run pytest tests/,npm test,uv run ruff check --fix src/ scripts/ tests/,uv run ruff format src/ scripts/ tests/Risk Assessment
find_stale_unused_deps.py) that does not affect application functionality.Next Steps
Machine Metadata
PR created automatically by Jules for task 12626500959362096490 started by @MasumRab
Summary by Sourcery
Standardize TODO annotations with owner metadata across backend, docs, and notebooks, and restore a utility script for detecting stale or unused dependencies.
Enhancements:
Chores: