feat(llm): batch triage RAG context extended - #106
Conversation
📝 WalkthroughWalkthroughThis PR adds a batched issue evaluation API to the LLM orchestrator (RAG-enabled triage), new BATCH_TRIAGE prompts, unit tests for batch triage and RAG flows, plus assorted type-hint, import, and documentation whitespace adjustments across the codebase. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Orch as LLMOrchestrator
participant Retriever as ContextRetriever
participant LLM as LLMClient
Client->>Orch: evaluate_issues_batch(issues, source_code)
Orch->>Retriever: retrieve_similar(source_code, top_k=3)
Retriever-->>Orch: rag_context (snippets)
Orch->>Orch: build issues_json payload (issue_id→message/line/category/severity)
Orch->>LLM: generate(system=BATCH_TRIAGE_SYSTEM_PROMPT,<br/>prompt=BATCH_TRIAGE_PROMPT, temperature=0.1)
LLM-->>Orch: response (JSON string)
Orch->>Orch: parse JSON → Dict[issue_id,float] (fallbacks on error)
Orch-->>Client: Dict[issue_id → confidence_score]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 |
|
@shrutu0929 please update the description for the pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
refactron/llm/orchestrator.py (1)
325-328: Consider usinglogger.exceptionfor better debugging.Using
logger.exceptioninstead oflogger.errorwill automatically include the stack trace, which aids debugging when batch triage fails unexpectedly.Proposed fix
except Exception as e: - logger.error(f"Batch triage failed: {e}") + logger.exception("Batch triage failed") # Fallback: return default confidence return {str(k): 0.5 for k in issues_data.keys()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/orchestrator.py` around lines 325 - 328, The except block that handles batch triage failures currently calls logger.error(f"Batch triage failed: {e}") which omits the stack trace; replace that call with logger.exception("Batch triage failed") so the exception and traceback are logged, keeping the existing fallback return {str(k): 0.5 for k in issues_data.keys()} intact; update the except Exception as e block in orchestrator.py where batch triage is handled to use logger.exception and do not change the returned default-confidence mapping.tests/test_llm_batch_triage.py (1)
37-81: Good test coverage for the happy path.The test correctly validates JSON parsing, context retrieval invocation, and result mapping. However, consider adding a test case for issues with duplicate
rule_idvalues to verify the collision handling behavior (related to the issue flagged in orchestrator.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_llm_batch_triage.py` around lines 37 - 81, Add a new unit test (e.g., test_evaluate_issues_batch_duplicate_rule_id) that uses LLMOrchestrator.evaluate_issues_batch with a list of CodeIssue objects where two issues share the same CodeIssue.rule_id; call evaluate_issues_batch with mocked mock_llm_client and mock_retriever, then assert the returned mapping contains a single entry for that duplicate rule_id (collision handled) and still includes entries for other issues, and assert the total number of keys equals the expected count after deduplication; reference LLMOrchestrator.evaluate_issues_batch, CodeIssue.rule_id, and the existing test_evaluate_issues_batch to mirror setup and mock assertions.
🤖 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/llm/orchestrator.py`:
- Around line 282-299: The current loop building issues_data uses rule_id as the
dict key which causes collisions when multiple CodeIssue objects share the same
rule_id; update the logic in the block that computes issue_id (the loop over
issues in orchestrator.py where variables issues, issue, issue_id, and
issues_data are used) to ensure uniqueness by making the key a composite (e.g.,
append the loop index or a running counter: f"{issue_id}_{i}" when rule_id
exists) or by storing values in a list under that key (so multiple entries for
the same rule_id are preserved); adjust downstream usages that expect single
objects accordingly (e.g., mapping/confidence consumers) so they handle multiple
issues per rule_id.
In `@refactron/llm/prompts.py`:
- Around line 96-115: The SYSTEM_PROMPT and BATCH_TRIAGE_PROMPT conflict: update
the batch flow used by evaluate_issues_batch so the model receives a dedicated
system message for batch triage (or add a clear overriding sentence at the top
of BATCH_TRIAGE_PROMPT) that explicitly states the expected output is a flat
JSON map of issue IDs to confidence scores only; also make the source code fence
language-agnostic in BATCH_TRIAGE_PROMPT by replacing the hardcoded "```python"
with a generic "```" or parameterize the language placeholder so non-Python
files are handled correctly.
---
Nitpick comments:
In `@refactron/llm/orchestrator.py`:
- Around line 325-328: The except block that handles batch triage failures
currently calls logger.error(f"Batch triage failed: {e}") which omits the stack
trace; replace that call with logger.exception("Batch triage failed") so the
exception and traceback are logged, keeping the existing fallback return
{str(k): 0.5 for k in issues_data.keys()} intact; update the except Exception as
e block in orchestrator.py where batch triage is handled to use logger.exception
and do not change the returned default-confidence mapping.
In `@tests/test_llm_batch_triage.py`:
- Around line 37-81: Add a new unit test (e.g.,
test_evaluate_issues_batch_duplicate_rule_id) that uses
LLMOrchestrator.evaluate_issues_batch with a list of CodeIssue objects where two
issues share the same CodeIssue.rule_id; call evaluate_issues_batch with mocked
mock_llm_client and mock_retriever, then assert the returned mapping contains a
single entry for that duplicate rule_id (collision handled) and still includes
entries for other issues, and assert the total number of keys equals the
expected count after deduplication; reference
LLMOrchestrator.evaluate_issues_batch, CodeIssue.rule_id, and the existing
test_evaluate_issues_batch to mirror setup and mock assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cdbce78-b278-4bb9-b9ae-9cd1c381d9ba
📒 Files selected for processing (3)
refactron/llm/orchestrator.pyrefactron/llm/prompts.pytests/test_llm_batch_triage.py
There was a problem hiding this comment.
Pull request overview
Adds a new “batch triage” capability to LLMOrchestrator to score multiple issues in a single LLM call, including optional RAG context, and introduces tests for expected parsing and fallback behavior.
Changes:
- Added
evaluate_issues_batch()to batch-score issue true-positive confidence with optional RAG context retrieval. - Introduced a new
BATCH_TRIAGE_PROMPTtemplate for batch triage requests. - Added a dedicated test suite covering normal parsing, empty input, LLM errors, and invalid JSON responses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_llm_batch_triage.py | New tests validating batch triage JSON parsing and fallback behaviors. |
| refactron/llm/prompts.py | Adds the BATCH_TRIAGE_PROMPT used to request per-issue confidence scores. |
| refactron/llm/orchestrator.py | Implements evaluate_issues_batch() integrating RAG retrieval + LLM call + JSON parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| issues = [ | ||
| CodeIssue( | ||
| category=IssueCategory.COMPLEXITY, | ||
| level=IssueLevel.WARNING, | ||
| message="Too complex", | ||
| file_path=Path("test.py"), | ||
| line_number=10, | ||
| ), | ||
| CodeIssue( | ||
| category=IssueCategory.STYLE, | ||
| level=IssueLevel.INFO, | ||
| message="Line too long", | ||
| file_path=Path("test.py"), | ||
| line_number=20, | ||
| ), | ||
| CodeIssue( | ||
| category=IssueCategory.CODE_SMELL, | ||
| level=IssueLevel.WARNING, | ||
| message="Bad smell", | ||
| file_path=Path("test.py"), | ||
| line_number=30, | ||
| rule_id="E101", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Batch triage currently has no test for multiple issues sharing the same rule_id (e.g., two STYLE issues both with rule_id="E501"). This is an important edge case because rule IDs are not unique per occurrence; add a test asserting the method returns distinct keys (and does not drop/overwrite one of the issues).
| # Ensure we return a Dict[str, float] | ||
| result = {} | ||
| for k, v in data.items(): | ||
| try: | ||
| result[str(k)] = float(v) | ||
| except (ValueError, TypeError): | ||
| result[str(k)] = 0.5 # Fallback for parsing errors | ||
| return result |
There was a problem hiding this comment.
evaluate_issues_batch documents confidence scores as 0.0–1.0, but the parsed floats are returned without clamping/validation. If the LLM returns values outside the range (or NaN/inf), downstream logic may misbehave. Consider normalizing by clamping into [0.0, 1.0] (and treating non-finite values as the fallback).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_llm_batch_triage.py (1)
1-132: Consider additional edge case coverage (optional).The test suite covers the main scenarios well. For completeness, you might consider adding tests for:
- Retriever failure: Verify behavior when
retriever.retrieve_similarraises an exception (the implementation logs a warning and continues with empty context).- Partial JSON validity: Test when LLM returns JSON with some unparseable values (e.g.,
{"issue_0": "high"}instead of a float).These are optional improvements and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_llm_batch_triage.py` around lines 1 - 132, Add two optional edge-case tests for LLMOrchestrator.evaluate_issues_batch: one where the ContextRetriever.retrieve_similar raises an exception (mock it to raise, assert evaluate_issues_batch still returns scores and that the exception is handled without crashing and that generate is still called), and one where the LLM returns JSON with a non-float value for a key (e.g., {"issue_0":"high","issue_1":0.7}) to confirm evaluate_issues_batch parses valid floats and falls back to 0.5 for unparseable entries; reference the LLMOrchestrator.evaluate_issues_batch method, the ContextRetriever.retrieve_similar call, and the llm_client.generate behavior when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_llm_batch_triage.py`:
- Around line 1-132: Add two optional edge-case tests for
LLMOrchestrator.evaluate_issues_batch: one where the
ContextRetriever.retrieve_similar raises an exception (mock it to raise, assert
evaluate_issues_batch still returns scores and that the exception is handled
without crashing and that generate is still called), and one where the LLM
returns JSON with a non-float value for a key (e.g.,
{"issue_0":"high","issue_1":0.7}) to confirm evaluate_issues_batch parses valid
floats and falls back to 0.5 for unparseable entries; reference the
LLMOrchestrator.evaluate_issues_batch method, the
ContextRetriever.retrieve_similar call, and the llm_client.generate behavior
when adding these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: beff2ead-3351-4016-87ce-b2681faacb9c
📒 Files selected for processing (7)
tests/test_cli.pytests/test_llm_batch_triage.pytests/test_patterns_feedback.pytests/test_patterns_integration.pytests/test_performance_optimization.pytests/test_rag_indexer.pytests/test_refactron.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_patterns_integration.py
- tests/test_performance_optimization.py
- tests/test_refactron.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 42 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, issue in enumerate(issues): | ||
| # Determine a stable issue ID | ||
| issue_id = getattr(issue, "rule_id", None) | ||
| if not issue_id: | ||
| issue_id = f"issue_{i}" | ||
|
|
||
| issues_data[issue_id] = { | ||
| "message": issue.message, |
There was a problem hiding this comment.
In evaluate_issues_batch, using rule_id as the dict key can silently overwrite entries when multiple issues share the same rule_id (common when a rule triggers on multiple lines). This will drop issues from issues_data and from the returned score map. Consider making the key unique (e.g., include index/line number) or mapping each rule_id to a list of occurrences and returning scores per occurrence.
| patterns_set1 = {p.pattern_hash for p in patterns1.values()} | ||
| patterns_set2 = {p.pattern_hash for p in patterns2.values()} | ||
|
|
||
| # In most cases, these sets should be disjoint if patterns were learned | ||
| if patterns_set1 and patterns_set2: | ||
| # Basic check that isolation works at directory level (already checked above) | ||
| pass |
There was a problem hiding this comment.
This conditional block computes patterns_set1/patterns_set2 but then does pass, so it no longer asserts anything about pattern isolation when both sets are non-empty. Either remove this dead check entirely or replace it with a deterministic assertion (e.g., verify the on-disk pattern files are confined to each project’s storage dir, or assert on a property that cannot collide like storage path).
| def test_evaluate_issues_batch(mock_llm_client, mock_retriever): | ||
| """Test that batch evaluation correctly parses JSON map from the LLM.""" | ||
| orchestrator = LLMOrchestrator(retriever=mock_retriever, llm_client=mock_llm_client) | ||
|
|
||
| issues = [ | ||
| CodeIssue( | ||
| category=IssueCategory.COMPLEXITY, | ||
| level=IssueLevel.WARNING, | ||
| message="Too complex", | ||
| file_path=Path("test.py"), | ||
| line_number=10, | ||
| ), | ||
| CodeIssue( | ||
| category=IssueCategory.STYLE, | ||
| level=IssueLevel.INFO, | ||
| message="Line too long", | ||
| file_path=Path("test.py"), | ||
| line_number=20, | ||
| ), | ||
| CodeIssue( | ||
| category=IssueCategory.CODE_SMELL, | ||
| level=IssueLevel.WARNING, | ||
| message="Bad smell", | ||
| file_path=Path("test.py"), | ||
| line_number=30, | ||
| rule_id="E101", | ||
| ), | ||
| ] | ||
|
|
||
| source_code = "def complex_function():\n pass\n" * 10 | ||
|
|
||
| result = orchestrator.evaluate_issues_batch(issues, source_code) | ||
|
|
||
| # Check that ContextRetriever was called for RAG Context | ||
| mock_retriever.retrieve_similar.assert_called_once() | ||
| assert "def complex_function" in mock_retriever.retrieve_similar.call_args[0][0] | ||
|
|
||
| # Check JSON map parsing | ||
| assert isinstance(result, dict) | ||
| assert result.get("issue_0") == 0.85 | ||
| assert result.get("issue_1") == 0.12 | ||
| assert result.get("E101") == 0.95 | ||
|
|
||
| # Ensure there's exactly 3 keys corresponding to the 3 returned mapping | ||
| assert len(result) == 3 | ||
|
|
||
|
|
||
| def test_evaluate_issues_batch_empty_issues(mock_llm_client, mock_retriever): | ||
| """Test batch evaluation handles empty issues correctly.""" | ||
| orchestrator = LLMOrchestrator(retriever=mock_retriever, llm_client=mock_llm_client) | ||
|
|
||
| result = orchestrator.evaluate_issues_batch([], "source") | ||
| assert result == {} | ||
| mock_llm_client.generate.assert_not_called() | ||
|
|
||
|
|
||
| def test_evaluate_issues_batch_fallback_on_error(mock_llm_client, mock_retriever): | ||
| """Test batch evaluation handles LLM errors using a fallback mechanism.""" | ||
| mock_llm_client.generate.side_effect = Exception("LLM Error") | ||
|
|
||
| orchestrator = LLMOrchestrator(retriever=mock_retriever, llm_client=mock_llm_client) | ||
|
|
||
| issues = [ | ||
| CodeIssue( | ||
| category=IssueCategory.STYLE, | ||
| level=IssueLevel.INFO, | ||
| message="Line too long", | ||
| file_path=Path("test.py"), | ||
| line_number=20, | ||
| ) | ||
| ] | ||
|
|
||
| result = orchestrator.evaluate_issues_batch(issues, "source") | ||
|
|
||
| # It should fallback to 0.5 confidence for 'issue_0' | ||
| assert result == {"issue_0": 0.5} | ||
|
|
||
|
|
||
| def test_evaluate_issues_batch_fallback_on_bad_json(mock_llm_client, mock_retriever): | ||
| """Test batch evaluation handles invalid JSON appropriately.""" | ||
| mock_llm_client.generate.return_value = "not a json string at all" | ||
| orchestrator = LLMOrchestrator(retriever=mock_retriever, llm_client=mock_llm_client) | ||
|
|
||
| issues = [ | ||
| CodeIssue( | ||
| category=IssueCategory.STYLE, | ||
| level=IssueLevel.INFO, | ||
| message="Line too long", | ||
| file_path=Path("test.py"), | ||
| line_number=20, | ||
| ) | ||
| ] | ||
|
|
||
| result = orchestrator.evaluate_issues_batch(issues, "source") | ||
|
|
||
| assert result == {"issue_0": 0.5} |
There was a problem hiding this comment.
Batch triage tests don’t currently cover a few important behaviors introduced in evaluate_issues_batch: (1) multiple issues with the same rule_id (to ensure no overwrites), (2) LLM returning a partial map (to ensure defaults are filled), and (3) out-of-range scores (to ensure clamping). Adding these cases would protect against regressions in the fallback/normalization logic.
| try: | ||
| response_text = self.client.generate( | ||
| prompt=prompt, system=SYSTEM_PROMPT, temperature=0.1 | ||
| ) |
There was a problem hiding this comment.
evaluate_issues_batch calls self.client.generate(..., system=SYSTEM_PROMPT, ...), but SYSTEM_PROMPT specifies a fixed JSON schema (explanation, proposed_code, etc.) that conflicts with the batch triage requirement of returning a plain {issue_id: score} map. This will likely push the model toward the wrong output format in production. Consider using a dedicated system prompt for batch triage (or pass system=None) that matches the expected JSON map.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_patterns_integration.py (1)
262-268: Drop the no-op isolation block.This branch never asserts anything, so it looks like the test is validating hash isolation when it actually isn't. Since the real invariant here is storage-dir isolation, I'd either remove this block or replace it with an explicit assertion for the property you want to guarantee.
✂️ Simplify the test
- patterns_set1 = {p.pattern_hash for p in patterns1.values()} - patterns_set2 = {p.pattern_hash for p in patterns2.values()} - - # In most cases, these sets should be disjoint if patterns were learned - if patterns_set1 and patterns_set2: - # Basic check that isolation works at directory level (already checked above) - pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_patterns_integration.py` around lines 262 - 268, The no-op branch checking patterns_set1 and patterns_set2 does nothing; remove the entire if patterns_set1 and patterns_set2: ... pass block or replace it with an explicit assertion that matches the intended invariant (e.g., assert patterns_set1.isdisjoint(patterns_set2) to enforce hash isolation, or assert something about storage-dir separation using patterns1 and patterns2 if the real guarantee is directory-level isolation); update references to patterns_set1, patterns_set2, patterns1, and patterns2 accordingly so the test either enforces disjoint hashes or clearly validates storage-dir isolation.
🤖 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/llm/backend_client.py`:
- Around line 95-96: The code currently returns cast(str, data["content"])
without runtime checks; replace that with explicit validation: after data =
response.json() ensure data is a mapping and contains the "content" key and that
content is an instance of str (not None); if validation fails, raise a clear
error (e.g., ValueError) that includes the unexpected value/type so callers like
orchestrator._clean_json_response() don't get AttributeError when calling
.strip(); update the return to return the validated str.
In `@refactron/llm/client.py`:
- Line 82: The code currently uses cast() in GroqClient.generate to coerce
response.choices[0].message.content to str; replace that with an explicit
runtime check: retrieve response.choices, access
response.choices[0].message.content, verify it is a non-None str (and optionally
non-empty), and if not raise a clear exception (e.g., ValueError or
RuntimeError) that includes contextual details (choice index, message role/id,
and the raw response or response.choices structure) so callers like
LLMOrchestrator._clean_json_response can surface a meaningful API-response error
instead of an AttributeError; update GroqClient.generate to perform this
validation and raise the descriptive error when content is None or not a string.
In `@refactron/llm/orchestrator.py`:
- Around line 331-338: The current conversion loop returns only present keys and
leaves values unbounded; update the logic that builds result (the dict created
in the snippet in orchestrator.py) to iterate over the full set of expected
issue IDs (e.g., a known list like self.issue_ids or expected_issue_ids), for
each key read data.get(key) or data.get(str(key)), attempt to parse float, clamp
the parsed value into the [0.0, 1.0] range, and if parsing fails or the key is
missing set the value to the fallback 0.5; keep result as Dict[str, float] and
return it.
In `@refactron/rag/parser.py`:
- Around line 84-91: The Windows fallback only checks for ".dll" so
tree_sitter_python ".pyd" extension modules are missed; update the logic in the
parser (the platform check that sets ext in refactron.rag.parser, used by
CodeParser initialization/fallback) to consider ".pyd" as a valid Windows
extension (either by adding ".pyd" to the Windows branch or by attempting both
".dll" and ".pyd") so the fallback lookup will find tree_sitter_python on
Windows.
---
Nitpick comments:
In `@tests/test_patterns_integration.py`:
- Around line 262-268: The no-op branch checking patterns_set1 and patterns_set2
does nothing; remove the entire if patterns_set1 and patterns_set2: ... pass
block or replace it with an explicit assertion that matches the intended
invariant (e.g., assert patterns_set1.isdisjoint(patterns_set2) to enforce hash
isolation, or assert something about storage-dir separation using patterns1 and
patterns2 if the real guarantee is directory-level isolation); update references
to patterns_set1, patterns_set2, patterns1, and patterns2 accordingly so the
test either enforces disjoint hashes or clearly validates storage-dir isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85664dd2-49f1-4afe-afaa-a4e76c7eae79
📒 Files selected for processing (40)
docs/advanced/ci-cd.mdxdocs/advanced/monitoring.mdxdocs/advanced/performance.mdxdocs/api-reference/refactron-class.mdxdocs/docs.jsondocs/essentials/authentication.mdxdocs/essentials/configuration.mdxdocs/essentials/installation.mdxdocs/guides/ai-features.mdxdocs/guides/code-analysis.mdxdocs/guides/pattern-learning.mdxdocs/guides/refactoring.mdxdocs/introduction.mdxdocs/quickstart.mdxdocs/resources/faq.mdxdocumentation/docs/CLI_REFERENCE.mddocumentation/docs/api/analyzers.mddocumentation/docs/api/autofix.mddocumentation/docs/api/cicd.mddocumentation/docs/api/core.mddocumentation/docs/api/llm.mddocumentation/docs/api/patterns.mddocumentation/docs/api/rag.mddocumentation/docs/api/refactorers.mdrefactron/cli.pyrefactron/core/repositories.pyrefactron/core/workspace.pyrefactron/llm/backend_client.pyrefactron/llm/client.pyrefactron/llm/models.pyrefactron/llm/orchestrator.pyrefactron/llm/safety.pyrefactron/rag/indexer.pyrefactron/rag/parser.pyrefactron/rag/retriever.pyscripts/analyze_feedback_data.pytests/test_backend_client.pytests/test_llm_orchestrator.pytests/test_patterns_integration.pytests/test_rag_indexer.py
💤 Files with no reviewable changes (8)
- documentation/docs/api/analyzers.md
- documentation/docs/api/llm.md
- documentation/docs/api/autofix.md
- documentation/docs/api/refactorers.md
- documentation/docs/api/cicd.md
- documentation/docs/api/rag.md
- documentation/docs/api/core.md
- documentation/docs/api/patterns.md
✅ Files skipped from review due to trivial changes (17)
- documentation/docs/CLI_REFERENCE.md
- docs/quickstart.mdx
- refactron/core/workspace.py
- docs/advanced/ci-cd.mdx
- docs/guides/ai-features.mdx
- docs/api-reference/refactron-class.mdx
- docs/guides/code-analysis.mdx
- docs/guides/pattern-learning.mdx
- docs/essentials/installation.mdx
- docs/docs.json
- docs/essentials/configuration.mdx
- docs/essentials/authentication.mdx
- docs/advanced/performance.mdx
- docs/introduction.mdx
- docs/resources/faq.mdx
- refactron/rag/retriever.py
- docs/guides/refactoring.mdx
| data = response.json() | ||
| return data["content"] | ||
| return cast(str, data["content"]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "backend_client.py" | head -5Repository: Refactron-ai/Refactron_lib
Length of output: 103
🏁 Script executed:
find . -type f -name "orchestrator.py" | head -5Repository: Refactron-ai/Refactron_lib
Length of output: 101
🏁 Script executed:
wc -l ./refactron/llm/backend_client.pyRepository: Refactron-ai/Refactron_lib
Length of output: 107
🏁 Script executed:
wc -l ./refactron/llm/orchestrator.pyRepository: Refactron-ai/Refactron_lib
Length of output: 105
🏁 Script executed:
cat -n ./refactron/llm/backend_client.py | sed -n '85,105p'Repository: Refactron-ai/Refactron_lib
Length of output: 1120
🏁 Script executed:
cat -n ./refactron/llm/orchestrator.py | grep -A 5 -B 5 "_clean_json_response"Repository: Refactron-ai/Refactron_lib
Length of output: 1583
🏁 Script executed:
cat -n ./refactron/llm/orchestrator.py | sed -n '75,110p'Repository: Refactron-ai/Refactron_lib
Length of output: 1706
🏁 Script executed:
cat -n ./refactron/llm/orchestrator.py | sed -n '315,345p'Repository: Refactron-ai/Refactron_lib
Length of output: 1374
🏁 Script executed:
cat -n ./refactron/llm/backend_client.py | sed -n '1,50p'Repository: Refactron-ai/Refactron_lib
Length of output: 1846
🏁 Script executed:
rg "def generate" ./refactron/llm/backend_client.py -A 20Repository: Refactron-ai/Refactron_lib
Length of output: 672
🏁 Script executed:
cat -n ./refactron/llm/backend_client.py | sed -n '39,105p'Repository: Refactron-ai/Refactron_lib
Length of output: 2811
🏁 Script executed:
rg "isinstance.*str" ./refactron/llm/orchestrator.py | head -5Repository: Refactron-ai/Refactron_lib
Length of output: 121
Add runtime validation for backend content before returning.
The cast(str, data["content"]) provides no runtime enforcement—if the backend returns {"content": null} or a non-string value, the function violates its -> str contract and returns that value. Downstream, orchestrator's _clean_json_response() calls .strip() on it, causing an AttributeError. Validate the payload at the client boundary and raise a clear error instead:
Suggested fix
data = response.json()
- return cast(str, data["content"])
+ content = data.get("content")
+ if not isinstance(content, str):
+ raise RuntimeError("Backend LLM proxy returned invalid content")
+ return content📝 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.
| data = response.json() | |
| return data["content"] | |
| return cast(str, data["content"]) | |
| data = response.json() | |
| content = data.get("content") | |
| if not isinstance(content, str): | |
| raise RuntimeError("Backend LLM proxy returned invalid content") | |
| return content |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/llm/backend_client.py` around lines 95 - 96, The code currently
returns cast(str, data["content"]) without runtime checks; replace that with
explicit validation: after data = response.json() ensure data is a mapping and
contains the "content" key and that content is an instance of str (not None); if
validation fails, raise a clear error (e.g., ValueError) that includes the
unexpected value/type so callers like orchestrator._clean_json_response() don't
get AttributeError when calling .strip(); update the return to return the
validated str.
| ) | ||
|
|
||
| return response.choices[0].message.content | ||
| return cast(str, response.choices[0].message.content) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure to understand the codebase
find . -type f -name "*.py" | grep -E "(client|orchestrator)" | head -20Repository: Refactron-ai/Refactron_lib
Length of output: 253
🏁 Script executed:
# Examine the specific line in question
cat -n refactron/llm/client.py | sed -n '75,90p'Repository: Refactron-ai/Refactron_lib
Length of output: 665
🏁 Script executed:
# Get the full context of the GroqClient class and generate method
cat -n refactron/llm/client.pyRepository: Refactron-ai/Refactron_lib
Length of output: 3336
🏁 Script executed:
# Check the LLMOrchestrator._clean_json_response method
cat -n refactron/llm/orchestrator.pyRepository: Refactron-ai/Refactron_lib
Length of output: 16002
🏁 Script executed:
# Search for calls to GroqClient.generate to see how return values are used
rg "\.generate\(" refactron/llm/Repository: Refactron-ai/Refactron_lib
Length of output: 382
🏁 Script executed:
# Let's check the test files to see if they provide insights into expected behavior
cat -n tests/test_groq_client.pyRepository: Refactron-ai/Refactron_lib
Length of output: 4614
🏁 Script executed:
# Check if there's any error handling or null checks elsewhere in the codebase
rg "message\.content" --type pyRepository: Refactron-ai/Refactron_lib
Length of output: 262
🏁 Script executed:
# Let's look at the BackendLLMClient for comparison on how it handles responses
cat -n refactron/llm/backend_client.pyRepository: Refactron-ai/Refactron_lib
Length of output: 4651
🏁 Script executed:
# Check Groq SDK type hints for message.content
python3 << 'EOF'
import ast
import sys
# Try to get info about Groq SDK's message.content field
try:
from groq.types.chat import ChatCompletion
import inspect
# Check the type hints
sig = inspect.signature(ChatCompletion)
print("ChatCompletion signature:")
print(sig)
# Try to find message content type
print("\nTrying to inspect ChatCompletion.Choice.Message:")
from groq.types.chat.chat_completion_message import ChatCompletionMessage
print(inspect.signature(ChatCompletionMessage))
except Exception as e:
print(f"Could not import Groq SDK: {e}")
print("This is expected in the sandbox - checking if groq is even installed...")
import subprocess
result = subprocess.run(["pip", "list"], capture_output=True, text=True)
if "groq" in result.stdout.lower():
print("Groq is installed")
else:
print("Groq is not installed")
EOFRepository: Refactron-ai/Refactron_lib
Length of output: 213
🏁 Script executed:
# Search for any null checks or error handling in the codebase related to response handling
rg "content.*None|None.*content" --type py -A 2 -B 2Repository: Refactron-ai/Refactron_lib
Length of output: 1213
🏁 Script executed:
# Check if there are any integration tests that might show actual Groq API behavior
cat -n tests/test_llm_orchestrator.py | head -100Repository: Refactron-ai/Refactron_lib
Length of output: 4082
🌐 Web query:
Groq SDK message content nullable optional
💡 Result:
In the Groq TypeScript/Node SDK (groq-sdk), an assistant message’s content is both optional and nullable:
content?: string | Array<ChatCompletionContentPartText> | null- Docstring: “Required unless
tool_callsorfunction_callis specified.” (so tool-calling assistant messages may omitcontent). [1]
In practice, when the model returns a tool call with no text, some users have observed choice.message.content being undefined (even though the type is string | null), and reported it as a typing/API mismatch. [2]
Sources:
[1] Groq TypeScript SDK type definition (ChatCompletionAssistantMessageParam.content) https://github.com/groq/groq-typescript/blob/main/src/resources/chat/completions.ts
[2] Groq Community thread (“Bug typescript SDK types”) https://community.groq.com/t/bug-typescript-sdk-types/421
Add explicit runtime validation for message.content instead of relying on cast().
cast() only affects static type checking and does not validate the actual value at runtime. The Groq SDK can return None for message.content (particularly when tool calls are made without accompanying text). When this happens, GroqClient.generate() will return None while its type signature claims str, causing LLMOrchestrator._clean_json_response() to fail at line 347 when it calls .strip() with an AttributeError rather than a clear error about the API response.
Suggested fix
- return cast(str, response.choices[0].message.content)
+ content = response.choices[0].message.content
+ if content is None:
+ raise RuntimeError("Groq returned an empty response")
+ return content📝 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.
| return cast(str, response.choices[0].message.content) | |
| content = response.choices[0].message.content | |
| if content is None: | |
| raise RuntimeError("Groq returned an empty response") | |
| return content |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/llm/client.py` at line 82, The code currently uses cast() in
GroqClient.generate to coerce response.choices[0].message.content to str;
replace that with an explicit runtime check: retrieve response.choices, access
response.choices[0].message.content, verify it is a non-None str (and optionally
non-empty), and if not raise a clear exception (e.g., ValueError or
RuntimeError) that includes contextual details (choice index, message role/id,
and the raw response or response.choices structure) so callers like
LLMOrchestrator._clean_json_response can surface a meaningful API-response error
instead of an AttributeError; update GroqClient.generate to perform this
validation and raise the descriptive error when content is None or not a string.
| # Ensure we return a Dict[str, float] | ||
| result = {} | ||
| for k, v in data.items(): | ||
| try: | ||
| result[str(k)] = float(v) | ||
| except (ValueError, TypeError): | ||
| result[str(k)] = 0.5 # Fallback for parsing errors | ||
| return result |
There was a problem hiding this comment.
Return a complete, bounded confidence map.
A valid-but-partial LLM JSON response currently drops missing issues entirely, and out-of-range values are returned as-is. That breaks the issue_id -> confidence (0.0–1.0) contract and makes downstream handling brittle.
🛠️ Suggested fix
- result = {}
- for k, v in data.items():
- try:
- result[str(k)] = float(v)
- except (ValueError, TypeError):
- result[str(k)] = 0.5 # Fallback for parsing errors
+ result = {issue_id: 0.5 for issue_id in issues_data}
+ for k, v in data.items():
+ issue_id = str(k)
+ if issue_id not in result:
+ continue
+ try:
+ score = float(v)
+ if score != score or score < 0.0 or score > 1.0:
+ raise ValueError
+ result[issue_id] = score
+ except (ValueError, TypeError):
+ pass
return result📝 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.
| # Ensure we return a Dict[str, float] | |
| result = {} | |
| for k, v in data.items(): | |
| try: | |
| result[str(k)] = float(v) | |
| except (ValueError, TypeError): | |
| result[str(k)] = 0.5 # Fallback for parsing errors | |
| return result | |
| # Ensure we return a Dict[str, float] | |
| result = {issue_id: 0.5 for issue_id in issues_data} | |
| for k, v in data.items(): | |
| issue_id = str(k) | |
| if issue_id not in result: | |
| continue | |
| try: | |
| score = float(v) | |
| if score != score or score < 0.0 or score > 1.0: | |
| raise ValueError | |
| result[issue_id] = score | |
| except (ValueError, TypeError): | |
| pass | |
| return result |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 338-338: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@refactron/llm/orchestrator.py` around lines 331 - 338, The current conversion
loop returns only present keys and leaves values unbounded; update the logic
that builds result (the dict created in the snippet in orchestrator.py) to
iterate over the full set of expected issue IDs (e.g., a known list like
self.issue_ids or expected_issue_ids), for each key read data.get(key) or
data.get(str(key)), attempt to parse float, clamp the parsed value into the
[0.0, 1.0] range, and if parsing fails or the key is missing set the value to
the fallback 0.5; keep result as Dict[str, float] and return it.
| # Find the correct shared library extension | ||
| system = platform.system() | ||
| if system == "Windows": | ||
| ext = ".dll" | ||
| elif system == "Darwin": | ||
| ext = ".dylib" | ||
| else: | ||
| ext = ".so" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the tree_sitter_python Python package on Windows, what filename extension do the compiled wheel modules use (.pydor.dll)? Also, are .pyd files the standard CPython extension-module format on Windows?
💡 Result:
- For
tree_sitter_pythonon Windows, the compiled extension module is built as a CPython extension module namedtree_sitter_python._binding(fromsetup.py’sExtension(name="_binding", ...)withext_package="tree_sitter_python"), which on Windows is produced with the.pydsuffix (not.dll). [1][2] - Yes:
.pydis the standard CPython extension-module filename on Windows—it’s a DLL used specifically as an importable Python extension module. [2]
Sources
[1] tree-sitter/tree-sitter-python setup.py (PyPI package build config) — (github.com)
[2] Python docs: Defining extension modules (notes .pyd on Windows) — (docs.python.org)
Citations:
- 1: https://github.com/tree-sitter/tree-sitter-python/blob/master/setup.py
- 2: https://docs.python.org/3.15/c-api/extension-modules.html?utm_source=openai
Support Windows .pyd modules in the fallback lookup.
The fallback code only searches for .dll on Windows, but tree_sitter_python is compiled as a .pyd extension module (the standard CPython extension format on Windows). This causes CodeParser() initialization to fail on Windows in the fallback path.
Suggested fix
- if system == "Windows":
- ext = ".dll"
- elif system == "Darwin":
- ext = ".dylib"
- else:
- ext = ".so"
+ if system == "Windows":
+ exts = (".pyd", ".dll")
+ elif system == "Darwin":
+ exts = (".dylib",)
+ else:
+ exts = (".so",)
# Look for common names of the compiled language file
lib_path = None
for fname in os.listdir(pkg_dir):
- if fname.endswith(ext):
+ if fname.endswith(exts):
lib_path = os.path.join(pkg_dir, fname)
break📝 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.
| # Find the correct shared library extension | |
| system = platform.system() | |
| if system == "Windows": | |
| ext = ".dll" | |
| elif system == "Darwin": | |
| ext = ".dylib" | |
| else: | |
| ext = ".so" | |
| # Find the correct shared library extension | |
| system = platform.system() | |
| if system == "Windows": | |
| exts = (".pyd", ".dll") | |
| elif system == "Darwin": | |
| exts = (".dylib",) | |
| else: | |
| exts = (".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 84 - 91, The Windows fallback only
checks for ".dll" so tree_sitter_python ".pyd" extension modules are missed;
update the logic in the parser (the platform check that sets ext in
refactron.rag.parser, used by CodeParser initialization/fallback) to consider
".pyd" as a valid Windows extension (either by adding ".pyd" to the Windows
branch or by attempting both ".dll" and ".pyd") so the fallback lookup will find
tree_sitter_python on Windows.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
refactron/llm/orchestrator.py (1)
332-344:⚠️ Potential issue | 🟠 MajorEnsure bounded, complete confidence map.
The current implementation iterates only over
data.items()(LLM response), so if the LLM returns a partial response, some issue IDs will be missing. Additionally, values are not clamped to[0.0, 1.0], which breaks the documented contract.🛠️ Proposed fix to ensure completeness and value bounds
- # Ensure we return a Dict[str, float] - result = {} - for k, v in data.items(): - try: - result[str(k)] = float(v) - except (ValueError, TypeError): - result[str(k)] = 0.5 # Fallback for parsing errors - return result + # Ensure we return a complete Dict[str, float] with bounded values + result = {issue_id: 0.5 for issue_id in issues_data} + for k, v in data.items(): + issue_id = str(k) + if issue_id not in result: + continue # Ignore unexpected keys from LLM + try: + score = float(v) + result[issue_id] = max(0.0, min(1.0, score)) + except (ValueError, TypeError): + pass # Keep default 0.5 + return 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 332 - 344, The code currently builds the confidence map only from data.items() and doesn't clamp values; change the logic to iterate over all issue IDs from issues_data.keys(), for each id attempt to parse data.get(id) (or data.get(str(id))) to float, on ValueError/TypeError or missing key use 0.5 as fallback, then clamp the parsed value into the [0.0, 1.0] range before assigning to result[str(id)]; apply the same completeness-and-clamping behavior in the except block (replace the comprehension there) so the returned Dict[str, float] always contains every issue id from issues_data and values are bounded.
🧹 Nitpick comments (1)
refactron/llm/orchestrator.py (1)
341-344: Uselogging.exceptionto include traceback.Per static analysis hint (TRY400),
logging.exceptionautomatically includes the traceback, which aids debugging when batch triage fails.♻️ Proposed fix
except Exception as e: - logger.error(f"Batch triage failed: {e}") + logger.exception("Batch triage failed") # Fallback: return default confidence return {str(k): 0.5 for k in issues_data.keys()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@refactron/llm/orchestrator.py` around lines 341 - 344, The except block handling failures in the batch triage logic currently uses logger.error(f"Batch triage failed: {e}") which omits the traceback; change this to use logger.exception (or logging.exception) in the except Exception as e block so the full stack trace is logged while keeping the fallback return {str(k): 0.5 for k in issues_data.keys()} intact—locate the except Exception as e handler in orchestrator.py (the batch triage error handling) and replace the logger.error call with logger.exception(...) to include traceback details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@refactron/llm/orchestrator.py`:
- Around line 332-344: The code currently builds the confidence map only from
data.items() and doesn't clamp values; change the logic to iterate over all
issue IDs from issues_data.keys(), for each id attempt to parse data.get(id) (or
data.get(str(id))) to float, on ValueError/TypeError or missing key use 0.5 as
fallback, then clamp the parsed value into the [0.0, 1.0] range before assigning
to result[str(id)]; apply the same completeness-and-clamping behavior in the
except block (replace the comprehension there) so the returned Dict[str, float]
always contains every issue id from issues_data and values are bounded.
---
Nitpick comments:
In `@refactron/llm/orchestrator.py`:
- Around line 341-344: The except block handling failures in the batch triage
logic currently uses logger.error(f"Batch triage failed: {e}") which omits the
traceback; change this to use logger.exception (or logging.exception) in the
except Exception as e block so the full stack trace is logged while keeping the
fallback return {str(k): 0.5 for k in issues_data.keys()} intact—locate the
except Exception as e handler in orchestrator.py (the batch triage error
handling) and replace the logger.error call with logger.exception(...) to
include traceback details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1eb247cd-94b4-4c35-8f2a-7c8c6011279a
📒 Files selected for processing (3)
refactron/llm/orchestrator.pyrefactron/llm/prompts.pytests/test_llm_batch_triage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_llm_batch_triage.py
closes #107
Currently,
LLMOrchestrator
generates refactoring suggestions one by one, which is inefficient when validating multiple flagged issues. To better suppress false-positive code smells and optimize context building, this PR introduces a mechanism to batch evaluate multiple flagged issues in a single file while providing project-wide RAG context.
🛠️ Changes Implemented
refactron/llm/prompts.py
)
Added a new BATCH_TRIAGE_PROMPT system template.
Guides the LLM to output a precise JSON map (Dict[str, float]) relating specific issue_id values to confidence scores ranging from 0.0 (false positive) to 1.0 (true positive).
2. Batch Evaluation Logic (
refactron/llm/orchestrator.py
)
Implemented the
evaluate_issues_batch(issues, source_code)
method in the
LLMOrchestrator
class.
Integrates ContextRetriever.retrieve_similar(source_code) to intelligently attach RAG definitions (like project-wide constants or architecture notes) dynamically into the evaluation context.
Builds an aggregated JSON dictionary of issues before making a single batch call to the underlying LLM.
Handles LLM output scrubbing and graceful JSON map decoding.
Contains automatic fallbacks for LLM failures or malformed responses.
3. Unit Testing & Validations (
tests/test_llm_batch_triage.py
)
Added robust, independent testing specifically focused on batched triage.
Verifies context retrieval flow (MockContextRetriever assertions).
Validates accurate parsing of the structured JSON mapping returned by the mock LLM client.
Includes tests for edge cases (e.g., empty lists, completely invalid JSON responses, and network-related runtime exceptions).
4. Code Quality
Addressed typing consistency (Dict types) and line-length limits in accordance with project standards.
Files have been successfully formatted using black and isort, and have passed flake8 pre-commit linting checks. All associated tests successfully pass pytest.
Summary by CodeRabbit
Release Notes
New Features
Tests
Summary by CodeRabbit
New Features
Tests
Documentation
Chores