Fix/thread project root into verification#194
Open
shrutu0929 wants to merge 2 commits into
Open
Conversation
AutoFixEngine.fix_file() always constructed VerificationEngine(project_root=file_path.parent), which breaks monorepos and layouts where tests/config live above the edited file. - Add optional project_root parameter to fix_file(). - When omitted, discover the project/VCS root by walking up from the file to the nearest .git/.hg/.svn or pyproject.toml/setup.py/setup.cfg marker, falling back to the file directory. - Add _discover_project_root() helper and tests covering discovery, fallback, and explicit pass-through to VerificationEngine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR enhances the verification system by adding intelligent project-root discovery to avoid hardcoded assumptions about repository structure. ChangesVerification System Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
refactron/autofix/engine.py (1)
28-28: 💤 Low valueConsider a more robust file-vs-directory check.
The suffix-based check (
file_path.suffix) assumes paths without extensions are directories, which may not always hold. While this works for typical Python files (.pysuffix present), edge cases like extension-less executables or config files could behave unexpectedly.If
file_pathis guaranteed to exist at call time,file_path.is_file()would be more reliable. Otherwise, the current approach is acceptable with a comment noting the assumption.♻️ Alternative approach (if file existence is guaranteed)
- start = file_path.parent if file_path.suffix else file_path + start = file_path.parent if file_path.is_file() else file_path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@refactron/autofix/engine.py` at line 28, The current assignment "start = file_path.parent if file_path.suffix else file_path" uses suffix to detect files; replace this with a more robust check using file_path.is_file() (i.e., "start = file_path.parent if file_path.is_file() else file_path") if file_path is guaranteed to exist at call time, or, if existence cannot be assumed, leave the logic but add a clarifying comment near the "start" assignment explaining the suffix-based assumption and why it's acceptable; refer to the symbol file_path and the variable start in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@refactron/autofix/engine.py`:
- Line 17: The module constant _PROJECT_ROOT_MARKERS is missing a type
annotation; add an explicit annotation such as using typing.Tuple[str, ...]
(e.g. _PROJECT_ROOT_MARKERS: Tuple[str, ...]) and ensure you import Tuple from
typing at the top of refactron/autofix/engine.py so the constant is statically
typed and satisfies mypy's disallow_untyped_defs rule.
In `@refactron/verification/checks/test_gate.py`:
- Around line 118-123: Remove the trailing whitespace on the changed blank lines
in the test discovery block around the variables test_dirs, search_dirs, and
excluded_dirs (the hunk containing "test_dirs = ...", "search_dirs = ...",
"excluded_dirs = ...", and the "for root_dir in search_dirs:" line) to satisfy
the pre-commit trailing-whitespace hook; also remove trailing spaces on the
other reported blank lines in the same file near the test_gate.py hunk (the
changes around the regions reported as 130-131 and 144-145) so no blank lines
contain trailing spaces.
- Around line 112-113: The cache key for _test_file_cache uses module_name =
file_path.stem which collides for different files with the same stem; change the
key to use a unique path-based identifier instead (e.g., compute module_key from
file_path.relative_to(self.project_root).as_posix() when self.project_root is
set, otherwise use file_path.resolve().as_posix()) and replace uses of
module_name for cache lookups/assignments with module_key; apply the same change
to the other occurrences referenced around the _test_file_cache usage (the
blocks where module_name is set at the other locations).
---
Nitpick comments:
In `@refactron/autofix/engine.py`:
- Line 28: The current assignment "start = file_path.parent if file_path.suffix
else file_path" uses suffix to detect files; replace this with a more robust
check using file_path.is_file() (i.e., "start = file_path.parent if
file_path.is_file() else file_path") if file_path is guaranteed to exist at call
time, or, if existence cannot be assumed, leave the logic but add a clarifying
comment near the "start" assignment explaining the suffix-based assumption and
why it's acceptable; refer to the symbol file_path and the variable start in
your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91d446ca-3401-46ef-b91d-bb82b9ceb211
📒 Files selected for processing (3)
refactron/autofix/engine.pyrefactron/verification/checks/test_gate.pytests/test_fix_file_project_root.py
|
|
||
| # Directory markers used to locate the project/VCS root when no explicit | ||
| # root is supplied to fix_file(). | ||
| _PROJECT_ROOT_MARKERS = (".git", ".hg", ".svn", "pyproject.toml", "setup.py", "setup.cfg") |
Contributor
There was a problem hiding this comment.
Add type annotation to module constant.
The constant _PROJECT_ROOT_MARKERS lacks a type annotation. As per coding guidelines, type annotations are required in refactron/ with mypy disallow_untyped_defs = true enabled.
📝 Proposed fix
+from typing import Dict, List, Optional, Tuple
+
# Directory markers used to locate the project/VCS root when no explicit
# root is supplied to fix_file().
-_PROJECT_ROOT_MARKERS = (".git", ".hg", ".svn", "pyproject.toml", "setup.py", "setup.cfg")
+_PROJECT_ROOT_MARKERS: Tuple[str, ...] = (
+ ".git", ".hg", ".svn", "pyproject.toml", "setup.py", "setup.cfg"
+)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@refactron/autofix/engine.py` at line 17, The module constant
_PROJECT_ROOT_MARKERS is missing a type annotation; add an explicit annotation
such as using typing.Tuple[str, ...] (e.g. _PROJECT_ROOT_MARKERS: Tuple[str,
...]) and ensure you import Tuple from typing at the top of
refactron/autofix/engine.py so the constant is statically typed and satisfies
mypy's disallow_untyped_defs rule.
Comment on lines
112
to
113
| module_name = file_path.stem | ||
| search_root = self.project_root or file_path.parent |
Contributor
There was a problem hiding this comment.
Cache key collision can return wrong test files for same-stem modules.
module_name = file_path.stem is too coarse for _test_file_cache. Two different files with the same stem will share cache entries and can reuse unrelated test selections.
💡 Proposed fix
- module_name = file_path.stem
+ module_name = file_path.stem
+ cache_key = str(file_path.resolve())
search_root = self.project_root or file_path.parent
@@
- if module_name in self._test_file_cache:
- return self._test_file_cache[module_name]
+ if cache_key in self._test_file_cache:
+ return self._test_file_cache[cache_key]
@@
- self._test_file_cache[module_name] = test_files
+ self._test_file_cache[cache_key] = test_files
return test_filesAlso applies to: 131-133, 145-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@refactron/verification/checks/test_gate.py` around lines 112 - 113, The cache
key for _test_file_cache uses module_name = file_path.stem which collides for
different files with the same stem; change the key to use a unique path-based
identifier instead (e.g., compute module_key from
file_path.relative_to(self.project_root).as_posix() when self.project_root is
set, otherwise use file_path.resolve().as_posix()) and replace uses of
module_name for cache lookups/assignments with module_key; apply the same change
to the other occurrences referenced around the _test_file_cache usage (the
blocks where module_name is set at the other locations).
Comment on lines
+118
to
+123
|
|
||
| test_dirs = [d for d in [search_root / "tests", search_root / "test"] if d.is_dir()] | ||
| search_dirs = test_dirs if test_dirs else [search_root] | ||
| excluded_dirs = {".git", ".rag", "__pycache__", "venv", ".venv", "env", "node_modules"} | ||
|
|
||
| for root_dir in search_dirs: |
Contributor
There was a problem hiding this comment.
Remove trailing whitespace in this hunk to unblock pre-commit.
Pre-commit is currently failing on trailing-whitespace, and these changed blank lines appear to be part of that failure.
Also applies to: 130-131, 144-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@refactron/verification/checks/test_gate.py` around lines 118 - 123, Remove
the trailing whitespace on the changed blank lines in the test discovery block
around the variables test_dirs, search_dirs, and excluded_dirs (the hunk
containing "test_dirs = ...", "search_dirs = ...", "excluded_dirs = ...", and
the "for root_dir in search_dirs:" line) to satisfy the pre-commit
trailing-whitespace hook; also remove trailing spaces on the other reported
blank lines in the same file near the test_gate.py hunk (the changes around the
regions reported as 130-131 and 144-145) so no blank lines contain trailing
spaces.
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.
solve #187
AutoFixEngine.fix_file() previously constructed its verification engine with VerificationEngine(project_root=file_path.parent), hardcoding the project root to the directory of the file being fixed.
This is incorrect for monorepos and any layout where the test suite or project configuration lives in a directory above the edited file — TestSuiteGate._find_relevant_tests would search from the wrong
root and miss the relevant tests, and the behavior diverged from refactron verify --project-root, which already accepts an explicit root.
This change adds an optional project_root parameter to fix_file(). When a caller passes a root explicitly, it is forwarded unchanged to VerificationEngine, so callers that already know the correct
root (such as RefactronPipeline) can ensure verification runs against it. When no root is supplied, fix_file() now discovers one by walking up from the file to the nearest ancestor containing a VCS or
project marker (.git, .hg, .svn, pyproject.toml, setup.py, or setup.cfg), falling back to the file's own directory only when no marker is found. The discovery logic is implemented in a new
_discover_project_root() helper in the same module.
The change is accompanied by a new test file, tests/test_fix_file_project_root.py, with six tests covering root discovery via .git and pyproject.toml, the fallback to the file directory when no marker
exists, explicit pass-through of project_root to VerificationEngine, and confirmation that discovery resolves to the repo root rather than file_path.parent. All new and related tests pass, and the
changed files are clean under black, isort, and flake8.
Summary by CodeRabbit
Release Notes
New Features
project_rootparameter to file fixing operations for explicit project root configuration during verification.Performance Improvements