feat: ReactAgent core loop with system prompt builder - #361
Conversation
Implement the ReAct-style agent that replaces Plan-and-Execute for code generation. The agent iterates: LLM call → tool execution → observe results, terminating when the LLM responds with text only. Key features: - ReAct loop with 30-iteration hard cap and temperature 0.0 - 3-layer system prompt: base rules, project context, task details - Per-edit lint gate (ruff on modified files after edit/create) - Final verification with gates + bounded mini ReAct retry loop - Path traversal protection on lint operations - Conversation history trimming to prevent context overflow - Exception handling for graceful FAILED status on errors - Intent preview instruction for high-complexity tasks 10 unit tests covering loop termination, tool dispatch, prompt construction, verification retry, exception handling, and path safety.
|
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:
WalkthroughAdds a new headless ReAct-style autonomous agent (ReactAgent) that iteratively calls an LLM, dispatches tool calls, runs per-edit Ruff linting, manages conversation trimming, and performs bounded final verification with retry. Also adds lifecycle event types and comprehensive unit tests for agent behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReactAgent
participant LLMProvider
participant ToolExecutor
participant Ruff
participant Gates
Client->>ReactAgent: run(task_id)
ReactAgent->>ReactAgent: Load task context & build system prompt
loop ReAct Loop (max 30 iterations)
ReactAgent->>LLMProvider: Call LLM with system prompt & messages
LLMProvider-->>ReactAgent: Response (tool_calls or text-only)
alt Text-only response
ReactAgent->>ReactAgent: Break loop
else Tool calls present
ReactAgent->>ToolExecutor: execute_tool(tool_call)
ToolExecutor->>Ruff: Run lint check (if edit/create)
Ruff-->>ToolExecutor: Lint results
ToolExecutor-->>ReactAgent: Tool result with lint annotations
ReactAgent->>ReactAgent: Append result to messages, trim if needed
end
end
ReactAgent->>Gates: Run final verification (gates)
Gates-->>ReactAgent: Verification result
alt Verification failed & retries < max
ReactAgent->>ReactAgent: Enter bounded verification loop (up to 5 iterations)
loop Verification Retry Loop
ReactAgent->>LLMProvider: Call LLM with error context
LLMProvider-->>ReactAgent: Response
ReactAgent->>ToolExecutor: Execute corrective tools
ToolExecutor-->>ReactAgent: Tool results
ReactAgent->>Gates: Re-run verification
Gates-->>ReactAgent: Verification result
alt Verification passed
ReactAgent->>ReactAgent: Break retry loop
end
end
end
ReactAgent->>ReactAgent: Emit AGENT_COMPLETED or AGENT_FAILED
ReactAgent-->>Client: Return AgentStatus
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Add
|
Code Review: ReactAgent ImplementationThis is an excellent implementation of the ReAct agent core loop. The code is clean, well-structured, and follows the architectural plan from ✅ StrengthsArchitecture & Design
ReAct Loop Implementation
System Prompt (3-Layer Structure)
Verification Strategy
Test Coverage
🔍 Minor Issues & Suggestions1. Inconsistent Lines 107 and 230 use different
This is actually correct, but worth noting that 2. Missing per-edit lint in verification retry loop (Medium priority) The main Recommendation: Extract the lint logic into a helper method and reuse it in both loops: def _execute_tool_with_lint(self, tc: ToolCall) -> ToolResult:
"""Execute a tool call and append lint errors if it's an edit/create."""
result = execute_tool(tc, self.workspace.repo_path)
if tc.name in ("edit_file", "create_file") and not result.is_error:
lint_output = self._run_ruff_on_file(tc.input.get("path", ""))
if lint_output:
result = ToolResult(
tool_call_id=result.tool_call_id,
content=result.content + f"\n\nLINT ERRORS (must fix before continuing):\n{lint_output}",
is_error=result.is_error,
)
return resultThen use it in both loops. 3. History trimming could be more sophisticated (Low priority)
This is not critical for the initial implementation but worth considering for Phase E. 4. Test coverage gap: per-edit lint gate (Medium priority) No test explicitly verifies that lint errors are appended to tool results after edit_file/create_file. Consider adding: def test_per_edit_lint_appends_errors(self, workspace, provider, mock_context):
"""When edit_file produces lint errors, they should be appended to tool result."""
# Mock edit_file call that creates a file with lint errors
# Verify the tool result contains "LINT ERRORS (must fix before continuing)"5. Line 361 catches 🛡️ Security
📊 Performance
🎯 Adherence to CLAUDE.md Guidelines
📝 Documentation
✅ Final RecommendationAPPROVE with minor suggestions. This is high-quality code that correctly implements the ReAct architecture. The suggested improvements are all minor and can be addressed in follow-up PRs if desired. The implementation is ready to merge and unblocks Phase 2.5 progress. Great work! Suggested follow-ups (not blockers):
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@codeframe/core/react_agent.py`:
- Around line 90-188: Emit lifecycle and loop-state events using core/events.py:
in ReactAgent.run() fire a "agent.start" event at the beginning (include task_id
and workspace_id), and fire "agent.completed" or "agent.failed" with final
status and task_id on return/exception; inside _react_loop() emit
"agent.iteration.start" before each LLM call and "agent.iteration.end" after
processing the iteration (include task_id, workspace_id, iteration index, and
system_prompt summary), and for each tool dispatch in the loop emit
"agent.tool.dispatched" with tool name and tool_call id and "agent.tool.result"
with tool_call id, result.is_error, and any lint info; call the exporter
functions from core/events.py (import at top) so messages/emitted payloads
reference unique symbols like ReactAgent.run, ReactAgent._react_loop,
response.tool_calls, execute_tool, and ToolResult to ensure observability
without changing existing control flow.
- Around line 334-366: The _run_ruff_on_file function currently runs ruff but
doesn't emit verification diagnostics; import the events module and add event
emissions following the gates.py pattern: emit a "gate.started" (or equivalent)
event before calling subprocess.run with gate name like "ruff_check" and the
file path, then on completion emit a "gate.finished" event with passed=True and
no message when returncode == 0, and when returncode != 0 emit a failed
diagnostic with passed=False, include gate name "ruff_check", the captured
stdout/stderr as the error message, and any suggested fixes (e.g., hint to run
ruff --fix) as suggestions; also emit a failed diagnostic on exceptions
(subprocess.TimeoutExpired, FileNotFoundError) with appropriate error text and
passed=False. Ensure the import of events is added and use the same event
keys/fields used in gates.py so callers can consume these diagnostics.
- Fix _trim_messages to preserve first message (pop from index 1, not 0) - Extract _execute_tool_with_lint helper, use in both main loop and verification retry - Add path traversal protection in _run_ruff_on_file - Add tests for per-edit lint behavior and path safety
Updated Code Review: ReactAgent ImplementationI've reviewed the latest changes to this PR. Excellent work addressing the previous feedback! ✅ Issue Resolved Since Last ReviewPer-edit lint in verification retry loop - The code now correctly uses Current StateThe implementation is solid and ready to merge. All acceptance criteria from #347 are met:
Remaining Minor Suggestions (Non-Blocking)These are low-priority improvements that can be addressed in follow-up PRs:
CodeRabbit's Event Emission SuggestionCodeRabbit suggested adding event emissions for observability (agent lifecycle events, iteration tracking, tool dispatch events). This is a good architectural suggestion but should be done in a follow-up PR to:
Final RecommendationAPPROVE - This PR is ready to merge. The code is well-structured, thoroughly tested, and correctly implements the ReAct architecture per the spec. The minor suggestions above are optimizations that don't block merging. Great work on this implementation! 🚀 |
| while len(messages) > 3 and total > _MAX_HISTORY_CHARS: | ||
| removed = messages.pop(1) | ||
| total -= len(str(removed)) | ||
| if len(messages) > 1 and messages[1].get("role") == "user": | ||
| removed = messages.pop(1) | ||
| total -= len(str(removed)) |
There was a problem hiding this comment.
🟡 Medium
core/react_agent.py:390 The trimming logic assumes messages[1] after popping an assistant message will be its paired user message, but if the original messages[1] was a user message (not assistant), the pairing breaks. Consider checking the role before popping the first message, or always removing in fixed pairs.
- while len(messages) > 3 and total > _MAX_HISTORY_CHARS:
- removed = messages.pop(1)
- total -= len(str(removed))
- if len(messages) > 1 and messages[1].get("role") == "user":
- removed = messages.pop(1)
- total -= len(str(removed))
+ while len(messages) > 3 and total > _MAX_HISTORY_CHARS:
+ if messages[1].get("role") == "assistant":
+ removed = messages.pop(1)
+ total -= len(str(removed))
+ if len(messages) > 1 and messages[1].get("role") == "user":
+ removed = messages.pop(1)
+ total -= len(str(removed))
+ else:
+ # User message without assistant; remove single message
+ removed = messages.pop(1)
+ total -= len(str(removed))🚀 Want me to fix this? Reply ex: "fix it for me".
- Add 7 new EventType constants (AGENT_STARTED/COMPLETED/FAILED, ITERATION_STARTED/COMPLETED, TOOL_DISPATCHED/RESULT) - Emit agent.start/completed/failed in run() for lifecycle tracking - Emit iteration.start/end in _react_loop() with task_id, iteration index, and system prompt summary - Emit tool.dispatched/result per tool call with lint error flag - Emit GATES_STARTED/COMPLETED in _run_ruff_on_file() with diagnostics - Add _emit() helper that suppresses failures (observability never crashes the agent) - Add 5 tests verifying event emissions across success, failure, iteration, tool dispatch, and exception paths
Final Code Review: ReactAgent Implementation (Third Review)I've completed a comprehensive review of this PR. This implementation continues to be excellent and ready to merge. All previous feedback has been addressed. ✅ Implementation QualityArchitecture
Core Features Validated
Test Coverage
🎯 Adherence to Repository StandardsPer CLAUDE.md guidelines:
📋 Previous Review Items - All Resolved
💡 Minor Observations (Non-Blocking)These are minor refinement opportunities that don't block merging:
🔐 Security Review
📊 Performance Characteristics
✅ Final RecommendationAPPROVE and ready to merge. This is production-quality code that correctly implements the ReAct architecture per spec. All acceptance criteria from #347 are met, comprehensive tests pass, and the implementation follows all repository architectural guidelines. The minor observations listed above are optimization opportunities that can be addressed in future PRs if desired. They don't impact correctness or functionality. Excellent work on this implementation! 🚀 Summary of changes since last review:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/core/react_agent.py`:
- Around line 377-397: _trim_messages is leaving orphaned assistant messages
(tool_calls) because it pops at index 1 and can leave assistant+assistant
sequences; change the trimming strategy in _trim_messages so it preserves the
first complete pair (messages[0] and messages[1]) and only removes whole
assistant+user pairs starting at index 2 onward until total <=
_MAX_HISTORY_CHARS, i.e., compute the removal start index = 2 (or the first pair
boundary after the preserved pair), and in the loop always pop two messages
together (assistant then user) or skip a lone assistant by also removing its
following user if present, updating total accordingly to ensure no assistant is
left without its corresponding user/result.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@codeframe/core/react_agent.py`:
- Around line 177-215: The user message dict appended to messages omits the
required "content" field; update the code that builds and appends the user
message (the messages.append call that currently uses {"role": "user",
"tool_results": tool_results}) to include "content": "" so it matches the
Message.to_dict() protocol and mirrors assistant_msg; keep all other fields
(tool_results) as-is to ensure protocol compliance across adapters (refer to
assistant_msg, tool_results, messages and Message.to_dict()).
In `@tests/core/test_react_agent.py`:
- Around line 29-40: The workspace fixture is passing a string to
Workspace.created_at but the Workspace dataclass expects a datetime; update the
fixture (the workspace() function) to construct a datetime object (e.g., via
datetime.fromisoformat("2026-01-01T00:00:00+00:00")) and add the necessary
import from datetime at the top of the test file so Workspace(...,
created_at=...) receives a datetime instance.
🧹 Nitpick comments (1)
tests/core/test_react_agent.py (1)
431-439: Test assertion doesn't actually verify lint content was appended.The test creates a file with a lint error and calls edit_file, but then only verifies that
execute_toolwas called. It doesn't actually verify that lint errors were appended to the tool result content. The comment on lines 436-438 acknowledges this limitation, but this means the test doesn't fully validate the per-edit lint behavior it claims to test.Consider adding a more targeted unit test for
_execute_tool_with_lintthat mocks_run_ruff_on_fileto return known lint output and verifies the result content contains the lint errors.
…dex 2 Previous logic popped at index 1, which could orphan the assistant message at index 0 by removing its corresponding user/tool_results message. Now preserves the first complete pair (messages[0:2]) and removes whole assistant+user pairs starting from index 2 onward.
Code Review: ReactAgent Implementation (Fourth Review)I've reviewed the latest commit (e62cda7) which fixes the message trimming logic. This PR is production-ready and approved for merge. ✅ All Previous Issues Resolved1. Per-edit lint in verification retry ✅ 2. Event emissions ✅
3. Message trimming logic ✅ 📋 Implementation StatusAll acceptance criteria from #347 are complete:
🔍 Code Quality AssessmentArchitecture (Excellent)
ReAct Loop (Excellent)
System Prompt (Excellent)
Verification Strategy (Excellent)
Event System (Excellent)
Test Coverage (Excellent)
🛡️ Security Review
📊 Performance Characteristics
🎯 Adherence to Repository GuidelinesPer CLAUDE.md requirements:
💡 Non-Blocking ObservationsThese are minor refinement opportunities for future PRs: 1. Exception handling breadth (codeframe/core/react_agent.py:432-439) 2. History trimming optimization (lines 459-479) 3. Test enhancement ✅ Final RecommendationAPPROVED - Ready to merge immediately. This is production-quality code that correctly implements the ReAct architecture per the specification. All acceptance criteria from #347 are met, comprehensive tests pass with zero regressions, and the implementation follows all repository architectural guidelines. The code quality is exceptional:
The minor observations listed above are optimization opportunities that don't impact correctness or functionality. They can be addressed in future PRs if desired. Congratulations on this excellent implementation! 🚀 This unblocks Phase 2.5 progress and provides a solid foundation for the ReAct-based agent system. Summary of changes across all commits:
|
- Add "content": "" to user messages with tool_results in both main loop and verification retry loop to match Message.to_dict() protocol - Use datetime objects instead of ISO strings for Workspace.created_at and Task.created_at/updated_at in test fixtures to match type hints
Final Review: ReactAgent Implementation ✅I've completed my review of the latest commit (e62cda7). This PR is production-ready and approved for merge. SummaryThis is an excellent implementation of the ReAct agent architecture that correctly implements all acceptance criteria from #347. The code quality is exceptional, follows all repository architectural guidelines from CLAUDE.md, and includes comprehensive test coverage. ✅ All Implementation Requirements MetCore Features:
Test Coverage:
🎯 Architecture QualityAdherence to CLAUDE.md Guidelines:
Design Excellence:
🔒 Security Review
📋 Changes Since Previous ReviewsAll feedback from previous reviews has been addressed:
💡 Non-Blocking ObservationsThese are minor refinement opportunities for future PRs:
✅ Final RecommendationAPPROVED - Ready to merge immediately. 🚀 This is production-quality code that correctly implements the ReAct architecture per specification. All acceptance criteria are met, comprehensive tests pass with zero regressions, and the implementation follows all repository architectural guidelines. Congratulations on this excellent implementation! This unblocks Phase 2.5 progress and provides a solid foundation for the ReAct-based agent system. Review consistency note: This is my fifth review of this PR. All issues raised in previous reviews have been addressed. The minor observations listed above are optimization opportunities that don't impact correctness or functionality. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/core/react_agent.py`:
- Around line 389-439: The _run_ruff_on_file function has three issues: change
the passed computation to rely only on result.returncode == 0 (remove the
fallback on empty stdout), update exception handling (subprocess.TimeoutExpired
and FileNotFoundError) to emit EventType.GATES_COMPLETED with passed: False and
a diagnostics string describing the error (e.g., "ruff timed out" or "ruff not
found"), and add a suggestions field to every GATES_COMPLETED event payload with
actionable fixes (for failures include items like "run `ruff check <file>`
locally to see violations", "install ruff: `pip install ruff`", or "increase
timeout and re-run"); reference symbols: _run_ruff_on_file,
EventType.GATES_COMPLETED, result.returncode, subprocess.TimeoutExpired,
FileNotFoundError.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/core/test_react_agent.py`:
- Around line 393-444: The test
TestPerEditLint::test_lint_errors_appended_to_tool_result creates bad.py under
tmp_path so the lint runner (which runs relative to workspace.repo_path) may not
see it; update the test to create/write the file under workspace.repo_path
(e.g., join workspace.repo_path with "bad.py" when creating bad_file) and remove
the unused tmp_path fixture and any references to it so the lint runner will run
against the file in the workspace; ensure references to bad_file and its
contents remain the same and keep mock_exec_tool usage unchanged.
| class TestPerEditLint: | ||
| """Tests for per-edit lint gate behavior.""" | ||
|
|
||
| @patch("codeframe.core.react_agent.gates") | ||
| @patch("codeframe.core.react_agent.execute_tool") | ||
| @patch("codeframe.core.react_agent.ContextLoader") | ||
| def test_lint_errors_appended_to_tool_result( | ||
| self, mock_ctx_loader, mock_exec_tool, mock_gates, workspace, provider, mock_context, tmp_path | ||
| ): | ||
| """When edit_file produces a file with lint errors, _execute_tool_with_lint | ||
| should append the lint output to the tool result content.""" | ||
| from codeframe.core.react_agent import ReactAgent | ||
|
|
||
| # Create a Python file with a lint error in the workspace | ||
| bad_file = tmp_path / "bad.py" | ||
| bad_file.write_text("import os\n") # unused import → F401 | ||
|
|
||
| # LLM calls edit_file, then responds with text (done) | ||
| provider.add_tool_response( | ||
| [ToolCall(id="tc1", name="edit_file", input={"path": "bad.py", "edits": []})] | ||
| ) | ||
| provider.add_text_response("Done editing.") | ||
|
|
||
| mock_ctx_loader.return_value.load.return_value = mock_context | ||
|
|
||
| # execute_tool succeeds | ||
| mock_exec_tool.return_value = ToolResult( | ||
| tool_call_id="tc1", content="Edit applied." | ||
| ) | ||
|
|
||
| mock_gates.run.return_value = _gate_passed() | ||
|
|
||
| agent = ReactAgent(workspace=workspace, llm_provider=provider) | ||
| agent.run("task-1") | ||
|
|
||
| # Check that the LLM received tool results. The second call (text response) | ||
| # should have been preceded by a user message with tool_results. | ||
| second_call = provider.get_call(1) | ||
| messages = second_call["messages"] | ||
|
|
||
| # Find the user message with tool_results | ||
| user_msgs_with_results = [ | ||
| m for m in messages if m.get("tool_results") | ||
| ] | ||
| assert len(user_msgs_with_results) >= 1 | ||
|
|
||
| # The tool result content should include lint output if ruff found errors. | ||
| # Since ruff may or may not be installed in CI, we just verify the | ||
| # _execute_tool_with_lint method was used (execute_tool was called). | ||
| mock_exec_tool.assert_called_once() | ||
|
|
||
|
|
There was a problem hiding this comment.
Per-edit lint test doesn’t place the file inside the workspace.
The lint runner operates relative to workspace.repo_path, so creating bad.py under tmp_path risks bypassing the lint path entirely, making the test less representative. Create the file under workspace.repo_path (and drop the unused tmp_path fixture).
🔧 Suggested fix
- def test_lint_errors_appended_to_tool_result(
- self, mock_ctx_loader, mock_exec_tool, mock_gates, workspace, provider, mock_context, tmp_path
- ):
+ def test_lint_errors_appended_to_tool_result(
+ self, mock_ctx_loader, mock_exec_tool, mock_gates, workspace, provider, mock_context
+ ):
@@
- bad_file = tmp_path / "bad.py"
+ bad_file = workspace.repo_path / "bad.py"
bad_file.write_text("import os\n") # unused import → F401🤖 Prompt for AI Agents
In `@tests/core/test_react_agent.py` around lines 393 - 444, The test
TestPerEditLint::test_lint_errors_appended_to_tool_result creates bad.py under
tmp_path so the lint runner (which runs relative to workspace.repo_path) may not
see it; update the test to create/write the file under workspace.repo_path
(e.g., join workspace.repo_path with "bad.py" when creating bad_file) and remove
the unused tmp_path fixture and any references to it so the lint runner will run
against the file in the workspace; ensure references to bad_file and its
contents remain the same and keep mock_exec_tool usage unchanged.
- Use returncode == 0 only for passed (remove empty-stdout fallback that masked non-zero exits from config errors) - Split exception handling: TimeoutExpired emits passed=False with "ruff timed out", FileNotFoundError emits passed=False with "ruff not found" (was incorrectly reporting passed=True) - Add suggestions field to GATES_COMPLETED on failure/error with actionable next steps for each failure mode
Final Code Review: ReactAgent Implementation ✅I've reviewed the complete implementation across all commits. This PR is production-ready and approved for merge. ✅ Implementation SummaryThis is an excellent implementation of the ReAct agent architecture that correctly implements all acceptance criteria from #347. The code demonstrates: Core Architecture
Key Features Validated
Test Coverage
🎯 Adherence to Repository StandardsPer CLAUDE.md guidelines:
📋 Evolution Across ReviewsAll feedback from previous reviews has been successfully addressed:
💡 Non-Blocking ObservationsThese are minor refinement opportunities for future PRs (not blockers):
🔒 Security Review
✅ Final RecommendationAPPROVED - Ready to merge immediately. 🚀 This is production-quality code that correctly implements the ReAct architecture per specification. The code quality is exceptional with clean architecture, comprehensive event emission for observability, robust error handling, and well-structured test coverage. Congratulations on this excellent implementation! This unblocks Phase 2.5 progress and provides a solid foundation for the ReAct-based agent system. Review consistency note: This is a final consolidated review acknowledging all previous feedback. All issues raised in earlier reviews have been addressed. The minor observations above are optimization opportunities that don't impact correctness or functionality and can be addressed in future PRs if desired. |
1 participant
Summary
Implements #347: ReactAgent core loop with 3-layer system prompt builder.
ReactAgentclass incodeframe/core/react_agent.py— a ReAct-style agent that iterates LLM calls with tool executiontests/core/test_react_agent.pycovering all acceptance criteriaAgentclass (no modifications to existing code)What's Included
ReactAgent (
codeframe/core/react_agent.py, 393 lines)edit_file/create_fileand appends errors to tool resultsTest Suite (
tests/core/test_react_agent.py, 398 lines)10 tests using MockProvider:
Acceptance Criteria
ReactAgentclass withrun(task_id) → AgentStatusadapters/llm/base.pytypes (Tool, ToolCall, ToolResult)tools.py:execute_tool()Test plan
uv run pytest tests/core/test_react_agent.py -v— 10/10 passuv run pytest— 3827 passed, 23 skipped, 0 failures (no regressions)uv run ruff check— cleanCloses #347
Summary by CodeRabbit
New Features
Tests
Chores