[agent] cleanup: Enforce owner metadata on TODOs and identify unused dependencies - #369
Open
google-labs-jules[bot] wants to merge 1 commit into
Open
[agent] cleanup: Enforce owner metadata on TODOs and identify unused dependencies#369google-labs-jules[bot] wants to merge 1 commit into
google-labs-jules[bot] wants to merge 1 commit into
Conversation
…dependencies Adds owner=agent to legacy structured TODO comments that were missing owner attribution. Excludes scripts/extract_todos_structured.py from parsing its own comment templates. Adds scripts/find_stale_unused_deps.py as a standalone MVP to identify >90-day-old unused dependencies across frontend and backend manifests.
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 |
Reviewer's GuideNormalizes structured TODO comments to include owner metadata, updates the TODO extraction script to avoid self-parsing and document the new TODO format, and adds a standalone utility to detect stale, unused dependencies in frontend and backend projects using git blame and simple source scans. Class diagram for helper functions in find_stale_unused_deps.pyclassDiagram
class FindStaleUnusedDepsScript {
<<module>>
AGE_THRESHOLD_DAYS : int
REPORT_FILE : str
main()
}
class FrontendChecker {
<<function_group>>
check_frontend() list~str~
}
class BackendChecker {
<<function_group>>
check_backend() list~str~
}
class GitBlameHelper {
<<function_group>>
get_file_blame_lines(filepath) list~dict~
get_age_days(timestamp) int
}
class UsageHeuristic {
<<function_group>>
is_used(dep_name, search_dirs, extensions) bool
}
FindStaleUnusedDepsScript ..> FrontendChecker : uses
FindStaleUnusedDepsScript ..> BackendChecker : uses
FrontendChecker ..> GitBlameHelper : uses
BackendChecker ..> GitBlameHelper : uses
FrontendChecker ..> UsageHeuristic : uses
BackendChecker ..> UsageHeuristic : uses
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 2 issues, and left some high level feedback:
- In
is_used, the recursive walk currently only skipsnode_modulesand.venv; consider also excluding other generated or vendor directories (e.g.dist,.next,.git,__pycache__) to reduce noise and improve performance of the dependency scan. - The
AGE_THRESHOLD_DAYSandREPORT_FILEsettings infind_stale_unused_deps.pyare hardcoded; exposing these as CLI arguments or environment variables would make the script more flexible for different workflows and thresholds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `is_used`, the recursive walk currently only skips `node_modules` and `.venv`; consider also excluding other generated or vendor directories (e.g. `dist`, `.next`, `.git`, `__pycache__`) to reduce noise and improve performance of the dependency scan.
- The `AGE_THRESHOLD_DAYS` and `REPORT_FILE` settings in `find_stale_unused_deps.py` are hardcoded; exposing these as CLI arguments or environment variables would make the script more flexible for different workflows and thresholds.
## Individual Comments
### Comment 1
<location path="scripts/find_stale_unused_deps.py" line_range="39-48" />
<code_context>
+def is_used(dep_name, search_dirs, extensions):
</code_context>
<issue_to_address>
**suggestion (performance):** The is_used check repeatedly walks the same directories per dependency, which can become expensive on larger codebases.
`is_used` does a full `os.walk` of the search dirs for each dependency, so runtime scales with (#deps × tree size). Consider walking the tree once, reading each file a single time, and checking all dependencies against that content (or against an in-memory index keyed by dependency-like tokens) to cut down traversal and I/O cost.
Suggested implementation:
```python
@functools.lru_cache(maxsize=1)
def _load_source_files(search_dirs_tuple, extensions_tuple):
"""
Walk search_dirs once, cache matching source file contents in memory.
The cache is keyed only by the tuples of search_dirs and extensions, so
repeated calls to is_used with the same parameters avoid both directory
traversal and file I/O.
"""
files_content = []
for d in search_dirs_tuple:
for root, dirs, files in os.walk(d):
# Avoid descending into large vendor trees that don't affect our deps.
if 'node_modules' in dirs:
dirs.remove('node_modules')
for filename in files:
ext = os.path.splitext(filename)[1]
if ext not in extensions_tuple:
continue
filepath = os.path.join(root, filename)
try:
with open(filepath, "r", encoding="utf-8", errors="ignore") as f:
files_content.append(f.read())
except OSError:
# Ignore unreadable files and continue.
continue
return files_content
def is_used(dep_name, search_dirs, extensions):
"""Simple grep-based heuristic to check if a dependency is imported.
This implementation walks the search_dirs once (per unique search_dirs/
extensions combination) and reuses the cached file contents for all
dependency checks, avoiding repeated os.walk and file I/O.
"""
# Convert hyphens to underscores for python (e.g. langchain-core -> langchain_core)
py_dep = dep_name.replace('-', '_')
# Ensure cache key is hashable.
search_dirs_tuple = tuple(search_dirs)
extensions_tuple = tuple(extensions)
file_contents = _load_source_files(search_dirs_tuple, extensions_tuple)
# We will search for occurrences of dep_name or py_dep in source files.
for content in file_contents:
if dep_name in content or py_dep in content:
return True
return False
```
1. At the top of `scripts/find_stale_unused_deps.py`, add `import functools` if it is not already imported:
- `import functools`
2. The original implementation of `is_used` likely contained logic **after** the `dirs.remove('node_modules')` line (e.g., iterating files, reading them, and doing the string search). That code should be removed so that only the new cached implementation shown in the REPLACE block remains.
3. Call sites of `is_used` can remain unchanged; the function signature is identical. This change only optimizes traversal and I/O by caching directory walks and file reads across dependency checks.
</issue_to_address>
### Comment 2
<location path="scripts/find_stale_unused_deps.py" line_range="115-120" />
<code_context>
+ if in_deps and line == ']':
+ in_deps = False
+ continue
+ if in_deps and line.startswith('"'):
+ # parse dependency name
+ dep = line.split('"')[1]
+ # clean versions like "fastapi>=0.100" -> "fastapi"
+ dep = re.split(r'[=><~]', dep)[0]
+ deps.append(dep)
+
+ blame = get_file_blame_lines(pyproj)
</code_context>
<issue_to_address>
**suggestion:** Matching dependencies in blame output using a simple substring can misattribute ages when names are prefixes of others.
In `check_backend`, the age logic currently uses `if dep in b_line['content']`, which can misattribute changes when one dep is a substring of another (e.g., `fastapi` vs `fastapi-utils`) or when the name appears only in comments. Consider matching a more precise TOML pattern (e.g., `"{dep}` followed by a version delimiter) or reusing the dependency‑parsing logic to extract dep names from blame lines and compare them exactly.
</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
+39
to
+48
| def is_used(dep_name, search_dirs, extensions): | ||
| """Simple grep-based heuristic to check if a dependency is imported.""" | ||
| # This is a naive MVP check. | ||
| # Convert hyphens to underscores for python (e.g. langchain-core -> langchain_core) | ||
| py_dep = dep_name.replace('-', '_') | ||
|
|
||
| # We will search for occurrences of dep_name or py_dep in source files. | ||
| for d in search_dirs: | ||
| for root, dirs, files in os.walk(d): | ||
| if 'node_modules' in dirs: |
There was a problem hiding this comment.
suggestion (performance): The is_used check repeatedly walks the same directories per dependency, which can become expensive on larger codebases.
is_used does a full os.walk of the search dirs for each dependency, so runtime scales with (#deps × tree size). Consider walking the tree once, reading each file a single time, and checking all dependencies against that content (or against an in-memory index keyed by dependency-like tokens) to cut down traversal and I/O cost.
Suggested implementation:
@functools.lru_cache(maxsize=1)
def _load_source_files(search_dirs_tuple, extensions_tuple):
"""
Walk search_dirs once, cache matching source file contents in memory.
The cache is keyed only by the tuples of search_dirs and extensions, so
repeated calls to is_used with the same parameters avoid both directory
traversal and file I/O.
"""
files_content = []
for d in search_dirs_tuple:
for root, dirs, files in os.walk(d):
# Avoid descending into large vendor trees that don't affect our deps.
if 'node_modules' in dirs:
dirs.remove('node_modules')
for filename in files:
ext = os.path.splitext(filename)[1]
if ext not in extensions_tuple:
continue
filepath = os.path.join(root, filename)
try:
with open(filepath, "r", encoding="utf-8", errors="ignore") as f:
files_content.append(f.read())
except OSError:
# Ignore unreadable files and continue.
continue
return files_content
def is_used(dep_name, search_dirs, extensions):
"""Simple grep-based heuristic to check if a dependency is imported.
This implementation walks the search_dirs once (per unique search_dirs/
extensions combination) and reuses the cached file contents for all
dependency checks, avoiding repeated os.walk and file I/O.
"""
# Convert hyphens to underscores for python (e.g. langchain-core -> langchain_core)
py_dep = dep_name.replace('-', '_')
# Ensure cache key is hashable.
search_dirs_tuple = tuple(search_dirs)
extensions_tuple = tuple(extensions)
file_contents = _load_source_files(search_dirs_tuple, extensions_tuple)
# We will search for occurrences of dep_name or py_dep in source files.
for content in file_contents:
if dep_name in content or py_dep in content:
return True
return False- At the top of
scripts/find_stale_unused_deps.py, addimport functoolsif it is not already imported:import functools
- The original implementation of
is_usedlikely contained logic after thedirs.remove('node_modules')line (e.g., iterating files, reading them, and doing the string search). That code should be removed so that only the new cached implementation shown in the REPLACE block remains. - Call sites of
is_usedcan remain unchanged; the function signature is identical. This change only optimizes traversal and I/O by caching directory walks and file reads across dependency checks.
Comment on lines
+115
to
+120
| if in_deps and line.startswith('"'): | ||
| # parse dependency name | ||
| dep = line.split('"')[1] | ||
| # clean versions like "fastapi>=0.100" -> "fastapi" | ||
| dep = re.split(r'[=><~]', dep)[0] | ||
| deps.append(dep) |
There was a problem hiding this comment.
suggestion: Matching dependencies in blame output using a simple substring can misattribute ages when names are prefixes of others.
In check_backend, the age logic currently uses if dep in b_line['content'], which can misattribute changes when one dep is a substring of another (e.g., fastapi vs fastapi-utils) or when the name appears only in comments. Consider matching a more precise TOML pattern (e.g., "{dep} followed by a version delimiter) or reusing the dependency‑parsing logic to extract dep names from blame lines and compare them exactly.
Owner
|
Jules Session Analysis: This PR has merge conflicts. Recommended fix: |
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
1 participant
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
owner=agent. Thescripts/extract_todos_structured.pyscript was updated to avoid self-parsing. Additionally, the standalone scriptscripts/find_stale_unused_deps.pywas introduced to scanpackage.jsonandpyproject.tomlagainst the codebase usinggit blame --line-porcelainto identify unused dependencies older than 90 days.Scan Results
TODOs
Convention Enforcement
Verification
python3 scripts/extract_todos_structured.py,uv run pytest tests/Risk Assessment
Next Steps
scripts/find_stale_unused_deps.pylocally and review the output text report to assess if dependency removals should proceed.Machine Metadata
Checklist for reviewers:
PR created automatically by Jules for task 899862534564244020 started by @MasumRab
Summary by Sourcery
Normalize structured TODO comments to include explicit owner metadata and introduce a utility for identifying stale, unused frontend and backend dependencies.
Enhancements:
Chores: