test: add CLI integration tests for ReactAgent runtime parameters #369
Conversation
Exercise the full CLI → runtime → ReactAgent path with MockProvider to catch wiring issues that unit tests with mocked dependencies miss.
WalkthroughAdds CLI integration tests for the ReactAgent engine: introduces a module-level mock completion, imports runtime helpers, and a TestReactAgentIntegration class with tests for verbose mode, dry-run, and streaming output logging via the CLI→runtime→ReactAgent path. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
No actionable comments were generated in the recent review. 🎉 Comment |
Add CLI integration tests that verify ReactAgent runtime parameters in
|
Code ReviewSummaryThis PR adds 3 well-structured CLI integration tests for ReactAgent runtime parameters, closing the testing gap identified in PR #367. The tests follow established patterns and provide valuable end-to-end coverage. ✅ Strengths
💡 Suggestions1. Minor: Import Organization (Low Priority)The streaming test imports modules inline (lines 1039, 1044): from codeframe.core import runtime
from codeframe.core.streaming import run_output_existsRecommendation: Move to top-level imports for consistency with file conventions (see line 21: Rationale: While inline imports work fine and may have been used to avoid pytest collection issues, the file already imports from 2. Enhancement Opportunity: Verify Follow Output Content (Optional)The streaming test verifies that Suggestion: assert follow_result.exit_code == 0, f"follow failed: {follow_result.output}"
# Optional: Verify follow actually shows the agent output
assert "Task analysis complete" in follow_result.output or len(follow_result.output) > 0Rationale: This would ensure 3. Test Independence ConsiderationAll three tests use the same Recommendation: Verify tests pass when run in isolation and in different orders: pytest tests/cli/test_v2_cli_integration.py::TestReactAgentIntegration::test_react_dry_run -v
pytest tests/cli/test_v2_cli_integration.py::TestReactAgentIntegration -v --random-orderNote: If 🔒 No Issues Found
📋 Checklist
VerdictLGTM ✅ with minor optional enhancements suggested above. The tests provide solid end-to-end coverage of ReactAgent runtime parameters through the CLI path. The implementation is clean, well-documented, and follows established patterns. The adaptation from the original plan (dry-run expecting success rather than failure) shows good judgment based on PR #367's implementation. Reviewed according to CLAUDE.md Phase 2.5-C guidelines |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/cli/test_v2_cli_integration.py`:
- Around line 1040-1046: list_runs returns runs ordered newest-first, so change
the test to pick the first element instead of the last: replace uses of runs[-1]
with runs[0] (and consider renaming last_run to recent_run) where
runtime.list_runs(ws, task_id=task_list[0].id) is consumed before calling
run_output_exists(ws, <run_id>), ensuring you assert the output exists for the
most recent run.
Captures mock_llm return value and asserts call_count >= 1 in all three tests, matching the existing pattern in TestAIAgentExecution.
Follow-up ReviewI've reviewed the PR again and noticed CodeRabbit identified a bug in the streaming test that I missed in my initial review. Bug Found by CodeRabbitIssue: Line 1045 uses runs[-1] to get the "last" run, but list_runs() returns runs newest-first (confirmed in runtime.py:287 and SQL query ORDER BY started_at DESC at line 307). Impact: The test is checking the oldest run instead of the most recent one, which could cause false positives/negatives. Fix needed in test_react_streaming_output_log: Change line 1045 from: To: Then update line 1048 to use recent_run.id instead of last_run.id. Updated VerdictRequires fix before merge. Once the runs[-1] to runs[0] change is made, this will be a solid addition. All other aspects of the implementation remain excellent. Credit to CodeRabbit for catching this indexing issue. Previous feedback (import organization, output content verification, test independence) remains as optional suggestions - the indexing bug is the only blocking issue. |
- Fix runs[-1] → runs[0]: list_runs returns newest first (CodeRabbit) - Move inline imports to top-level for consistency (Claude) - Assert follow output is non-empty to verify content display (Claude) - Rename last_run → latest_run for clarity
Final Review - All Feedback Addressed ✅I've reviewed the updated PR after all three commits, and I'm pleased to confirm that all previous review feedback has been properly addressed. Changes AppliedCommit 1 (3085515): Initial implementation
Commit 2 (9b27929): CodeRabbit feedback
Commit 3 (528e91a): Combined feedback from CodeRabbit and Claude
Code Quality AssessmentStrengths:
Security: ✅ No concerns - uses test fixtures and mock providers VerificationThe fixes correctly address:
VerdictAPPROVED ✅ - Ready to merge This PR delivers solid end-to-end testing for ReactAgent runtime parameters. The implementation is clean, well-documented, and properly responds to review feedback. Excellent work on the iterative improvements! Closes #368 Review iteration 2/2 - all feedback addressed |
1 participant
Summary
Implements #368: adds 3 CLI integration tests exercising the full
CLI → runtime → ReactAgentpath with MockProvider.--verbose --engine reactproduces[ReactAgent]prefixed output--dry-run --engine reactcompletes successfully (PR feat: extend ReactAgent interface to support full runtime parameters (#362) #367 added full dry_run support)output.logvia RunOutputLogger andcf work followreads itTest plan
TestReactAgentIntegration)CliRunnerwithMockProvider(no real LLM calls)@pytest.mark.v2via file-levelpytestmarkImplementation notes
dry_runsupport to ReactAgentCloses #368
Summary by CodeRabbit