Repository navigation
fix(api/chat): wire chat endpoint through LLMService.chat() — closes silent canned-response regression (#7047) - #7101
Merged
Merged
Conversation
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…silent canned-response regression (#7047) Two #3185 LLMInterface-retirement misses shipped silently in the same helper, causing chat() to silently return a canned "I'm currently unable to generate a response" string for every user request: Bug 1 — broken module path (api/chat.py:106): from llm_service import LLMService # module never existed Canonical path is services.llm_service. The import was inside ``get_llm_service()`` so it deferred to first call rather than boot — silent until a chat request arrives, then ModuleNotFoundError. Bug 2 — feature-degradation guard around a stale method (api/chat.py:543): if hasattr(llm_service, "generate_response"): return await llm_service.generate_response(...) else: return {"content": "I'm currently unable to generate a response..."} LLMService doesn't expose generate_response (that was an LLMInterface method). The hasattr guard always took the else branch, so every request received the canned fallback regardless of whether LLMService was healthy. Major user-facing functional regression. Fix --- 1. Update lazy import to ``services.llm_service.LLMService``. 2. Replace the hasattr/else dance with a real ``llm_service.chat(...)`` call. Map LLMService's contract: - messages ← llm_context (already OpenAI-format) - conversation_id ← session_id (per-conversation overrides) - request_id ← request_id (passes through **kwargs for tracing) Read response.error / response.content per LLMResponse's Pydantic model (verified shapes against services/llm_service.py:99-112). 3. User-facing fallback strings are unchanged on error — internal error reasons are logged via ``logger.warning`` but never leaked to the response. Verification ------------ Added ``api/chat_generate_ai_response_test.py`` — 5 tests pinning the contract: - happy path: response.content surfaces as result["content"] - chat() args: messages/conversation_id/request_id passed through - LLM returns error: fallback string, error reason NOT leaked to user - chat() raises: fallback string, exception detail NOT leaked - **regression pin** for the original bug: hasattr(llm_service, "generate_response") path can never silently swallow output again — test fails if a future change reintroduces it - **regression pin** for the import path: assert source contains ``from services.llm_service import LLMService`` $ python3 -m pytest autobot-backend/api/chat_generate_ai_response_test.py -xvs ============================== 5 passed in 2.80s =============================== Per the feedback memory I just saved (verify_return_shape_in_method_migration): - LLMResponse.content: str ✓ verified - LLMResponse.error: Optional[str] ✓ verified - chat() return type: LLMResponse (inspected via inspect.signature) Closes #7047 (items 1 + 2). The remaining sites flagged in the issue body (async_chat_workflow.py, modern_ai_integration.py, nl_database_service.py) are tracked for separate per-site verification — their contexts differ enough that batching them risks the same return- shape miss this PR caught with tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 7, 2026
mrveiss
pushed a commit
that referenced
this pull request
May 7, 2026
Promotes the existing private ``_build_mock_response`` in ``tests/fixtures/mocks.py`` to a public, fully-parameterized ``make_llm_response`` factory. The migration tests I authored across PRs #7101 / #7118 / #7131 each rolled their own LLMResponse stub — three different patterns for the same shape. Combined with 4 pre-existing patterns elsewhere, that's 7 ad-hoc mock styles. Why this matters ---------------- The factory's primary value: returns the **real** ``LLMResponse`` from ``llm_interface_pkg.models`` when importable. If a future field rename or type change happens on that dataclass, every test that uses the factory fails at fixture-construction — instead of silently producing wrong-shape mocks the way ``MagicMock(content=...)`` would. This is the class of drift documented in ``feedback_verify_return_shape_in_method_migration.md`` (#7069 lesson). Changes ------- - ``tests/fixtures/mocks.py``: promoted ``_build_mock_response`` → public ``make_llm_response`` with keyword-only args (``content``, ``error``, ``model``, ``provider``). Underscore alias retained as a back-compat one-liner — no callers break. - ``tests/fixtures/__init__.py``: re-exported. - ``tests/fixtures/test_make_llm_response.py``: 7 contract tests pinning the factory's behavior — most importantly the ``isinstance(LLMResponse)`` check that catches future drift. - Migrated my 2 in-repo callers off ``class _StubResponse``: - api/chat_generate_ai_response_test.py - orchestration/workflow_documentation_summary_test.py Both now import ``from tests.fixtures import make_llm_response`` and wrap it in a same-name pytest fixture so the existing test code doesn't need rewriting. Out of scope (deliberately not in this PR) ------------------------------------------ The 4 pre-existing ad-hoc patterns (each has different historical reasons — keeping the migration surface small): - tests/orchestration/test_subagent_dispatcher_reflection.py:31 (``_make_llm_response`` factory) - tests/api/test_knowledge_grounding.py (5 inline ``MagicMock(content=...)``) These can migrate incrementally; the canonical helper is now available when someone touches those files. PR #7131 (perf benchmarks, still open) deliberately not touched — amending an open PR mid-flight invites conflicts. It can migrate after this lands. Verification ------------ $ python3 -m pytest \ tests/fixtures/test_make_llm_response.py \ api/chat_generate_ai_response_test.py \ orchestration/workflow_documentation_summary_test.py -xvs ============================== 19 passed in 2.58s =============================== - 7 new contract tests (factory behavior pinned) - 5 existing api/chat tests still green via canonical fixture - 7 existing workflow_documentation tests still green via canonical fixture Closes #7134 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrveiss
added a commit
that referenced
this pull request
May 7, 2026
…7139) Promotes the existing private ``_build_mock_response`` in ``tests/fixtures/mocks.py`` to a public, fully-parameterized ``make_llm_response`` factory. The migration tests I authored across PRs #7101 / #7118 / #7131 each rolled their own LLMResponse stub — three different patterns for the same shape. Combined with 4 pre-existing patterns elsewhere, that's 7 ad-hoc mock styles. Why this matters ---------------- The factory's primary value: returns the **real** ``LLMResponse`` from ``llm_interface_pkg.models`` when importable. If a future field rename or type change happens on that dataclass, every test that uses the factory fails at fixture-construction — instead of silently producing wrong-shape mocks the way ``MagicMock(content=...)`` would. This is the class of drift documented in ``feedback_verify_return_shape_in_method_migration.md`` (#7069 lesson). Changes ------- - ``tests/fixtures/mocks.py``: promoted ``_build_mock_response`` → public ``make_llm_response`` with keyword-only args (``content``, ``error``, ``model``, ``provider``). Underscore alias retained as a back-compat one-liner — no callers break. - ``tests/fixtures/__init__.py``: re-exported. - ``tests/fixtures/test_make_llm_response.py``: 7 contract tests pinning the factory's behavior — most importantly the ``isinstance(LLMResponse)`` check that catches future drift. - Migrated my 2 in-repo callers off ``class _StubResponse``: - api/chat_generate_ai_response_test.py - orchestration/workflow_documentation_summary_test.py Both now import ``from tests.fixtures import make_llm_response`` and wrap it in a same-name pytest fixture so the existing test code doesn't need rewriting. Out of scope (deliberately not in this PR) ------------------------------------------ The 4 pre-existing ad-hoc patterns (each has different historical reasons — keeping the migration surface small): - tests/orchestration/test_subagent_dispatcher_reflection.py:31 (``_make_llm_response`` factory) - tests/api/test_knowledge_grounding.py (5 inline ``MagicMock(content=...)``) These can migrate incrementally; the canonical helper is now available when someone touches those files. PR #7131 (perf benchmarks, still open) deliberately not touched — amending an open PR mid-flight invites conflicts. It can migrate after this lands. Verification ------------ $ python3 -m pytest \ tests/fixtures/test_make_llm_response.py \ api/chat_generate_ai_response_test.py \ orchestration/workflow_documentation_summary_test.py -xvs ============================== 19 passed in 2.58s =============================== - 7 new contract tests (factory behavior pinned) - 5 existing api/chat tests still green via canonical fixture - 7 existing workflow_documentation tests still green via canonical fixture Closes #7134 Co-authored-by: t <t@t> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
P1 user-facing regression. Since #3185 (LLMInterface retirement on 2026-05-04), the chat endpoint has been silently returning a canned "I'm currently unable to generate a response. Please try again." string for every user request. Two bugs in one helper:
Bug 1 — broken module path
```python
api/chat.py:106 (before)
from llm_service import LLMService # ← module doesn't exist
```
Canonical path is `services.llm_service`. Import was inside `get_llm_service()`, so it deferred to first call — would have raised `ModuleNotFoundError` on the first chat request had bug 2 not masked it.
Bug 2 — hasattr guard around a deleted method
```python
api/chat.py:543 (before)
if hasattr(llm_service, "generate_response"): # always False post-#3185
return await llm_service.generate_response(...)
else:
return {"content": "I'm currently unable to..."}
```
`LLMService` doesn't expose `generate_response`. Every call took the else branch.
Fix
Regression pins (5 tests added)
```
$ python3 -m pytest autobot-backend/api/chat_generate_ai_response_test.py -xvs
============================== 5 passed in 2.80s ===============================
```
Out of scope (deferred per PR-discipline / different contexts)
The other sites flagged in #7047 (async_chat_workflow.py:344, modern_ai_integration.py:673, nl_database_service.py:549) need per-site verification — return-shape and call-context differ enough that batching them risks the verify-return-shape lesson again. #7047 stays open until those land.
Closes parts 1 + 2 of #7047