fix(phase1b): make CLI backend tests actually run#1532
Conversation
Problem: After PR #1531 merged, 19/34 shipped unit tests for the Phase 1b CLI Backend Protocol were failing because they were written against fabricated APIs that do not exist on this codebase: - test_yaml_cli_backend.py: used AgentsGenerator(config=...) and .create_agents_and_tasks(); neither the kwarg nor the method exist. - test_cli_backend_flag.py, test_external_agents_backcompat.py: called PraisonAI().parse_args() which deliberately short-circuits to parse_known_args([]) when PYTEST_CURRENT_TEST is in env, so patched sys.argv was ignored and --help never raised SystemExit. The Phase 1b *feature* works end-to-end (verified via manual CLI smoke: 'praisonai backends list' prints claude-code, --cli-backend flag accepts registered ids, mutual exclusion fires). The tests, however, were dead weight; CI just skipped them. Fix: 1. Extract the YAML cli_backend resolution block (23 lines) from agents_generator.py into a new module-level helper _resolve_yaml_cli_backend(cli_backend_config, logger). Zero behaviour change for existing callers; PraisonAgent(..., cli_backend=...) wiring is unchanged. 2. Rewrite test_yaml_cli_backend.py to call that helper directly (8 tests). Covers: string id, dict+overrides, missing id, None, import error, unknown id, invalid type, plus a real end-to-end resolve against the registered claude-code backend. 3. Rewrite test_cli_backend_flag.py to drive the CLI via subprocess (7 tests), stripping PYTEST_* env vars so parse_args does not short-circuit. Covers: --cli-backend in --help, registered backend accepted, unknown backend rejected, mutual exclusion with --external-agent, backends list subcommand (bare and with 'list'), and unknown subcommand reporting. 4. Rewrite test_external_agents_backcompat.py as a mix of direct imports (ExternalAgentsHandler, ClaudeCodeIntegration) and subprocess argparse checks (6 tests). Guards against regression of --external-agent {claude,gemini,codex,cursor} and --external-agent-direct. Result: 34/34 tests pass locally (6 backcompat + 7 CLI + 8 YAML + 13 existing claude-backend). No production code behaviour change - only test suite + a small helper extraction in agents_generator.py.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracted inline YAML Changes
Sequence Diagram(s)(omitted — changes are a localized refactor and test adjustments; no new multi-component sequential flow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI backend resolution logic by introducing a module-level helper function, _resolve_yaml_cli_backend, which improves testability and code reuse. The test suite has been significantly updated to use subprocess-based integration tests for the CLI surface, ensuring that the --cli-backend and legacy --external-agent flags behave correctly in a real environment. Additionally, the PR maintains backward compatibility for existing external agent integrations. I have no feedback to provide as the changes are well-structured and the test coverage is robust.
![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
🧹 Nitpick comments (2)
src/praisonai/tests/unit/test_external_agents_backcompat.py (1)
13-36: Reasonable subprocess harness; path assumptions are coupled to the current layout.The 4-level
REPO_ROOTwalk and hard-codedsrc/praisonai-agents+src/praisonaiPYTHONPATH entries will silently break if this test file is moved or if either sibling package is renamed. That's acceptable pragmatism for a phase-1b fix, but a conftest-level fixture (e.g., computingREPO_ROOTviapytest.rootdirand reusing_build_env/_run_clibetween this file andtest_cli_backend_flag.py) would avoid the duplication you now have across both files. Happy to send a follow-up that lifts these helpers into a shared fixture if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_external_agents_backcompat.py` around lines 13 - 36, The REPO_ROOT, _build_env, and _run_cli helpers duplicate logic and hard-code path walks; refactor them into a shared pytest fixture in conftest.py that computes the repository root via pytest.rootdir (or request.config.rootdir) and exposes a reusable CLI runner/env builder to tests instead of the local REPO_ROOT/_build_env/_run_cli functions in test_external_agents_backcompat.py and test_cli_backend_flag.py; update those tests to use the new fixture to build PYTHONPATH entries (for praisonai-agents and praisonai) and to run the CLI, removing the four-level os.path walk and duplicate helper code.src/praisonai/tests/unit/test_cli_backend_flag.py (1)
58-72: Minor docstring/comment drift vs. actual args.Lines 60-62 describe passing "an unrealistic prompt that won't trigger LLM work" and relying on the timeout to abort, but the argv here is
--cli-backend claude-code --help—--helpshort-circuits argparse and exits 0 before any LLM/prompt path runs, so thetry/except TimeoutExpired+pytest.skipare effectively dead branches. Either drop the skip path or switch to a non---helpargv that actually exercises the short timeout. Keeping--helpis fine; just realign the comment.📝 Proposed comment tweak
- # We intentionally pass an unrealistic prompt that won't trigger LLM work and - # rely on the default timeout to abort quickly. We only assert that argparse - # accepts the flag (no "invalid choice" or "unrecognized" in stderr). - try: - r = _run_cli( - "--cli-backend", "claude-code", "--help", - timeout=15, - ) - except subprocess.TimeoutExpired: - pytest.skip("CLI startup exceeded timeout; unrelated to flag parsing") + # Use --help to short-circuit argparse; we only want to prove the flag+value + # is accepted (no "invalid choice" / "unrecognized" in stderr). + r = _run_cli("--cli-backend", "claude-code", "--help", timeout=15)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_cli_backend_flag.py` around lines 58 - 72, The test test_cli_backend_flag_accepts_registered_backend uses "--help" which short-circuits argparse, so the try/except TimeoutExpired and pytest.skip are dead code; update the test by removing the try/except/pytest.skip block and its timeout argument (used with _run_cli), and adjust the docstring/comment to state that "--cli-backend claude-code --help" is used to exercise argparse only (no LLM run), or alternatively replace "--help" with a realistic prompt (and keep the timeout/skip) if you want to exercise runtime behavior; locate the test function test_cli_backend_flag_accepts_registered_backend and the _run_cli call to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Around line 156-176: Pre-seed the local variable `label` from
`cli_backend_config` before attempting `from praisonai.cli_backends import
resolve_cli_backend` so the `except ImportError` log can show the requested
backend; specifically, set `label = cli_backend_config` when it's a string and
when it's a dict set `label = cli_backend_config.get('id') or "<missing>"` (or
fallback to the type name) before the import, then keep the existing branches
that call `resolve_cli_backend` and the `logger.warning` in the `except` to
reference that pre-populated `label`.
---
Nitpick comments:
In `@src/praisonai/tests/unit/test_cli_backend_flag.py`:
- Around line 58-72: The test test_cli_backend_flag_accepts_registered_backend
uses "--help" which short-circuits argparse, so the try/except TimeoutExpired
and pytest.skip are dead code; update the test by removing the
try/except/pytest.skip block and its timeout argument (used with _run_cli), and
adjust the docstring/comment to state that "--cli-backend claude-code --help" is
used to exercise argparse only (no LLM run), or alternatively replace "--help"
with a realistic prompt (and keep the timeout/skip) if you want to exercise
runtime behavior; locate the test function
test_cli_backend_flag_accepts_registered_backend and the _run_cli call to
implement the change.
In `@src/praisonai/tests/unit/test_external_agents_backcompat.py`:
- Around line 13-36: The REPO_ROOT, _build_env, and _run_cli helpers duplicate
logic and hard-code path walks; refactor them into a shared pytest fixture in
conftest.py that computes the repository root via pytest.rootdir (or
request.config.rootdir) and exposes a reusable CLI runner/env builder to tests
instead of the local REPO_ROOT/_build_env/_run_cli functions in
test_external_agents_backcompat.py and test_cli_backend_flag.py; update those
tests to use the new fixture to build PYTHONPATH entries (for praisonai-agents
and praisonai) and to run the CLI, removing the four-level os.path walk and
duplicate helper code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24e94e28-ac67-47b9-bf30-7ba0aa762ec9
📒 Files selected for processing (4)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/tests/unit/test_cli_backend_flag.pysrc/praisonai/tests/unit/test_external_agents_backcompat.pysrc/praisonai/tests/unit/test_yaml_cli_backend.py
Greptile SummaryThis PR fixes 19 silently-skipped unit tests from PR #1531 by (a) extracting a module-level Confidence Score: 5/5Safe to merge — production code is unchanged and all remaining findings are P2 style/maintenance suggestions. All four changed files have clear, correct logic. The one production-code change (_resolve_yaml_cli_backend extraction) is a pure refactor with no behavior change on the hot path. The two P2 findings (helper duplication, one live-registry dependency in a test) do not affect correctness or reliability of the shipped feature. No files require special attention for merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["AgentsGenerator.generate_crew_and_kickoff()"] -->|"details.get('cli_backend')"| B["_resolve_yaml_cli_backend(config, logger)"]
B --> C{config type?}
C -->|None / falsy| D["return None"]
C -->|str| E["resolve_cli_backend(id)"]
C -->|dict| F{has 'id' key?}
C -->|other| G["raise ValueError"]
F -->|yes| H["resolve_cli_backend(id, overrides=overrides or {})"]
F -->|no| I["raise ValueError"]
E -->|success| J["return backend instance"]
H -->|success| J
G -->|caught| K["logger.warning → return None"]
I -->|caught| K
E -->|ImportError| L["logger.warning → return None"]
H -->|ImportError| L
E -->|Exception| K
H -->|Exception| K
J --> M["PraisonAgent(cli_backend=backend)"]
D --> N["PraisonAgent(cli_backend=None)"]
K --> N
L --> N
Reviews (3): Last reviewed commit: "docs(tests): add inline comments documen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job ✅ Final Architecture Review CompleteConducting comprehensive review per AGENTS.md guidelines and addressing all reviewer feedback. Review Tasks:
Phase 1: Architecture Review
Phase 2: Fix Valid Issues ✅ COMPLETED
Phase 3: Final Verdict ✅ APPROVED 🏆 Final Architectural Approval✅ APPROVED FOR MERGE This PR successfully addresses the silent test failures from PR #1531 while maintaining full compliance with AGENTS.md architecture guidelines. 📋 Architecture Compliance Summary✅ Protocol-Driven Core
✅ Backward Compatible
✅ Performance
✅ Agent-Centric Design
🔧 Issues FixedApplied fixes for all valid reviewer feedback:
🎯 What This PR Achieves
🚀 RecommendationAPPROVE AND MERGE - This PR represents excellent engineering:
The Phase 1b CLI backend feature is now properly tested and ready for production use. |
- Fix ImportError path to preserve requested backend ID in log messages - Remove dead try/except code in --help test - Fix trailing PYTHONPATH separator issue in test environments - Add missing returncode assertion for error cases Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/praisonai/tests/unit/test_external_agents_backcompat.py (2)
13-39: Optional: hoistREPO_ROOT,_build_env, and_run_cliinto a sharedconftest.py.The identical helpers live in
src/praisonai/tests/unit/test_cli_backend_flag.py. Extracting them to aconftest.py(or a small_cli_subprocess.pyhelper module) eliminates the duplication and keeps PYTHONPATH/PYTEST_* handling consistent if future tests join the subprocess pattern. Not a blocker.♻️ Proposed conftest extraction
# src/praisonai/tests/unit/conftest.py import os import subprocess import sys import pytest REPO_ROOT = os.path.abspath( os.path.join(os.path.dirname(__file__), "..", "..", "..", "..") ) def _build_env(): env = {k: v for k, v in os.environ.items() if not k.startswith("PYTEST_")} parts = [ os.path.join(REPO_ROOT, "src", "praisonai-agents"), os.path.join(REPO_ROOT, "src", "praisonai"), ] if env.get("PYTHONPATH"): parts.append(env["PYTHONPATH"]) env["PYTHONPATH"] = os.pathsep.join(parts) return env `@pytest.fixture` def run_cli(): def _run(*args, timeout=30): return subprocess.run( [sys.executable, "-m", "praisonai.cli.main", *args], capture_output=True, text=True, timeout=timeout, cwd=REPO_ROOT, env=_build_env(), ) return _run🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_external_agents_backcompat.py` around lines 13 - 39, Move the duplicated test helpers into a shared test fixture/module: hoist REPO_ROOT, _build_env, and _run_cli into a new src/praisonai/tests/unit/conftest.py (or a small helper module) and export a pytest fixture (e.g., run_cli) that wraps the existing _run_cli behavior; replace direct uses of _build_env/_run_cli in test_external_agents_backcompat.py and test_cli_backend_flag.py with the new fixture to remove duplication and ensure consistent PYTHONPATH/PYTEST_* handling across tests.
86-90: Minor: consider asserting the expected argparse error text explicitly.
assert "invalid choice" in r.stderr or "notreal" in r.stderrwill also pass on unrelated failures that happen to echonotreal(e.g., a crash before argparse). Tightening to require"invalid choice"(and optionally the value) makes the test strictly validate argparse rejection rather than generic failure modes. Same pattern applies totest_cli_backend_flag_rejects_unknown_backendin the sibling file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_external_agents_backcompat.py` around lines 86 - 90, The test test_external_agent_rejects_unknown_value currently uses a loose assertion that may pass for unrelated failures; change its assertion to explicitly require argparse's rejection text by asserting that "invalid choice" is in r.stderr (optionally also assert "notreal" appears) instead of the disjunction. Apply the same tightening to the sibling test_cli_backend_flag_rejects_unknown_backend so both tests explicitly check for the argparse "invalid choice" error message.src/praisonai/tests/unit/test_cli_backend_flag.py (1)
13-13: Nit:pytestappears unused in this module.No fixtures, markers, or
pytest.raisesare referenced — the import can be dropped unless you plan to add parametrization/fixtures soon. Leaving it in is harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_cli_backend_flag.py` at line 13, The import statement "import pytest" in the test module is unused; remove the top-level "import pytest" line from test_cli_backend_flag.py (or the module containing the import) so there are no unused imports left, unless you intend to add fixtures/parametrization that will use pytest later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/tests/unit/test_cli_backend_flag.py`:
- Around line 97-101: Add a brief inline comment on the second line of the
test_backends_bare_subcommand_lists test (just above the _run_cli call)
documenting that this test depends on the CLI "backends" handler treating
subcommand None as equivalent to "list" (see the backends handler in cli.main);
mention that if the handler changes to require an explicit "list" the test will
fail. This makes the coupling discoverable for future maintainers.
---
Nitpick comments:
In `@src/praisonai/tests/unit/test_cli_backend_flag.py`:
- Line 13: The import statement "import pytest" in the test module is unused;
remove the top-level "import pytest" line from test_cli_backend_flag.py (or the
module containing the import) so there are no unused imports left, unless you
intend to add fixtures/parametrization that will use pytest later.
In `@src/praisonai/tests/unit/test_external_agents_backcompat.py`:
- Around line 13-39: Move the duplicated test helpers into a shared test
fixture/module: hoist REPO_ROOT, _build_env, and _run_cli into a new
src/praisonai/tests/unit/conftest.py (or a small helper module) and export a
pytest fixture (e.g., run_cli) that wraps the existing _run_cli behavior;
replace direct uses of _build_env/_run_cli in test_external_agents_backcompat.py
and test_cli_backend_flag.py with the new fixture to remove duplication and
ensure consistent PYTHONPATH/PYTEST_* handling across tests.
- Around line 86-90: The test test_external_agent_rejects_unknown_value
currently uses a loose assertion that may pass for unrelated failures; change
its assertion to explicitly require argparse's rejection text by asserting that
"invalid choice" is in r.stderr (optionally also assert "notreal" appears)
instead of the disjunction. Apply the same tightening to the sibling
test_cli_backend_flag_rejects_unknown_backend so both tests explicitly check for
the argparse "invalid choice" error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22a4117b-e562-456a-a13c-a641c45ca173
📒 Files selected for processing (3)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/tests/unit/test_cli_backend_flag.pysrc/praisonai/tests/unit/test_external_agents_backcompat.py
…dler Addresses remaining CodeRabbit nit on test_cli_backend_flag.py. No test logic change; only docstring additions that document the coupling between the subprocess tests and the 'backends' subcommand handler in praisonai.cli.main (default-to-list behaviour and '[red]Unknown backends subcommand: ...[/red]' error wording).
Summary
Follow-up to PR #1531 (Phase 1b). After that PR merged, 19/34 of its shipped unit tests were failing locally because they asserted against fabricated APIs. CI just skipped them so the breakage was silent. This PR makes the tests actually exercise the real code.
Production behaviour: unchanged. Only changes are (a) one tiny helper extraction in
agents_generator.pyfor testability, and (b) rewritten test files.Before / after
Before (on
mainat39f2c79d)Failures breakdown:
test_yaml_cli_backend.pyAgentsGenerator(config=...)— kwarg doesn't exist;.create_agents_and_tasks()— method doesn't existtest_cli_backend_flag.pyPraisonAI().parse_args()short-circuits toparse_known_args([])whenPYTEST_CURRENT_TESTis in env (seemain.py:1137-1138), so patchedsys.argvis ignored and--helpnever raisesSystemExittest_external_agents_backcompat.pyparse_args()short-circuitAfter (on this branch)
Plus
src/praisonai-agents/tests/unit/test_cli_backend_protocol.py— 8/8 still pass (no changes).Evidence that the feature still works (regression proof)
Changes
1.
src/praisonai/praisonai/agents_generator.py— small helper extractionMoved the 23-line YAML
cli_backendresolve block out ofgenerate_crew_and_kickoffinto a module-level function_resolve_yaml_cli_backend(cli_backend_config, logger). Zero behaviour change — the PraisonAgent construction still receivescli_backend=cli_backend_resolved. The extraction simply makes the logic unit-testable without spinning up a full generator.2.
test_yaml_cli_backend.py— 8 tests (was 6)Direct calls to
_resolve_yaml_cli_backend(...). Covers:resolve_cli_backend('claude-code')resolve_cli_backend('claude-code', overrides={...})id→None+ warning (no raise)None(no warning)ImportErrorin resolver →None+ warningNone+ warning123) →None+ warning'claude-code'id resolves without any mocking3.
test_cli_backend_flag.py— 7 tests (was 8)Subprocess-driven to avoid the
PYTEST_CURRENT_TESTshort-circuit. Child env has allPYTEST_*vars stripped. Each subprocess has a bounded timeout so hangs fail fast. Covers:--cli-backendin--help--external-agentbackends listsubcommandbackends(defaults to list)backends bogussubcommand4.
test_external_agents_backcompat.py— 6 tests (was 7)Mix of direct imports and subprocess argparse checks. Covers:
ExternalAgentsHandlerstill importable +list_integrations()preserves original 4ClaudeCodeIntegrationstill importable +.cli_command == 'claude'--external-agentin--helpclaude,gemini,codex,cursor) in--help--external-agent-directin--helpOut of scope
parse_args()PYTEST_CURRENT_TESTshort-circuit itself. That's a pre-existing CLI-wide testability issue unrelated to Phase 1b; touching it would balloon this PR.--cli-backendback-compat alias for--external-agent claude. Separate design decision.Verification
Closes the bug silently introduced by #1531. Refs #1530.
Summary by CodeRabbit
Refactor
Tests