Feat/pipeline config - #180
shrutu0929 wants to merge 1 commit into
Conversation
|
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 51 minutes and 47 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 (2)
📝 WalkthroughWalkthroughThis PR significantly expands refactron's capabilities by introducing LLM-based triage and batch-suggestion features, implementing incremental symbol-table analysis with file-change detection, adding keyword-mode fallback for RAG indexing, launching a new RefactronPipeline orchestration class, and enabling new CLI commands for AI-powered code fixing and platform-aware interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant AIFix as ai_fix Command
participant Refactron as Refactron Instance
participant LLMOrch as LLMOrchestrator
participant RAGIdx as RAGIndexer/ContextRetriever
participant Suggester as Suggestion Generator
User->>AIFix: Execute ai_fix with target file
AIFix->>Refactron: analyze(target_file)
Refactron->>Refactron: Parse file, detect issues
Refactron-->>AIFix: Return issues for file
AIFix->>AIFix: Filter to target file issues
AIFix->>LLMOrch: Create LLMOrchestrator(workspace_path)
LLMOrch->>RAGIdx: _ensure_retriever() - load or build index
RAGIdx-->>LLMOrch: ContextRetriever ready
AIFix->>LLMOrch: generate_batch_suggestion(issues, code)
LLMOrch->>RAGIdx: retrieve_similar(issue context)
RAGIdx-->>LLMOrch: Top 3 relevant chunks
LLMOrch->>Suggester: Build batch prompt + context
Suggester-->>LLMOrch: LLM response (JSON)
LLMOrch->>LLMOrch: Parse & validate proposed_code
LLMOrch-->>AIFix: RefactoringSuggestion
AIFix->>AIFix: Display suggestion, confidence
alt Interactive Mode
User->>AIFix: Approve changes
AIFix->>AIFix: Create backup session
AIFix->>AIFix: Overwrite file with proposed_code
AIFix-->>User: Completion message
else No Apply
AIFix-->>User: Suggestion shown (not applied)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
a03d5f2 to
a489d54
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
refactron/cli/analysis.py (2)
31-31:⚠️ Potential issue | 🔴 CriticalRemove unused
ContextRetrieverimport (pipeline failure).Pre-commit flake8 is failing with F401.
ContextRetrieveris no longer referenced in this module after the refactor toLLMOrchestrator.🔧 Proposed fix
-from refactron.rag.retriever import ContextRetriever🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/analysis.py` at line 31, Remove the unused import ContextRetriever from refactron.cli.analysis to fix the F401 flake8 error: open the module where ContextRetriever is imported (the top-level import line "from refactron.rag.retriever import ContextRetriever") and delete that import; verify only LLMOrchestrator and other needed symbols remain imported and run pre-commit/flake8 to confirm the pipeline passes.
211-228:⚠️ Potential issue | 🟠 MajorJSON output ignores
--fail-onthreshold.In JSON mode the exit code is hard-coded to
1 if summary["critical"] > 0 else 0, but the text-mode path (Lines 262–276) correctly honors--fail-on. CI/CD users passing--format json --fail-on ERRORwill silently get a0exit on ERROR-level issues.Consider computing
should_failonce before the JSON branch and reusing it here too.🔧 Proposed fix
- # JSON format — output raw JSON and exit immediately - if output_format == "json": - import json as _json - - issues_data = [ - ... - ] - payload = {**summary, "issues": issues_data} - click.echo(_json.dumps(payload, indent=2)) - raise SystemExit(1 if summary["critical"] > 0 else 0) + # Compute fail-on threshold (shared by JSON and text paths). + _LEVEL_RANK = {"INFO": 0, "WARNING": 1, "ERROR": 2, "CRITICAL": 3} + _SUMMARY_KEY = {"INFO": "info", "WARNING": "warnings", "ERROR": "errors", "CRITICAL": "critical"} + effective_fail_on = (fail_on or "CRITICAL").upper() + threshold = _LEVEL_RANK[effective_fail_on] + should_fail = any( + summary[_SUMMARY_KEY[lvl]] > 0 for lvl, rank in _LEVEL_RANK.items() if rank >= threshold + ) + + if output_format == "json": + import json as _json + issues_data = [ ... ] + payload = {**summary, "issues": issues_data} + click.echo(_json.dumps(payload, indent=2)) + raise SystemExit(1 if should_fail else 0)Then drop the duplicate block at Lines 262–276.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/analysis.py` around lines 211 - 228, The JSON branch currently decides exit code using summary["critical"] but ignores the --fail-on threshold; compute a single should_fail boolean before the JSON/text branching (derived from the parsed --fail-on setting and the summary counts) and use that should_fail to set the exit status in the JSON block (replace the hard-coded summary["critical"] check), then remove the duplicate exit decision in the later text-mode block so both formats share the same failure logic; look for variables/functions named output_format, summary, result.all_issues and the later text-mode exit logic to update and consolidate.tests/test_rag_indexer.py (2)
9-9:⚠️ Potential issue | 🔴 CriticalRemove unused
pytestimport (pipeline failure).Pre-commit flake8 is failing with F401. No
pytest.raises/fixture usage remains in this file after the Chroma-available refactor.🔧 Proposed fix
-import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rag_indexer.py` at line 9, Remove the unused pytest import from the top of the test file: delete the "import pytest" statement in tests/test_rag_indexer.py (the unused symbol is the pytest import) so flake8 F401 no longer fails; no other code changes are required since no pytest.raises or fixtures are used.
287-310:⚠️ Potential issue | 🟡 MinorInconsistent attribute:
llm_clientvsllm_integration.Other tests in this file were migrated to
indexer.llm_integrationandmock_llm.generate_chunk_summary(matching the production rename).test_index_file_with_summarizestill setsindexer.llm_client = mock_llmand mocksgenerate, which no longer wires into the summarization path. The assertion on Line 310 ("Summary:" in mock_chunk.content or chunks is not None) is also a tautology —chunksis always truthy — so it silently passes even if summarization never ran.🔧 Proposed fix
- mock_llm = MagicMock() - mock_llm.generate.return_value = "Says hello." - indexer.llm_client = mock_llm + mock_llm = MagicMock() + mock_llm.generate_chunk_summary.return_value = "Says hello." + indexer.llm_integration = mock_llm ... - assert "Summary:" in mock_chunk.content or chunks is not None + assert len(chunks) == 1 + mock_llm.generate_chunk_summary.assert_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rag_indexer.py` around lines 287 - 310, The test uses the old attribute llm_client and generate; update it to use the production names: set indexer.llm_integration = mock_llm and mock_llm.generate_chunk_summary.return_value = "Says hello." so _index_file(summarize=True) actually calls the mocked summarizer (targeting indexer._index_file and generate_chunk_summary). Replace the tautological assertion with a real check, e.g. assert "Summary:" in chunks[0].content or assert mock_llm.generate_chunk_summary.assert_called_with(mock_chunk) to verify summarization ran.refactron/cli/rag.py (1)
143-168:⚠️ Potential issue | 🟠 MajorHoist
LLMOrchestratorout of the result loop.When
--rerankis set, a freshLLMOrchestrator(workspace_path=local_path)is built for every result (each call re-loads the retriever/vector index). Construct it once before the loop.🔧 Proposed fix
console.print(f"\n[primary]Found {len(results)} results for:[/primary] {query}\n") + rerank_orchestrator = LLMOrchestrator(workspace_path=local_path) if rerank else None + for i, result in enumerate(results, 1): relevance_score = max(0, 1 - result.distance) * 100 # AI Reranking if enabled if rerank: try: - orchestrator = LLMOrchestrator(workspace_path=local_path) - prompt = ( + prompt = ( ... ) - ai_response = orchestrator.client.generate( + ai_response = rerank_orchestrator.client.generate( prompt=prompt,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/rag.py` around lines 143 - 168, The code constructs a new LLMOrchestrator inside the results loop when rerank is true, causing repeated expensive reloads; move creation of LLMOrchestrator(workspace_path=local_path) outside and before the for i, result in enumerate(results, 1): loop and reuse that single instance (e.g., create orchestrator = LLMOrchestrator(...) only once when rerank is set), keeping the existing try/except and ai_response logic unchanged so you still fallback to the distance-based score on errors.refactron/cli/refactor.py (1)
33-33:⚠️ Potential issue | 🟡 MinorRemove unused
ContextRetrieverimport (flake8 F401).The
document()change removed the only use ofContextRetriever, andai_fixdoesn't reference it either.🧹 Proposed fix
-from refactron.rag.retriever import ContextRetriever🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/refactor.py` at line 33, Remove the unused import ContextRetriever from refactron.cli.refactor (it's no longer used after the document() change and ai_fix doesn't reference it); update the import line that currently reads "from refactron.rag.retriever import ContextRetriever" by deleting ContextRetriever (or removing the entire import statement if nothing else is imported) so flake8 F401 is resolved.
🟡 Minor comments (14)
bad_code.py-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorInverted and unused min/max constants.
MIN_ITERATION_VALUE = 10is greater thanMAX_ITERATION_VALUE = 5— the names and values are inconsistent. Additionally, neither constant is referenced anywhere in this module. Remove them, or fix the values and wire them into the loop bounds if they were intended to replaceITERATION_LIMIT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bad_code.py` around lines 5 - 6, MIN_ITERATION_VALUE and MAX_ITERATION_VALUE are inverted and unused; either remove them or make them meaningful by fixing their values and using them to control loop bounds. If intended to replace ITERATION_LIMIT, set MIN_ITERATION_VALUE < MAX_ITERATION_VALUE (e.g., MIN_ITERATION_VALUE = 1, MAX_ITERATION_VALUE = 10), remove or update the old ITERATION_LIMIT usage, and replace occurrences of ITERATION_LIMIT with logic that validates and uses MIN_ITERATION_VALUE and MAX_ITERATION_VALUE (e.g., clamp or iterate between them) inside the function(s) that perform iterations.refactron/analysis/symbol_table.py-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorRemove the unused
Setimport.This is already failing pre-commit with
flake8: F401 'typing.Set' imported but unused.Proposed fix
-from typing import Any, Dict, List, Optional, Set +from typing import Any, Dict, List, Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/symbol_table.py` at line 12, The import line "from typing import Any, Dict, List, Optional, Set" in symbol_table.py includes an unused symbol Set which triggers flake8 F401; remove Set from the typing import so it reads "from typing import Any, Dict, List, Optional" to satisfy the linter and keep only used typing names.refactron/core/pipeline.py-29-39 (1)
29-39:⚠️ Potential issue | 🟡 Minorflake8 E501 in the docstring.
Pre-commit flags lines 35 and 39 (107 and 109 chars). Reflow the docstring paragraphs to fit in 100 cols.
As per coding guidelines: "Flake8 linting must use max-line-length of 100".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/pipeline.py` around lines 29 - 39, The docstring contains lines longer than the 100-char Flake8 limit; reflow the long paragraphs in the function docstring so every line is <=100 chars (wrap the sentences describing enable_incremental_analysis, use_incremental, Args, and Returns), keeping the same wording and references to enable_incremental_analysis, use_incremental, target, AnalysisResult, and target_path intact so the docstring remains accurate and lint-clean.refactron/core/pipeline.py-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minorflake8 E501 on the constructor signature.
Line 13 is 107 chars. Break it across lines.
🧹 Proposed fix
- def __init__(self, project_root: Optional[Union[str, Path]] = None, config_path: Optional[Union[str, Path]] = None): + def __init__( + self, + project_root: Optional[Union[str, Path]] = None, + config_path: Optional[Union[str, Path]] = None, + ):As per coding guidelines: "Use line length of 100 characters, enforced by black, isort, and flake8".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/pipeline.py` at line 13, The __init__ constructor signature in class Pipeline is longer than the 100-char limit, triggering flake8 E501; split the parameters across multiple lines (for example put each optional parameter on its own line or group them so the line length stays under 100) in the def __init__(...) declaration in refactron/core/pipeline.py, preserving the existing type hints Optional[Union[str, Path]] for project_root and config_path and keeping the same parameter order and default values so behavior of Pipeline.__init__ is unchanged.tests/test_symbol_table_incremental.py-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorDrop unused imports flagged by flake8 F401.
Pre-commit fails on
Path(line 3) andSymbolType(line 5); neither is referenced in the test bodies.🧹 Proposed fix
import json import time -from pathlib import Path -from refactron.analysis.symbol_table import SymbolTableBuilder, SymbolType +from refactron.analysis.symbol_table import SymbolTableBuilder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_symbol_table_incremental.py` around lines 1 - 5, Remove the unused imports causing flake8 F401: drop Path from the "from pathlib import Path" import and drop SymbolType from "from refactron.analysis.symbol_table import SymbolTableBuilder, SymbolType", leaving only the used SymbolTableBuilder import (or alternatively use the symbols in tests if they were intended). Ensure the import line now only includes SymbolTableBuilder and remove the standalone Path import.refactron/core/refactron.py-349-353 (1)
349-353:⚠️ Potential issue | 🟡 Minorflake8 E501 at line 351.
Pre-commit reports line 351 is 102 chars. Wrap the recovery_suggestion string (or hoist it into a constant).
🧹 Proposed fix
- recovery_suggestion="Check the file for syntax errors or encoding issues", + recovery_suggestion=( + "Check the file for syntax errors or encoding issues" + ),As per coding guidelines: "Use line length of 100 characters, enforced by black, isort, and flake8".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/refactron.py` around lines 349 - 353, The long string assigned to the recovery_suggestion in the error construction (the recovery_suggestion argument passed when building the error object in refactron/core/refactron.py) exceeds the 100-char line length and triggers flake8 E501; fix by either wrapping the string literal across multiple concatenated strings or moving it into a named constant (e.g., RECOVERY_SUGGESTION_SYNTAX_ERROR) and then using that constant in the recovery_suggestion= parameter so the line length stays under 100 chars while preserving the same message.refactron/rag/indexer.py-262-282 (1)
262-282:⚠️ Potential issue | 🟡 Minormypy: indexed assignment into
chunk_dict["metadata"]is rejected.Pre-commit mypy fails at line 276 because the nested metadata dict's value type is inferred as
Collection[str]from the str-literal values in the initializer, sochunk_dict["metadata"][key] = valueisn't type-safe. Buildmetadataas an explicitly-typed local dict first and assemblechunk_dictafterwards.🧹 Proposed fix
for chunk in chunks: - chunk_dict = { - "content": chunk.content, - "metadata": { - "chunk_type": chunk.chunk_type, - "file_path": chunk.file_path, - "name": chunk.name, - "line_start": chunk.line_range[0], - "line_end": chunk.line_range[1], - }, - } - # Add extra metadata - for key, value in chunk.metadata.items(): - if value is not None: - chunk_dict["metadata"][key] = value + metadata: Dict[str, Any] = { + "chunk_type": chunk.chunk_type, + "file_path": chunk.file_path, + "name": chunk.name, + "line_start": chunk.line_range[0], + "line_end": chunk.line_range[1], + } + for key, value in chunk.metadata.items(): + if value is not None: + metadata[key] = value + chunk_dict: Dict[str, Any] = {"content": chunk.content, "metadata": metadata}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/indexer.py` around lines 262 - 282, The mypy error comes from assigning into chunk_dict["metadata"] whose type was inferred too narrowly; instead construct a local typed metadata dict (e.g., metadata: Dict[str, Any]) by copying the fixed fields (chunk_type, file_path, name, line_start, line_end) and then merging chunk.metadata entries (skipping None), and only after that build chunk_dict = {"content": chunk.content, "metadata": metadata}; update the loop over chunks to use these symbols (chunk, chunk.metadata, chunk_dict) and leave the JSON dump to self.chunk_storage as-is.refactron/rag/retriever.py-86-96 (1)
86-96:⚠️ Potential issue | 🟡 Minor
raise RuntimeError(...)insideexceptshould chain (B904) and line 95 is over 100 chars.Both issues in one block: ruff flags missing
from(so the originalget_collectionexception is suppressed in tracebacks) and flake8 reports E501 on line 95.🧹 Proposed fix
try: self.collection = self.client.get_collection(name=collection_name) - except Exception: + except Exception as exc: # If vector collection is missing but folder exists, # we might have indexed in keyword mode before if (self.index_path / "keyword_chunks.json").exists(): self.mode = "keyword" else: raise RuntimeError( - f"Index not found at {self.index_path}. Run 'refactron rag index' first." - ) + f"Index not found at {self.index_path}. " + "Run 'refactron rag index' first." + ) from excAs per coding guidelines: "Flake8 linting must use max-line-length of 100".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/retriever.py` around lines 86 - 96, The except block handling self.client.get_collection should re-raise the RuntimeError while chaining the original exception (e.g., use "from e") so the original traceback isn't lost; also shorten the RuntimeError message to <=100 characters (or split/format) to satisfy max-line-length; locate the block that sets self.mode to "keyword" when (self.index_path / "keyword_chunks.json").exists() and replace the bare raise RuntimeError(f"Index not found at {self.index_path}. Run 'refactron rag index' first.") with a chained, shorter RuntimeError that includes context and uses "from" to chain the caught exception.refactron/llm/prompts.py-60-93 (1)
60-93:⚠️ Potential issue | 🟡 MinorFix
flake8 E501violations flagged by pre-commit.Lines 60, 61, 75, and 93 exceed the 100-char limit. Since these are inside triple-quoted string constants, wrap using implicit string concatenation or break the lines manually; the content is instructional text so whitespace can be adjusted freely.
🧹 Proposed fix (illustrative)
-BATCH_SUGGESTION_SYSTEM_PROMPT = """You are an expert software architect and code refactoring specialist. -Your goal is to analyze multiple code issues in a file and provide a single, comprehensive fix that resolves all of them. +BATCH_SUGGESTION_SYSTEM_PROMPT = """\ +You are an expert software architect and code refactoring specialist. +Your goal is to analyze multiple code issues in a file and provide a +single, comprehensive fix that resolves all of them. ... - "confidence_score": "Float between 0.0 and 1.0 representing your confidence in this combined fix" + "confidence_score": "Float between 0.0 and 1.0 representing confidence in this combined fix" ... -Provide a single, comprehensive fix that resolves ALL the listed issues while maintaining consistency with the codebase. +Provide a single, comprehensive fix that resolves ALL the listed issues while maintaining +consistency with the codebase.As per coding guidelines: "Use line length of 100 characters, enforced by black, isort, and flake8".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/prompts.py` around lines 60 - 93, The triple-quoted prompt constants BATCH_SUGGESTION_SYSTEM_PROMPT and BATCH_SUGGESTION_PROMPT contain lines exceeding the 100-char flake8 E501 limit; fix by splitting long lines inside those string literals using implicit concatenation or breaking them into multiple short string literals joined by adjacency (preserving content, backslashes and escaped newlines) so the effective runtime string is unchanged; update the long instructional lines (the ones around the start of BATCH_SUGGESTION_SYSTEM_PROMPT and the long "Output JSON structure:" block, plus the long lines in BATCH_SUGGESTION_PROMPT where original_code and rag_context are presented) to be under 100 characters each while keeping the wording and required escape instructions intact.refactron/cli/refactor.py-620-689 (1)
620-689:⚠️ Potential issue | 🟡 MinorMultiple flake8 E501 violations in the new
ai_fixcommand.Pre-commit reports lines 622 (107), 660 (107), 683 (117), and 688 (111) over the 100-char limit. Wrap the console strings.
🧹 Proposed fix
- console.print( - "[red]Error: Please specify a single file. Directory analysis for ai-fix is coming soon.[/red]" - ) + console.print( + "[red]Error: Please specify a single file. " + "Directory analysis for ai-fix is coming soon.[/red]" + ) ... - console.print( - f" {idx}. [yellow]{issue.category.value}[/yellow]: {issue.message} (Line {issue.line_number})" - ) + console.print( + f" {idx}. [yellow]{issue.category.value}[/yellow]: " + f"{issue.message} (Line {issue.line_number})" + ) ... - console.print( - f"[dim]AI Confidence: {suggestion.llm_confidence:.2f}, Safety Score: {suggestion.confidence_score:.2f}[/dim]" - ) + console.print( + f"[dim]AI Confidence: {suggestion.llm_confidence:.2f}, " + f"Safety Score: {suggestion.confidence_score:.2f}[/dim]" + ) ... - console.print( - f"[red]Warning: Fix failed basic safety checks: {', '.join(suggestion.safety_result.issues)}[/red]" - ) + console.print( + "[red]Warning: Fix failed basic safety checks: " + f"{', '.join(suggestion.safety_result.issues)}[/red]" + )As per coding guidelines: "Use line length of 100 characters, enforced by black, isort, and flake8".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/refactor.py` around lines 620 - 689, Several console string literals exceed the 100-char line length (flake8 E501); shorten/wrap the long strings passed to console.print and the Panel/Markdown calls so no line exceeds 100 chars. Edit the long calls around the ai-fix flow — specifically the console.print that logs the "Error: Please specify a single file..." message (target_path check), the multiple console.prints listing issues (uses issue.category.value/issue.message), the console.status messages, and the Panel/console.print calls that show suggestion.explanation and suggestion.proposed_code (the Panel(... title=...) and style=... lines) — split long f-strings or use implicit string concatenation or shorter variables so each source line stays <=100 chars while preserving the exact output content and references to target_path, issues_to_fix, orchestrator.generate_batch_suggestion, SuggestionStatus, suggestion.explanation, and suggestion.proposed_code.refactron/rag/indexer.py-10-24 (1)
10-24:⚠️ Potential issue | 🟡 Minorflake8 E402: module-level imports must come before other top-level code.
Lines 23–24 (
from refactron.rag.chunker import CodeChunkandfrom refactron.rag.parser import CodeParser) sit after the runtimetry/exceptblock (lines 14–17) and theCHROMA_AVAILABLE = Noneassignment (line 21), causing the pre-commit failure. Move thechunker/parserimports up with the other stdlib/local imports, and keep only the lazy-import try/except sentinel below them.🧹 Proposed reordering
from typing import TYPE_CHECKING, Any, Dict, List, Optional, cast +from refactron.rag.chunker import CodeChunk +from refactron.rag.parser import CodeParser + if TYPE_CHECKING: from refactron.llm.orchestrator import LLMOrchestrator # LLMOrchestrator is imported lazily at runtime to keep it patchable in tests try: from refactron.llm.orchestrator import LLMOrchestrator as _LLMOrchestrator # noqa: F401 except Exception: _LLMOrchestrator = None # type: ignore # RAG dependencies are loaded lazily in __init__ to prevent CLI crashes # if libraries like PyTorch fail to initialize (common on some Windows environments). CHROMA_AVAILABLE = None -from refactron.rag.chunker import CodeChunk -from refactron.rag.parser import CodeParser🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/indexer.py` around lines 10 - 24, Move the module-level imports for CodeChunk and CodeParser above any top-level runtime code so flake8 E402 is satisfied: relocate "from refactron.rag.chunker import CodeChunk" and "from refactron.rag.parser import CodeParser" to join the other imports at the top, then keep the lazy-import try/except that defines _LLMOrchestrator and the CHROMA_AVAILABLE = None sentinel below them; ensure the try/except still assigns _LLMOrchestrator = None on exception and that CHROMA_AVAILABLE remains defined after the imports.refactron/llm/orchestrator.py-80-102 (1)
80-102:⚠️ Potential issue | 🟡 Minor
build_vector_indexdoesn't refreshself.workspace_path, leading to stale retriever state.If a caller invokes
build_vector_index(some_other_path)on an orchestrator whoseself.workspace_pathpoints elsewhere, the localworkspace_pathis used to build the index and to create the retriever inside this method (line 99), but a subsequent_ensure_retriever()call would retry against the originalself.workspace_path. Either updateself.workspace_path = workspace_pathhere, or document that this method is intended to be called only with the orchestrator's own workspace.Also note that the
except Exceptionat line 101 silently swallows index build failures, leaving the caller (e.g.refactron/cli/rag.py) unable to distinguish "indexed successfully" from "silently failed". Consider re-raising or returning a bool/result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/orchestrator.py` around lines 80 - 102, build_vector_index currently uses the passed workspace_path to build the index and recreate the retriever but does not update the orchestrator state, causing later calls to _ensure_retriever to operate against the old self.workspace_path; update self.workspace_path = workspace_path at the start (or after successful indexing) so the orchestrator state matches the retriever created (references: build_vector_index, self.workspace_path, _ensure_retriever, ContextRetriever, RAGIndexer), and change the broad except Exception that swallows failures to either re-raise the exception or return a success boolean (e.g., return True on success and False or raise on failure) so callers can detect index build failures instead of silently proceeding.refactron/llm/orchestrator.py-347-348 (1)
347-348:⚠️ Potential issue | 🟡 MinorConfidence parsing is inconsistent with
generate_suggestionand can needlessly fail the batch.
generate_suggestion(lines 183–196) tolerates strings like"0.9","90%", or ranges. Here a plainfloat(data.get("confidence_score", 0.7))is used twice — any non-numeric response raisesValueError, which is caught by the outerexceptat line 351 and downgrades a fully valid LLM suggestion toFAILED. SinceBATCH_SUGGESTION_SYSTEM_PROMPTasks the model for a "Float between 0.0 and 1.0 representing your confidence" as a string, this is a likely code path.Consider extracting the existing confidence-parsing block into a helper (e.g.,
_parse_confidence) and reusing it in both methods. Same comment applies to clamping —generate_suggestionclamps withmin(max(confidence, 0.0), 1.0)while this one does not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/orchestrator.py` around lines 347 - 348, The confidence parsing here is brittle: replace the direct float(...) calls in the block that sets confidence_score and llm_confidence with a reusable helper (e.g., _parse_confidence) that mirrors generate_suggestion's logic — accept numeric types and strings like "0.9", "90%", and ranges, normalize to a single float, clamp with min(max(value, 0.0), 1.0), and return a default when parsing fails; implement _parse_confidence (or extract the existing parsing from generate_suggestion) and call it for both confidence_score and llm_confidence (using the same parsed value) instead of float(data.get("confidence_score", 0.7)) so non-numeric but valid string responses won't cause ValueError and incorrect FAILED results.refactron/core/parallel.py-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorFix flake8 E501: docstring line exceeds 100 chars.
Line 75 is 111 characters, failing the pre-commit lint check (max-line-length=100). As per coding guidelines ("Use line length of 100 characters, enforced by black, isort, and flake8"), wrap the docstring.
🛠️ Proposed fix
- process_func: Function to process a single file. Should return - (FileMetrics, None, skip_warn) on success or (None, FileAnalysisError, None) on error. + process_func: Function to process a single file. Should return + (FileMetrics, None, skip_warn) on success or + (None, FileAnalysisError, None) on error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/parallel.py` around lines 74 - 75, The docstring line describing the process_func parameter exceeds 100 characters; update the docstring in refactron/core/parallel.py so the line wrapping keeps each line <=100 chars (e.g., split the sentence into two lines) while preserving the meaning and types (process_func, FileMetrics, FileAnalysisError, skip_warn). Ensure you break the long line into shorter lines at sensible boundaries (after commas or between clauses) so flake8 E501 is satisfied.
🧹 Nitpick comments (13)
bad_code.py (1)
14-17: Minor cleanups: collapse nestedifand rename unused loop variable.Per Ruff B007,
iis unused in the loop body. The two nestedifs can also be combined for readability.Proposed fix
- if x > THRESHOLD_VALUE: - if y < MAX_Y_VALUE: - for i in range(ITERATION_LIMIT): - print("doing something", x) + if x > THRESHOLD_VALUE and y < MAX_Y_VALUE: + for _ in range(ITERATION_LIMIT): + print("doing something", x)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bad_code.py` around lines 14 - 17, Collapse the two nested conditionals into a single combined check (replace the nested if x > THRESHOLD_VALUE: if y < MAX_Y_VALUE: with a single if x > THRESHOLD_VALUE and y < MAX_Y_VALUE:) and rename the unused loop variable i in the for loop over range(ITERATION_LIMIT) to an underscore (for _ in range(ITERATION_LIMIT)) to satisfy Ruff B007 and improve readability; update the block containing x, THRESHOLD_VALUE, y, MAX_Y_VALUE, and ITERATION_LIMIT accordingly.refactron/analysis/taint.py (2)
125-125: Duplicate/misplaced comment.
# 2. Iterative Taint Propagationduplicates the same heading already present at Line 101 directly above this loop. Looks like a stray artifact — consider removing to avoid misleading readers into thinking a new phase starts here.🧹 Proposed cleanup
- # 2. Iterative Taint Propagation # Process block statements current_taint = incoming_taint.copy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/taint.py` at line 125, Remove the stray duplicate comment "# 2. Iterative Taint Propagation" found immediately above the iterative loop in the taint propagation routine; keep the original heading already present earlier in the same function in refactron.analysis.taint and simply delete this redundant line so the code and comment flow remain clear and spacing is preserved.
82-91: Minor: indexing storesast.Attributenodes that are never consumed.
_check_sinkonly branches onisinstance(node, ast.Call)(Line 262), so everyast.Attributepushed into_statement_metahere is dead weight — it grows the per-statement list and the outer loop iterates over entries that can never match a sink. If attributes aren't needed for future source detection in this path, restrict indexing toast.Callonly; otherwise add a short comment documenting the intended future use.♻️ Proposed refactor
- for child in ast.walk(stmt): - if isinstance(child, (ast.Call, ast.Attribute)): - sensitive.append(child) + for child in ast.walk(stmt): + if isinstance(child, ast.Call): + sensitive.append(child)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/analysis/taint.py` around lines 82 - 91, _index_sensitive_nodes currently adds both ast.Call and ast.Attribute nodes into self._statement_meta but _check_sink only handles ast.Call, so remove ast.Attribute from the indexed set (only collect ast.Call) or, if attributes are intended for future use, add a short clarifying comment in _index_sensitive_nodes explaining why ast.Attribute entries are stored; update the tuple in the isinstance check in _index_sensitive_nodes to only include ast.Call (or add the comment) and ensure references to _statement_meta and _check_sink remain consistent.refactron/cli/main.py (1)
37-38: Moveimport osto module top.PEP 8 / isort prefers module-level imports. This inline import is unnecessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/main.py` around lines 37 - 38, The inline "import os" should be moved to the top of the module with the other imports to satisfy PEP8/isort; locate the "import os" occurrence in refactron.cli.main (currently an inline import) and relocate it to the module-level import block, grouping it appropriately with standard-library imports and re-running isort/flake8 to confirm import order and style.refactron/cli/rag.py (1)
71-84: RedundantRAGIndexerinstantiation to fetch stats.
orchestrator.build_vector_indexalready constructs aRAGIndexerinternally and callsindex_repository, which (perrefactron/rag/indexer.py) returns anIndexStats. The current code discards that and then re-instantiatesRAGIndexer(local_path)to callget_stats()— duplicating setup (SentenceTransformer load, Chroma client init).Consider having
LLMOrchestrator.build_vector_indexreturn theIndexStatsfromindex_repository, then reuse it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/rag.py` around lines 71 - 84, LLMOrchestrator.build_vector_index currently discards the IndexStats produced by RAGIndexer.index_repository, causing the CLI to re-instantiate RAGIndexer just to call get_stats(); change build_vector_index (LLMOrchestrator.build_vector_index) to return the IndexStats returned by RAGIndexer.index_repository (or propagate it up) and then in refactron/cli/rag.py use the returned stats directly instead of creating a new RAGIndexer(local_path) and calling get_stats(), so you avoid reloading SentenceTransformer and reinitializing the Chroma client.tests/test_rag_indexer.py (2)
73-84: Rename test — it no longer asserts a raise.The test now verifies keyword-mode fallback behavior; its name
test_raises_without_chromadbis misleading and will confuse future readers.🔧 Proposed fix
- def test_raises_without_chromadb(self, tmp_path): + def test_falls_back_to_keyword_without_chromadb(self, tmp_path): """When ChromaDB is unavailable the indexer falls back to keyword mode (no exception)."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rag_indexer.py` around lines 73 - 84, Rename the misleading test function test_raises_without_chromadb to a name that reflects its behavior (e.g., test_falls_back_to_keyword_mode_without_chromadb), update any references to it, and ensure the test still forces _mod.CHROMA_AVAILABLE = False and constructs refactron.rag.indexer.RAGIndexer(workspace_path=tmp_path) to assert indexer.mode == "keyword"; keep the finally block resetting _mod.CHROMA_AVAILABLE = None and do not change the test logic in RAGIndexer.
14-47: Global-state mutation inmake_indexer.The helper mutates
refactron.rag.indexermodule globals (CHROMA_AVAILABLE,SentenceTransformer,chromadb,Settings) and resets them infinallybefore the indexer is used by the test body. Because the reset happens in the helper'sfinally(executed asmake_indexerreturns), subsequent method calls on the returned indexer run against reset globals — that's fine for the cached instance, but risky if any code path re-reads the globals. Consider usingmonkeypatch/pytest.MonkeyPatchor a context manager to scope the overrides to each test for clearer isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rag_indexer.py` around lines 14 - 47, The helper make_indexer mutates module globals on refactron.rag.indexer (CHROMA_AVAILABLE, SentenceTransformer, chromadb, Settings) and restores them in finally, which can lead to globals being reset while the returned RAGIndexer instance might still re-read them; change make_indexer to accept pytest's monkeypatch (or use a context manager) and replace direct assignments with monkeypatch.setattr/_setitem on the refactron.rag.indexer module for CHROMA_AVAILABLE, SentenceTransformer, chromadb, and Settings so the overrides are scoped to the test and automatically reverted by pytest, keeping RAGIndexer instantiation and subsequent method calls safe; reference RAGIndexer, CHROMA_AVAILABLE, SentenceTransformer, chromadb, and Settings to locate the code to change.refactron/cli/analysis.py (1)
166-172: Deadlocals()guard.Both branches above already assign
target_path(Line 152 from the interactive selector, Line 168 from_validate_path), so"target_path" not in locals()is alwaysFalse. Safe to remove.🧹 Proposed cleanup
- else: - # Path explicitly provided, validate and use it - target_path = _validate_path(target) - - # Setup (only if not already set by interactive selector) - if "target_path" not in locals(): - target_path = _validate_path(target) - cfg = _load_config(config, profile, environment) + else: + # Path explicitly provided, validate and use it + target_path = _validate_path(target) + + cfg = _load_config(config, profile, environment)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/cli/analysis.py` around lines 166 - 172, The locals() guard around target_path is dead code because both branches already set target_path (the interactive selector on earlier branch and the else branch calling _validate_path), so remove the redundant "if 'target_path' not in locals(): target_path = _validate_path(target)" block; ensure only a single assignment path remains and keep the callsite using target_path unchanged (referencing target_path and _validate_path).tests/test_config_management.py (1)
903-919: Prefix unused unpacked values with_(ruff RUF059).Ruff flags several unused unpacked variables across this file (Lines 909, 915, 1229, 1247, 1253, 1268, 1278). Prefixing with
_keeps intent clear and silences the lint.Example for Line 909:
- _, errors, skips = p_seq.process_files(files, process_func) + _, errors, _skips = p_seq.process_files(files, process_func)Apply the same pattern to the other flagged lines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_management.py` around lines 903 - 919, Several unpacked return values are unused; rename them to indicate unused by prefixing with an underscore. For the examples in this diff, change the first unpack in p_seq.process_files to use a throwaway name (e.g., _ , errors, skips or _results, errors, skips) and likewise in p_thr.process_files rename the unused unpacked variables (results, errors, skips) to _results, _errors, _skips or single underscores; apply the same pattern to the other flagged occurrences (lines referenced) where functions like process_func return tuples but some elements are unused so prefix those local/unpacked variable names with "_" to satisfy RUF059.tests/test_pipeline.py (1)
38-53: Strengthen the override test.A few gaps worth closing:
- No
.refactron.yamlis written, so this test also exercises the "no config file" path implicitly — consider adding an explicit test for that scenario so the behavior is unambiguous.- No assertion that
MockRefactron/instance.analyzewas actually called, so the test would still pass if the pipeline short-circuited.- The PR summary highlights
use_incrementalas the explicit override; consider adding a symmetric case foruse_incremental=Falseoverriding a YAML that enables it, to lock in both directions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pipeline.py` around lines 38 - 53, Update the tests to make assertions explicit and add the symmetric override case: in test_pipeline_incremental_override, ensure no .refactron.yaml exists (or explicitly create one with a known setting) and after calling RefactronPipeline(project_root=...) .analyze(target_file, use_incremental=True) assert that MockRefactron was instantiated (MockRefactron.assert_called()) and that the created instance had analyze invoked (MockRefactron.return_value.analyze.assert_called_with(... or at least called)); also add a new test (e.g., test_pipeline_incremental_override_false) that writes a .refactron.yaml enabling incremental analysis, then calls pipeline.analyze(..., use_incremental=False) and asserts the config passed to MockRefactron (args[0]) has enable_incremental_analysis is False to lock in both override directions; reference RefactronPipeline, analyze, MockRefactron and the .refactron.yaml configuration keys when implementing.tests/test_symbol_table_incremental.py (1)
99-106:os.utimerestoringst_mtimeis filesystem-dependent and may silently defeat this test.Line 99 assumes
"x = 1"and"y = 2"produce identical file sizes (they do). But line 102 callsos.utimeand lines 105–106 assert the mtime and size remained unchanged — on filesystems with low mtime granularity (e.g., FAT32) or those that report nanosecond precision inconsistently, this pair of assertions can flake even though the intent is to exercise a hash-based change detection. If the assertions pass, the test is meaningful; if they fail, contributors may disable the test rather than realize it's environment-sensitive. Consider documenting this explicitly (or usingpytest.skipwhen size/mtime cannot be stabilized) so future debuggers don't chase a phantom issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_symbol_table_incremental.py` around lines 99 - 106, The test currently writes to file1, calls os.utime(file1, (original_mtime, original_mtime)) and then asserts file1.stat().st_mtime == original_mtime and st_size == original_size; make this robust by detecting filesystems or platforms where utime/granularity cannot be reliably restored and skipping or adjusting the assertions: after os.utime, re-read st_mtime and st_size (using file1.stat()) and if the mtime or size differs from original_mtime/original_size within an allowed tolerance (or if utime had no effect), call pytest.skip with a clear message about filesystem mtime granularity rather than failing the test, or else proceed to assert only file size and then rely on the content-hash path for change detection; reference file1, os.utime, and the two assertions to locate where to add the skip/tolerance logic.tests/test_rag_retriever.py (1)
14-41: Optional: foldtest_retriever_missing_indexsetup intomake_retriever.
make_retrieverresets globals infinallyimmediately after constructing the retriever, so the live retriever still works (mocks are captured onself). That's fine, buttest_retriever_missing_indexhas to duplicate the whole mock-globals dance because it needs to customizeget_collection.side_effectbefore instantiation. Consider letting the helper accept overrides (e.g.collection_side_effect,skip_instantiation) so the duplicated block at lines 77–95 can reuse it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rag_retriever.py` around lines 14 - 41, Update make_retriever to accept optional parameters (e.g., collection_side_effect=None, skip_instantiation=False) so callers can customize mock_chroma_client.get_collection behavior before creating the ContextRetriever; inside make_retriever set mock_chroma_client.get_collection.side_effect = collection_side_effect if provided, and if skip_instantiation is True return the mocks (mock_collection, mock_model, mock_chroma_client) instead of instantiating ContextRetriever, otherwise instantiate ContextRetriever as before, attach r._mock_collection and r._mock_model, and keep the existing finally cleanup that resets _mod.CHROMA_AVAILABLE and pops SentenceTransformer/chromadb/Settings; reference make_retriever, ContextRetriever, mock_chroma_client, mock_collection, mock_model, collection_side_effect, and skip_instantiation so test_retriever_missing_index can call make_retriever(collection_side_effect=..., skip_instantiation=False) or skip_instantiation=True to avoid duplicating the globals setup.refactron/llm/orchestrator.py (1)
198-217: Deduplicate the proposed_code sanitization between single and batch flows.Lines 200–217 and 322–337 are essentially the same logic (strip ``` fences, unwrap
{…}if `ast.parse` accepts the inner text). Extract into a private helper to avoid drift and keep both code paths in sync when the heuristic evolves.♻️ Suggested shape
def _clean_proposed_code(self, proposed_code: str) -> str: if not proposed_code: return proposed_code if proposed_code.startswith("```"): lines = proposed_code.split("\n") if lines[0].startswith("```"): lines.pop(0) if lines and lines[-1].startswith("```"): lines.pop(-1) proposed_code = "\n".join(lines).strip() if proposed_code.startswith("{") and proposed_code.endswith("}"): potential_code = proposed_code[1:-1].strip() try: ast.parse(potential_code) proposed_code = potential_code except SyntaxError: pass return proposed_code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/orchestrator.py` around lines 198 - 217, The proposed_code sanitization logic is duplicated in two places; extract it into a private helper (e.g., _clean_proposed_code(self, proposed_code: str) -> str) and replace both copies with calls to that helper; the helper should preserve current behavior: return early for falsy input, strip leading/trailing ``` fences by splitting on "\n" and popping fence lines, and if the result starts with "{" and ends with "}" attempt ast.parse on the inner text and adopt it only if parsing succeeds (catch SyntaxError), then return the cleaned string; update both code paths that reference proposed_code and ast.parse to call _clean_proposed_code instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf0b2b1-4578-403d-a6f9-76a87e8f656b
📒 Files selected for processing (29)
bad_code.pyrefactron/analysis/symbol_table.pyrefactron/analysis/taint.pyrefactron/cli/analysis.pyrefactron/cli/main.pyrefactron/cli/rag.pyrefactron/cli/refactor.pyrefactron/cli/ui.pyrefactron/cli/utils.pyrefactron/core/config.pyrefactron/core/inference.pyrefactron/core/parallel.pyrefactron/core/pipeline.pyrefactron/core/refactron.pyrefactron/core/workspace.pyrefactron/llm/backend_client.pyrefactron/llm/client.pyrefactron/llm/orchestrator.pyrefactron/llm/prompts.pyrefactron/rag/indexer.pyrefactron/rag/retriever.pytests/test_cli_patterns_rag.pytests/test_config_management.pytests/test_groq_client.pytests/test_performance_optimization.pytests/test_pipeline.pytests/test_rag_indexer.pytests/test_rag_retriever.pytests/test_symbol_table_incremental.py
| for i in range(ITERATION_LIMIT): | ||
| print("doing something", x) | ||
| return x + y | ||
| do_something_crazy(10, 5) No newline at end of file |
There was a problem hiding this comment.
Top-level invocation causes side effects on import.
do_something_crazy(10, 5) runs at import time, printing 100 lines to stdout whenever anything imports this module (including test collection / analyzer discovery). Guard it behind if __name__ == "__main__": or remove it.
Proposed fix
-do_something_crazy(10, 5)
+if __name__ == "__main__":
+ do_something_crazy(10, 5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bad_code.py` at line 19, The top-level call do_something_crazy(10, 5) causes
side effects on import; move or guard the invocation so it only runs when
executed as a script: either remove the call entirely or wrap it in an if
__name__ == "__main__": block and keep the do_something_crazy(10, 5) invocation
inside that block so imports no longer trigger the behavior.
| # Remove from exports | ||
| names_to_remove = [ | ||
| name | ||
| for name, sym in self.exports.items() | ||
| if self._normalize_path(sym.file_path) == norm_path | ||
| ] | ||
| for name in names_to_remove: | ||
| self.exports.pop(name, None) |
There was a problem hiding this comment.
Preserve same-name exports from other files during incremental removal.
If a.py and b.py both export foo, and b.py currently owns exports["foo"], deleting or reanalyzing b.py removes foo entirely even though a.py still has a valid global symbol. That breaks cross-file resolution after incremental updates.
Proposed fix
for name in names_to_remove:
self.exports.pop(name, None)
+ for scopes in self.symbols.values():
+ candidate = scopes.get("global", {}).get(name)
+ if candidate and candidate.type in (
+ SymbolType.CLASS,
+ SymbolType.FUNCTION,
+ SymbolType.VARIABLE,
+ ):
+ self.exports[name] = candidate
+ break🤖 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 102 - 109, When removing
exports coming from norm_path, don't blindly drop names that were overwritten by
that file; for each name in names_to_remove (computed from self.exports), check
self.globals (or the container of all symbols) for other symbols with the same
name whose _normalize_path(sym.file_path) != norm_path and that are valid global
symbols, and if found restore self.exports[name] to one of those remaining
symbols (choose deterministically, e.g., first match), otherwise pop the export;
use the existing helpers like _normalize_path and the mappings self.exports and
self.globals to locate candidates.
| def _index_sensitive_nodes(self): | ||
| """Pre-index statements that contain potential sinks or sources.""" | ||
| for node in self.data_flow.nodes: | ||
| for stmt in node.statements: | ||
| sensitive = [] | ||
| for child in ast.walk(stmt): | ||
| if isinstance(child, (ast.Call, ast.Attribute)): | ||
| sensitive.append(child) | ||
| if sensitive: | ||
| self._statement_meta[stmt] = sensitive |
There was a problem hiding this comment.
Add missing return type annotation to fix mypy failure.
Pipeline failure: mypy: Function is missing a return type annotation at Line 82. Per coding guidelines, refactron/**/*.py requires disallow_untyped_defs = true.
🔧 Proposed fix
- def _index_sensitive_nodes(self):
+ def _index_sensitive_nodes(self) -> None:
"""Pre-index statements that contain potential sinks or sources."""As per coding guidelines: "Type annotations are required in refactron/ with mypy disallow_untyped_defs = true enabled".
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 82-82: mypy: Function is missing a return type annotation [no-untyped-def]. Hint: Use '-> None' if the function does not return a value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/analysis/taint.py` around lines 82 - 91, The method
_index_sensitive_nodes is missing a return type annotation; annotate it with an
explicit return type (-> None) to satisfy mypy and the refactron coding
guidelines. Update the def _index_sensitive_nodes signature to include the
return annotation while leaving the body intact; no other behavior changes are
necessary. Ensure the change is applied to the _index_sensitive_nodes method
that iterates over self.data_flow.nodes and writes to self._statement_meta.
| targets = [stmt.target] | ||
|
|
||
| value = stmt.value # type: ignore[union-attr, attr-defined] | ||
| is_tainted = self._is_expression_tainted(value, current_taint, memo) # type: ignore[arg-type] |
There was a problem hiding this comment.
Wrap line to fix flake8 E501.
Pipeline failure: flake8: E501 line too long (106 > 100 characters) at Line 172. Per coding guidelines, line length must be ≤ 100 characters.
🔧 Proposed fix
- is_tainted = self._is_expression_tainted(value, current_taint, memo) # type: ignore[arg-type]
+ is_tainted = self._is_expression_tainted(
+ value, current_taint, memo # type: ignore[arg-type]
+ )As per coding guidelines: "Flake8 linting must use max-line-length of 100".
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 172-172: flake8: E501 line too long (106 > 100 characters)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/analysis/taint.py` at line 172, The line calling
self._is_expression_tainted(value, current_taint, memo) exceeds 100 chars; split
the call across multiple lines to keep length ≤100 characters (for example break
after the opening parenthesis or put each argument on its own line) so that the
invocation to _is_expression_tainted with arguments value, current_taint, and
memo is wrapped and flake8 E501 is resolved.
| # 1. Pre-check authentication status | ||
| import os | ||
|
|
||
| creds = load_credentials() | ||
| is_authenticated = False | ||
| if creds and creds.access_token: | ||
|
|
||
| # If using local GROQ, we bypass cloud authentication checks | ||
| if os.environ.get("GROQ_API_KEY"): | ||
| is_authenticated = True | ||
| elif creds and creds.access_token: | ||
| now = datetime.now(timezone.utc) | ||
| if not creds.expires_at or creds.expires_at > now: | ||
| is_authenticated = True |
There was a problem hiding this comment.
GROQ_API_KEY env var bypasses all authentication globally.
Any user who sets GROQ_API_KEY is treated as fully authenticated for every subcommand — including ones that hit the Refactron backend (repo, rag, telemetry, etc.), not just LLM-local flows. This is a security posture regression and lets a user skip token validation/expiry entirely by exporting an unrelated env var.
As per coding guidelines: "All CLI commands except login/logout/auth must check for valid access token stored via core/credentials.py before proceeding".
Consider scoping the bypass tightly — e.g. only for commands that actually invoke the local LLM path (suggest, ai-fix, refactor), or gate it behind an explicit opt-in like REFACTRON_LOCAL_MODE=1 and still require credentials for backend calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/cli/main.py` around lines 36 - 48, The current pre-check in main.py
treats any presence of GROQ_API_KEY as full authentication (GROQ_API_KEY,
load_credentials, is_authenticated) — tighten this: only allow the GROQ_API_KEY
bypass for local-LLM commands (e.g., "suggest", "ai-fix", "refactor") or when an
explicit opt-in env var (e.g., REFACTRON_LOCAL_MODE=1) is set; otherwise require
valid credentials from load_credentials() and validate creds.expires_at as
before. Update the is_authenticated logic to first determine the invoked
subcommand (from argv/CLI context) and only apply the GROQ_API_KEY shortcut when
subcommand ∈ localCommands OR REFACTRON_LOCAL_MODE == "1"; for all other
commands (repo, rag, telemetry, login/logout excluded) enforce token
presence/expiry check using creds.access_token and creds.expires_at.
| if self.config_path and self.config_path.exists(): | ||
| config = RefactronConfig.from_file(self.config_path) | ||
| else: | ||
| if not self.project_root: | ||
| try: | ||
| self.project_root = Refactron().detect_project_root(target_path) | ||
| except Exception: | ||
| self.project_root = target_path if target_path.is_dir() else target_path.parent | ||
|
|
||
| yaml_path = self.project_root / ".refactron.yaml" | ||
| if yaml_path.exists(): | ||
| config = RefactronConfig.from_file(yaml_path) | ||
| else: | ||
| config = RefactronConfig.default() | ||
|
|
||
| # 2. Allow explicit overrides | ||
| # In pipeline mode, incremental analysis is off by default for consistency | ||
| config.enable_incremental_analysis = use_incremental | ||
|
|
||
| refactron = Refactron(config) |
There was a problem hiding this comment.
Full Refactron() is constructed just to call detect_project_root, and then a second Refactron(config) is created for analysis.
Refactron.__init__ is heavy: it configures structured logging, starts Prometheus (if enabled), builds the AST cache, the incremental tracker, the parallel processor, the memory profiler, pattern storage, and — now — an LLMOrchestrator. Doing this once on line 47 purely to locate a project root and then doing it all over again on line 61 with the real config doubles the startup cost and, more importantly, applies side effects (logging reconfiguration, Prometheus server binding, telemetry events) using the default config before the user's YAML is loaded.
Recommendations (any one fixes the problem):
- Extract
detect_project_rootinto a module-level helper (or@staticmethod) that only walks the filesystem — it has no dependencies on instance state today. - Or, load the config first (possibly using
target_pathitself as a starting search point) and constructRefactrononly once, at line 61.
Additionally, if self.config_path is passed explicitly but doesn't exist (line 42), the code silently falls through to project-root-based discovery. A user-supplied path that doesn't exist is almost certainly a mistake and should raise FileNotFoundError rather than be ignored.
🧹 Proposed direction
- # 1. Resolve config the same way as CLI
- if self.config_path and self.config_path.exists():
- config = RefactronConfig.from_file(self.config_path)
- else:
- if not self.project_root:
- try:
- self.project_root = Refactron().detect_project_root(target_path)
- except Exception:
- self.project_root = target_path if target_path.is_dir() else target_path.parent
-
- yaml_path = self.project_root / ".refactron.yaml"
- if yaml_path.exists():
- config = RefactronConfig.from_file(yaml_path)
- else:
- config = RefactronConfig.default()
+ # 1. Resolve config the same way as CLI
+ if self.config_path is not None:
+ if not self.config_path.exists():
+ raise FileNotFoundError(f"Config file not found: {self.config_path}")
+ config = RefactronConfig.from_file(self.config_path)
+ else:
+ if not self.project_root:
+ self.project_root = _detect_project_root(target_path) # lightweight helper
+ yaml_path = self.project_root / ".refactron.yaml"
+ config = (
+ RefactronConfig.from_file(yaml_path)
+ if yaml_path.exists()
+ else RefactronConfig.default()
+ )🧰 Tools
🪛 Ruff (0.15.10)
[warning] 48-48: 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/core/pipeline.py` around lines 42 - 61, The code currently
constructs a full Refactron() just to call detect_project_root and then
constructs Refactron(config) again, causing heavy startup side-effects; refactor
by making detect_project_root a `@staticmethod` or module-level helper (move logic
from Refactron.detect_project_root into a pure function) and call that without
instantiating Refactron, or alternatively change the flow to load the
YAML/config first and instantiate Refactron only once as Refactron(config) for
analysis; also when self.config_path is provided but does not exist, raise
FileNotFoundError instead of silently falling back so a mistaken path (from
RefactronConfig.from_file/self.config_path usage) surfaces to the user.
| # Optional LLM Triage to suppress false positives | ||
| if self.llm_orchestrator and metrics.issues: | ||
| try: | ||
| start_triage = time.time() | ||
| logger.debug( | ||
| f"Running LLM triage on {len(metrics.issues)} issues in {file_path.name}" | ||
| ) | ||
| confidence_map = self.llm_orchestrator.evaluate_issues_batch( | ||
| metrics.issues, source_code | ||
| ) | ||
|
|
||
| filtered_issues = [] | ||
| for i, issue in enumerate(metrics.issues): | ||
| base_id = getattr(issue, "rule_id", None) or "issue" | ||
| line_number = getattr(issue, "line_number", None) | ||
| id_parts = [str(base_id)] | ||
| if line_number is not None: | ||
| id_parts.append(str(line_number)) | ||
| id_parts.append(str(i)) | ||
| issue_id = ":".join(id_parts) | ||
|
|
||
| matched_confidence = 1.0 | ||
| for k, v in confidence_map.items(): | ||
| if k == issue_id or k.startswith(f"{issue_id}_"): | ||
| matched_confidence = v | ||
| break | ||
|
|
||
| if matched_confidence >= 0.5: | ||
| filtered_issues.append(issue) | ||
| else: | ||
| logger.debug( | ||
| f"LLM suppressed false positive: {issue.message} (score: {matched_confidence})" | ||
| ) | ||
|
|
||
| metrics.issues = filtered_issues | ||
| logger.debug( | ||
| f"Completed LLM triage in {time.time() - start_triage:.2f}s. Kept {len(metrics.issues)} issues." | ||
| ) | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"LLM Triage failed for {file_path}, falling back to static results: {e}" | ||
| ) |
There was a problem hiding this comment.
Issue-ID reconstruction is duplicated from LLMOrchestrator.evaluate_issues_batch and will silently desync.
Lines 540–547 rebuild the same base_id:line:index ID scheme that evaluate_issues_batch uses internally (see refactron/llm/orchestrator.py:493-516). If the orchestrator ever changes its ID strategy (e.g. uses a hash, adds a field, or re-orders issues), this matching loop will silently miss every entry and — because matched_confidence defaults to 1.0 — all issues will pass triage, defeating the feature without any failure signal.
Consider having evaluate_issues_batch return results keyed by something stable the caller owns (e.g. issue index, or (issue, confidence) tuples) so the filter here can't drift.
Also: lines 559 and 564 exceed 100 chars — pre-commit flake8 E501 is failing on both.
🧹 Proposed line-length fix for the flake8 failures
- logger.debug(
- f"LLM suppressed false positive: {issue.message} (score: {matched_confidence})"
- )
+ logger.debug(
+ "LLM suppressed false positive: %s (score: %.3f)",
+ issue.message,
+ matched_confidence,
+ )
...
- logger.debug(
- f"Completed LLM triage in {time.time() - start_triage:.2f}s. Kept {len(metrics.issues)} issues."
- )
+ logger.debug(
+ "Completed LLM triage in %.2fs. Kept %d issues.",
+ time.time() - start_triage,
+ len(metrics.issues),
+ )As per coding guidelines: "Use line length of 100 characters, enforced by black, isort, and flake8".
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 559-559: flake8: E501 line too long (107 > 100 characters)
[error] 564-564: flake8: E501 line too long (116 > 100 characters)
🪛 Ruff (0.15.10)
[warning] 566-566: 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/core/refactron.py` around lines 528 - 569, The current LLM triage
reconstructs issue IDs locally (the id_parts/issue_id logic) and matches against
confidence_map returned from LLMOrchestrator.evaluate_issues_batch, which can
silently desync; change evaluate_issues_batch to return confidence results keyed
by a stable caller-owned key (preferably the issue index or return a list of
(index, confidence) tuples) and update this caller (the block using
self.llm_orchestrator.evaluate_issues_batch, confidence_map, and the matching
loop) to use the stable index-based key instead of rebuilding issue_id; also
split the long logger.debug and logger.warning strings into shorter concatenated
parts so the two messages comply with the 100-char line-length limit.
| # Robust parsing to handle older configuration formats | ||
| repo_full_name = data.get("repo_full_name", "unknown/unknown") | ||
| repo_name = data.get("repo_name") | ||
|
|
||
| # Derive repo_name from full_name if missing | ||
| if not repo_name and "/" in repo_full_name: | ||
| repo_name = repo_full_name.split("/")[-1] | ||
|
|
||
| return cls( | ||
| repo_id=data["repo_id"], | ||
| repo_name=data["repo_name"], | ||
| repo_full_name=data["repo_full_name"], | ||
| local_path=data["local_path"], | ||
| connected_at=data["connected_at"], | ||
| repo_name=repo_name or repo_full_name or "unknown", | ||
| repo_full_name=repo_full_name, | ||
| local_path=data.get("local_path", ""), | ||
| connected_at=data.get("connected_at", "unknown"), |
There was a problem hiding this comment.
Avoid turning malformed workspace records into active workspaces.
Defaulting repo_full_name to "unknown/unknown" and local_path to "" lets corrupted/old records pass as connected workspaces. Callers such as refactron/cli/rag.py, refactron/cli/refactor.py, and refactron/cli/analysis.py then display the fake repo name and use Path(current_workspace.local_path) without validation, which can target the current directory.
Prefer skipping/repairing invalid records at load time, or require repo_full_name and a non-empty local_path before constructing WorkspaceMapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/workspace.py` around lines 40 - 52, The current from-dict
logic silently defaults malformed records (repo_full_name="unknown/unknown" and
local_path="") and returns a WorkspaceMapping (cls) for invalid data; instead
validate before constructing: in the constructor/factory that sets
repo_full_name, repo_name and local_path, require a non-empty local_path and a
repo_full_name that contains a "/" (or otherwise meets your repo_full_name
format) and only derive repo_name when valid; if validation fails, do not return
a WorkspaceMapping—either return None or raise a clear ValueError so callers
(e.g., refactron/cli/rag.py, refactron/cli/refactor.py,
refactron/cli/analysis.py) can skip/repair the record—update the method that
currently uses repo_full_name, repo_name, local_path and cls(...) to implement
these checks and propagate the None/error to callers.
| # 1. Retrieve Context | ||
| context_snippets = [] | ||
| if self.retriever: | ||
| try: | ||
| # Use the first few issues for context retrieval | ||
| query = " ".join([i.message for i in issues[:3]]) | ||
| results = self.retriever.retrieve_similar(query, top_k=3) | ||
| context_snippets = [r.content for r in results] | ||
| except Exception as e: | ||
| logger.warning(f"Context retrieval failed: {e}") | ||
|
|
||
| rag_context = "\n\n".join(context_snippets) if context_snippets else "No context available." | ||
|
|
||
| # 2. Format issues for prompt | ||
| issues_details = "" | ||
| for idx, issue in enumerate(issues, 1): | ||
| issues_details += ( | ||
| f"{idx}. {issue.category.value} (Line {issue.line_number}): {issue.message}\n" | ||
| ) | ||
|
|
||
| # 3. Construct Prompt | ||
| prompt = BATCH_SUGGESTION_PROMPT.format( | ||
| issues_details=issues_details, | ||
| original_code=original_code, | ||
| rag_context=rag_context, | ||
| ) | ||
|
|
||
| # 4. Call LLM | ||
| response_text = "N/A" | ||
| try: | ||
| response_text = self.client.generate( | ||
| prompt=prompt, system=BATCH_SUGGESTION_SYSTEM_PROMPT, temperature=0.2 | ||
| ) | ||
|
|
||
| # Reuse cleaning and parsing logic | ||
| clean_text = self._clean_json_response(response_text) | ||
| data = json.loads(clean_text, strict=False) | ||
|
|
||
| proposed_code = data.get("proposed_code", "") | ||
|
|
||
| # Clean up hallucinations | ||
| if proposed_code.startswith("```"): | ||
| lines = proposed_code.split("\n") | ||
| if lines[0].startswith("```"): | ||
| lines.pop(0) | ||
| if lines and lines[-1].startswith("```"): | ||
| lines.pop(-1) | ||
| proposed_code = "\n".join(lines).strip() | ||
|
|
||
| if proposed_code.startswith("{") and proposed_code.endswith("}"): | ||
| potential_code = proposed_code[1:-1].strip() | ||
| try: | ||
| ast.parse(potential_code) | ||
| proposed_code = potential_code | ||
| except SyntaxError: | ||
| pass | ||
|
|
||
| suggestion = RefactoringSuggestion( | ||
| issue=issues[0], # Use first issue as primary reference | ||
| original_code=original_code, | ||
| context_files=[r.file_path for r in results] if self.retriever else [], |
There was a problem hiding this comment.
results can be unbound when retriever retrieval fails, forcing the whole batch suggestion to fail.
results is only assigned inside the inner try at line 288. If self.retriever is set but retrieve_similar(...) raises (e.g., transient RAG/Chroma error), execution continues to build the prompt and call the LLM, but then line 342 references results which is never bound. The resulting NameError is swallowed by the outer except at line 351, so a successful LLM response is discarded and the caller receives a FAILED suggestion — the exact opposite of graceful RAG degradation.
🛠️ Proposed fix
# 1. Retrieve Context
context_snippets = []
+ results = []
if self.retriever:
try:
# Use the first few issues for context retrieval
query = " ".join([i.message for i in issues[:3]])
results = self.retriever.retrieve_similar(query, top_k=3)
context_snippets = [r.content for r in results]
except Exception as e:
logger.warning(f"Context retrieval failed: {e}")
@@
suggestion = RefactoringSuggestion(
issue=issues[0], # Use first issue as primary reference
original_code=original_code,
- context_files=[r.file_path for r in results] if self.retriever else [],
+ context_files=[r.file_path for r in results],
proposed_code=proposed_code,🧰 Tools
🪛 Ruff (0.15.10)
[warning] 290-290: 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/llm/orchestrator.py` around lines 282 - 342, The variable results
may be referenced unbound when self.retriever is truthy but
retrieve_similar(...) raises; initialize results to an empty list before the
retriever block (or set results = [] in the except handler) so references later
(e.g., building RefactoringSuggestion.context_files = [r.file_path for r in
results]) are safe; modify the code around the retriever try/except (symbols:
self.retriever, retrieve_similar, results, context_snippets,
RefactoringSuggestion) to ensure results is always defined as a list even on
retrieval failure.
|
|
||
| import tempfile | ||
| from pathlib import Path | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
Remove unused MagicMock import (pipeline failure).
Pre-commit flake8 is failing with F401. Only patch is referenced.
🔧 Proposed fix
-from unittest.mock import patch, MagicMock
+from unittest.mock import patch📝 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.
| from unittest.mock import patch, MagicMock | |
| from unittest.mock import patch |
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 5-5: flake8: F401 'unittest.mock.MagicMock' imported but unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pipeline.py` at line 5, Remove the unused MagicMock import to fix
the flake8 F401 failure: in the import statement that currently reads "from
unittest.mock import patch, MagicMock" keep only "patch" (remove "MagicMock") so
only referenced symbols are imported in tests/test_pipeline.py.
|
This pull request has been automatically marked as stale because it has not had recent activity for 14 days. If you believe this PR is still relevant, please:
This helps us keep the repository focused on active contributions. Thank you! 🙏 |
|
This pull request has been automatically closed because it was marked as stale and had no recent activity for 3 days. If you believe this PR should be reopened, please:
Thank you for your understanding! 🙏 |
2 participants
solve #174
This update resolves an inconsistency where RefactronPipeline.analyze was ignoring user and project configurations by generating a fresh, blank configuration instead of loading existing settings. To fix this, the RefactronPipeline class has been implemented to automatically detect the project root and load settings from the .refactron.yaml file, ensuring that automated pipeline and session-based runs use the exact same analyzer configurations as the standard CLI refactron analyze command. It also introduces the use_incremental parameter as an explicit pipeline-only override to guarantee reproducible CI/CD runs. Finally, full testing coverage has been added to verify that configurations are parsed and mapped accurately during a pipeline flow.
Summary by CodeRabbit
New Features
ai_fixcommand for LLM-powered code fixes with backup support.Improvements