feat: MVP Phase 1 (Days 1-5) + interactive TUI issue viewer - #132
Conversation
Phase 1 Stabilize: - Day 1: Exception isolation for TaintAnalyzer/DataFlowAnalyzer with AnalysisSkipWarning - Day 2: SHA-256 content hashing for IncrementalAnalysisTracker; backup integrity validation - Day 3: --dry-run flag for autofix; generate_diff() and atomic file writes - Day 4: Test fixture files (6 fixtures, 8 validation tests) - Day 5: --no-cache flag for analyze; self-analysis gate (96 files, 0 crashes) Interactive TUI: - Arrow key navigation (termios raw mode) with cursor indicator - Enter to drill into severity groups and expand/collapse issues - b/n/p shortcuts for back/next/prev group navigation - Manual cursor erasure for flicker-free re-rendering (no Rich Live conflicts) - Non-interactive fallback for CI/CD (piped output or --no-interactive) - Logger noise suppressed via force=True basicConfig Semantic analysis layer: - CFG builder, data flow analysis, taint analysis, symbol table - Type inference engine 779 tests passing (21 new TUI + 8 fixture + Day 1-3 tests) 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 12 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 (15)
📝 WalkthroughWalkthroughThis PR adds semantic analysis infrastructure to Refactron, introducing control-flow-graph construction, taint analysis for security vulnerability detection, symbol table building, backup/incremental cache hardening via SHA-256, dry-run workflow support, and interactive TUI issue viewer. It also includes comprehensive documentation, fixtures, and integration tests across analysis, CLI, core, and autofix modules. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant Analyzer as TaintAnalyzer
participant CFG as CFGBuilder
participant DataFlow as DataFlowAnalyzer
participant SinkCheck as Sink Detector
participant Report as Result Reporter
User->>CFG: build_from_source(code)
CFG-->>Analyzer: CFGNode (entry)
Analyzer->>DataFlow: compute_reaching_definitions()
DataFlow-->>Analyzer: {node_id → Set[var_definitions]}
Analyzer->>Analyzer: iterate per CFG node
loop For each statement in node
Analyzer->>SinkCheck: check for sink calls
alt Sink found & argument tainted
SinkCheck-->>Report: TaintVulnerability
else No vulnerability
SinkCheck-->>Analyzer: continue
end
end
Analyzer-->>User: List[TaintVulnerability]
sequenceDiagram
participant User as User / CLI
participant Engine as AutoFixEngine
participant VerifyEngine as VerificationEngine
participant Filesystem as Filesystem
participant Report as Result / Diff
User->>Engine: fix_file(path, issues, dry_run=True)
Engine->>Engine: read source file
loop For each issue
Engine->>Engine: apply fixer sequentially
end
Engine->>Engine: generate unified diff
alt dry_run == False
Engine->>VerifyEngine: run_verification(modified_code)
alt Verification passes
VerifyEngine-->>Engine: verified
Engine->>Filesystem: atomic write (temp + replace)
Filesystem-->>Engine: file updated
else Verification fails
VerifyEngine-->>Engine: VerificationError
Engine-->>User: (abort, return error)
end
else dry_run == True
Engine-->>Report: skip write, return code + diff
end
Engine-->>User: (modified_code, diff)
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 |
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR delivers “MVP Phase 1” stabilization work plus a new interactive, keyboard-driven TUI issue viewer, and introduces the initial semantic analysis layer (CFG/data-flow/taint/symbols/type inference) along with extensive new tests and fixtures.
Changes:
- Adds analyzer exception isolation + semantic skip warnings/summary, plus cache/backup SHA-256 integrity hardening.
- Implements
--dry-runplumbing for autofix (unified diff generation + atomic writes) and adds an interactive TUI issue browser with non-interactive fallback. - Introduces semantic analysis modules (CFG builder, data-flow analyzer, taint analyzer, symbol table, inference wrapper) and new test fixtures validating expected analyzer behavior.
Reviewed changes
Copilot reviewed 31 out of 34 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tui_viewer.py | Pure-logic state-machine tests for the interactive TUI viewer. |
| tests/test_semantic_analysis.py | Integration tests for CFG construction, reaching-defs, and taint tracking. |
| tests/test_fixtures_behave_as_expected.py | Validates fixture files trigger expected analyzer rule IDs and behaviors. |
| tests/test_exception_isolation.py | Verifies semantic analysis failures degrade to warnings (not crashes) and skip-rate summary. |
| tests/test_dry_run.py | Tests unified diff generation and dry-run/apply behavior for autofix. |
| tests/test_cache_hardening.py | Tests SHA-256-based incremental cache invalidation and backup integrity/rollback behavior. |
| tests/fixtures/fixture_test_break_test.py | Fixture test suite to detect unsafe signature changes during transforms. |
| tests/fixtures/fixture_test_break.py | Fixture module with intentional issues + public API signature to protect. |
| tests/fixtures/fixture_safe_extract.py | Fixture with multiple safe-to-fix issues used for verification/fixtures. |
| tests/fixtures/fixture_import_break.py | Fixture to ensure import-fixing logic doesn’t break dotted attribute usages. |
| tests/fixtures/fixture_clean.py | Negative-control fixture that should produce zero issues. |
| tests/fixtures/fixture_bad_extract.py | Fixture intended to represent transforms that must be blocked by verification. |
| refactron/core/refactron.py | Adds semantic analysis isolation, skip warnings collection, and skip-rate summary. |
| refactron/core/models.py | Adds AnalysisSkipWarning dataclass. |
| refactron/core/inference.py | Adds astroid-based inference wrapper for semantic analysis. |
| refactron/core/incremental.py | Hardens incremental tracking using SHA-256 as the authoritative change signal. |
| refactron/core/backup.py | Stores SHA-256 in backup index + validates integrity before rollback restores. |
| refactron/core/analysis_result.py | Extends AnalysisResult with semantic skip warnings + summary. |
| refactron/cli/utils.py | Adjusts logging setup to reduce handler duplication and suppress noise by default. |
| refactron/cli/ui.py | Adds interactive TUI issue viewer + severity-grouped non-interactive output. |
| refactron/cli/refactor.py | Adds --dry-run flag to autofix CLI UX (preview semantics). |
| refactron/cli/analysis.py | Adds --no-cache, --no-interactive, and routes TTY output to the TUI viewer. |
| refactron/autofix/file_ops.py | Adds generate_diff() unified diff helper. |
| refactron/autofix/engine.py | Adds fix_file() supporting dry-run diffs and atomic apply writes. |
| refactron/analysis/taint.py | Introduces taint analysis engine (source-to-sink tracking). |
| refactron/analysis/symbol_table.py | Adds project symbol table builder + optional cache serialization. |
| refactron/analysis/data_flow.py | Introduces reaching-definitions data-flow analysis. |
| refactron/analysis/cfg/node.py | Adds CFG node model (predecessors/successors, edge types). |
| refactron/analysis/cfg/builder.py | Adds CFG builder producing basic-block graph for control structures. |
| refactron/analysis/cfg/init.py | Initializes CFG package. |
| refactron/analysis/init.py | Initializes analysis package. |
| dev-notes/Refactron_Comprehensive_MVP.md | Adds detailed MVP plan/roadmap documentation. |
| dev-notes/Refactron_Comprehensive_MVP.docx | Adds DOCX version of the MVP roadmap. |
| CLAUDE.md | Adds/updates repository guidance and summarizes Phase 1 deliverables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class CFGBuilder: | ||
| def __init__(self): | ||
| self.nodes: List[CFGNode] = [] | ||
| self.current_id = 0 | ||
| self.current_block: Optional[CFGNode] = None | ||
|
|
||
| # Stack for managing control flow targets | ||
| # loop_stack stores (break_target, continue_target) | ||
| self.loop_stack: List[tuple[CFGNode, CFGNode]] = [] | ||
|
|
||
| def _new_block(self) -> CFGNode: | ||
| """Create a new basic block.""" | ||
| node = CFGNode(id=self.current_id) | ||
| self.current_id += 1 | ||
| self.nodes.append(node) | ||
| return node | ||
|
|
||
| def build_from_source(self, source_code: str) -> CFGNode: | ||
| """Build CFG from source code string.""" | ||
| tree = ast.parse(source_code) | ||
| return self.build_from_ast(tree) | ||
|
|
||
| def build_from_ast(self, tree: ast.AST) -> CFGNode: | ||
| """Build CFG from AST.""" | ||
| entry_block = self._new_block() | ||
| self.current_block = entry_block | ||
|
|
||
| # We handle function definitions specially if we want interprocedural analysis later | ||
| # For now, we process top-level code or body of functions | ||
| if isinstance(tree, ast.Module): | ||
| self._process_statements(tree.body) | ||
| elif isinstance(tree, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| self._process_statements(tree.body) | ||
| else: | ||
| # Fallback for snippets | ||
| self._visit(tree) | ||
|
|
||
| return entry_block | ||
|
|
||
| def _process_statements(self, statements: List[ast.stmt]): | ||
| """Process a list of statements sequentially.""" | ||
| for stmt in statements: | ||
| self._visit(stmt) | ||
|
|
||
| def _visit(self, node: ast.AST): | ||
| """Dispatch visitor method.""" | ||
| method_name = f"_visit_{node.__class__.__name__}" | ||
| visitor = getattr(self, method_name, self._visit_generic) | ||
| visitor(node) | ||
|
|
||
| def _visit_generic(self, node: ast.AST): | ||
| """Default visitor for simple statements.""" | ||
| if self.current_block is None: | ||
| # Unreachable code or detached block | ||
| self.current_block = self._new_block() | ||
|
|
||
| self.current_block.statements.append(node) | ||
|
|
There was a problem hiding this comment.
Several methods here are missing return type annotations (e.g. __init__, _process_statements, _visit, _visit_generic, etc.). The repo’s mypy config has disallow_untyped_defs = true, so missing return annotations will fail CI/pre-commit. Add explicit -> None / appropriate return types across the class methods.
| class CFGNode: | ||
| id: int | ||
| statements: List[Any] = field(default_factory=list) # AST nodes in this block | ||
| predecessors: List["CFGNode"] = field(default_factory=list) | ||
| successors: List[tuple["CFGNode", EdgeType]] = field(default_factory=list) | ||
|
|
||
| def add_successor(self, node: "CFGNode", edge_type: EdgeType = EdgeType.NORMAL): | ||
| self.successors.append((node, edge_type)) | ||
| node.predecessors.append(self) | ||
|
|
||
| def __hash__(self): | ||
| return self.id | ||
|
|
||
| def __repr__(self): | ||
| return f"CFGNode(id={self.id}, stmts={len(self.statements)})" |
There was a problem hiding this comment.
successors is annotated as List[tuple[...]] without postponed evaluation; on Python 3.8 this will raise at import time. Also, add_successor, __hash__, and __repr__ are missing return type annotations which will fail mypy with disallow_untyped_defs=true. Use typing.Tuple[...] (or add from __future__ import annotations) and add explicit return types.
| # Worklist for blocks | ||
| worklist = [self.cfg_entry] | ||
| visited_config = set() # (node_id, frozenset(tainted_in)) | ||
|
|
There was a problem hiding this comment.
worklist and visited_config are defined but never used, which will fail flake8 (F841) under the repo’s pre-commit hooks. Either remove them or implement the intended worklist/path-sensitivity logic so these variables are actually used.
| # Worklist for blocks | |
| worklist = [self.cfg_entry] | |
| visited_config = set() # (node_id, frozenset(tainted_in)) |
| def _propagate_taint(self, stmt: ast.AST, current_taint: Set[str]) -> tuple[Set[str], Set[str]]: | ||
| """ |
There was a problem hiding this comment.
The return annotation -> tuple[Set[str], Set[str]] uses PEP 585 built-in generics and will raise on Python 3.8 at import time (no postponed annotations here). Use typing.Tuple[Set[str], Set[str]] or add from __future__ import annotations for Python 3.8 compatibility.
| try: | ||
| return str(_Path(file_path).relative_to(_Path.cwd())) | ||
| except ValueError: | ||
| return _Path(file_path).name |
There was a problem hiding this comment.
_relative_path() returns only the filename when file_path is already relative (because Path(file_path).relative_to(Path.cwd()) raises). That drops directory context and can make issue locations ambiguous. Consider: if file_path is relative, return it as-is; if absolute, try relative_to(cwd) and fall back to .name only when truly outside the cwd.
| try: | |
| return str(_Path(file_path).relative_to(_Path.cwd())) | |
| except ValueError: | |
| return _Path(file_path).name | |
| p = _Path(file_path) | |
| # If already a relative path, preserve it (including any directory components). | |
| if not p.is_absolute(): | |
| return str(p) | |
| # For absolute paths, try to make them relative to the current working directory. | |
| try: | |
| return str(p.relative_to(_Path.cwd())) | |
| except ValueError: | |
| # If the path is outside the cwd, fall back to just the filename. | |
| return p.name |
| use_interactive = sys.stdout.isatty() and not no_interactive | ||
|
|
||
| if use_interactive: | ||
| _interactive_issue_viewer(result, target_path) | ||
| else: |
There was a problem hiding this comment.
Interactive mode is enabled based only on sys.stdout.isatty(). The TUI reads raw keypresses from stdin; if stdout is a TTY but stdin is piped/redirected, _read_key() will block or error. Consider requiring both sys.stdin.isatty() and sys.stdout.isatty() (or gracefully falling back when stdin isn’t a TTY).
| class CFGBuilder: | ||
| def __init__(self): | ||
| self.nodes: List[CFGNode] = [] | ||
| self.current_id = 0 | ||
| self.current_block: Optional[CFGNode] = None | ||
|
|
||
| # Stack for managing control flow targets | ||
| # loop_stack stores (break_target, continue_target) | ||
| self.loop_stack: List[tuple[CFGNode, CFGNode]] = [] | ||
|
|
There was a problem hiding this comment.
This file uses PEP 585 built-in generics (e.g. tuple[CFGNode, CFGNode]) in runtime-evaluated annotations without from __future__ import annotations. Since the project supports Python 3.8, importing this module will raise TypeError: 'type' object is not subscriptable. Use typing.Tuple[...] (and typing.List/typing.Dict etc) or add from __future__ import annotations and ensure mypy/flake8 settings are consistent.
| if node.orelse: | ||
| orelse_block = self._new_block() | ||
| # The "False" edge from head actually goes to orelse if present | ||
| # Fix previous connection | ||
| loop_head.successors = [s for s in loop_head.successors if s[1] != EdgeType.FALSE] | ||
| loop_head.add_successor(orelse_block, EdgeType.FALSE) |
There was a problem hiding this comment.
When for/while ... else is present, this rewrites loop_head.successors to remove the FALSE edge, but it does not remove the corresponding predecessor link from the original target node (e.g. loop_exit.predecessors). This can leave the CFG in an inconsistent state (a predecessor without a matching successor edge), which will skew later data-flow/taint analyses. Prefer a helper that removes edges symmetrically (update both successors and predecessors) instead of mutating successors directly.
| from typing import Any, Dict, List, Optional, Set | ||
|
|
||
| from .cfg.builder import CFGBuilder | ||
| from .cfg.node import CFGNode, EdgeType | ||
|
|
||
|
|
||
| class DataFlowAnalyzer: | ||
| def __init__(self, cfg_entry: CFGNode): | ||
| self.entry_node = cfg_entry | ||
| self.nodes = self._collect_nodes(cfg_entry) | ||
|
|
||
| def _collect_nodes(self, entry: CFGNode) -> List[CFGNode]: | ||
| """BFS to collect all reachable nodes.""" | ||
| nodes = [] | ||
| visited = set() | ||
| queue = [entry] | ||
| visited.add(entry.id) | ||
|
|
||
| while queue: | ||
| node = queue.pop(0) | ||
| nodes.append(node) | ||
| for succ, _ in node.successors: | ||
| if succ.id not in visited: | ||
| visited.add(succ.id) | ||
| queue.append(succ) | ||
| return sorted(nodes, key=lambda n: n.id) | ||
|
|
||
| def compute_reaching_definitions(self) -> Dict[int, Set[tuple[str, int]]]: | ||
| """ | ||
| Compute Reaching Definitions for each block. | ||
| Returns a map: node_id -> set of (variable_name, definition_node_id) | ||
| definition_node_id can be the CFG node ID where it was defined. | ||
| """ | ||
| # specialized sets for gen/kill | ||
| # gen[n]: definitions generated in block n | ||
| # kill[n]: definitions killed in block n | ||
| gen: Dict[int, Set[tuple[str, int]]] = defaultdict(set) | ||
| kill: Dict[int, Set[str]] = defaultdict(set) |
There was a problem hiding this comment.
This module uses PEP 585 generics like Set[tuple[str, int]] in annotations, which will crash on Python 3.8 unless annotations are postponed. Replace with typing.Tuple[str, int] (and similar) or add from __future__ import annotations. Also, CFGBuilder and EdgeType are imported but unused, which will fail flake8 (F401).
| except: | ||
| pass |
There was a problem hiding this comment.
Avoid bare except: here. It will be flagged by flake8 (E722) and can also hide unexpected errors (e.g. KeyboardInterrupt/SystemExit). Catch a specific exception type (or Exception) and consider logging at debug level if inference fails.
| except: | |
| pass | |
| except Exception as exc: | |
| logger.debug( | |
| "Type inference failed for variable %s at %s:%s: %s", | |
| node.name, | |
| file_path, | |
| node.lineno, | |
| exc, | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
CLAUDE.md-84-84 (1)
84-84:⚠️ Potential issue | 🟡 MinorUpdate stale test-count milestone text.
Line 84 reports 758 tests, but this PR’s stated gate is 779 tests passing. Please sync this number to avoid misleading release/readiness context.
Suggested doc fix
-| 5 | Phase 1 gate: 758 tests green, self-analysis 96 files/0 crashes, added `--no-cache` flag to `analyze` | ✅ Done | +| 5 | Phase 1 gate: 779 tests green, self-analysis 96 files/0 crashes, added `--no-cache` flag to `analyze` | ✅ Done |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 84, Update the stale milestone count in the CLAUDE.md table row that currently reads "| 5 | Phase 1 gate: 758 tests green, self-analysis 96 files/0 crashes, added `--no-cache` flag to `analyze` | ✅ Done |" by replacing "758" with "779" so the Phase 1 gate line matches the PR's stated gate of 779 tests passing.CLAUDE.md-94-95 (1)
94-95:⚠️ Potential issue | 🟡 MinorDocument actual TUI keybindings to match shipped UX.
The CLI section currently omits arrow-key navigation and Enter-based drill/expand behavior. Add those bindings so contributor guidance matches runtime behavior.
Suggested doc fix
-- `refactron analyze` now shows an **interactive issue viewer** (TTY) with severity-grouped navigation (`[1-4]` to drill in, `[n/p/b/q]` to navigate) +- `refactron analyze` now shows an **interactive issue viewer** (TTY) with severity-grouped navigation (↑/↓ arrows to move cursor, `Enter` to drill in or expand/collapse, `[n/p/b/q]` for next/prev/back/quit)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 94 - 95, Update the CLI docs for the interactive TTY viewer for `refactron analyze` (and its non-interactive `--no-interactive` fallback) to list the runtime keybindings: in addition to numeric severity-group drill `[1-4]` and navigation `[n/p/b/q]`, document arrow-key navigation (Up/Down to move between issues, Left/Right or Tab/Shift-Tab if supported to move panes) and Enter to drill into/expand an issue (and Esc/Backspace to collapse/go back if applicable). Ensure the text around “interactive issue viewer (TTY)” mentions these exact keys and keeps existing numeric and letter controls for parity with shipped UX.refactron/core/inference.py-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorClear the current lint/type failures in this module.
CI is already red here:
Unionis unused, andget_node_type_name()is declared-> strbut still returnsAnythroughobj.name/getattr(...). Remove the import and coerce those branches tostrso this file passes the enforced checks.Proposed fix
-from typing import Any, List, Optional, Union +from typing import Any, List, Optional @@ - if isinstance(obj, nodes.ClassDef): - return obj.name + if isinstance(obj, nodes.ClassDef): + return str(obj.name) @@ - return getattr(obj, "name", str(type(obj))) + name = getattr(obj, "name", None) + return str(name) if name is not None else type(obj).__name__Also applies to: 56-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/inference.py` at line 6, Remove the unused Union import and update get_node_type_name to always return a str: delete Union from the imports (keep Any, List, Optional if used) and coerce branches that return obj.name or getattr(obj, "name", None) to str (e.g., return str(obj.name) or return str(getattr(obj, "name", ""))) so the function signature -> str is satisfied and the module passes lint/type checks; apply the same coercion for any other branches in lines ~56-67 that return a non-str name.refactron/analysis/symbol_table.py-8-11 (1)
8-11:⚠️ Potential issue | 🟡 MinorThis file still fails the enforced lint/type gates.
Pre-commit is already red here:
asdict,Set, andUnionare unused, andadd_symbol(),_analyze_file(),_visit_node(), and_save_cache()are missing annotations under the repo's mypy settings. Please clear these before merge so the new module can pass CI.Also applies to: 65-65, 139-151, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/symbol_table.py` around lines 8 - 11, Remove unused imports asdict, Set, and Union from the top imports to satisfy linters, then add explicit type annotations for the four functions flagged by mypy: annotate add_symbol(...) with its parameter types and return type (e.g., add_symbol(self, symbol: <appropriate Symbol type>) -> None), annotate _analyze_file(self, path: Path) -> None (or the correct return type), annotate _visit_node(self, node: Any) -> None (use ast.AST or the correct node type) and annotate _save_cache(self) -> None (or the actual return type); import any missing typing names (e.g., Any, ast.AST) used in those annotations so mypy and linters pass.refactron/analysis/symbol_table.py-187-190 (1)
187-190:⚠️ Potential issue | 🟡 MinorDon't swallow all inference failures here.
The bare
exceptmasks real bugs in the new inference layer and leavesinferred_typeempty with no trace, while CI is already red onE722. If best-effort inference is intentional, catchExceptionexplicitly and log at debug level.Proposed fix
try: symbol.inferred_type = self.inference_engine.get_node_type_name(node) - except: - pass + except Exception as err: + logger.debug( + "Failed to infer type for %s at %s:%s: %s", + node.name, + file_path, + node.lineno, + err, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/symbol_table.py` around lines 187 - 190, The bare except in the symbol inference block (where symbol.inferred_type is assigned from self.inference_engine.get_node_type_name(node)) swallows all errors and triggers E722; change it to catch Exception explicitly and log the exception at debug level instead of silently passing. Update the try/except around the get_node_type_name call to except Exception as e: and call the module/class logger (or self.logger) to debug-log a contextual message including the exception and node/symbol identity so inference failures remain best-effort but are traceable.tests/test_dry_run.py-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorDrop the unused
pytestimport.Pre-commit is already red on this line with
F401, and the module does not referencepytestdirectly.Proposed fix
-import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_dry_run.py` at line 14, Remove the unused top-level import statement "import pytest" (the bare pytest import) from the test module to resolve the F401 lint error; ensure no code in the module relies on pytest being imported and run the linter/tests after removing it.tests/test_tui_viewer.py-30-43 (1)
30-43:⚠️ Potential issue | 🟡 MinorUse explicit
Optional[str]for thesuggestionparameter.PEP 484 prohibits implicit
Optional(using= Nonewithout theOptionaltype hint).🔧 Proposed fix
+from typing import Optional + + def _make_issue( level: IssueLevel = IssueLevel.WARNING, message: str = "test issue", line: int = 10, - suggestion: str = None, + suggestion: Optional[str] = None, ) -> CodeIssue:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tui_viewer.py` around lines 30 - 43, The helper function _make_issue uses a default None for suggestion without an explicit Optional annotation; update the signature of _make_issue to type-hint suggestion as Optional[str] (import Optional from typing) so it reads suggestion: Optional[str] = None, and keep the rest of the function (returning CodeIssue) unchanged to satisfy PEP 484 rules.tests/test_exception_isolation.py-14-17 (1)
14-17:⚠️ Potential issue | 🟡 MinorRemove unused import to fix pipeline failure.
The
pytestimport is flagged as unused by flake8. While pytest is the test framework, it's not directly used in this file (nopytest.mark,pytest.raises, etc.).🔧 Proposed fix
-import pytest - from refactron import Refactron -from refactron.core.models import AnalysisSkipWarning # noqa: F401 (import drives the test fail) +from refactron.core.models import AnalysisSkipWarning # noqa: F401🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_exception_isolation.py` around lines 14 - 17, Remove the unused top-level import of pytest in tests/test_exception_isolation.py: delete the line "import pytest" so only the necessary imports (Refactron and AnalysisSkipWarning) remain; ensure no other references to pytest (e.g., pytest.mark or pytest.raises) exist in the file before committing.tests/test_exception_isolation.py-90-97 (1)
90-97:⚠️ Potential issue | 🟡 MinorRemove unused variable to fix pipeline failure.
original_analyzeis assigned but never used, causing the flake8 F841 error in CI.🔧 Proposed fix
call_count = {"n": 0} - original_analyze = None def maybe_crash(*args, **kwargs):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_exception_isolation.py` around lines 90 - 97, The variable original_analyze is assigned but never used causing a flake8 F841; remove the unused assignment by deleting the original_analyze variable declaration (the line "original_analyze = None") in the test around the maybe_crash helper and call_count setup so only call_count and maybe_crash remain referenced.refactron/core/refactron.py-383-404 (1)
383-404:⚠️ Potential issue | 🟡 MinorFix type annotation syntax and approve exception isolation pattern.
The
tuple[...]syntax at lines 385 and 406 causes mypy errors. UseTuple[...]from typing for compatibility.The broad
except Exceptionat line 397 is correct here—this implements the "transparent degradation" pattern from the MVP spec where semantic analysis failures should never crash the analysis run.🔧 Proposed fix for type annotations
def _run_semantic_analysis( self, file_path: Path, source_code: str - ) -> "tuple[list, Optional[AnalysisSkipWarning]]": + ) -> Tuple[list, Optional[AnalysisSkipWarning]]: """Run TaintAnalyzer on *source_code* with full exception isolation.Also update the import at the top of the file:
-from typing import List, Optional, Tuple, Union +from typing import List, Optional, Tuple, Union(Tuple should already be imported based on existing usage)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/refactron.py` around lines 383 - 404, Change the return type annotation of _run_semantic_analysis from the Python 3.9+ "tuple[list, Optional[AnalysisSkipWarning]]" form to typing.Tuple, e.g. Tuple[List, Optional[AnalysisSkipWarning]], and ensure Tuple (and List/Optional if not already) are imported from typing at the top of the file; leave the broad except Exception block (the exception isolation pattern using CFGBuilder, TaintAnalyzer and returning AnalysisSkipWarning) unchanged.refactron/analysis/cfg/node.py-29-33 (1)
29-33:⚠️ Potential issue | 🟡 MinorAdd missing return type annotations to fix mypy errors.
🔧 Proposed fix
- def __hash__(self): + def __hash__(self) -> int: return self.id - def __repr__(self): + def __repr__(self) -> str: return f"CFGNode(id={self.id}, stmts={len(self.statements)})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/cfg/node.py` around lines 29 - 33, The __hash__ and __repr__ methods on CFGNode are missing return type annotations causing mypy errors; update the method signatures for CFGNode.__hash__ to specify -> int and CFGNode.__repr__ to specify -> str, keeping their current implementations unchanged so mypy recognizes the correct return types.refactron/core/refactron.py-406-406 (1)
406-406:⚠️ Potential issue | 🟡 MinorFix return type annotation syntax.
Same issue as
_run_semantic_analysis— useTupleinstead oftuplefor compatibility.🔧 Proposed fix
- def _analyze_file(self, file_path: Path) -> "tuple[FileMetrics, Optional[AnalysisSkipWarning]]": + def _analyze_file(self, file_path: Path) -> Tuple[FileMetrics, Optional[AnalysisSkipWarning]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/refactron.py` at line 406, The return type annotation of _analyze_file currently uses built-in lowercase tuple[...] which is incompatible in this codebase style (same as _run_semantic_analysis); change the annotation to use typing.Tuple, i.e. Tuple[FileMetrics, Optional[AnalysisSkipWarning]], and ensure Tuple is imported from typing (add it to existing typing imports if missing) so the function signature reads with Tuple instead of tuple.tests/test_tui_viewer.py-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorRemove unused
pytestimport to fix pipeline failure.The flake8 F401 error indicates
pytestis imported but never used. The test functions are discovered by naming convention, not by pytest decorators.🔧 Proposed fix
from pathlib import Path -import pytest - from refactron.cli.ui import (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tui_viewer.py` at line 9, Remove the unused top-level import "pytest" in the test module (the "import pytest" line) to fix the flake8 F401 error; tests are discovered by naming convention so no pytest symbols are required—delete that import statement or replace it with a used import if needed.refactron/cli/ui.py-348-368 (1)
348-368:⚠️ Potential issue | 🟡 MinorExisting guard prevents most Windows issues, but Git Bash falls through.
The
_interactive_issue_viewer()is only called whensys.stdout.isatty()is True (refactron/cli/analysis.py:183), which blocks execution on native Windows (cmd/PowerShell) and prevents the error there. However, on Git Bash or other MSYS2 environments,isatty()returns True despite termios being unavailable, causing aModuleNotFoundError. Consider adding explicit error handling in_read_key()or checking platform-specific availability before the import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/ui.py` around lines 348 - 368, The _read_key() function currently unconditionally imports termios/tty which raises ModuleNotFoundError on some Windows-like environments (e.g., Git Bash); update _read_key() to first try importing termios and tty in a try/except ImportError block and, on ImportError or when os.name == "nt", use a Windows-safe fallback (e.g., use msvcrt.getwch/getch or a non-blocking/sane fallback that maps Enter to KEY_ENTER) so the function always returns a single key string (including arrow escape sequences where possible) and always restores previous terminal state; reference _read_key(), KEY_ENTER, and sys.stdin in your change.tests/test_cache_hardening.py-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorRemove the unused imports.
timeandpytestare not referenced anywhere in this file, so pre-commit is currently failing on F401.Suggested cleanup
import hashlib import os -import time from pathlib import Path - -import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cache_hardening.py` around lines 13 - 16, The imports "time" and "pytest" in tests/test_cache_hardening.py are unused (causing F401); remove the unused import statements for time and pytest and keep only the required imports (e.g., Path) so the file contains only referenced symbols; update the import block in that file accordingly and run the test/linters to confirm the F401 is resolved.
🧹 Nitpick comments (2)
refactron/analysis/taint.py (1)
70-73: Avoid sharing a mutable defaultTaintConfig.
DEFAULT_TAINT_CONFIGcontains lists, so using that instance as the constructor default means one mutation can leak into futureTaintAnalyzer()objects. DefaultconfigtoNoneand copy the selected configuration on entry.Suggested cleanup
- def __init__(self, cfg_entry: CFGNode, config: TaintConfig = DEFAULT_TAINT_CONFIG): + def __init__(self, cfg_entry: CFGNode, config: Optional[TaintConfig] = None): self.cfg_entry = cfg_entry - self.config = config + base_config = config or DEFAULT_TAINT_CONFIG + self.config = TaintConfig( + sources=list(base_config.sources), + sinks=list(base_config.sinks), + sanitizers=list(base_config.sanitizers), + ) self.data_flow = DataFlowAnalyzer(cfg_entry)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/taint.py` around lines 70 - 73, The constructor for TaintAnalyzer currently uses the mutable DEFAULT_TAINT_CONFIG as a default which can cause shared-state bugs; change the __init__ signature to accept config: Optional[TaintConfig] = None, and inside __init__ set self.config = (config.copy() if config is not None else DEFAULT_TAINT_CONFIG.copy()) or otherwise create a fresh TaintConfig instance (copying lists inside) so each TaintAnalyzer gets its own config; update references to DEFAULT_TAINT_CONFIG and the TaintAnalyzer.__init__ implementation accordingly.tests/test_fixtures_behave_as_expected.py (1)
124-132: Use the active interpreter for the nested pytest run.Hard-coding
python3makes this test depend onPATHand can invoke a different virtualenv than the one running the suite.sys.executablekeeps the subprocess on the same interpreter.Suggested cleanup
def test_test_break_test_actually_passes(): """fixture_test_break_test.py must pass when run against the unmodified fixture.""" import subprocess + import sys test_file = FIXTURES_DIR / "fixture_test_break_test.py" result = subprocess.run( - ["python3", "-m", "pytest", str(test_file), "-x", "--no-header", "-q"], + [sys.executable, "-m", "pytest", str(test_file), "-x", "--no-header", "-q"], capture_output=True, text=True, timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_fixtures_behave_as_expected.py` around lines 124 - 132, The subprocess invocation hard-codes "python3" which can use the wrong interpreter; update the call that builds the argv for subprocess.run (the one assigning result from subprocess.run) to use the current interpreter via sys.executable (add an import for sys at the top of the module if missing) and replace "python3" with sys.executable so the nested pytest run uses the same virtualenv; keep the rest of the subprocess.run parameters (["-m", "pytest", str(test_file), "-x", "--no-header", "-q"], capture_output=True, text=True, timeout=30) unchanged.
🤖 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/analysis/cfg/builder.py`:
- Around line 118-130: The loop target is appended as a raw AST expression
(node.target) so the data-flow/taint pass (which only recognizes
ast.Assign/ast.AnnAssign) doesn't see the for-loop binding as a definition;
change the builder to create and append a proper assignment node representing
the loop binding (e.g., construct an ast.Assign or ast.AnnAssign with
targets=[node.target] and an appropriate value placeholder) before calling
_process_statements so that the symbol is treated as a real definition; update
the block where loop_head/loop_body/current_block and node.target are used to
append that synthetic Assign/AnnAssign instead of the bare expression.
- Around line 137-143: When you replace the loop head's FALSE successor (e.g.,
the code that reassigns loop_head.successors to filter out EdgeType.FALSE before
loop_head.add_successor(orelse_block, EdgeType.FALSE)), also remove loop_head
from the removed target block's predecessors and ensure the newly linked
orelse_block gets loop_head in its predecessors; likewise apply the same
predecessor-fix to the other identical rewrite site (the second occurrence
around the 177-180 region). Concretely: capture the removed successor target(s)
when filtering loop_head.successors, for each removed_target do
removed_target.predecessors = [p for p in removed_target.predecessors if p is
not loop_head], then call loop_head.add_successor(orelse_block, EdgeType.FALSE)
(or ensure orelse_block.predecessors includes loop_head) so predecessor lists
remain consistent.
- Around line 39-47: build_from_source currently iterates a Module's body but
doesn't build CFG for top-level functions because FunctionDef/AsyncFunctionDef
are not visited specially and fall through to _visit_generic; update the module
processing to explicitly handle ast.FunctionDef and ast.AsyncFunctionDef by
invoking _process_statements on their .body (and similarly ensure nested/async
defs are handled), or add dedicated visitor methods visit_FunctionDef /
visit_AsyncFunctionDef that call _process_statements, so the builder (methods
build_from_source/_visit/_visit_generic/_process_statements) creates CFG blocks
for function bodies; apply the same fix to the other occurrence referenced
(lines ~62-68).
In `@refactron/analysis/cfg/node.py`:
- Around line 18-27: The type annotation for successors uses the PEP 585 generic
tuple syntax (tuple["CFGNode", EdgeType]) which fails on older Python versions;
update the annotation on the CFGNode class to use typing.Tuple instead (e.g.,
successors: List[Tuple["CFGNode", EdgeType]]) and add Tuple to the typing
imports so tests can collect, leaving add_successor, predecessors, and EdgeType
usage unchanged.
- Around line 6-8: The import list should add Tuple and remove the unused
Optional; update any annotations using the runtime-generic form tuple[...]
(e.g., the annotation at the location referenced around line 23) to use
typing.Tuple[...] instead to avoid "type object is not subscriptable" on older
Pythons. Concretely, in the top-level imports replace "from typing import Any,
List, Optional" with "from typing import Any, List, Tuple" and change any
occurrences of "tuple[...]" to "Tuple[...]" (keep all other names such as
dataclass/Enum unchanged).
In `@refactron/analysis/data_flow.py`:
- Around line 8-11: Remove unused imports Any and Optional and the unused
CFGBuilder and EdgeType imports; replace uses of PEP 585 tuple[...] with
typing.Tuple[...] by adding Tuple to the typing imports; add an explicit
annotation for node_gen as Set[Tuple[str, int]] (i.e., node_gen: Set[Tuple[str,
int]] = set()) so its element types are clear to mypy; and wrap the long line
around the node/edge processing logic that exceeds flake8 width so it complies
with the line-length limit. Locate these fixes in
refactron/analysis/data_flow.py around the top-level typing imports and where
node_gen and tuple[...] annotations are declared/used and adjust
imports/annotations accordingly.
In `@refactron/analysis/symbol_table.py`:
- Around line 75-83: self.exports is keyed only by bare symbol.name so symbols
with the same name from different modules overwrite each other; change the
exports key to be module-qualified (e.g. include symbol.module or
symbol.module_path when storing in self.exports, such as using
f"{symbol.module}:{symbol.name}" or a nested dict self.exports[module][name]) in
the code that sets self.exports (the block checking symbol.scope == "global" and
symbol.type in (SymbolType.CLASS, SymbolType.FUNCTION, SymbolType.VARIABLE)),
and update resolve_reference to first look up the module-qualified key (or
consult the per-module dict) before falling back to an unqualified lookup to
preserve backward compatibility.
- Around line 124-128: The current build_for_project block returns cached
symbols from _load_cache whenever cache_dir exists, causing stale symbol tables;
instead, do not early-return cached results—either validate cache freshness
(e.g., compare source mtimes) or for the interim change remove the "return
cached" path so build_for_project continues and rebuilds the symbol table even
when _load_cache() returns data; update build_for_project and/or _load_cache
usage so cached data is only used after a validity check (or only for metadata)
and never blindly returned without invalidation.
In `@refactron/analysis/taint.py`:
- Around line 238-242: The sink matching only checks positional arguments
(node.args) so keyword-argument taint flows are missed; update the call-site
check in the taint analyzer to also inspect node.keywords for the target
parameter name or position from the TaintSink (use sink_def.arg_index and/or
sink_def.arg_name if available) and call the existing _is_expression_tainted for
those keyword values as well; locate the logic that looks up sink_def =
self._sinks[func_name] and extend it to iterate node.keywords (matching by arg
name or mapping arg_index to parameter name) and treat matching keyword.value
the same as positional args for taint checks.
- Around line 75-78: The current self._sources = {s.name for s in
config.sources} loses each source's kind and causes name-only matches across
kinds; change self._sources to preserve kind information (e.g., a dict mapping
kind -> set(names) or a set of (name, kind) tuples) and update all lookup logic
that uses self._sources (any is_source/is_taint checks and code in the region
referenced around lines 171-220) to perform kind-aware matching (check the
specific kind key or tuple rather than plain name). Ensure config.sources
entries (TaintSource) are iterated to populate the new structure and adapt any
callers that expected a plain name set to use the kind-specific membership test.
In `@refactron/autofix/engine.py`:
- Around line 179-194: The inline temp-file write in the if-block (checking
dry_run and current_code != code) bypasses the backup/rollback logic; replace
the mkstemp/open/replace sequence that writes current_code to file_path with a
call into the backup-aware writer exported by refactron/autofix/file_ops.py so
fix_file(..., dry_run=False) uses the canonical backup/indexing flow.
Concretely, import the backup-aware write function from file_ops and invoke it
with file_path and current_code (instead of creating tmp_path), preserving the
same atomic semantics but ensuring a rollback point is created.
In `@refactron/cli/refactor.py`:
- Around line 245-250: The --dry-run option is declared but never used; update
the CLI handler that declares the click.option to accept a dry_run parameter and
wire it into the autofix code path so no files are written and a unified diff is
produced: when dry_run is True, call the autofix path on each file using
AutoFixEngine.fix_file(..., dry_run=True) or the engine's diff-generation method
(e.g., generate_unified_diff or similar) instead of persisting changes, and
print the unified diff output; ensure the same wiring is applied to the other
occurrence referenced around lines 297-299 so both code paths honor dry_run.
In `@refactron/core/analysis_result.py`:
- Around line 28-29: The AnalysisResult currently stores semantic_skip_warnings
and semantic_skip_summary but does not surface them; update the
AnalysisResult.summary() and AnalysisResult.report() methods to include a brief
note (e.g., "Semantic analysis skipped: N warnings" and/or semantic_skip_summary
text when present) in their returned/printed output so users see skip counts or
summary; reference the semantic_skip_warnings and semantic_skip_summary fields
and add the message near other summary/report sections to ensure it appears in
the standard user-facing output.
In `@refactron/core/incremental.py`:
- Around line 131-137: The legacy fallback that trusts mtime when previous state
lacks "sha256" is unsafe; in the function handling file change detection (e.g.,
where previous = ... and previous.get("mtime", 0) is used in
refactron/core/incremental.py), treat entries missing "sha256" as changed so
they get re-baselined: detect absence of previous.get("sha256") and return True
(or otherwise force an update path such as calling update_file_state() to
compute and persist a hash) instead of falling back to mtime; this ensures old
state entries are refreshed and the blind spot for same-size/timestamped edits
is closed.
In `@refactron/core/inference.py`:
- Around line 82-87: The isinstance check is using the wrong symbol
nodes.Instance; replace checks of isinstance(obj, nodes.Instance) with
isinstance(obj, Instance) (e.g., in the block that iterates inferred objects
where obj is checked and ancestor.name compared) and add the top-level import
from astroid: from astroid import Instance so the runtime type check succeeds
and ancestry logic runs as intended.
In `@tests/fixtures/fixture_import_break.py`:
- Line 12: The unused import of sys triggers flake8 F401; to keep the fixture
semantics while silencing the lint, change the import statement for sys (the
"sys" symbol) to include an explicit suppression comment such as appending " #
noqa: F401" (preserving any existing inline comment), so the line becomes:
import sys # noqa: F401.
In `@tests/fixtures/fixture_test_break_test.py`:
- Line 15: The top-level import "from fixture_test_break import calculate_total"
in tests/fixtures/fixture_test_break_test.py is intentionally after a sys.path
mutation and triggers flake8 E402; fix it by appending an explicit E402 ignore
comment to that import (e.g., add "# noqa: E402") so the delayed import of
calculate_total is allowed without failing pre-commit.
In `@tests/fixtures/fixture_test_break.py`:
- Line 13: The unused import "import math" in
tests/fixtures/fixture_test_break.py triggers flake8 F401; silence it explicitly
by annotating the import with a noqa for F401 (i.e., add a trailing comment to
the "import math" line) so the fixture intent remains but the linter will ignore
the unused-import error.
---
Minor comments:
In `@CLAUDE.md`:
- Line 84: Update the stale milestone count in the CLAUDE.md table row that
currently reads "| 5 | Phase 1 gate: 758 tests green, self-analysis 96 files/0
crashes, added `--no-cache` flag to `analyze` | ✅ Done |" by replacing "758"
with "779" so the Phase 1 gate line matches the PR's stated gate of 779 tests
passing.
- Around line 94-95: Update the CLI docs for the interactive TTY viewer for
`refactron analyze` (and its non-interactive `--no-interactive` fallback) to
list the runtime keybindings: in addition to numeric severity-group drill
`[1-4]` and navigation `[n/p/b/q]`, document arrow-key navigation (Up/Down to
move between issues, Left/Right or Tab/Shift-Tab if supported to move panes) and
Enter to drill into/expand an issue (and Esc/Backspace to collapse/go back if
applicable). Ensure the text around “interactive issue viewer (TTY)” mentions
these exact keys and keeps existing numeric and letter controls for parity with
shipped UX.
In `@refactron/analysis/cfg/node.py`:
- Around line 29-33: The __hash__ and __repr__ methods on CFGNode are missing
return type annotations causing mypy errors; update the method signatures for
CFGNode.__hash__ to specify -> int and CFGNode.__repr__ to specify -> str,
keeping their current implementations unchanged so mypy recognizes the correct
return types.
In `@refactron/analysis/symbol_table.py`:
- Around line 8-11: Remove unused imports asdict, Set, and Union from the top
imports to satisfy linters, then add explicit type annotations for the four
functions flagged by mypy: annotate add_symbol(...) with its parameter types and
return type (e.g., add_symbol(self, symbol: <appropriate Symbol type>) -> None),
annotate _analyze_file(self, path: Path) -> None (or the correct return type),
annotate _visit_node(self, node: Any) -> None (use ast.AST or the correct node
type) and annotate _save_cache(self) -> None (or the actual return type); import
any missing typing names (e.g., Any, ast.AST) used in those annotations so mypy
and linters pass.
- Around line 187-190: The bare except in the symbol inference block (where
symbol.inferred_type is assigned from
self.inference_engine.get_node_type_name(node)) swallows all errors and triggers
E722; change it to catch Exception explicitly and log the exception at debug
level instead of silently passing. Update the try/except around the
get_node_type_name call to except Exception as e: and call the module/class
logger (or self.logger) to debug-log a contextual message including the
exception and node/symbol identity so inference failures remain best-effort but
are traceable.
In `@refactron/cli/ui.py`:
- Around line 348-368: The _read_key() function currently unconditionally
imports termios/tty which raises ModuleNotFoundError on some Windows-like
environments (e.g., Git Bash); update _read_key() to first try importing termios
and tty in a try/except ImportError block and, on ImportError or when os.name ==
"nt", use a Windows-safe fallback (e.g., use msvcrt.getwch/getch or a
non-blocking/sane fallback that maps Enter to KEY_ENTER) so the function always
returns a single key string (including arrow escape sequences where possible)
and always restores previous terminal state; reference _read_key(), KEY_ENTER,
and sys.stdin in your change.
In `@refactron/core/inference.py`:
- Line 6: Remove the unused Union import and update get_node_type_name to always
return a str: delete Union from the imports (keep Any, List, Optional if used)
and coerce branches that return obj.name or getattr(obj, "name", None) to str
(e.g., return str(obj.name) or return str(getattr(obj, "name", ""))) so the
function signature -> str is satisfied and the module passes lint/type checks;
apply the same coercion for any other branches in lines ~56-67 that return a
non-str name.
In `@refactron/core/refactron.py`:
- Around line 383-404: Change the return type annotation of
_run_semantic_analysis from the Python 3.9+ "tuple[list,
Optional[AnalysisSkipWarning]]" form to typing.Tuple, e.g. Tuple[List,
Optional[AnalysisSkipWarning]], and ensure Tuple (and List/Optional if not
already) are imported from typing at the top of the file; leave the broad except
Exception block (the exception isolation pattern using CFGBuilder, TaintAnalyzer
and returning AnalysisSkipWarning) unchanged.
- Line 406: The return type annotation of _analyze_file currently uses built-in
lowercase tuple[...] which is incompatible in this codebase style (same as
_run_semantic_analysis); change the annotation to use typing.Tuple, i.e.
Tuple[FileMetrics, Optional[AnalysisSkipWarning]], and ensure Tuple is imported
from typing (add it to existing typing imports if missing) so the function
signature reads with Tuple instead of tuple.
In `@tests/test_cache_hardening.py`:
- Around line 13-16: The imports "time" and "pytest" in
tests/test_cache_hardening.py are unused (causing F401); remove the unused
import statements for time and pytest and keep only the required imports (e.g.,
Path) so the file contains only referenced symbols; update the import block in
that file accordingly and run the test/linters to confirm the F401 is resolved.
In `@tests/test_dry_run.py`:
- Line 14: Remove the unused top-level import statement "import pytest" (the
bare pytest import) from the test module to resolve the F401 lint error; ensure
no code in the module relies on pytest being imported and run the linter/tests
after removing it.
In `@tests/test_exception_isolation.py`:
- Around line 14-17: Remove the unused top-level import of pytest in
tests/test_exception_isolation.py: delete the line "import pytest" so only the
necessary imports (Refactron and AnalysisSkipWarning) remain; ensure no other
references to pytest (e.g., pytest.mark or pytest.raises) exist in the file
before committing.
- Around line 90-97: The variable original_analyze is assigned but never used
causing a flake8 F841; remove the unused assignment by deleting the
original_analyze variable declaration (the line "original_analyze = None") in
the test around the maybe_crash helper and call_count setup so only call_count
and maybe_crash remain referenced.
In `@tests/test_tui_viewer.py`:
- Around line 30-43: The helper function _make_issue uses a default None for
suggestion without an explicit Optional annotation; update the signature of
_make_issue to type-hint suggestion as Optional[str] (import Optional from
typing) so it reads suggestion: Optional[str] = None, and keep the rest of the
function (returning CodeIssue) unchanged to satisfy PEP 484 rules.
- Line 9: Remove the unused top-level import "pytest" in the test module (the
"import pytest" line) to fix the flake8 F401 error; tests are discovered by
naming convention so no pytest symbols are required—delete that import statement
or replace it with a used import if needed.
---
Nitpick comments:
In `@refactron/analysis/taint.py`:
- Around line 70-73: The constructor for TaintAnalyzer currently uses the
mutable DEFAULT_TAINT_CONFIG as a default which can cause shared-state bugs;
change the __init__ signature to accept config: Optional[TaintConfig] = None,
and inside __init__ set self.config = (config.copy() if config is not None else
DEFAULT_TAINT_CONFIG.copy()) or otherwise create a fresh TaintConfig instance
(copying lists inside) so each TaintAnalyzer gets its own config; update
references to DEFAULT_TAINT_CONFIG and the TaintAnalyzer.__init__ implementation
accordingly.
In `@tests/test_fixtures_behave_as_expected.py`:
- Around line 124-132: The subprocess invocation hard-codes "python3" which can
use the wrong interpreter; update the call that builds the argv for
subprocess.run (the one assigning result from subprocess.run) to use the current
interpreter via sys.executable (add an import for sys at the top of the module
if missing) and replace "python3" with sys.executable so the nested pytest run
uses the same virtualenv; keep the rest of the subprocess.run parameters (["-m",
"pytest", str(test_file), "-x", "--no-header", "-q"], capture_output=True,
text=True, timeout=30) unchanged.
🪄 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: 881035a0-1f43-4d66-86a3-fced0ae6988b
⛔ Files ignored due to path filters (1)
dev-notes/Refactron_Comprehensive_MVP.docxis excluded by!**/*.docx
📒 Files selected for processing (33)
CLAUDE.mddev-notes/Refactron_Comprehensive_MVP.mdrefactron/analysis/__init__.pyrefactron/analysis/cfg/__init__.pyrefactron/analysis/cfg/builder.pyrefactron/analysis/cfg/node.pyrefactron/analysis/data_flow.pyrefactron/analysis/symbol_table.pyrefactron/analysis/taint.pyrefactron/autofix/engine.pyrefactron/autofix/file_ops.pyrefactron/cli/analysis.pyrefactron/cli/refactor.pyrefactron/cli/ui.pyrefactron/cli/utils.pyrefactron/core/analysis_result.pyrefactron/core/backup.pyrefactron/core/incremental.pyrefactron/core/inference.pyrefactron/core/models.pyrefactron/core/refactron.pytests/fixtures/fixture_bad_extract.pytests/fixtures/fixture_clean.pytests/fixtures/fixture_import_break.pytests/fixtures/fixture_safe_extract.pytests/fixtures/fixture_test_break.pytests/fixtures/fixture_test_break_test.pytests/test_cache_hardening.pytests/test_dry_run.pytests/test_exception_isolation.pytests/test_fixtures_behave_as_expected.pytests/test_semantic_analysis.pytests/test_tui_viewer.py
| # We handle function definitions specially if we want interprocedural analysis later | ||
| # For now, we process top-level code or body of functions | ||
| if isinstance(tree, ast.Module): | ||
| self._process_statements(tree.body) | ||
| elif isinstance(tree, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| self._process_statements(tree.body) | ||
| else: | ||
| # Fallback for snippets | ||
| self._visit(tree) |
There was a problem hiding this comment.
Module builds currently miss intra-function control flow.
When build_from_source() receives a full module, top-level ast.FunctionDef / ast.AsyncFunctionDef nodes fall through to _visit_generic() because there is no dedicated visitor. The semantic pipeline in refactron/core/refactron.py therefore never builds CFG blocks for ordinary function bodies, so reaching definitions and block-level taint propagation inside real code are mostly invisible.
Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/analysis/cfg/builder.py` around lines 39 - 47, build_from_source
currently iterates a Module's body but doesn't build CFG for top-level functions
because FunctionDef/AsyncFunctionDef are not visited specially and fall through
to _visit_generic; update the module processing to explicitly handle
ast.FunctionDef and ast.AsyncFunctionDef by invoking _process_statements on
their .body (and similarly ensure nested/async defs are handled), or add
dedicated visitor methods visit_FunctionDef / visit_AsyncFunctionDef that call
_process_statements, so the builder (methods
build_from_source/_visit/_visit_generic/_process_statements) creates CFG blocks
for function bodies; apply the same fix to the other occurrence referenced
(lines ~62-68).
| # Head connects to Body (True) and Exit (False/Done) | ||
| loop_head.statements.append(node.iter) # Approximation | ||
| loop_head.add_successor(loop_body, EdgeType.TRUE) | ||
| loop_head.add_successor(loop_exit, EdgeType.FALSE) | ||
|
|
||
| # Push loop context for break/continue | ||
| self.loop_stack.append((loop_exit, loop_head)) | ||
|
|
||
| # Process Body | ||
| self.current_block = loop_body | ||
| # Assignment of target happens at start of body | ||
| self.current_block.statements.append(node.target) | ||
| self._process_statements(node.body) |
There was a problem hiding this comment.
Treat for target binding as a real definition site.
Line 129 stores node.target as a bare AST expression, but refactron/analysis/data_flow.py Lines 53-69 and refactron/analysis/taint.py Lines 151-168 only treat ast.Assign / ast.AnnAssign as definitions. That means for item in source: never defines or taints item, which drops common loop-carried flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/analysis/cfg/builder.py` around lines 118 - 130, The loop target is
appended as a raw AST expression (node.target) so the data-flow/taint pass
(which only recognizes ast.Assign/ast.AnnAssign) doesn't see the for-loop
binding as a definition; change the builder to create and append a proper
assignment node representing the loop binding (e.g., construct an ast.Assign or
ast.AnnAssign with targets=[node.target] and an appropriate value placeholder)
before calling _process_statements so that the symbol is treated as a real
definition; update the block where loop_head/loop_body/current_block and
node.target are used to append that synthetic Assign/AnnAssign instead of the
bare expression.
| if node.orelse: | ||
| orelse_block = self._new_block() | ||
| # The "False" edge from head actually goes to orelse if present | ||
| # Fix previous connection | ||
| loop_head.successors = [s for s in loop_head.successors if s[1] != EdgeType.FALSE] | ||
| loop_head.add_successor(orelse_block, EdgeType.FALSE) | ||
|
|
There was a problem hiding this comment.
Keep predecessor links consistent when replacing the loop FALSE edge.
Lines 141-142 and 179-180 rewrite loop_head.successors, but the removed target keeps loop_head in its predecessors list. Both refactron/analysis/data_flow.py and refactron/analysis/taint.py consume node.predecessors, so loop-else graphs will carry facts along a path that no longer exists.
Also applies to: 177-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/analysis/cfg/builder.py` around lines 137 - 143, When you replace
the loop head's FALSE successor (e.g., the code that reassigns
loop_head.successors to filter out EdgeType.FALSE before
loop_head.add_successor(orelse_block, EdgeType.FALSE)), also remove loop_head
from the removed target block's predecessors and ensure the newly linked
orelse_block gets loop_head in its predecessors; likewise apply the same
predecessor-fix to the other identical rewrite site (the second occurrence
around the 177-180 region). Concretely: capture the removed successor target(s)
when filtering loop_head.successors, for each removed_target do
removed_target.predecessors = [p for p in removed_target.predecessors if p is
not loop_head], then call loop_head.add_successor(orelse_block, EdgeType.FALSE)
(or ensure orelse_block.predecessors includes loop_head) so predecessor lists
remain consistent.
| # Legacy fallback: no hash stored — use mtime (pre-SHA-256 state entries) | ||
| previous_mtime = previous.get("mtime", 0) | ||
| if current_mtime != previous_mtime: | ||
| logger.debug(f"Changed file detected (mtime mismatch, no hash): {file_path}") | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
Legacy cache entries without sha256 should be re-baselined, not trusted.
Falling back to mtime when the stored hash is missing preserves the exact blind spot this change is trying to close: a same-size edit with a restored timestamp is still treated as unchanged. Because unchanged files never go through update_file_state(), old state files can keep missing those edits indefinitely. Treat hash-less entries as changed once, or lazily backfill a hash, so upgrades are safe.
Safer migration behavior
- # Legacy fallback: no hash stored — use mtime (pre-SHA-256 state entries)
- previous_mtime = previous.get("mtime", 0)
- if current_mtime != previous_mtime:
- logger.debug(f"Changed file detected (mtime mismatch, no hash): {file_path}")
- return True
-
- return False
+ logger.debug(
+ "Legacy incremental state without sha256 detected for %s; forcing rebaseline",
+ file_path,
+ )
+ return True📝 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.
| # Legacy fallback: no hash stored — use mtime (pre-SHA-256 state entries) | |
| previous_mtime = previous.get("mtime", 0) | |
| if current_mtime != previous_mtime: | |
| logger.debug(f"Changed file detected (mtime mismatch, no hash): {file_path}") | |
| return True | |
| return False | |
| logger.debug( | |
| "Legacy incremental state without sha256 detected for %s; forcing rebaseline", | |
| file_path, | |
| ) | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/incremental.py` around lines 131 - 137, The legacy fallback
that trusts mtime when previous state lacks "sha256" is unsafe; in the function
handling file change detection (e.g., where previous = ... and
previous.get("mtime", 0) is used in refactron/core/incremental.py), treat
entries missing "sha256" as changed so they get re-baselined: detect absence of
previous.get("sha256") and return True (or otherwise force an update path such
as calling update_file_state() to compute and persist a hash) instead of falling
back to mtime; this ensures old state entries are refreshed and the blind spot
for same-size/timestamped edits is closed.
| if isinstance(obj, nodes.Instance): | ||
| if obj.name == type_name: | ||
| return True | ||
| # Check ancestry | ||
| for ancestor in obj.ancestors(): | ||
| if ancestor.name == type_name: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the astroid version targeted by this repository, what object type does node.infer()return for class instances, and should subtype checks useastroid.Instance, astroid.bases.Instance, or astroid.nodes.Instance?
💡 Result:
In recent astroid versions (4.x), node.infer returns an astroid.bases.Instance object for class instances. Subtype checks should use isinstance(result, astroid.bases.Instance).
Citations:
- 1: https://pylint.pycqa.org/projects/astroid/en/latest/inference.html
- 2: https://pylint.pycqa.org/projects/astroid/en/latest/api/general.html
- 3: https://pylint.readthedocs.io/projects/astroid/en/v4.0.4/_modules/astroid/bases.html
- 4: https://pylint.pycqa.org/projects/astroid/en/latest/api/nodes/astroid.nodes.ClassDef.html
🏁 Script executed:
# First, check what astroid version is being used
find . -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" -o -name "requirements*.txt" \) | head -20Repository: Refactron-ai/Refactron_lib
Length of output: 128
🏁 Script executed:
# Also check the imports in the inference.py file
head -50 refactron/core/inference.pyRepository: Refactron-ai/Refactron_lib
Length of output: 1811
🏁 Script executed:
# Look at the full context around lines 82-87
sed -n '70,100p' refactron/core/inference.pyRepository: Refactron-ai/Refactron_lib
Length of output: 855
🏁 Script executed:
# Check astroid version in requirements
cat requirements.txtRepository: Refactron-ai/Refactron_lib
Length of output: 169
🏁 Script executed:
# Also check pyproject.toml
cat pyproject.tomlRepository: Refactron-ai/Refactron_lib
Length of output: 3233
🏁 Script executed:
# Search for how nodes.Instance is actually used or defined in astroid
# Let's check if there are any other uses of nodes.Instance in this codebase
rg "nodes\.Instance" --type pyRepository: Refactron-ai/Refactron_lib
Length of output: 145
🌐 Web query:
astroid nodes.Instance class what is it - difference between nodes.Instance and bases.Instance
💡 Result:
In the Astroid library (used by Pylint for Python AST analysis and inference), there is no nodes.Instance class. The nodes module contains classes like ClassDef, FunctionDef, etc., but no Instance among the parse tree nodes. The bases.Instance class (from astroid.bases) is a special proxy object representing a class instance during static inference. It is not a parse tree node but an inference result yielded by node.infer when a node is inferred to be an instance of a ClassDef. It proxies attribute access to the underlying class definition for type analysis. bases.BaseInstance is the parent class providing basic instance lookup methods, while Instance adds specific instance behaviors like infer_binary_op and bool_value inference for class instances (checking bool/nonzero or len). Containers like List, Tuple inherit from bases.Instance via multiple inheritance to support instance-like inference. The query likely refers to bases.Instance (not nodes.Instance), with the difference being BaseInstance is more generic while Instance is specialized for regular class instances.
Citations:
- 1: https://pylint.pycqa.org/projects/astroid/en/latest/api/general.html
- 2: https://pylint.readthedocs.io/projects/astroid/en/v4.0.4/_modules/astroid/bases.html
- 3: https://pylint.pycqa.org/projects/astroid/en/latest/_modules/astroid/nodes/node_classes.html
- 4: https://pylint.readthedocs.io/projects/astroid/en/v2.11.7/api/base_nodes.html
🌐 Web query:
astroid 3.0 Instance type inference node.infer() return value
💡 Result:
In Astroid 3.0, the node.infer method returns a generator yielding InferenceResult objects, which can be AST nodes (like Const), special inference classes such as Uninferable (singleton for uninferable values), or Instance (for inferred class instances). For instance type inference, when a node is inferred to be an instance of a known class, node.infer yields an Instance object from astroid.bases, with the underlying class in its _proxied attribute. Instance supports getattr for attribute access on the inferred instance and pytype returning the class qname. This behavior is unchanged from prior versions, with Astroid 3.0 adding Python 3.12 support and inference improvements but no breaking changes to infer return types.
Citations:
- 1: https://pylint.readthedocs.io/projects/astroid/en/v3.3.10/inference.html
- 2: https://pylint.pycqa.org/projects/astroid/en/latest/inference.html
- 3: https://pylint.pycqa.org/projects/astroid/en/latest/_modules/astroid/nodes/node_ng.html
- 4: https://pylint.readthedocs.io/projects/astroid/en/v4.0.4/_modules/astroid/bases.html
- 5: https://github.com/pylint-dev/astroid
🌐 Web query:
astroid.Instance public API import how to check isinstance class instance
💡 Result:
To import the astroid.Instance class in Python: python from astroid.bases import Instance astroid.Instance is a special proxy node representing a class instance during static inference, not a regular Python class instance. It is created internally by astroid's inference engine, e.g., ClassDef.instantiate_class returns an Instance. To check if an astroid node is an Instance (i.e., represents a class instance): Use isinstance(node, Instance): python from astroid.bases import Instance if isinstance(some_node, Instance): print("This node represents a class instance") print("Underlying class:", some_node.proxied.name) Alternatively, check the pytype method, which returns the qualified class name for Instance nodes (e.g., 'module.ClassName'): python if "Instance of" in some_node.display_type: print("This is an Instance node") Instance inherits from Proxy and BaseInstance, with key methods: - proxied: The ClassDef it represents an instance of. - pytype: Returns self.proxied.qname (the class's qualified name). - getattr(name): Looks up attributes via MRO. - display_type: Returns "Instance of {class_name}". Instance nodes are yielded by node.infer when inference determines a value is a class instance. Regular AST nodes like ClassDef have pytype returning 'builtins.type'. Official documentation: Astroid General API and bases module source code confirm Instance is in astroid.bases and used for isinstance checks in inference contexts.
Citations:
- 1: https://pylint.pycqa.org/projects/astroid/en/latest/api/general.html
- 2: https://pylint.pycqa.org/projects/astroid/en/latest/_modules/astroid/nodes/node_classes.html
- 3: https://pylint.readthedocs.io/projects/astroid/en/v4.0.4/_modules/astroid/bases.html
- 4: https://pylint.pycqa.org/projects/astroid/en/latest/inference.html
🌐 Web query:
astroid public API Instance import astroid.Instance direct vs astroid.bases.Instance
💡 Result:
In recent versions of astroid (e.g., 4.2.0-dev0 and 4.1.1), the Instance class is defined in the astroid.bases module, as shown in the source code of bases.py where class Instance(BaseInstance): is explicitly declared. BaseInstance is also in astroid.bases. However, the official API documentation (General API pages) lists and documents class astroid.Instance(proxied: ClassDef| None) Bases: BaseInstance, indicating that astroid.Instance is directly importable from the top-level astroid package. This suggests that astroid's init.py re-exports Instance (and BaseInstance) from astroid.bases for public API convenience, making "from astroid import Instance" the recommended public API usage. The astroid.bases.Instance path is the internal module location but not the intended public import path. Older documentation (e.g., Astroid 2.11.7) shows some nodes inheriting from astroid.bases.Instance, confirming bases as the origin, but current docs promote the top-level import. Use "from astroid import Instance" for the public API. Direct usage of "from astroid.bases import Instance" works but bypasses the public API surface.
Citations:
- 1: https://pylint.pycqa.org/projects/astroid/en/latest/api/general.html
- 2: https://pylint.readthedocs.io/projects/astroid/en/v4.0.4/_modules/astroid/bases.html
- 3: https://pylint.readthedocs.io/projects/astroid/en/stable/api/general.html
- 4: https://github.com/pylint-dev/astroid
Fix incorrect astroid type check for class instances.
The code uses isinstance(obj, nodes.Instance) but nodes.Instance does not exist in astroid. The nodes module contains only parse tree nodes (ClassDef, FunctionDef, etc.), not inference result types. For class instances yielded by node.infer(), use isinstance(obj, Instance) where Instance is imported from the top-level astroid package. Without this fix, the isinstance check will fail and ancestry checking is skipped entirely.
Context
if isinstance(obj, nodes.Instance):
if obj.name == type_name:
return True
# Check ancestry
for ancestor in obj.ancestors():
if ancestor.name == type_name:Change to:
if isinstance(obj, Instance):
if obj.name == type_name:
return True
# Check ancestry
for ancestor in obj.ancestors():
if ancestor.name == type_name:Add Instance to the imports at the top of the file:
from astroid import Instance🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/inference.py` around lines 82 - 87, The isinstance check is
using the wrong symbol nodes.Instance; replace checks of isinstance(obj,
nodes.Instance) with isinstance(obj, Instance) (e.g., in the block that iterates
inferred objects where obj is checked and ancestor.name compared) and add the
top-level import from astroid: from astroid import Instance so the runtime type
check succeeds and ancestry logic runs as intended.
- Remove unused imports (Any, Dict, Union, Optional, asdict, Set, time, pytest) - Add return type annotations to CFGBuilder, CFGNode, SymbolTable methods - Replace bare tuple[...] with typing.Tuple[...] for Python 3.8 compat - Fix bare except → except Exception in symbol_table.py - Move collections.defaultdict import to top of taint.py (E402) - Remove dead code (worklist, visited_config) from taint.py - Break long lines > 100 chars in taint.py, data_flow.py, refactron.py - Add # noqa: F401/E402 to intentional fixture imports - Remove unused local var original_analyze in test_exception_isolation.py - Fix inference.py: cast return values to str to satisfy mypy no-any-return Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tespace) - taint.py: extend type: ignore to cover attr-defined as well as union-attr - test_tui_viewer.py: fix isort import ordering - dev-notes/Refactron_Comprehensive_MVP.md: strip trailing whitespace Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…13.2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Phase 1 Stabilize:
Interactive TUI:
Semantic analysis layer:
779 tests passing (21 new TUI + 8 fixture + Day 1-3 tests)
Summary by CodeRabbit
Release Notes
New Features
--dry-runflag to preview refactoring changes safely without modifying files.Improvements
--no-cacheand--no-interactiveCLI options for advanced workflows.Documentation