feat:fixed pytests and flake8 formatting - #128
Merged
Merged
Conversation
(cherry picked from commit 07d7cd9)
Drop files that were unintentionally carried into the single-commit PR branch to keep the branch focused. Made-with: Cursor
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDefers eager top-level imports via a module-level getattr (lazy-loading); adds StructuredLogger context-manager and close(); reworks tree-sitter Language/Parser initialization and validation with multi-API/platform fallbacks; makes CLI/tests use explicit UTF-8 I/O; adjusts tests for cross-platform cleanup, mocking, and conditional tree-sitter skipping. Changes
Sequence Diagram(s)sequenceDiagram
participant CP as CodeParser
participant TSP as tree_sitter package
participant FS as Filesystem
participant TS as tree-sitter Language/Parser
CP->>TSP: inspect API / version
alt Language(...) constructors available
TSP-->>CP: Language object
else need shared lib
TSP-->>CP: package install location
CP->>FS: locate platform-specific shared lib
FS-->>CP: lib path
CP->>TS: Language(lib_path, "python")
TS-->>CP: Language object
end
CP->>TS: attempt to build Parser via probed APIs
TS-->>CP: Parser instance or error
CP->>TS: parse trivial bytes to validate
alt parses OK
TS-->>CP: parse tree (valid)
else failure / None
TS-->>CP: None or raise
CP-->>CP: raise RuntimeError/ValueError with file info
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
omsherikar
self-requested a review
March 23, 2026 21:02
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
refactron/rag/parser.py (1)
89-107:⚠️ Potential issue | 🔴 CriticalCritical: Orphaned code block causes syntax error.
This code block appears to be leftover from the previous implementation and should have been removed during the refactoring. It creates an unterminated
tryblock that causes invalid syntax (flagged by Ruff: "Expectedexceptorfinallyaftertryblock").The
@staticmethoddecorator at line 108 starts a new method definition while still inside this unclosedtryblock, making the entire file unparseable Python.This entire section (lines 89-107) should be removed as part of the refactoring.
🐛 Remove orphaned code
- try: - PY_LANGUAGE = Language(lang, "python") - except TypeError: - # Try using the path to the compiled library (for very old or CI bindings) - try: - import os - import platform - - pkg_dir = os.path.dirname(tspython.__file__) - - # Find the correct shared library extension - system = platform.system() - if system == "Windows": - ext = ".dll" - elif system == "Darwin": - ext = ".dylib" - else: - ext = ".so" -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/parser.py` around lines 89 - 107, The file contains an orphaned try/except fragment around the Language(lang, "python") initialization (references to Language(lang, "python") and tspython) that leaves a dangling try block and causes a syntax error before the following `@staticmethod`; remove the entire leftover block starting from the try that attempts Language(lang, "python") fallback through the platform/extension detection so the staticmethod that follows is no longer nested inside an unclosed try.tests/test_backup.py (1)
575-594:⚠️ Potential issue | 🟡 MinorTest does not exercise the intended code path.
The test name and docstring indicate it should test the scenario where a specific session ID is provided but doesn't exist. However, the mock setup (
list_sessions.return_value = []) causes an early return at Line 435 inrefactron/cli/refactor.pybeforeget_session()is ever called.This test currently duplicates the behavior of
test_rollback_no_sessionsrather than testing the "session not found" error path (Lines 437-444 inrefactor.py), which should exit with code 1 and print "Session not found: nonexistent".🐛 Proposed fix to properly test the nonexistent session scenario
def test_rollback_nonexistent_session(self, monkeypatch): - """Test rollback with nonexistent session.""" + """Test rollback with a session ID that doesn't exist.""" from unittest.mock import MagicMock from click.testing import CliRunner import refactron.cli.refactor as refactor_mod from refactron.cli import main mock_system = MagicMock() - mock_system.list_sessions.return_value = [] + # Return non-empty list so the early "no sessions" return is not triggered + mock_system.list_sessions.return_value = [{"id": "existing_session"}] mock_system.backup_manager.get_session.return_value = None monkeypatch.setattr(refactor_mod, "BackupRollbackSystem", lambda *a, **kw: mock_system) runner = CliRunner() with runner.isolated_filesystem(): result = runner.invoke(main, ["rollback", "--session", "nonexistent"]) - assert result.exit_code == 0 - assert "No backup sessions found" in result.output + assert result.exit_code == 1 + assert "Session not found: nonexistent" in result.output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_backup.py` around lines 575 - 594, The test test_rollback_nonexistent_session currently stubs mock_system.list_sessions to [] which triggers the "no sessions" path; change the mock so list_sessions returns a non-empty list (e.g., ["session1"]) while backup_manager.get_session returns None so the CLI code in refactor.py reaches the "session not found" branch; keep the monkeypatch of BackupRollbackSystem to return mock_system, then update assertions to expect exit_code == 1 and that result.output contains "Session not found: nonexistent" to verify the intended error path.
🧹 Nitpick comments (1)
refactron/core/logging_config.py (1)
150-155: Improve handler cleanup diagnostics with explicit error logging.The
except Exception: passpattern (lines 153–154) silently discards errors during handler closure. While handler removal still occurs, this hides diagnostics that could help troubleshoot lock issues on Windows. Log specific exceptions at debug level and use afinallyblock to make the guaranteed cleanup intent explicit.Suggested improvement
for handler in list(self.logger.handlers): try: handler.close() - except Exception: - pass - self.logger.removeHandler(handler) + except (OSError, ValueError) as exc: + logging.getLogger(__name__).debug( + "Failed to close log handler %r", handler, exc_info=exc + ) + finally: + self.logger.removeHandler(handler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/core/logging_config.py` around lines 150 - 155, The loop over self.logger.handlers should not swallow exceptions; update the cleanup to use a try/except/finally so handler.close() errors are caught and logged (at debug level) and removal always occurs; specifically wrap handler.close() in try/except Exception as exc and call self.logger.debug(...) with the exception details, and move self.logger.removeHandler(handler) into a finally block to guarantee removal (referencing handler.close() and self.logger.removeHandler(handler) in the method that iterates over self.logger.handlers).
🤖 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/rag/parser.py`:
- Around line 108-159: The helper methods _parser_works and _build_parser are
never used and reference a missing _tree_sitter_minor_version; update __init__
to initialize the parser by checking TREE_SITTER_AVAILABLE, calling a language
builder (use self._build_language() or add it if missing) and assigning
self.parser = self._build_parser(language), raising the same RuntimeError when
TREE_SITTER_AVAILABLE is false; and add the missing `@staticmethod`
_tree_sitter_minor_version() that imports tree_sitter, reads
tree_sitter.__version__ (defaulting to "0.20.0") and returns
int(version.split(".")[1]) so _build_parser can use it.
- Around line 67-88: The __init__ of CodeParser is returning Language objects
and references os/platform before they are imported and calls a missing helper;
fix it by removing all returns from __init__ and instead determine the
tree-sitter Language value (using tspython.language()), implement a helper
method _tree_sitter_minor_version() in the class, ensure os and platform are
imported at module level, and initialize and assign self.parser (e.g., create a
Parser, set its language) so parse_file can use it; keep the Language
construction logic (handling int, PyCapsule, and legacy shared-lib path search)
but convert those early returns into assignments to a local lang_obj used to
call self.parser.set_language(lang_obj).
---
Outside diff comments:
In `@refactron/rag/parser.py`:
- Around line 89-107: The file contains an orphaned try/except fragment around
the Language(lang, "python") initialization (references to Language(lang,
"python") and tspython) that leaves a dangling try block and causes a syntax
error before the following `@staticmethod`; remove the entire leftover block
starting from the try that attempts Language(lang, "python") fallback through
the platform/extension detection so the staticmethod that follows is no longer
nested inside an unclosed try.
In `@tests/test_backup.py`:
- Around line 575-594: The test test_rollback_nonexistent_session currently
stubs mock_system.list_sessions to [] which triggers the "no sessions" path;
change the mock so list_sessions returns a non-empty list (e.g., ["session1"])
while backup_manager.get_session returns None so the CLI code in refactor.py
reaches the "session not found" branch; keep the monkeypatch of
BackupRollbackSystem to return mock_system, then update assertions to expect
exit_code == 1 and that result.output contains "Session not found: nonexistent"
to verify the intended error path.
---
Nitpick comments:
In `@refactron/core/logging_config.py`:
- Around line 150-155: The loop over self.logger.handlers should not swallow
exceptions; update the cleanup to use a try/except/finally so handler.close()
errors are caught and logged (at debug level) and removal always occurs;
specifically wrap handler.close() in try/except Exception as exc and call
self.logger.debug(...) with the exception details, and move
self.logger.removeHandler(handler) into a finally block to guarantee removal
(referencing handler.close() and self.logger.removeHandler(handler) in the
method that iterates over self.logger.handlers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8198d82-0050-4288-bdc4-082b57c75c8c
📒 Files selected for processing (9)
refactron/__init__.pyrefactron/cli/analysis.pyrefactron/cli/ui.pyrefactron/core/logging_config.pyrefactron/rag/parser.pytests/test_backup.pytests/test_cicd.pytests/test_logging.pytests/test_patterns_feedback.py
omsherikar
approved these changes
Mar 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
refactron/rag/parser.py (1)
111-122: Preserve the root cause when shared-library probing fails.Line 116 currently swallows all exceptions in the probe loop; if all candidates fail, the final
RuntimeErrorloses the actual reason.Proposed refactor
- for fname in sorted(os.listdir(pkg_dir)): + last_error: Optional[Exception] = None + for fname in sorted(os.listdir(pkg_dir)): if fname.endswith(ext): lib_path = os.path.join(pkg_dir, fname) try: return Language(lib_path, "python") - except Exception: + except (OSError, TypeError, ValueError) as exc: + last_error = exc continue raise RuntimeError( "Could not initialise tree-sitter Python language. " "Try: pip install --upgrade tree-sitter tree-sitter-python" - ) + ) from last_error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/parser.py` around lines 111 - 122, The probe loop in parser.py swallows all exceptions and loses the root cause; modify the for-loop around Language(lib_path, "python") to capture the last caught exception (e.g., assign it to last_exc inside the except block) and after the loop raise the final RuntimeError using exception chaining (raise RuntimeError("Could not initialise ...") from last_exc) so the original error from the Language(...) attempt is preserved for debugging; ensure variable names referenced are fname, lib_path, Language and last_exc.
🤖 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/rag/parser.py`:
- Around line 70-76: The current try/except around the
importlib.metadata.pkg_version call catches Exception too broadly; change it to
only handle the package-not-installed case by catching
importlib.metadata.PackageNotFoundError (or
importlib_metadata.PackageNotFoundError for compatibility) and return 0 in that
branch, and remove the broad except so any other errors (e.g.,
ValueError/IndexError while parsing ver) propagate rather than being silently
swallowed; locate the block using the symbols pkg_version and ver and update the
except handling accordingly.
---
Nitpick comments:
In `@refactron/rag/parser.py`:
- Around line 111-122: The probe loop in parser.py swallows all exceptions and
loses the root cause; modify the for-loop around Language(lib_path, "python") to
capture the last caught exception (e.g., assign it to last_exc inside the except
block) and after the loop raise the final RuntimeError using exception chaining
(raise RuntimeError("Could not initialise ...") from last_exc) so the original
error from the Language(...) attempt is preserved for debugging; ensure variable
names referenced are fname, lib_path, Language and last_exc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dfd8c1f-13ac-453c-8c58-8ebd0e6be346
📒 Files selected for processing (2)
refactron/rag/parser.pytests/test_backup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_backup.py
Comment on lines
+70
to
+76
| try: | ||
| from importlib.metadata import version as pkg_version | ||
|
|
||
| ver = pkg_version("tree-sitter") | ||
| return int(ver.split(".")[1]) | ||
| except Exception: | ||
| return 0 |
Contributor
There was a problem hiding this comment.
Narrow the blind exception in version detection.
Line 75 catches Exception and silently returns 0, which can hide unexpected failures and route initialization down the wrong compatibility path.
Proposed fix
`@staticmethod`
def _tree_sitter_minor_version() -> int:
"""Return the minor version of the installed tree-sitter package."""
try:
- from importlib.metadata import version as pkg_version
+ from importlib.metadata import PackageNotFoundError, version as pkg_version
ver = pkg_version("tree-sitter")
return int(ver.split(".")[1])
- except Exception:
+ except (ImportError, PackageNotFoundError, IndexError, ValueError):
return 0📝 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
| try: | |
| from importlib.metadata import version as pkg_version | |
| ver = pkg_version("tree-sitter") | |
| return int(ver.split(".")[1]) | |
| except Exception: | |
| return 0 | |
| `@staticmethod` | |
| def _tree_sitter_minor_version() -> int: | |
| """Return the minor version of the installed tree-sitter package.""" | |
| try: | |
| from importlib.metadata import PackageNotFoundError, version as pkg_version | |
| ver = pkg_version("tree-sitter") | |
| return int(ver.split(".")[1]) | |
| except (ImportError, PackageNotFoundError, IndexError, ValueError): | |
| return 0 |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 75-75: 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/rag/parser.py` around lines 70 - 76, The current try/except around
the importlib.metadata.pkg_version call catches Exception too broadly; change it
to only handle the package-not-installed case by catching
importlib.metadata.PackageNotFoundError (or
importlib_metadata.PackageNotFoundError for compatibility) and return 0 in that
branch, and remove the broad except so any other errors (e.g.,
ValueError/IndexError while parsing ver) propagate rather than being silently
swallowed; locate the block using the symbols pkg_version and ver and update the
except handling accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rag/parser.py`:
- Around line 64-68: Resolve and remove all Git conflict markers in class
CodeParser so the file parses; choose and keep the unified implementations that
use CodeParser._get_language() from __init__, include a single coherent
_tree_sitter_minor_version() plus a single language construction helper (keep
one canonical method named _build_language or consolidate into _get_language —
ensure the symbol referenced in __init__ exists), preserve the 0.22+ handling
branch for building languages and the 0.20.x fallback comments/cases, and
standardize the exception handling to provide a clear single error message (no
duplicate handlers or conflicting messages). Concretely: delete all <<<<<<<,
=======, >>>>>>> markers, keep the final selected method names (ensure __init__
calls match the method you keep, e.g., _get_language or _build_language), merge
the differing logic from lines noted (minor version detection, language
construction, fallback comment, and error message) into one consistent
implementation so CodeParser imports without SyntaxError.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
refactron/rag/parser.py (1)
75-80:⚠️ Potential issue | 🟡 MinorNarrow exception handling in version detection.
Catching
Exceptionhere can mask unexpected failures and silently force the fallback version path. Restrict this to expected metadata/import errors.Suggested patch
if version is None: try: - from importlib.metadata import version as _pkg_version + from importlib.metadata import PackageNotFoundError, version as _pkg_version version = _pkg_version("tree-sitter") - except Exception: + except (ImportError, PackageNotFoundError): version = "0.20.0"#!/bin/bash # Verify current broad exception usage in version detection. rg -n "def _tree_sitter_minor_version|except Exception|importlib.metadata|_pkg_version" refactron/rag/parser.py -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/rag/parser.py` around lines 75 - 80, The try/except that sets version via _pkg_version("tree-sitter") should stop catching all Exceptions; narrow it to expected metadata/import failures. Replace the broad except Exception in the block that calls _pkg_version with a specific except tuple such as (ImportError, ModuleNotFoundError, PackageNotFoundError) and import PackageNotFoundError from importlib.metadata (or importlib_metadata for older Python) so only missing-package or import errors fall back to version = "0.20.0" while other errors propagate; keep the local names _pkg_version and version unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_rag_chunker.py`:
- Around line 15-19: The test's try/except should only catch RuntimeError from
CodeParser.__init__ so it doesn't mask real errors; update the block in
tests/test_rag_chunker.py to instantiate CodeParser and only except RuntimeError
(not Exception), preserving the current return True/False behavior when
tree-sitter is missing or misinitialized by CodeParser.__init__.
In `@tests/test_rag_parser.py`:
- Around line 20-24: The test currently catches all exceptions when
instantiating CodeParser which can mask unexpected errors; update the try/except
to only catch RuntimeError (the expected failure mode from CodeParser
initialization) so the test continues to fail for other exceptions—locate the
test block that constructs CodeParser in tests/test_rag_parser.py and replace
the broad "except Exception" with "except RuntimeError" (or the specific
exception class raised by CodeParser) and leave the rest of the logic unchanged.
---
Duplicate comments:
In `@refactron/rag/parser.py`:
- Around line 75-80: The try/except that sets version via
_pkg_version("tree-sitter") should stop catching all Exceptions; narrow it to
expected metadata/import failures. Replace the broad except Exception in the
block that calls _pkg_version with a specific except tuple such as (ImportError,
ModuleNotFoundError, PackageNotFoundError) and import PackageNotFoundError from
importlib.metadata (or importlib_metadata for older Python) so only
missing-package or import errors fall back to version = "0.20.0" while other
errors propagate; keep the local names _pkg_version and version unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cb34f46-151f-4f54-ae15-a29867c0d668
📒 Files selected for processing (3)
refactron/rag/parser.pytests/test_rag_chunker.pytests/test_rag_parser.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Improvements