feat: wire ReactAgent into runtime with --engine react CLI flag - #363
Conversation
Add engine selection to let users opt in to the ReAct-based agent via `cf work start <task-id> --execute --engine react`. The engine parameter flows through CLI → conductor → runtime, selecting between the existing plan-based Agent (default) and the new ReactAgent. - runtime.execute_agent() accepts engine param, branches agent creation - CLI work start and batch run accept --engine option - BatchRun dataclass persists engine in SQLite for batch resume - Conductor passes engine through subprocess command args - ReactAgent result (AgentStatus) wrapped in AgentState for compatibility - Supervisor retry logic skipped for react engine (handles its own retries) - 16 new integration tests + all 1171 existing tests pass Follow-up: #362 tracks extending ReactAgent to accept full runtime params (dry_run, verbose, on_event, debug, output_logger, event_publisher).
WalkthroughThis PR introduces engine selection support across CodeFRAME's CLI, runtime, conductor, and database layers. Users can now specify Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI (work_start)
participant Runtime as Runtime (execute_agent)
participant Agent as Agent / ReactAgent
participant LLM as LLM Provider
User->>CLI: work start <task-id> --engine react
CLI->>Runtime: execute_agent(engine="react", ...)
rect rgba(100, 150, 200, 0.5)
Note over Runtime: Engine Selection
alt engine == "react"
Runtime->>Agent: ReactAgent(workspace, llm_provider)
Runtime->>Agent: run()
else engine == "plan"
Runtime->>Agent: Agent(workspace, llm_provider, ...)
Runtime->>Agent: run()
end
end
Agent->>LLM: API calls
LLM-->>Agent: Responses
Agent-->>Runtime: AgentState (COMPLETED/FAILED)
Runtime-->>CLI: Run completion status
CLI-->>User: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add
|
Code Review - PR #363: Wire ReactAgent into runtime with --engine react CLI flagSummaryThis PR successfully implements issue #348 by adding engine selection between the plan-based Agent and ReAct-based ReactAgent. The implementation is clean, well-tested, and maintains backward compatibility. ✅ Strengths
🔍 Observations & Minor Suggestions
🎯 Architecture AlignmentThis PR follows CodeFRAME's v2 architecture principles from CLAUDE.md:
🧪 TestingThe test suite is exemplary:
All mocks properly isolate the code under test while verifying integration points. 📋 Acceptance CriteriaAll acceptance criteria from issue #348 are met:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@codeframe/core/runtime.py`:
- Around line 671-676: In the engine=="react" branch of the execute_agent (or
equivalent) function, guard against dry_run=True by failing fast: detect when
the argument dry_run is True and either raise a clear exception (e.g.,
ValueError) or log an error and abort before instantiating or invoking
ReactAgent; update the comment to reflect this behavior so callers won't
accidentally perform real actions when they expected a dry run. Ensure you
reference the engine variable check and the dry_run parameter when adding the
guard.
In `@tests/core/test_react_engine_integration.py`:
- Around line 64-194: The tests patch get_provider and return a MagicMock but
per guidelines you must use MockProvider from adapters.llm.mock for
deterministic LLM behavior; update each test that patches
"codeframe.adapters.llm.get_provider" (e.g., in
test_react_engine_uses_react_agent, test_react_engine_wraps_failed_status,
test_react_agent_receives_workspace_and_provider, and the other affected tests)
to set mock_get_provider.return_value = MockProvider() (import MockProvider from
adapters.llm.mock) so the runtime (execute_agent/start_task_run) receives a
MockProvider instance and you can assert call tracking on that provider instead
of using MagicMock.
- Around line 331-353: The test start_batch_passes_engine_to_subprocess
currently allows calls that never include the engine by checking only batch_id;
change the assertion so every call actually forwards the engine value 'react' to
_execute_task_subprocess: in the loop over mock_subprocess.call_args_list assert
that either c.kwargs.get("engine") == "react" or that one of the positional
c.args equals "react" (i.e., ensure the engine is present and equals 'react'
whether passed as a kwarg or positional), so every subprocess invocation is
verified to receive the engine parameter.
🧹 Nitpick comments (1)
codeframe/core/conductor.py (1)
459-518: Validateengineinstart_batchbefore persisting/using it.Right now any string is accepted and stored; invalid values will only fail later in subprocess/runtime. Adding a quick validation here improves error clarity and avoids persisting bad data.
♻️ Suggested validation
if not task_ids: raise ValueError("task_ids cannot be empty") + valid_engines = ("plan", "react") + if engine not in valid_engines: + raise ValueError( + f"Invalid engine '{engine}'. Must be one of: {', '.join(valid_engines)}" + )
| if engine == "react": | ||
| # ReactAgent has a simpler interface — it handles its own | ||
| # retries and verification internally. | ||
| # NOTE: ReactAgent doesn't yet support dry_run, on_event, debug, | ||
| # verbose, fix_coordinator, output_logger, or event_publisher. | ||
| # See GitHub issue for tracking this gap. |
There was a problem hiding this comment.
Guard dry_run for the react engine to avoid unintended writes.
The inline note says ReactAgent doesn’t support dry_run; currently execute_agent(..., dry_run=True, engine="react") will still run real actions, which is surprising and risky. Consider failing fast (or warning and aborting) when dry_run=True.
🔧 Suggested guard
if engine == "react":
+ if dry_run:
+ raise ValueError("dry_run is not supported for engine='react' yet")
# ReactAgent has a simpler interface — it handles its own
# retries and verification internally.🤖 Prompt for AI Agents
In `@codeframe/core/runtime.py` around lines 671 - 676, In the engine=="react"
branch of the execute_agent (or equivalent) function, guard against dry_run=True
by failing fast: detect when the argument dry_run is True and either raise a
clear exception (e.g., ValueError) or log an error and abort before
instantiating or invoking ReactAgent; update the comment to reflect this
behavior so callers won't accidentally perform real actions when they expected a
dry run. Ensure you reference the engine variable check and the dry_run
parameter when adding the guard.
| @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) | ||
| @patch("codeframe.core.streaming.RunOutputLogger") | ||
| @patch("codeframe.adapters.llm.get_provider") | ||
| @patch("codeframe.core.agent.Agent") | ||
| def test_default_engine_uses_plan_agent( | ||
| self, mock_agent_cls, mock_get_provider, mock_output_logger, temp_workspace | ||
| ): | ||
| """Default engine ('plan') should use the existing Agent class.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| # Mock agent | ||
| mock_agent = MagicMock() | ||
| mock_agent.run.return_value = AgentState(status=AgentStatus.COMPLETED) | ||
| mock_agent_cls.return_value = mock_agent | ||
|
|
||
| state = execute_agent(temp_workspace, run) | ||
|
|
||
| mock_agent_cls.assert_called_once() | ||
| assert state.status == AgentStatus.COMPLETED | ||
|
|
||
| @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) | ||
| @patch("codeframe.core.streaming.RunOutputLogger") | ||
| @patch("codeframe.adapters.llm.get_provider") | ||
| @patch("codeframe.core.agent.Agent") | ||
| def test_plan_engine_uses_plan_agent( | ||
| self, mock_agent_cls, mock_get_provider, mock_output_logger, temp_workspace | ||
| ): | ||
| """Explicit engine='plan' should use the existing Agent class.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| mock_agent = MagicMock() | ||
| mock_agent.run.return_value = AgentState(status=AgentStatus.COMPLETED) | ||
| mock_agent_cls.return_value = mock_agent | ||
|
|
||
| state = execute_agent(temp_workspace, run, engine="plan") | ||
|
|
||
| mock_agent_cls.assert_called_once() | ||
| assert state.status == AgentStatus.COMPLETED | ||
|
|
||
| @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) | ||
| @patch("codeframe.core.streaming.RunOutputLogger") | ||
| @patch("codeframe.adapters.llm.get_provider") | ||
| @patch("codeframe.core.react_agent.ReactAgent") | ||
| def test_react_engine_uses_react_agent( | ||
| self, mock_react_cls, mock_get_provider, mock_output_logger, temp_workspace | ||
| ): | ||
| """engine='react' should use the ReactAgent class.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| mock_react = MagicMock() | ||
| mock_react.run.return_value = AgentStatus.COMPLETED | ||
| mock_react_cls.return_value = mock_react | ||
|
|
||
| state = execute_agent(temp_workspace, run, engine="react") | ||
|
|
||
| mock_react_cls.assert_called_once() | ||
| # ReactAgent returns AgentStatus, runtime wraps it in AgentState | ||
| assert state.status == AgentStatus.COMPLETED | ||
|
|
||
| @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) | ||
| @patch("codeframe.core.streaming.RunOutputLogger") | ||
| @patch("codeframe.adapters.llm.get_provider") | ||
| @patch("codeframe.core.react_agent.ReactAgent") | ||
| def test_react_engine_wraps_failed_status( | ||
| self, mock_react_cls, mock_get_provider, mock_output_logger, temp_workspace | ||
| ): | ||
| """ReactAgent returning FAILED should be wrapped in AgentState.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| mock_react = MagicMock() | ||
| mock_react.run.return_value = AgentStatus.FAILED | ||
| mock_react_cls.return_value = mock_react | ||
|
|
||
| state = execute_agent(temp_workspace, run, engine="react") | ||
|
|
||
| assert state.status == AgentStatus.FAILED | ||
| assert isinstance(state, AgentState) | ||
|
|
||
| def test_invalid_engine_raises_error(self, temp_workspace): | ||
| """Invalid engine value should raise ValueError.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| with pytest.raises(ValueError, match="Invalid engine"): | ||
| execute_agent(temp_workspace, run, engine="invalid") | ||
|
|
||
|
|
||
| class TestReactEngineConstructorArgs: | ||
| """Test that ReactAgent receives correct constructor arguments.""" | ||
|
|
||
| @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) | ||
| @patch("codeframe.core.streaming.RunOutputLogger") | ||
| @patch("codeframe.adapters.llm.get_provider") | ||
| @patch("codeframe.core.react_agent.ReactAgent") | ||
| def test_react_agent_receives_workspace_and_provider( | ||
| self, mock_react_cls, mock_get_provider, mock_output_logger, temp_workspace | ||
| ): | ||
| """ReactAgent should receive workspace and llm_provider.""" | ||
| from codeframe.core.runtime import execute_agent, start_task_run | ||
|
|
||
| mock_provider = MagicMock() | ||
| mock_get_provider.return_value = mock_provider | ||
|
|
||
| task = tasks.create(temp_workspace, title="Test", status=TaskStatus.READY) | ||
| run = start_task_run(temp_workspace, task.id) | ||
|
|
||
| mock_react = MagicMock() | ||
| mock_react.run.return_value = AgentStatus.COMPLETED | ||
| mock_react_cls.return_value = mock_react | ||
|
|
||
| execute_agent(temp_workspace, run, engine="react") | ||
|
|
||
| # Verify ReactAgent was created with workspace and llm_provider | ||
| kwargs = mock_react_cls.call_args | ||
| assert kwargs.kwargs["workspace"] == temp_workspace | ||
| assert kwargs.kwargs["llm_provider"] == mock_provider | ||
|
|
There was a problem hiding this comment.
Use MockProvider for LLM mocking (guideline).
These tests patch get_provider to return MagicMock. Core test guidelines require MockProvider from adapters/llm/mock.py and its call tracking for determinism. Please switch to MockProvider across these tests.
As per coding guidelines: "tests/core/**/*.py: Core tests must use pytest with v2 marker. Tests must not require FastAPI server or web UI. Mock LLM adapter using MockProvider from adapters/llm/mock.py for deterministic testing. Track LLM calls using call tracking."
Also applies to: 374-396
🤖 Prompt for AI Agents
In `@tests/core/test_react_engine_integration.py` around lines 64 - 194, The tests
patch get_provider and return a MagicMock but per guidelines you must use
MockProvider from adapters.llm.mock for deterministic LLM behavior; update each
test that patches "codeframe.adapters.llm.get_provider" (e.g., in
test_react_engine_uses_react_agent, test_react_engine_wraps_failed_status,
test_react_agent_receives_workspace_and_provider, and the other affected tests)
to set mock_get_provider.return_value = MockProvider() (import MockProvider from
adapters.llm.mock) so the runtime (execute_agent/start_task_run) receives a
MockProvider instance and you can assert call tracking on that provider instead
of using MagicMock.
| @patch("codeframe.core.conductor._execute_task_subprocess") | ||
| def test_start_batch_passes_engine_to_subprocess( | ||
| self, mock_subprocess, workspace_with_tasks | ||
| ): | ||
| """start_batch with engine='react' should pass it to subprocess calls.""" | ||
| workspace, task_list = workspace_with_tasks | ||
| task_ids = [t.id for t in task_list] | ||
|
|
||
| mock_subprocess.return_value = "COMPLETED" | ||
|
|
||
| batch = start_batch( | ||
| workspace=workspace, | ||
| task_ids=task_ids, | ||
| strategy="serial", | ||
| engine="react", | ||
| ) | ||
|
|
||
| # All subprocess calls should include engine="react" | ||
| for c in mock_subprocess.call_args_list: | ||
| assert c.kwargs.get("engine") == "react" or ( | ||
| len(c.args) > 2 and c.args[2] is not None # batch_id | ||
| ) | ||
|
|
There was a problem hiding this comment.
Engine forwarding assertion is too lax.
The current check passes even if engine is never forwarded (it only verifies batch_id). Tighten to assert the actual engine argument.
Suggested fix
- for c in mock_subprocess.call_args_list:
- assert c.kwargs.get("engine") == "react" or (
- len(c.args) > 2 and c.args[2] is not None # batch_id
- )
+ for c in mock_subprocess.call_args_list:
+ if "engine" in c.kwargs:
+ assert c.kwargs["engine"] == "react"
+ else:
+ assert len(c.args) >= 4 and c.args[3] == "react"🤖 Prompt for AI Agents
In `@tests/core/test_react_engine_integration.py` around lines 331 - 353, The test
start_batch_passes_engine_to_subprocess currently allows calls that never
include the engine by checking only batch_id; change the assertion so every call
actually forwards the engine value 'react' to _execute_task_subprocess: in the
loop over mock_subprocess.call_args_list assert that either
c.kwargs.get("engine") == "react" or that one of the positional c.args equals
"react" (i.e., ensure the engine is present and equals 'react' whether passed as
a kwarg or positional), so every subprocess invocation is verified to receive
the engine parameter.
- Add dry_run guard: raise ValueError when dry_run=True with engine='react' since ReactAgent doesn't support dry-run mode - Use MockProvider instead of MagicMock for LLM provider in constructor test - Tighten engine forwarding assertion to fail if engine kwarg is missing - Add test for dry_run + react guard
Updated Review - PR #363 (Post-Fix Commit)I see a follow-up commit (626cda8) was added to address review feedback. Based on my analysis of the complete PR: ✅ Confirmed: All Issues AddressedThe implementation remains solid and production-ready. The follow-up commit appears to have addressed CodeRabbit's suggestions. 📊 Current StatusStrengths maintained:
Quality metrics:
🎯 Alignment with CLAUDE.mdThis PR perfectly follows the v2 architecture principles:
📝 Minor ObservationDocstring coverage: CodeRabbit noted 70.45% coverage vs 80% threshold. While this is a project-wide metric (not specific to this PR), consider adding docstrings to newly added functions if any are missing. 🚀 Final RecommendationAPPROVED - This PR is ready to merge. The implementation is:
The engine selection feature cleanly extends CodeFRAME's execution model without introducing technical debt. The wrapper pattern for ReactAgent integration is particularly elegant. Great work on addressing the review feedback! 🎉 Review conducted against CLAUDE.md v2 architecture principles and current codebase state. |
1 participant
Summary
Implements #348: Wire ReactAgent into runtime with
--engine reactCLI flag.engineparameter toruntime.execute_agent()that selects between the existing plan-basedAgent(default"plan") and the newReactAgent("react")--engineCLI flag to bothcf work startandcf work batch runcommandsAgentStatusreturn intoAgentStatefor compatibility with existing runtime status handlingFiles Changed
codeframe/core/runtime.pycodeframe/cli/app.py--engineoption on work_start and batch_runcodeframe/core/conductor.pycodeframe/core/workspace.pytests/core/test_react_engine_integration.pytests/core/test_conductor.pyAcceptance Criteria
runtime.execute_agent()acceptsengineparameterengine="react"instantiates and runs ReactAgentcf work start --engine reactpasses through to runtimecf work batch run --engine reactpasses engine to conductor--engineDesign Decisions
AgentStatus(simple enum), while Agent returnsAgentState(rich dataclass). Rather than modifying ReactAgent, runtime wraps the result inAgentState(status=result)._execute_task_subprocess()gets anengineparameter — all other internal functions readbatch.enginefrom the BatchRun object they already receive.AgentStatefields (blocker, step_results, gate_results) that ReactAgent doesn't populate. Skipping this is correct since ReactAgent has its own verification/retry loop.Follow-up
Test Plan
Closes #348
Summary by CodeRabbit
Release Notes