feat: implementation of Refactron pipeline with timing persistence, o…#184
Closed
shrutu0929 wants to merge 1 commit into
Closed
feat: implementation of Refactron pipeline with timing persistence, o…#184shrutu0929 wants to merge 1 commit into
shrutu0929 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughA comprehensive pipeline orchestration system is introduced, enabling programmatic analysis, issue queuing, and auto-fix application with session tracking and backup support. Integration includes a new CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as run command
participant Pipeline as RefactronPipeline
participant Config as RefactronConfig
participant Refactron as Refactron.analyze
participant Engine as AutoFixEngine
participant Backup as BackupRollbackSystem
participant FS as File System
CLI->>Pipeline: __init__(project_root, config_path)
Pipeline->>Config: Load from .refactron.yaml or default
CLI->>Pipeline: analyze(target_path, use_incremental)
Pipeline->>Refactron: analyze(target_path)
Refactron-->>Pipeline: AnalysisResult
Pipeline->>Pipeline: Record analyze_ms
CLI->>Pipeline: queue_issues(CodeIssue list)
loop Per issue
Pipeline->>Engine: can_fix(issue) [fast path]
alt Fast path fails
Pipeline->>Engine: Preview candidate fixers
end
Pipeline->>Pipeline: Cache fixer_name by rule_id
end
Pipeline->>Pipeline: Record queue_ms
CLI->>Pipeline: apply(queued_issues, preview, fail_fast)
alt Not preview mode
Pipeline->>Backup: Create backup session
Backup-->>Pipeline: backup_session_id
end
loop Per file
Pipeline->>FS: Read current file text
loop Per issue in file
Pipeline->>Engine: fix(issue, current_code, preview)
Engine-->>Pipeline: Success or failure
end
alt Success and not preview
Pipeline->>FS: Write fixed code
Pipeline->>Pipeline: files_succeeded++
else Failure or fail_fast triggered
Pipeline->>Pipeline: Accumulate blocked_fixes
Pipeline->>Pipeline: files_failed++
end
end
Pipeline->>Pipeline: Record apply_ms
CLI->>Pipeline: verify(target_path)
Pipeline->>Refactron: analyze(target_path) [rerun]
Refactron-->>Pipeline: AnalysisResult
Pipeline->>Pipeline: Record verify_ms
Pipeline-->>CLI: Session summary with all metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…ptimized fixer resolution, and multi-file apply semantics
shrutu0929
force-pushed
the
feat/multi-file-apply-semantics
branch
from
472ded2 to
3c1b45a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
tests/test_pipeline_session.py (1)
62-85: Use realFixResultinstances in these mocks.The production result type exposes
fixed, notfixed_code; theseMagicMockresults mask that API mismatch. Using the real dataclass would make the tests catch the runtime bug.Proposed direction
+from refactron.autofix.models import FixResult @@ - pipeline.autofix_engine.fix = MagicMock(return_value=MagicMock(success=True, fixed_code="")) + pipeline.autofix_engine.fix = MagicMock(return_value=FixResult(success=True, fixed="")) @@ - success_fix = MagicMock(success=True, fixed_code="fixed") - fail_fix = MagicMock(success=False, reason="Blocked") + success_fix = FixResult(success=True, fixed="fixed") + fail_fix = FixResult(success=False, reason="Blocked") @@ - fail_fix = MagicMock(success=False, reason="Blocked") - success_fix = MagicMock(success=True, fixed_code="fixed") + fail_fix = FixResult(success=False, reason="Blocked") + success_fix = FixResult(success=True, fixed="fixed")Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pipeline_session.py` around lines 62 - 85, Replace the MagicMock-based fake return values for pipeline.autofix_engine.fix with real FixResult dataclass instances so tests exercise the real API (it exposes "fixed" rather than "fixed_code"); specifically, wherever you create MagicMock(success=True, fixed_code="...") or MagicMock(success=False, reason="...") (e.g., the first apply block and the success_fix/fail_fix variables used in test_pipeline_apply_multi_file_best_effort), instantiate FixResult with the correct fields (fixed=True/False, fixed_code or reason as appropriate) and use those instances as the return_value/side_effect for RefactronPipeline.autofix_engine.fix so the tests catch API mismatches.
🤖 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/cli/run.py`:
- Line 58: The console.print call uses an unnecessary f-string literal with no
interpolation, causing an F541 pre-commit failure; replace the f-string in the
console.print invocation (the line that calls console.print("[green]Pipeline
session completed successfully.[/green]")) with a plain string literal (remove
the leading f) so the call uses a regular quoted string passed to console.print.
- Around line 28-56: The CLI currently accepts --fail-fast but never uses it;
update the run command to either remove the option or wire it into the apply
step — implement an explicit apply phase after queueing that calls
RefactronPipeline.apply (or the appropriate apply method on the pipeline
instance) and pass fail_fast=fail_fast into that call so the pipeline honours
the flag; locate the run function and the pipeline creation/queueing code
(RefactronPipeline(), .analyze(), .queue_issues()) and add the apply invocation
and any necessary preview/backup invocation sequence so the safety-first flow
(preview → backup → apply → optional rollback) is preserved.
In `@refactron/core/pipeline.py`:
- Line 146: The current code sets self.session.files_attempted = len(file_map)
before fail-fast/grouped processing which counts files that are never processed;
instead remove that bulk assignment and increment self.session.files_attempted
by 1 inside the per-file processing loop at the point where a file is actually
about to be attempted (e.g., inside the loop that iterates grouped files in
pipeline.py, where the code calls the per-file processing logic), and apply the
same change to the other occurrences noted (the blocks around the ranges
corresponding to lines 160-163 and 197-202) so each attempted file increments
the counter only when it is truly attempted.
- Around line 126-129: The long docstring line in the "Orchestration Policies"
block (the sentence starting "Fail-fast (fail_fast=True): Stops the entire
application process on the first file failure.") exceeds 100 characters; wrap
that sentence into one or more lines under 100 characters inside the same
docstring, preserving the existing indentation and triple-quote style so the
docstring formatting and semantics remain unchanged.
- Line 7: Add proper type annotations to finish core typing in
refactron/core/pipeline.py: annotate the module-level _fixer_cache (e.g.,
Dict[str, Any] or a more specific Dict[str, FixerType]) and update any untyped
function signatures to use explicit return and parameter types (replace bare
dict usages with typed Dict[str, Any] and ensure functions that read optional
string values return Optional[str] by handling missing keys or casting). Update
the import line to only import the typing names you actually use (e.g.,
Optional, Dict, List, Any, Union) and adjust the functions/methods that access
the fixer cache and the dict-reading logic (refer to _fixer_cache, any functions
that return Optional[str], and the functions using the fixer lookup) so mypy’s
disallow_untyped_defs passes. Ensure all updated symbols have concrete types
consistent across the module.
- Around line 151-159: The code currently ignores failed backup paths and only
logs a warning on backup errors; update the block that runs when not preview and
self._config.backup_enabled and file_map to capture the results from
backup_system.prepare_for_refactoring (including any failed paths), assign
session_id to self.session.backup_session_id only on success, then call
BackupManager.validate_backup_integrity(session_id) to get (valid_paths,
corrupt_paths) and if there are any failed/ corrupt paths or an exception from
prepare_for_refactoring, abort the refactoring (do not proceed to apply/write
files) by raising an exception or returning an error from the pipeline;
reference the prepare_for_refactoring call, the self.session.backup_session_id
assignment, and BackupManager.validate_backup_integrity(session_id) in your
changes.
- Around line 168-174: The loop currently calls self.autofix_engine.fix() with
item["issue"] and reads a non-existent fixed_code field; update it to honor any
fallback-mapped fixer by using the selected fixer/fallback from item (e.g.,
item.get("fixer_name") or the mapped issue object) when invoking
AutoFixEngine.fix/apply so the correct fixer is used, and replace reads of
FixResult.fixed_code with the actual FixResult.fixed field and assign
current_code = fix_result.fixed when fix_result.success is true.
In `@tests/test_pipeline_session.py`:
- Line 3: Remove the unused top-level import "time" from
tests/test_pipeline_session.py to satisfy the F401 pre-commit check; simply
delete the import line that references the symbol time (or if later needed,
replace it with a direct usage or a pytest fixture), then run the
pre-commit/flake8 checks to confirm the import warning is resolved.
In `@tests/test_pipeline.py`:
- Around line 110-111: The inline comment above the assertion for
calls["preview"] is longer than 100 characters and fails pre-commit; shorten or
wrap that comment so no line exceeds 100 chars (e.g., split into two lines or
rephrase) near the assertion in tests/test_pipeline.py where the comment reads
"Should only have run preview 1 time, because the subsequent issues have the
same rule_id and hit the cache."; ensure the wrapped/commented text preserves
the meaning and adheres to the 100-char line-length rule enforced by
black/flake8.
---
Nitpick comments:
In `@tests/test_pipeline_session.py`:
- Around line 62-85: Replace the MagicMock-based fake return values for
pipeline.autofix_engine.fix with real FixResult dataclass instances so tests
exercise the real API (it exposes "fixed" rather than "fixed_code");
specifically, wherever you create MagicMock(success=True, fixed_code="...") or
MagicMock(success=False, reason="...") (e.g., the first apply block and the
success_fix/fail_fix variables used in
test_pipeline_apply_multi_file_best_effort), instantiate FixResult with the
correct fields (fixed=True/False, fixed_code or reason as appropriate) and use
those instances as the return_value/side_effect for
RefactronPipeline.autofix_engine.fix so the tests catch API mismatches.
🪄 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: b4791f92-3b4c-48a2-b8ad-6d52d8324ed4
📒 Files selected for processing (7)
.refactron.yamlrefactron/cli/main.pyrefactron/cli/run.pyrefactron/core/pipeline.pyrefactron/core/pipeline_session.pytests/test_pipeline.pytests/test_pipeline_session.py
Comment on lines
+28
to
+56
| @click.option( | ||
| "--fail-fast", | ||
| is_flag=True, | ||
| help="Stop processing if an error occurs during fix application", | ||
| ) | ||
| def run(target: Optional[str], incremental: bool, verbose: bool, fail_fast: bool) -> None: | ||
| """ | ||
| Run Refactron as a connected pipeline session. | ||
| """ | ||
| console.print() | ||
| _auth_banner("Pipeline Run") | ||
| console.print() | ||
|
|
||
| target_path = _validate_path(target) if target else Path.cwd() | ||
| debug_mode = os.getenv("REFACTRON_DEBUG") == "1" | ||
|
|
||
| with console.status("[primary]Executing pipeline run...[/primary]"): | ||
| try: | ||
| pipeline = RefactronPipeline() | ||
| # 1. Analyze | ||
| result = pipeline.analyze(target_path, use_incremental=incremental) | ||
|
|
||
| # 2. Queue | ||
| queued = pipeline.queue_issues(result.all_issues) | ||
|
|
||
| # 3. Apply (simulated for now in 'run' command unless --apply added, | ||
| # but we'll show the summary infrastructure) | ||
| # For this task, we assume 'run' might be extended to apply fixes. | ||
| # If we don't apply, session counts remain 0. |
Contributor
There was a problem hiding this comment.
Wire --fail-fast to fix application or hide it for now.
The command accepts --fail-fast, but it only runs analyze + queue, so users cannot trigger the fail-fast behavior described by the PR. Either add an explicit apply path that passes fail_fast=fail_fast, or remove the option until run supports applying fixes.
One safe direction
`@click.option`(
"--fail-fast",
is_flag=True,
help="Stop processing if an error occurs during fix application",
)
-def run(target: Optional[str], incremental: bool, verbose: bool, fail_fast: bool) -> None:
+@click.option(
+ "--apply",
+ "apply_fixes",
+ is_flag=True,
+ help="Apply queued fixes after analysis and backup preparation",
+)
+def run(
+ target: Optional[str],
+ incremental: bool,
+ verbose: bool,
+ fail_fast: bool,
+ apply_fixes: bool,
+) -> None:
@@
# 2. Queue
queued = pipeline.queue_issues(result.all_issues)
- # 3. Apply (simulated for now in 'run' command unless --apply added,
- # but we'll show the summary infrastructure)
- # For this task, we assume 'run' might be extended to apply fixes.
- # If we don't apply, session counts remain 0.
+ # 3. Apply only when explicitly requested.
+ if apply_fixes:
+ pipeline.apply(queued, fail_fast=fail_fast)Based on learnings, all refactoring must go through safety-first pipeline: preview → backup → apply → optional rollback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/cli/run.py` around lines 28 - 56, The CLI currently accepts
--fail-fast but never uses it; update the run command to either remove the
option or wire it into the apply step — implement an explicit apply phase after
queueing that calls RefactronPipeline.apply (or the appropriate apply method on
the pipeline instance) and pass fail_fast=fail_fast into that call so the
pipeline honours the flag; locate the run function and the pipeline
creation/queueing code (RefactronPipeline(), .analyze(), .queue_issues()) and
add the apply invocation and any necessary preview/backup invocation sequence so
the safety-first flow (preview → backup → apply → optional rollback) is
preserved.
| # For this task, we assume 'run' might be extended to apply fixes. | ||
| # If we don't apply, session counts remain 0. | ||
|
|
||
| console.print(f"[green]Pipeline session completed successfully.[/green]") |
Contributor
There was a problem hiding this comment.
Remove the unnecessary f-string prefix.
This line has no interpolation and currently fails pre-commit with F541.
Proposed fix
- console.print(f"[green]Pipeline session completed successfully.[/green]")
+ console.print("[green]Pipeline session completed successfully.[/green]")📝 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.
Suggested change
| console.print(f"[green]Pipeline session completed successfully.[/green]") | |
| console.print("[green]Pipeline session completed successfully.[/green]") |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 58-58: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/cli/run.py` at line 58, The console.print call uses an unnecessary
f-string literal with no interpolation, causing an F541 pre-commit failure;
replace the f-string in the console.print invocation (the line that calls
console.print("[green]Pipeline session completed successfully.[/green]")) with a
plain string literal (remove the leading f) so the call uses a regular quoted
string passed to console.print.
| from collections import defaultdict | ||
| from pathlib import Path | ||
| import time | ||
| from typing import Optional, Union, List, Dict, Any |
Contributor
There was a problem hiding this comment.
Finish the core type annotations and type the fixer cache.
This resolves the unused typing import, _fixer_cache mypy failure, and the Optional[str] return issue from reading an untyped dict.
Proposed fix
-from typing import Optional, Union, List, Dict, Any
+from typing import Dict, List, Optional, Union
@@
- def __init__(
+ def __init__(
self,
project_root: Optional[Union[str, Path]] = None,
config_path: Optional[Union[str, Path]] = None,
- ):
+ ) -> None:
@@
- self._fixer_cache = {}
+ self._fixer_cache: Dict[str, Optional[str]] = {}As per coding guidelines, refactron/**/*.py: Type annotations are required in refactron/ with mypy disallow_untyped_defs = true enabled.
Also applies to: 24-28, 44-44
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 7-7: flake8 F401 'typing.Any' imported but unused.
[error] 7-7: flake8 F401 'typing.Dict' imported but unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/pipeline.py` at line 7, Add proper type annotations to finish
core typing in refactron/core/pipeline.py: annotate the module-level
_fixer_cache (e.g., Dict[str, Any] or a more specific Dict[str, FixerType]) and
update any untyped function signatures to use explicit return and parameter
types (replace bare dict usages with typed Dict[str, Any] and ensure functions
that read optional string values return Optional[str] by handling missing keys
or casting). Update the import line to only import the typing names you actually
use (e.g., Optional, Dict, List, Any, Union) and adjust the functions/methods
that access the fixer cache and the dict-reading logic (refer to _fixer_cache,
any functions that return Optional[str], and the functions using the fixer
lookup) so mypy’s disallow_untyped_defs passes. Ensure all updated symbols have
concrete types consistent across the module.
Comment on lines
+126
to
+129
| Orchestration Policies: | ||
| - Best-effort (default): Continues applying fixes to other files even if one file fails. | ||
| - Fail-fast (fail_fast=True): Stops the entire application process on the first file failure. | ||
|
|
Contributor
There was a problem hiding this comment.
Wrap the long docstring line.
Line 128 exceeds the configured 100-character limit and fails pre-commit.
Proposed fix
- - Fail-fast (fail_fast=True): Stops the entire application process on the first file failure.
+ - Fail-fast (fail_fast=True): Stops the entire application process on the first
+ file failure.As per coding guidelines, **/*.py: Use line length of 100 characters, enforced by black, isort, and flake8.
📝 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.
Suggested change
| Orchestration Policies: | |
| - Best-effort (default): Continues applying fixes to other files even if one file fails. | |
| - Fail-fast (fail_fast=True): Stops the entire application process on the first file failure. | |
| Orchestration Policies: | |
| - Best-effort (default): Continues applying fixes to other files even if one file fails. | |
| - Fail-fast (fail_fast=True): Stops the entire application process on the first | |
| file failure. | |
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 128-128: flake8 E501 line too long (101 > 100 characters).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/pipeline.py` around lines 126 - 129, The long docstring line
in the "Orchestration Policies" block (the sentence starting "Fail-fast
(fail_fast=True): Stops the entire application process on the first file
failure.") exceeds 100 characters; wrap that sentence into one or more lines
under 100 characters inside the same docstring, preserving the existing
indentation and triple-quote style so the docstring formatting and semantics
remain unchanged.
| for item in queued_issues: | ||
| file_map[item["issue"].file_path].append(item) | ||
|
|
||
| self.session.files_attempted = len(file_map) |
Contributor
There was a problem hiding this comment.
Count only files that are actually attempted.
files_attempted is set to every grouped file before fail-fast processing, so files skipped after the first failure are reported as attempted. Increment it inside the per-file loop instead.
Proposed fix
- self.session.files_attempted = len(file_map)
-
# 2. Setup Backup
@@
# 3. Apply fixes file by file
for file_path, items in file_map.items():
+ self.session.files_attempted += 1
success = TrueAlso applies to: 160-163, 197-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/pipeline.py` at line 146, The current code sets
self.session.files_attempted = len(file_map) before fail-fast/grouped processing
which counts files that are never processed; instead remove that bulk assignment
and increment self.session.files_attempted by 1 inside the per-file processing
loop at the point where a file is actually about to be attempted (e.g., inside
the loop that iterates grouped files in pipeline.py, where the code calls the
per-file processing logic), and apply the same change to the other occurrences
noted (the blocks around the ranges corresponding to lines 160-163 and 197-202)
so each attempted file increments the counter only when it is truly attempted.
Comment on lines
+151
to
+159
| if not preview and self._config.backup_enabled and file_map: | ||
| try: | ||
| session_id, _ = backup_system.prepare_for_refactoring( | ||
| list(file_map.keys()), description=f"Pipeline session {self.session.id}" | ||
| ) | ||
| self.session.backup_session_id = session_id | ||
| except Exception as e: | ||
| logger.warning(f"Failed to create backup session: {e}") | ||
|
|
Contributor
There was a problem hiding this comment.
Do not apply fixes after backup creation or integrity failures.
prepare_for_refactoring() returns failed backup paths, but they are ignored, and a backup exception only logs a warning before the code continues to write files. That can leave users without rollback coverage.
Proposed direction
- session_id, _ = backup_system.prepare_for_refactoring(
+ session_id, failed_files = backup_system.prepare_for_refactoring(
list(file_map.keys()), description=f"Pipeline session {self.session.id}"
)
self.session.backup_session_id = session_id
+ valid_paths, corrupt_paths = (
+ backup_system.backup_manager.validate_backup_integrity(session_id)
+ )
+ unsafe_paths = set(failed_files) | set(corrupt_paths)
+ if unsafe_paths:
+ raise RuntimeError(
+ f"Backup validation failed for: "
+ f"{', '.join(str(path) for path in unsafe_paths)}"
+ )
except Exception as e:
- logger.warning(f"Failed to create backup session: {e}")
+ logger.error("Failed to prepare backup session: %s", e)
+ raiseBased on learnings, all refactoring must go through safety-first pipeline: preview → backup → apply → optional rollback. As per coding guidelines, refactron/core/*.py: Use BackupManager.validate_backup_integrity(session_id) for backup integrity validation, returning (valid_paths, corrupt_paths).
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 157-157: 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 151 - 159, The code currently
ignores failed backup paths and only logs a warning on backup errors; update the
block that runs when not preview and self._config.backup_enabled and file_map to
capture the results from backup_system.prepare_for_refactoring (including any
failed paths), assign session_id to self.session.backup_session_id only on
success, then call BackupManager.validate_backup_integrity(session_id) to get
(valid_paths, corrupt_paths) and if there are any failed/ corrupt paths or an
exception from prepare_for_refactoring, abort the refactoring (do not proceed to
apply/write files) by raising an exception or returning an error from the
pipeline; reference the prepare_for_refactoring call, the
self.session.backup_session_id assignment, and
BackupManager.validate_backup_integrity(session_id) in your changes.
Comment on lines
+168
to
+174
| for item in items: | ||
| issue = item["issue"] | ||
| fix_result = self.autofix_engine.fix(issue, current_code, preview=preview) | ||
| results.append(fix_result) | ||
|
|
||
| if fix_result.success: | ||
| current_code = fix_result.fixed_code |
Contributor
There was a problem hiding this comment.
Use the selected fixer and the actual FixResult.fixed field.
Fallback resolution stores fixer_name, but apply() ignores it and calls AutoFixEngine.fix() with the original issue, so fallback-mapped issues still fail. This block also reads fixed_code, which does not exist on FixResult.
Proposed direction
# Apply each fixer sequentially for this file
for item in items:
issue = item["issue"]
- fix_result = self.autofix_engine.fix(issue, current_code, preview=preview)
+ fixer_name = item["fixer_name"]
+ original_rule_id = issue.rule_id
+ if fixer_name != issue.rule_id:
+ issue.rule_id = fixer_name
+ try:
+ fix_result = self.autofix_engine.fix(
+ issue, current_code, preview=preview
+ )
+ finally:
+ issue.rule_id = original_rule_id
results.append(fix_result)
if fix_result.success:
- current_code = fix_result.fixed_code
+ if fix_result.fixed is None:
+ success = False
+ self.session.blocked_fixes.append(
+ {
+ "file": str(file_path),
+ "issue": issue.message,
+ "reason": "Fixer succeeded without fixed code",
+ }
+ )
+ continue
+ current_code = fix_result.fixed🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 174-174: mypy [attr-defined]: "FixResult" has no attribute "fixed_code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/core/pipeline.py` around lines 168 - 174, The loop currently calls
self.autofix_engine.fix() with item["issue"] and reads a non-existent fixed_code
field; update it to honor any fallback-mapped fixer by using the selected
fixer/fallback from item (e.g., item.get("fixer_name") or the mapped issue
object) when invoking AutoFixEngine.fix/apply so the correct fixer is used, and
replace reads of FixResult.fixed_code with the actual FixResult.fixed field and
assign current_code = fix_result.fixed when fix_result.success is true.
| @@ -0,0 +1,163 @@ | |||
| """Tests for PipelineSession and pipeline timing persistence.""" | |||
|
|
|||
| import time | |||
Contributor
There was a problem hiding this comment.
Remove the unused import.
time is not used and fails pre-commit with F401.
Proposed fix
-import time
from pathlib import Path🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 3-3: flake8 F401 'time' imported but unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pipeline_session.py` at line 3, Remove the unused top-level import
"time" from tests/test_pipeline_session.py to satisfy the F401 pre-commit check;
simply delete the import line that references the symbol time (or if later
needed, replace it with a direct usage or a pytest fixture), then run the
pre-commit/flake8 checks to confirm the import warning is resolved.
Comment on lines
+110
to
+111
| # Should only have run preview 1 time, because the subsequent issues have the same rule_id and hit the cache. | ||
| assert calls["preview"] == 1 |
Contributor
There was a problem hiding this comment.
Wrap the long comment to unblock pre-commit.
Line 111 exceeds the configured 100-character limit.
Proposed fix
- # Should only have run preview 1 time, because the subsequent issues have the same rule_id and hit the cache.
+ # Should only have run preview 1 time, because subsequent issues have the same
+ # rule_id and hit the cache.
assert calls["preview"] == 1As per coding guidelines, **/*.py: Use line length of 100 characters, enforced by black, isort, and flake8.
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 111-111: flake8 E501 line too long (117 > 100 characters).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pipeline.py` around lines 110 - 111, The inline comment above the
assertion for calls["preview"] is longer than 100 characters and fails
pre-commit; shorten or wrap that comment so no line exceeds 100 chars (e.g.,
split into two lines or rephrase) near the assertion in tests/test_pipeline.py
where the comment reads "Should only have run preview 1 time, because the
subsequent issues have the same rule_id and hit the cache."; ensure the
wrapped/commented text preserves the meaning and adheres to the 100-char
line-length rule enforced by black/flake8.
|
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! 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
solve #178 This task formalizes multi-file orchestration semantics within the RefactronPipeline, transitioning from simple independent updates to a sophisticated batch-processing model. By implementing mandatory file grouping, the pipeline now ensures that multiple fixes for a single file are applied sequentially to the source code before an atomic write-back, effectively eliminating I/O redundancy and stale code conflicts. The introduction of a "fail-fast" policy, accessible via the new --fail-fast CLI flag, provides users with a safety mechanism to halt the entire process immediately upon the first failure, while the default "best-effort" mode continues to capture metrics for all files. All application attempts are now backed by a single, integrated backup session, and the system provides comprehensive visibility through detailed success/failure metadata and a blocked_fixes registry, ensuring that no refactoring attempt is ever silently lost or skipped without documentation.
Summary by CodeRabbit
New Features
runCLI command for analyzing code and applying automated fixes, with options for incremental analysis, verbose output, and fail-fast behavior..refactron.yaml) enabling metrics and logging control.Tests