fix(#239): show install guidance when litellm extra is missing#246
fix(#239): show install guidance when litellm extra is missing#246
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
b8bc8d6| Triggered by: PR opened
Branch: sam/issue-239-pm-missing-litellm | 2 files, +76/-2 | CI: Test Python 3.12 pending 0 https://github.com/Q00/ouroboros/actions/runs/23671290447/job/68965149987
Issue #239 Requirements
| Requirement | Status |
|---|---|
ouroboros/cli/commands/pm.py |
CHECKED IN REVIEW — Evidence inspected in changed files (src/ouroboros/cli/commands/pm.py; tests/unit/cli/test_pm_missing_litellm.py) |
ouroboros/providers/litellm_adapter.py |
CHECKED IN REVIEW — Evidence inspected in changed files (src/ouroboros/cli/commands/pm.py; tests/unit/cli/test_pm_missing_litellm.py) |
pm.py imports LiteLLMAdapter |
CHECKED IN REVIEW — Evidence inspected in changed files (src/ouroboros/cli/commands/pm.py; tests/unit/cli/test_pm_missing_litellm.py) |
litellm_adapter.py imports litellm |
CHECKED IN REVIEW — Evidence inspected in changed files (src/ouroboros/cli/commands/pm.py; tests/unit/cli/test_pm_missing_litellm.py) |
Previous Review Follow-up
First review — no previous findings.
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | tests/unit/cli/test_pm_missing_litellm.py:38 | Low | High | The new regression test only matches "optional LiteLLM dependency", so it would still pass if the actionable install guidance were removed. Since the PR goal is to replace a traceback with specific remediation, the assertion should cover the install instructions too. |
| 2 | tests/unit/cli/test_pm_missing_litellm.py:13 | Medium | High | Coverage stops at _create_pm_litellm_adapter() and never exercises the actual CLI path. A CliRunner test for ouroboros pm would verify the real contract here: friendly error output and non-zero exit instead of a traceback. |
| 3 | src/ouroboros/cli/commands/pm.py:42 | Low | Medium | The new guard only rewrites ModuleNotFoundError when exc.name == "litellm". If the missing optional dependency surfaces as a nested module name such as litellm.*, the CLI will still fall back to the raw traceback path. |
| 4 | tests/unit/cli/test_pm_missing_litellm.py:31 | Low | Medium | The import shim is tightly coupled to the adapter’s current import litellm form. If the adapter switches to a different import style, this regression test can silently stop covering the intended failure mode even though user-facing behavior is unchanged. |
| 5 | src/ouroboros/cli/commands/pm.py:44 | Style | Medium | The optional-dependency install guidance is now hardcoded in the PM command. If packaging guidance changes elsewhere, this string will drift; centralizing optional-dependency messaging would reduce that maintenance risk. |
Test Coverage
tests/unit/cli/test_pm_missing_litellm.py:13 should be complemented with a CLI-level test covering _run_pm_interview/pm_command so the reviewable behavior is exit code + stderr/stdout formatting, not just the helper exception.
tests/unit/cli/test_pm_missing_litellm.py:38 should assert the remediation text itself, not only that some RuntimeError mentioning LiteLLM was raised.
src/ouroboros/cli/commands/pm.py:42 has no regression test for the non-litellm ModuleNotFoundError path, so there is nothing preventing future changes from accidentally masking unrelated import failures.
Design Notes
The patch moves the fragile optional import behind a small helper and converts one known dependency failure into a user-facing CLI error, which is a good direction and keeps the main interview flow readable. The remaining design gap is that dependency-diagnostic policy now lives inline in pm.py; if other commands need similar optional-provider handling, a shared helper would keep behavior and messaging consistent.
Follow-up Recommendations
- Consider adding targeted regression tests for the most failure-prone branches touched in this PR.
- If any behavior change is user-visible, mirror it in docs/changelog before merge.
Files Reviewed
src/ouroboros/cli/commands/pm.pytests/unit/cli/test_pm_missing_litellm.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Summary
litellmin the PM CLI path and convert it into an actionable user-facing errorpmbehavior without changing dependency packaging in this PRTesting
PYTHONPATH=src pytest tests/unit/cli/test_pm_missing_litellm.py -qruff check src/ouroboros/cli/commands/pm.py tests/unit/cli/test_pm_missing_litellm.pyCloses #239.