Feature/mvp phase2 verification - #137
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add syntax validation check that verifies transformed code via ast.parse, libcst CST roundtrip, dangerous call detection (eval/exec/os.system), and import count tracking. Includes 8 unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…estore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add verify=False parameter to AutoFixEngine.fix_file() that runs the VerificationEngine pipeline before writing. When verification blocks, the original code is returned with no diff and no bytes written. - Add fixture_bad_extract_test.py for TestSuiteGate coverage - Add 7 end-to-end integration tests covering all fixture scenarios - Add __test__ = False to TestSuiteGate to prevent pytest collection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 18 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA comprehensive Verification Engine system is introduced with orchestrated checks (syntax validation, import integrity, and test suite gating) that validate code transformations before applying fixes. The engine integrates into AutoFixEngine with an optional Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (--verify flag)
participant AFE as AutoFixEngine.fix_file
participant VE as VerificationEngine
participant SV as SyntaxVerifier
participant IV as ImportIntegrityVerifier
participant TG as TestSuiteGate
CLI->>AFE: fix_file(..., verify=True)
AFE->>AFE: Transform code
AFE->>AFE: Generate diff
alt verify enabled & code changed
AFE->>VE: verify(original, transformed, file_path)
VE->>SV: check.verify()
SV-->>VE: CheckResult (syntax validation)
alt syntax passes
VE->>IV: check.verify()
IV-->>VE: CheckResult (import integrity)
alt imports valid
VE->>TG: check.verify()
TG-->>VE: CheckResult (test suite)
else import removed/unresolvable
VE->>VE: Short-circuit remaining checks
end
else syntax fails
VE->>VE: Short-circuit remaining checks
end
VE-->>AFE: VerificationResult (safe_to_apply, passed, confidence_score)
alt safe_to_apply == False
AFE->>AFE: Return (original_code, None) - abort write
else safe_to_apply == True
AFE->>AFE: Write transformed code (if not dry_run)
end
else verify disabled or no changes
AFE->>AFE: Normal fix_file behavior
end
AFE-->>CLI: (fixed_code, diff_or_None)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds a Phase 2 “Verification Engine” to validate code transformations (syntax, import integrity, and a test-suite gate) before allowing AutoFix to apply changes, with accompanying unit/integration tests and CLI plumbing.
Changes:
- Introduces
VerificationEngine, check implementations (SyntaxVerifier,ImportIntegrityVerifier,TestSuiteGate), and frozen result contracts. - Wires optional verification into
AutoFixEngine.fix_file(..., verify=True)and adds a--verifyflag to the CLI. - Adds unit + integration tests and fixtures to exercise the verification pipeline end-to-end.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
refactron/verification/result.py |
Adds CheckResult / VerificationResult dataclass contracts. |
refactron/verification/engine.py |
Implements the orchestrator and confidence scoring. |
refactron/verification/checks/syntax.py |
Adds syntax/CST/dangerous-call verification. |
refactron/verification/checks/imports.py |
Adds removed-import reference checks + new import resolution checks. |
refactron/verification/checks/test_gate.py |
Adds subprocess-based pytest gate with swap/restore behavior. |
refactron/verification/report.py |
Adds Rich formatting for verification output. |
refactron/verification/__init__.py, refactron/verification/checks/__init__.py |
Exposes the verification API surface. |
refactron/autofix/engine.py |
Adds verify option to gate writes on verification outcome. |
refactron/cli/refactor.py |
Adds --verify flag to autofix command signature/options. |
tests/test_verification_result.py |
Contract tests for frozen result types. |
tests/test_verification_engine.py |
Unit tests for orchestration + default checks ordering. |
tests/test_syntax_verifier.py |
Unit tests for SyntaxVerifier behavior. |
tests/test_import_verifier.py |
Unit tests for ImportIntegrityVerifier behavior. |
tests/test_test_gate.py |
Unit tests for TestSuiteGate swap/restore + failure cases. |
tests/test_verification_integration.py |
End-to-end integration tests (engine + fixtures + autofix dry-run). |
tests/fixtures/fixture_bad_extract_test.py |
Fixture test module used by TestSuiteGate. |
CLAUDE.md |
Updates roadmap/API documentation to reflect Phase 2 completion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| check_name: str | ||
| passed: bool | ||
| blocking_reason: str | ||
| confidence: float | ||
| duration_ms: int | ||
| details: Dict[str, Any] | ||
|
|
There was a problem hiding this comment.
CheckResult is declared as a frozen dataclass, but details is a mutable dict, so callers can still mutate the object in-place (e.g., cr.details['x']=...) and violate the “locked contract” guarantee. Consider using an immutable type (e.g., Mapping[str, Any] with a defensive copy, or a frozen/tuple-based structure) to make the freeze meaningful.
| safe_to_apply: bool | ||
| passed: bool | ||
| checks_run: List[str] | ||
| checks_passed: List[str] | ||
| checks_failed: List[str] | ||
| skipped_checks: List[Tuple[str, str]] | ||
| blocking_reason: Optional[str] | ||
| confidence_score: float | ||
| verification_ms: int | ||
| check_results: List[CheckResult] |
There was a problem hiding this comment.
VerificationResult is frozen but contains multiple mutable List[...] fields (checks_run, checks_passed, checks_failed, skipped_checks, check_results). These can still be mutated after construction, which undermines the invariants documented in the docstring. Prefer immutable containers (e.g., Tuple[...]) and/or convert inputs to tuples in the constructor/factory.
| checks_run: List[str] = [] | ||
| checks_passed: List[str] = [] | ||
| checks_failed: List[str] = [] | ||
| skipped_checks: List[tuple] = [] | ||
| blocking_reason: Optional[str] = None |
There was a problem hiding this comment.
skipped_checks is typed as List[tuple] here, but VerificationResult.skipped_checks is List[Tuple[str, str]]. This will trip mypy and weakens the contract; annotate it as List[Tuple[str, str]] (and import Tuple) to match the result type.
| env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}, | ||
| cwd=str(file_path.parent), | ||
| ) |
There was a problem hiding this comment.
Running pytest with cwd=file_path.parent can break imports and config discovery for real projects (tests typically assume the repo/project root as CWD/rootdir). Prefer running with cwd=self.project_root (and require/resolve a project root) so the test environment matches normal pytest runs.
| cmd = ["python3", "-m", "pytest", "-x", "-q"] | ||
| cmd += [str(f) for f in test_files] | ||
| result = subprocess.run( | ||
| cmd, | ||
| timeout=45, |
There was a problem hiding this comment.
The subprocess command hard-codes python3, which can run the wrong interpreter (e.g., outside the active venv) and will fail on Windows. Use sys.executable -m pytest instead. Also consider disabling pytest’s cache provider (e.g., -p no:cacheprovider) because the gate can otherwise create .pytest_cache, which conflicts with “dry-run writes nothing” expectations.
| from refactron.verification import VerificationEngine | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| ve = VerificationEngine(project_root=file_path.parent) | ||
| vr = ve.verify(code, current_code, file_path) |
There was a problem hiding this comment.
VerificationEngine is instantiated with project_root=file_path.parent, which is usually the module directory, not the repository root. This can cause TestSuiteGate to miss relevant tests (or treat “no tests found” as a pass), weakening verification. Consider passing a true project root (workspace root) or making it a required/configured parameter for verification.
- verification/__init__.py: collapse multi-line import for isort 5.13.2 - test_test_gate.py: remove unused os, shutil imports - test_verification_engine.py: remove unused pytest, VerificationResult imports - test_verification_integration.py: remove unused pytest import - engine.py: declare checks attribute before branch to fix mypy no-redef - engine.py: wrap math.prod result in float() to fix mypy no-any-return Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
refactron/cli/refactor.py (1)
258-273:⚠️ Potential issue | 🟠 Major
--verifyis parsed but not wired, so the flag is currently ineffective.Line 272 introduces
verify, but this value is never used in theautofixflow shown here, so users can pass--verifywithout any verification actually running.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/refactor.py` around lines 258 - 273, The --verify flag is parsed into the autofix function as the verify parameter but never used; wire it into the autofix flow by invoking the existing verification routine before applying fixes (e.g., call the project verification helper — run_verification_checks / verify_project / verify_changes — from inside autofix when verify is True), ensure failures abort or skip applying fixes, and propagate the verify flag into any helper like perform_autofix or apply_fixes so the verification behavior is respected end-to-end; update autofix to check the verify boolean and act accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@refactron/autofix/engine.py`:
- Around line 192-193: The project_root passed to VerificationEngine is too
narrow (project_root=file_path.parent) and can miss repo-level tests; change the
instantiation in the code that creates VerificationEngine (the ve =
VerificationEngine(...)/ve.verify(...) call) to compute a repository or
workspace root instead of the file's immediate parent — e.g., implement and call
a helper like find_repository_root(file_path) that walks up parents to locate a
repo marker (.git, pyproject.toml, setup.cfg, or a provided workspace root) and
pass that path as project_root so verify() runs against the full repository test
context.
In `@refactron/verification/__init__.py`:
- Around line 3-10: The import block in __init__.py is not sorted per
isort/black; reorder the imports so they follow isort with profile = black
(group stdlib, third-party, local, and apply alphabetical within groups) for the
symbols ImportIntegrityVerifier, SyntaxVerifier, TestSuiteGate, BaseCheck,
VerificationEngine, CheckResult and VerificationResult, then run the project's
pre-commit/formatting hooks (or run isort) and commit the changes so the file
matches the pipeline's normalized import ordering.
In `@refactron/verification/checks/imports.py`:
- Around line 86-92: The import-name collection currently uses alias.asname or
alias.name directly, but for ast.Import entries alias.name can be a dotted path
like "os.path" while the actual bound name is "os"; update the logic in the
import scanning block (the branch checking isinstance(node, ast.Import)) to use
alias.asname or the first component of alias.name (e.g., alias.name.split(".",
1)[0]) so dotted imports bind correctly; keep the ast.ImportFrom handling
unchanged (still using alias.asname or alias.name).
- Around line 46-49: The loop that currently checks only the top-level name
(top_level) should instead call importlib.util.find_spec on the full module path
(mod) and must skip relative imports; update the loop in the code that iterates
new_modules to first ignore any module strings that start with '.' (or otherwise
indicate a relative import from _extract_module_names), then call
importlib.util.find_spec(mod) and set details["unresolvable_import"] = mod when
that returns None; also ensure _extract_module_names is not returning relative
imports or that you filter them before resolution.
In `@refactron/verification/checks/syntax.py`:
- Around line 31-43: The current broad except in the libcst round-trip block
swallows unexpected errors (e.g., ImportError, runtime bugs) and turns them into
ordinary check failures; change the handler so only genuine parsing/round-trip
errors are converted to self._fail and all other exceptions are re-raised.
Specifically, keep the libcst.parse_module and roundtrip logic but replace the
blanket except Exception as e with logic that catches only libcst
parsing/round-trip exceptions (e.g., libcst.ParserSyntaxError and plain
SyntaxError from roundtrips) and returns self._fail(...) for those cases, while
re-raising any other exception (including ImportError and other runtime errors)
so the VerificationEngine preserves the “no unexpected exceptions” contract.
- Around line 45-55: The current check uses _find_dangerous_calls() which
returns a Set[str], so it only detects new call types not additional call sites;
change the comparison to detect added occurrences by having
_find_dangerous_calls return a multiset/sequence of call occurrences (e.g.,
tuples like (name, lineno, col) or a Counter[name] of counts) instead of
Set[str], then compute new_calls by subtracting counts or by set-diffing
occurrences (use collections.Counter or compare occurrence tuples) and update
the error details and message in the same failure path (the block that calls
self._fail) to list the new sites; apply the same change where else you compare
original vs transformed dangerous calls (the other check similar to the one
using _find_dangerous_calls).
In `@refactron/verification/checks/test_gate.py`:
- Around line 108-146: The current logic in _find_relevant_tests uses
module_name = file_path.stem which only yields the basename (e.g., "syntax") and
misses imports that use the full dotted package path (e.g.,
"refactron.verification.checks.syntax") or relative imports; change
_find_relevant_tests to compute the dotted module path relative to the package
root (use self.project_root or package root to get a relative path from
file_path, convert path separators to dots, and strip a trailing __init__ if
present) and pass that full dotted name(s) into _imports_module; also update
_imports_module to recognize relative ImportFrom forms (node.level > 0) by
resolving them against the computed package path (or accept matches where
alias/name equals the final module segment for relative imports) so imports like
"from ..checks import syntax" or "from refactron.verification.checks import
syntax" are detected for functions _find_relevant_tests and _imports_module.
- Around line 21-22: The __init__ method in test_gate.py is missing a return
type annotation; update the constructor signature for the class that defines def
__init__(self, project_root: Optional[Path] = None) to include an explicit ->
None return type (i.e. def __init__(... ) -> None:), ensuring it matches
refactron's mypy rule disallowing untyped defs and leaving all other parameter
types unchanged; no other logic changes are required.
- Around line 57-67: The subprocess that runs pytest currently uses
cwd=str(file_path.parent) which can mismatch the test discovery root used by
_find_relevant_tests(); change the cwd passed to subprocess.run to use
self.project_root (when set/non-None) so pytest executes from the same root as
the search, falling back to file_path.parent only if self.project_root is not
available; update the subprocess.run invocation in the block that builds
cmd/test_files and references file_path, test_files and env accordingly.
In `@refactron/verification/engine.py`:
- Around line 73-89: The variable passed is never set to False when an
individual check result (cr) has cr.passed == False; update the failure branch
inside the loop in the verification logic (the block handling cr.passed in the
method that iterates self.checks) to set passed = False immediately when
cr.passed is False, then continue with the existing logic that appends to
checks_failed, sets blocking_reason from cr.blocking_reason if unset,
short-circuits remaining checks by populating skipped_checks, and breaks; ensure
the symbol names referenced are the same (passed, cr.passed, checks_failed,
blocking_reason, skipped_checks, self.checks) so the state reflects any failed
check.
- Around line 26-43: The constructor __init__ is missing a return type
annotation and redefines self.checks with inconsistent typing; add the return
annotation "-> None" to __init__ and ensure self.checks is declared once with a
consistent annotated type (e.g., self.checks: List[BaseCheck]) before the
conditional, then assign either the passed checks or the default list of
SyntaxVerifier(), ImportIntegrityVerifier(), and
TestSuiteGate(project_root=project_root) to that single annotated attribute to
avoid the mypy no-redef error.
In `@tests/test_test_gate.py`:
- Around line 3-5: Remove the unused imports causing flake8 F401 in
tests/test_test_gate.py by deleting the import statements for os and shutil and
leaving only the required import(s) (e.g., from pathlib import Path) used by the
tests; update the import line(s) in that file so no unused names remain to
satisfy the linter.
In `@tests/test_verification_engine.py`:
- Around line 5-8: Remove the unused imports to satisfy pre-commit: drop the
top-level import of pytest and remove VerificationResult from the from-import
list in tests/test_verification_engine.py so only the used symbols (BaseCheck,
VerificationEngine, CheckResult) remain; update the import statement referencing
VerificationResult to only import CheckResult and the classes actually used in
the file.
- Around line 122-126: The test test_clean_code_passes_all is non-hermetic
because it uses Path("/tmp") which lets TestSuiteGate scan/operate on host
files; change the test to use the pytest tmp_path fixture instead of
Path("/tmp"), pass project_root=tmp_path and file_path=tmp_path / "test.py", and
create that test.py file in tmp_path before calling VerificationEngine.verify so
the test is fully isolated; reference VerificationEngine, TestSuiteGate and
test_clean_code_passes_all when making the substitution.
In `@tests/test_verification_integration.py`:
- Line 5: The file imports pytest but never uses it, causing flake8 F401; remove
the unused import statement (the "import pytest" line) from
tests/test_verification_integration.py so the module no longer contains the
unused symbol and the pre-commit lint error is resolved.
- Around line 90-111: Update the test_dry_run_and_verify_writes_nothing test to
also assert the returned dry-run outputs from engine.fix_file: verify that the
variables fixed_code and diff are what the contract promises (e.g., fixed_code
is the expected modified content and diff is non-empty or contains the expected
hunks) after calling AutoFixEngine().fix_file(file_path, issues, dry_run=True,
verify=True); keep the existing assertion that the file on disk (after) equals
original but add explicit assertions on fixed_code and diff to ensure the
function returns the produced fix and patch instead of empty values.
---
Outside diff comments:
In `@refactron/cli/refactor.py`:
- Around line 258-273: The --verify flag is parsed into the autofix function as
the verify parameter but never used; wire it into the autofix flow by invoking
the existing verification routine before applying fixes (e.g., call the project
verification helper — run_verification_checks / verify_project / verify_changes
— from inside autofix when verify is True), ensure failures abort or skip
applying fixes, and propagate the verify flag into any helper like
perform_autofix or apply_fixes so the verification behavior is respected
end-to-end; update autofix to check the verify boolean and act accordingly.
🪄 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: 4bafa3ab-db32-4c34-b44c-a5233b5dc8af
📒 Files selected for processing (18)
CLAUDE.mdrefactron/autofix/engine.pyrefactron/cli/refactor.pyrefactron/verification/__init__.pyrefactron/verification/checks/__init__.pyrefactron/verification/checks/imports.pyrefactron/verification/checks/syntax.pyrefactron/verification/checks/test_gate.pyrefactron/verification/engine.pyrefactron/verification/report.pyrefactron/verification/result.pytests/fixtures/fixture_bad_extract_test.pytests/test_import_verifier.pytests/test_syntax_verifier.pytests/test_test_gate.pytests/test_verification_engine.pytests/test_verification_integration.pytests/test_verification_result.py
| ve = VerificationEngine(project_root=file_path.parent) | ||
| vr = ve.verify(code, current_code, file_path) |
There was a problem hiding this comment.
Verification root is too narrow; file_path.parent can under-verify.
Line 192 sets project_root to the file’s immediate directory, which can exclude repository-level tests and make --verify pass on incomplete checks for nested files.
Proposed fix
- ve = VerificationEngine(project_root=file_path.parent)
+ project_root = file_path.parent
+ for parent in [file_path.parent, *file_path.parents]:
+ if (parent / ".git").exists() or (parent / ".refactron.yaml").exists():
+ project_root = parent
+ break
+ ve = VerificationEngine(project_root=project_root)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ve = VerificationEngine(project_root=file_path.parent) | |
| vr = ve.verify(code, current_code, file_path) | |
| project_root = file_path.parent | |
| for parent in [file_path.parent, *file_path.parents]: | |
| if (parent / ".git").exists() or (parent / ".refactron.yaml").exists(): | |
| project_root = parent | |
| break | |
| ve = VerificationEngine(project_root=project_root) | |
| vr = ve.verify(code, current_code, file_path) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/autofix/engine.py` around lines 192 - 193, The project_root passed
to VerificationEngine is too narrow (project_root=file_path.parent) and can miss
repo-level tests; change the instantiation in the code that creates
VerificationEngine (the ve = VerificationEngine(...)/ve.verify(...) call) to
compute a repository or workspace root instead of the file's immediate parent —
e.g., implement and call a helper like find_repository_root(file_path) that
walks up parents to locate a repo marker (.git, pyproject.toml, setup.cfg, or a
provided workspace root) and pass that path as project_root so verify() runs
against the full repository test context.
| for mod in new_modules: | ||
| top_level = mod.split(".")[0] | ||
| if importlib.util.find_spec(top_level) is None: | ||
| details["unresolvable_import"] = mod |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import importlib.util
print("top-level xml:", importlib.util.find_spec("xml") is not None)
print("missing xml submodule:", importlib.util.find_spec("xml.not_a_real_submodule") is None)
PYRepository: Refactron-ai/Refactron_lib
Length of output: 117
🏁 Script executed:
cat -n refactron/verification/checks/imports.py | sed -n '40,115p'Repository: Refactron-ai/Refactron_lib
Length of output: 3416
Check the full module path instead of just the top-level package, and exclude relative imports
Line 48 checks only the top-level package, so imports like xml.missing_submodule can incorrectly pass if the top-level package exists. Additionally, the _extract_module_names method includes relative imports without filtering; these cannot be resolved with importlib.util.find_spec() since it only handles absolute imports.
Required fixes
for mod in new_modules:
- top_level = mod.split(".")[0]
- if importlib.util.find_spec(top_level) is None:
+ if importlib.util.find_spec(mod) is None:
details["unresolvable_import"] = mod- elif isinstance(node, ast.ImportFrom) and node.module:
+ elif isinstance(node, ast.ImportFrom) and node.module and node.level == 0:
modules.add(node.module)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/verification/checks/imports.py` around lines 46 - 49, The loop that
currently checks only the top-level name (top_level) should instead call
importlib.util.find_spec on the full module path (mod) and must skip relative
imports; update the loop in the code that iterates new_modules to first ignore
any module strings that start with '.' (or otherwise indicate a relative import
from _extract_module_names), then call importlib.util.find_spec(mod) and set
details["unresolvable_import"] = mod when that returns None; also ensure
_extract_module_names is not returning relative imports or that you filter them
before resolution.
| if isinstance(node, ast.Import): | ||
| for alias in node.names: | ||
| names.add(alias.asname or alias.name) | ||
| elif isinstance(node, ast.ImportFrom): | ||
| for alias in node.names: | ||
| names.add(alias.asname or alias.name) | ||
| return names |
There was a problem hiding this comment.
Handle dotted import bindings correctly
On Line 88, alias.name keeps dotted paths (e.g., os.path), but the bound name in code is os. This can miss removed-but-still-used imports and let broken transforms pass verification.
Suggested fix
if isinstance(node, ast.Import):
for alias in node.names:
- names.add(alias.asname or alias.name)
+ names.add(alias.asname or alias.name.split(".")[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/verification/checks/imports.py` around lines 86 - 92, The
import-name collection currently uses alias.asname or alias.name directly, but
for ast.Import entries alias.name can be a dotted path like "os.path" while the
actual bound name is "os"; update the logic in the import scanning block (the
branch checking isinstance(node, ast.Import)) to use alias.asname or the first
component of alias.name (e.g., alias.name.split(".", 1)[0]) so dotted imports
bind correctly; keep the ast.ImportFrom handling unchanged (still using
alias.asname or alias.name).
| # Step 2: libcst roundtrip | ||
| try: | ||
| import libcst | ||
|
|
||
| tree = libcst.parse_module(transformed) | ||
| roundtripped = tree.code | ||
| libcst.parse_module(roundtripped) | ||
| except Exception as e: | ||
| return self._fail( | ||
| f"CST round-trip failed: {e}", | ||
| start, | ||
| details, | ||
| ) |
There was a problem hiding this comment.
Don't downgrade unexpected verifier failures into ordinary check failures.
This except Exception converts import/runtime bugs in the LibCST step into a normal blocked transformation. That means the engine can no longer honor VerificationResult.passed's “no unexpected exceptions” contract. Only translate expected parse/round-trip errors to _fail() and let anything else bubble to VerificationEngine.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 38-38: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/verification/checks/syntax.py` around lines 31 - 43, The current
broad except in the libcst round-trip block swallows unexpected errors (e.g.,
ImportError, runtime bugs) and turns them into ordinary check failures; change
the handler so only genuine parsing/round-trip errors are converted to
self._fail and all other exceptions are re-raised. Specifically, keep the
libcst.parse_module and roundtrip logic but replace the blanket except Exception
as e with logic that catches only libcst parsing/round-trip exceptions (e.g.,
libcst.ParserSyntaxError and plain SyntaxError from roundtrips) and returns
self._fail(...) for those cases, while re-raising any other exception (including
ImportError and other runtime errors) so the VerificationEngine preserves the
“no unexpected exceptions” contract.
| def test_clean_code_passes_all(self): | ||
| engine = VerificationEngine(project_root=Path("/tmp")) | ||
| code = "import os\n\nos.getcwd()\n" | ||
| result = engine.verify(code, code, Path("/tmp/test.py")) | ||
| assert result.safe_to_apply is True |
There was a problem hiding this comment.
Keep the real-check passing test hermetic.
This test points VerificationEngine at the host /tmp, so TestSuiteGate can scan arbitrary machine files and, on the passing path, even try to operate on /tmp/test.py if it finds a matching test. Use tmp_path and a file under it so the result does not depend on the environment.
🧪 Make the pass-path deterministic
- def test_clean_code_passes_all(self):
- engine = VerificationEngine(project_root=Path("/tmp"))
+ def test_clean_code_passes_all(self, tmp_path):
+ engine = VerificationEngine(project_root=tmp_path)
code = "import os\n\nos.getcwd()\n"
- result = engine.verify(code, code, Path("/tmp/test.py"))
+ file_path = tmp_path / "test.py"
+ file_path.write_text(code, encoding="utf-8")
+ result = engine.verify(code, code, file_path)
assert result.safe_to_apply is True🧰 Tools
🪛 Ruff (0.15.7)
[error] 123-123: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 125-125: Probable insecure usage of temporary file or directory: "/tmp/test.py"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_verification_engine.py` around lines 122 - 126, The test
test_clean_code_passes_all is non-hermetic because it uses Path("/tmp") which
lets TestSuiteGate scan/operate on host files; change the test to use the pytest
tmp_path fixture instead of Path("/tmp"), pass project_root=tmp_path and
file_path=tmp_path / "test.py", and create that test.py file in tmp_path before
calling VerificationEngine.verify so the test is fully isolated; reference
VerificationEngine, TestSuiteGate and test_clean_code_passes_all when making the
substitution.
| def test_dry_run_and_verify_writes_nothing(self): | ||
| """dry_run=True + verify=True should produce diff and result but write nothing.""" | ||
| from refactron.autofix.engine import AutoFixEngine | ||
| from refactron.core.models import CodeIssue, IssueCategory, IssueLevel | ||
|
|
||
| file_path = FIXTURES_DIR / "fixture_safe_extract.py" | ||
| original = file_path.read_text(encoding="utf-8") | ||
|
|
||
| engine = AutoFixEngine() | ||
| issues = [ | ||
| CodeIssue( | ||
| category=IssueCategory.DEPENDENCY, | ||
| level=IssueLevel.WARNING, | ||
| message="unused import", | ||
| file_path=file_path, | ||
| line_number=9, | ||
| rule_id="DEP001", | ||
| ), | ||
| ] | ||
| fixed_code, diff = engine.fix_file(file_path, issues, dry_run=True, verify=True) | ||
| after = file_path.read_text(encoding="utf-8") | ||
| assert after == original |
There was a problem hiding this comment.
Assert the returned dry-run outputs.
This test only proves the fixture file was not rewritten. It would still pass if fix_file(..., verify=True) returned empty outputs, and Ruff is already flagging both locals as unused at Line 109. Add assertions for fixed_code and diff so the test covers the contract it describes.
✅ Tighten the test
fixed_code, diff = engine.fix_file(file_path, issues, dry_run=True, verify=True)
+ assert fixed_code is not None
+ assert fixed_code != original
+ assert diff
after = file_path.read_text(encoding="utf-8")
assert after == original🧰 Tools
🪛 Ruff (0.15.7)
[warning] 109-109: Unpacked variable fixed_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 109-109: Unpacked variable diff is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_verification_integration.py` around lines 90 - 111, Update the
test_dry_run_and_verify_writes_nothing test to also assert the returned dry-run
outputs from engine.fix_file: verify that the variables fixed_code and diff are
what the contract promises (e.g., fixed_code is the expected modified content
and diff is non-empty or contains the expected hunks) after calling
AutoFixEngine().fix_file(file_path, issues, dry_run=True, verify=True); keep the
existing assertion that the file on disk (after) equals original but add
explicit assertions on fixed_code and diff to ensure the function returns the
produced fix and patch instead of empty values.
Summary by CodeRabbit
Release Notes
--verifyCLI flag to enable safety checks when using autofix.