diff --git a/agents/smart_chat.py b/agents/smart_chat.py new file mode 100644 index 0000000..5fcb607 --- /dev/null +++ b/agents/smart_chat.py @@ -0,0 +1,113 @@ +# agents/smart_chat.py +""" +SmartChatAgent +=============== +The conversational assistant behind POST /assistant/start, /assistant/chat, +/assistant/end (backend/routes_ai.py). + +This did not exist anywhere in the repository. `backend/routes_ai.py` did +`from app.smart_chat import SmartChatAgent` against a module that was never +committed -- there is no `app/` package at all in this codebase -- and that +missing import took the entire FastAPI app down at boot, over these three +endpoints out of several dozen (see the app-boot fix in the previous PR). + +It is not built from nothing. `orchestration/chat_workflow.py`'s +`ChatManager` already implements the real conversational pipeline this needs: +multi-turn history, follow-up detection, intent classification with context, +clarification prompts, routing through the super-graph, and follow-up +suggestions. `SmartChatAgent` is a thin adapter over it, in the shape +`routes_ai.py` already expects (`agent.context.session_id`, +`agent.chat_sync(message)` returning `{"content", "metadata": {...}}`) -- +built to match a real call site, not invented against no spec. + +Placed in `agents/`, not `app/`: every other specialized agent already lives +here, and creating a one-file `app/` package just to preserve a broken import +path that was never real would be structure for its own sake. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, Dict, List, Optional + +from orchestration.chat_workflow import ChatManager + + +@dataclass +class ChatContext: + """Minimal session context. `routes_ai.py` reads `.session_id` off this.""" + + session_id: str + user_email: Optional[str] = None + + +class SmartChatAgent: + """Conversational assistant for one chat session. + + One instance per session (routes_ai.py caches instances in `_chat_agents` + keyed by session_id) -- state (conversation history) lives in the + `ChatManager` it wraps, scoped to this instance's session. + """ + + def __init__(self, repo=None, user_email: Optional[str] = None, *, use_llm: bool = True): + # `repo` is accepted for interface compatibility with how every other + # call site in this codebase constructs agents (TasksAgent(repo), + # MeetingAgent(repo), ...). It isn't threaded through further: the + # underlying ChatManager / process_user_request pipeline constructs + # its own DataRepo() internally, the same pattern every other agent + # in this codebase already uses. Passing a different repo instance + # here would not currently change what data the chat pipeline reads. + self.repo = repo + + # use_llm=False has no caller anywhere in this codebase (routes_ai.py + # always passes True) and the underlying ChatManager has no + # non-LLM mode to fall back to -- intent classification and response + # generation both go through the gateway. Rather than silently + # ignoring the flag, an unsupported request for it fails clearly. + if not use_llm: + raise NotImplementedError( + "SmartChatAgent has no non-LLM mode; the underlying chat " + "pipeline requires a configured LLM gateway for intent " + "classification and response generation." + ) + + self._manager = ChatManager() + session_id = self._manager.start_session(user_email=user_email or "") + self.context = ChatContext(session_id=session_id, user_email=user_email) + + def chat_sync(self, message: str) -> Dict[str, Any]: + """Process one message and return a response in the shape + routes_ai.py expects. + + Synchronous: matches the existing call site + (`result = agent.chat_sync(message)`, no `await`). The underlying + gateway call is itself synchronous (see governance/litellm_gateway.py), + so there is no event loop to bridge here, unlike the Graph API calls + in repos/data_repo.py. + """ + result = self._manager.process_message( + session_id=self.context.session_id, + user_email=self.context.user_email or "", + user_input=message, + ) + + return { + "content": result.get("response", "I couldn't process that request."), + "metadata": { + "intent": result.get("intent", "unknown"), + "confidence": result.get("confidence"), + "reasoning_trace": result.get("reasoning_trace", []), + "agents_invoked": result.get("agents_invoked", []), + "is_followup": result.get("is_followup", False), + "needs_clarification": result.get("needs_clarification", False), + "followup_suggestions": result.get("followup_suggestions", []), + }, + } + + def get_history(self) -> List[Dict[str, Any]]: + """Full conversation history for this session.""" + return self._manager.get_conversation_history(self.context.session_id) + + def end(self) -> None: + """End the session -- flushes a summary to episodic memory.""" + self._manager.end_session(self.context.session_id) diff --git a/backend/routes_ai.py b/backend/routes_ai.py index ae543cc..5c45053 100644 --- a/backend/routes_ai.py +++ b/backend/routes_ai.py @@ -13,39 +13,16 @@ router = APIRouter() -# SmartChatAgent (backing the /assistant/* endpoints below) was never -# committed to this repo -- `app.smart_chat` does not exist anywhere in the -# tree. That used to take the entire router down at import time, which took -# down the whole FastAPI app with it, over three endpoints out of the -# thirteen in this file. -# -# The other ten endpoints (plan_today, nudges, weekly reports, wellness -# score, email/meeting analysis, EOD reports, burnout check) don't touch -# SmartChatAgent at all and have no reason to be unavailable because of it. -# The import is optional now; only /assistant/* is affected, and it fails -# with a clear 501 rather than either fabricating a chat agent or refusing to -# boot the app. -try: - from app.smart_chat import SmartChatAgent - _SMART_CHAT_AVAILABLE = True -except ImportError: - SmartChatAgent = None # type: ignore[assignment] - _SMART_CHAT_AVAILABLE = False - -# Keep a cache of SmartChatAgent instances per user -_chat_agents: Dict[str, Any] = {} - - -def _require_smart_chat() -> None: - if not _SMART_CHAT_AVAILABLE: - raise HTTPException( - status_code=501, - detail=( - "The conversational assistant is not available: app.smart_chat " - "was never implemented in this codebase. The other endpoints " - "in this router are unaffected." - ), - ) +# SmartChatAgent used to be imported as `from app.smart_chat import +# SmartChatAgent` -- a package that never existed anywhere in this repo, and +# that missing import took the entire FastAPI app down at boot. It's now +# implemented in agents/smart_chat.py (alongside every other specialized +# agent) as a real adapter over orchestration/chat_workflow.py's already- +# working ChatManager. See that module's docstring for the full story. +from agents.smart_chat import SmartChatAgent + +# Keep a cache of SmartChatAgent instances per session +_chat_agents: Dict[str, SmartChatAgent] = {} @router.post('/ai/plan_today') @@ -92,13 +69,12 @@ async def wellness_score(payload: dict): @router.post('/assistant/start') async def assistant_start(payload: dict): - _require_smart_chat() user_email = payload.get('user_email') if not user_email: raise HTTPException(status_code=400, detail='user_email required') # Create a new SmartChatAgent for this user/session repo = DataRepo() - agent = SmartChatAgent(repo, use_llm=True) + agent = SmartChatAgent(repo, user_email, use_llm=True) session_id = agent.context.session_id _chat_agents[session_id] = agent return {"session_id": session_id} @@ -106,42 +82,45 @@ async def assistant_start(payload: dict): @router.post('/assistant/chat') async def assistant_chat(payload: dict): - _require_smart_chat() session_id = payload.get('session_id') user_email = payload.get('user_email') message = payload.get('message') if not user_email or not message: raise HTTPException(status_code=400, detail='user_email and message required') - + # Get or create SmartChatAgent if session_id and session_id in _chat_agents: agent = _chat_agents[session_id] else: # Create new agent repo = DataRepo() - agent = SmartChatAgent(repo, use_llm=True) + agent = SmartChatAgent(repo, user_email, use_llm=True) session_id = agent.context.session_id _chat_agents[session_id] = agent - + # Use SmartChatAgent's chat_sync for comprehensive response try: result = agent.chat_sync(message) return { "response": result.get("content", "I couldn't process that request."), "intent": result.get("metadata", {}).get("intent", "unknown"), - "confidence": result.get("metadata", {}).get("confidence", 0), + "confidence": result.get("metadata", {}).get("confidence"), "reasoning_trace": result.get("metadata", {}).get("reasoning_trace", []), "session_id": session_id } except Exception as e: + # A chat UI showing "something went wrong" as a reply is normal, + # expected degradation -- unlike fabricating business data, telling + # the user their own request failed is honest. What was wrong before + # was the hardcoded confidence: 0.8 on an error path, which claimed + # 80% confidence in a response that was, by definition, a failure. import traceback print(f"[ERROR] SmartChatAgent failed: {e}") traceback.print_exc() - # Fallback to simple response return { "response": f"I encountered an issue processing your request. Error: {str(e)}", "intent": "error", - "confidence": 0.8, + "confidence": None, "session_id": session_id } @@ -153,6 +132,7 @@ async def assistant_end(payload: dict): raise HTTPException(status_code=400, detail='session_id required') # Clean up agent if session_id in _chat_agents: + _chat_agents[session_id].end() del _chat_agents[session_id] return {"status": "ended"} diff --git a/orchestration/chat_workflow.py b/orchestration/chat_workflow.py index b7b678a..8471e15 100644 --- a/orchestration/chat_workflow.py +++ b/orchestration/chat_workflow.py @@ -125,6 +125,9 @@ def process_message( user_input, intent, history ) + confidence: Optional[float] = None + reasoning_trace: List[Any] = [] + if needs_clarification: # Return clarification question response = clarification @@ -136,9 +139,14 @@ def process_message( user_email=user_email, session_id=session_id ) - + response = result.get("response", "I'm here to help!") agents_invoked = result.get("agents_used", []) + # process_user_request already computes these; they were being + # discarded here rather than surfaced to callers that need them + # (e.g. SmartChatAgent.chat_sync's metadata). + confidence = result.get("confidence") + reasoning_trace = result.get("reasoning_trace", []) # Create turn record turn = ConversationTurn( @@ -164,6 +172,8 @@ def process_message( return { "response": response, "intent": intent, + "confidence": confidence, + "reasoning_trace": reasoning_trace, "agents_invoked": agents_invoked, "is_followup": is_followup, "needs_clarification": needs_clarification, diff --git a/tests/test_app_boots.py b/tests/test_app_boots.py index 5160587..38b5efb 100644 --- a/tests/test_app_boots.py +++ b/tests/test_app_boots.py @@ -5,6 +5,10 @@ backend/routes_ai.py importing `app.smart_chat.SmartChatAgent`, a module that was never committed anywhere in this repository -- took the entire FastAPI app down at import time over three endpoints out of several dozen. + +SmartChatAgent is real now (agents/smart_chat.py, a follow-up PR) -- the +assistant endpoints are tested with a fake gateway in test_smart_chat.py, +not here. This file stays focused on "does the app boot at all." """ from __future__ import annotations @@ -21,23 +25,27 @@ def client(): # gateway eagerly and this test environment has no LLM configured, which # correctly raises LLMNotConfiguredError (see test_gateway.py -- that's # the fail-closed behavior working as intended, not a bug). This suite is - # about the SEPARATE SmartChatAgent import regression, so a 500 from an - # unrelated, expected cause should come back as a normal response to - # assert against, not propagate and fail these tests for the wrong reason. + # about "does the app boot," so an unrelated 500 should come back as a + # normal response to assert against, not propagate and fail these tests + # for the wrong reason. with TestClient(app_module.app, raise_server_exceptions=False) as c: yield c class TestAppImports: - def test_app_module_imports_without_smart_chat(self): + def test_app_module_imports_without_app_dot_smart_chat(self): """The regression this test exists for: importing backend.app must - not require app.smart_chat, which does not exist in this repo.""" + not require the never-existed `app.smart_chat` package.""" import sys import backend.app # noqa: F401 assert "app.smart_chat" not in sys.modules + def test_smart_chat_agent_is_importable_from_agents(self): + """Where it actually lives now.""" + from agents.smart_chat import SmartChatAgent # noqa: F401 + def test_app_registers_a_substantial_number_of_routes(self): """`len(app.routes)` is not a stable thing to assert on: newer FastAPI represents each include_router() call as a single lazy @@ -57,8 +65,8 @@ def test_app_registers_a_substantial_number_of_routes(self): class TestUnaffectedEndpointsStillWork: - """The ten /ai/* endpoints that don't touch SmartChatAgent must be - completely unaffected by its absence.""" + """The /ai/* endpoints that don't touch SmartChatAgent must be completely + unaffected by whatever state it's in.""" def test_nudges_endpoint_responds(self, client): r = client.get("/api/v1/ai/nudges") @@ -71,23 +79,22 @@ def test_weekly_reports_endpoint_responds(self, client): assert r.status_code != 404 -class TestAssistantEndpointsFailClearly: - """The three endpoints that DO need SmartChatAgent must answer 501 -- - not fabricate a response, not 500, not silently do nothing.""" +class TestAssistantEndpointsAreRegistered: + """The real conversational behavior is tested with a fake gateway in + test_smart_chat.py. Here: just confirm the routes exist and fail for the + RIGHT reason in an unconfigured test environment (no LLM), not a 404.""" - def test_assistant_start_returns_501(self, client): + def test_assistant_start_route_exists(self, client): r = client.post("/api/v1/assistant/start", json={"user_email": "a@example.com"}) - assert r.status_code == 501 - assert "smart_chat" in r.json()["detail"].lower() or "assistant" in r.json()["detail"].lower() + assert r.status_code != 404 - def test_assistant_chat_returns_501(self, client): + def test_assistant_chat_route_exists(self, client): r = client.post( "/api/v1/assistant/chat", json={"session_id": "x", "user_email": "a@example.com", "message": "hi"}, ) - assert r.status_code == 501 + assert r.status_code != 404 - def test_error_names_the_missing_capability_not_a_generic_failure(self, client): - r = client.post("/api/v1/assistant/start", json={"user_email": "a@example.com"}) - detail = r.json()["detail"] - assert "never implemented" in detail or "not available" in detail + def test_assistant_end_route_exists(self, client): + r = client.post("/api/v1/assistant/end", json={"session_id": "x"}) + assert r.status_code != 404 diff --git a/tests/test_smart_chat.py b/tests/test_smart_chat.py new file mode 100644 index 0000000..bc7e3bb --- /dev/null +++ b/tests/test_smart_chat.py @@ -0,0 +1,170 @@ +"""SmartChatAgent and the ChatManager it wraps. + +The LLM gateway and the super-graph (which itself fans out to Redis-backed, +gateway-using subgraphs for every specialized agent) are stubbed at the two +points orchestration/chat_workflow.py itself calls them -- +`EnhancedLiteLLMGateway` and `process_user_request`. That is the real +dependency boundary of the code under test; stubbing it lets ChatManager's own +logic (turn history, follow-up detection, clarification, response shaping) +run for real without needing a live LLM or a live Redis for the six +specialized-agent subgraphs a full super-graph invocation would otherwise touch. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + +import pytest + + +class FakeGateway: + """Deterministic stand-in for EnhancedLiteLLMGateway.call(). + + Returns a canned intent based on keywords in the prompt, mirroring what a + real intent-classification call would produce -- this is a fixture for + the *test*, not mock product data; nothing in the running app ever uses it. + """ + + def __init__(self, *args, **kwargs): + pass + + def call(self, prompt: str, **kwargs) -> str: + lower = prompt.lower() + if "task" in lower: + return "task" + if "wellness" in lower: + return "wellness" + if "meeting" in lower: + return "meeting" + return "chat" + + +def _fake_process_user_request(user_input: str, user_email: str, session_id: Optional[str] = None) -> Dict[str, Any]: + return { + "response": f"Handled: {user_input}", + "intent": "task", + "confidence": 0.87, + "agents_used": ["tasks_agent"], + "actions": [], + "reasoning_trace": ["classified intent", "routed to tasks_agent"], + "session_id": session_id, + } + + +@pytest.fixture(autouse=True) +def stub_chat_dependencies(monkeypatch): + import orchestration.chat_workflow as chat_workflow_module + + monkeypatch.setattr(chat_workflow_module, "EnhancedLiteLLMGateway", FakeGateway) + monkeypatch.setattr(chat_workflow_module, "process_user_request", _fake_process_user_request) + + # Reset the module-level singleton so each test gets a fresh ChatManager + # built with the stubbed gateway, rather than reusing one from a previous + # test (or one that would have failed to construct with the real one). + chat_workflow_module._chat_manager = None + + +class TestChatManagerCore: + def test_start_session_creates_an_empty_history(self): + from orchestration.chat_workflow import ChatManager + + mgr = ChatManager() + session_id = mgr.start_session("a@example.com") + assert mgr.get_conversation_history(session_id) == [] + + def test_process_message_routes_through_super_graph_for_a_normal_request(self): + """"my tasks" specifically, not e.g. "my P0 tasks" -- the latter hits + ChatManager's own clarification check (task intent without "my tasks" + or "plan" in the message), which is real, existing, deliberate + behavior this test must not fight.""" + from orchestration.chat_workflow import ChatManager + + mgr = ChatManager() + session_id = mgr.start_session("a@example.com") + result = mgr.process_message(session_id, "a@example.com", "Show me my tasks") + + assert result["response"] == "Handled: Show me my tasks" + assert result["intent"] == "task" + assert result["confidence"] == 0.87 + assert result["agents_invoked"] == ["tasks_agent"] + assert result["needs_clarification"] is False + + def test_clarification_path_never_reaches_the_super_graph(self): + """A vague, short message should short-circuit to a clarification + question rather than route through the (stubbed) super-graph.""" + from orchestration.chat_workflow import ChatManager + + mgr = ChatManager() + session_id = mgr.start_session("a@example.com") + result = mgr.process_message(session_id, "a@example.com", "help") + + assert result["needs_clarification"] is True + assert result["agents_invoked"] == [] + + def test_history_accumulates_across_turns(self): + from orchestration.chat_workflow import ChatManager + + mgr = ChatManager() + session_id = mgr.start_session("a@example.com") + mgr.process_message(session_id, "a@example.com", "Show me my tasks") + mgr.process_message(session_id, "a@example.com", "and my meetings too") + + history = mgr.get_conversation_history(session_id) + assert len(history) == 2 + + def test_end_session_removes_it(self): + from orchestration.chat_workflow import ChatManager + + mgr = ChatManager() + session_id = mgr.start_session("a@example.com") + mgr.process_message(session_id, "a@example.com", "Show me my tasks") + mgr.end_session(session_id) + assert mgr.get_conversation_history(session_id) == [] + + +class TestSmartChatAgent: + """The thin adapter -- agents/smart_chat.py -- over ChatManager, in the + shape backend/routes_ai.py actually calls it in.""" + + def test_construction_sets_a_session_id(self): + from agents.smart_chat import SmartChatAgent + + agent = SmartChatAgent(repo=None, user_email="a@example.com", use_llm=True) + assert agent.context.session_id + assert agent.context.user_email == "a@example.com" + + def test_use_llm_false_is_rejected_not_silently_ignored(self): + """No caller anywhere in this codebase passes use_llm=False, and there + is no non-LLM mode to fall back to -- this must fail loudly if ever + requested, not silently behave as if it were True.""" + from agents.smart_chat import SmartChatAgent + + with pytest.raises(NotImplementedError): + SmartChatAgent(repo=None, use_llm=False) + + def test_chat_sync_returns_the_shape_routes_ai_expects(self): + from agents.smart_chat import SmartChatAgent + + agent = SmartChatAgent(repo=None, user_email="a@example.com", use_llm=True) + result = agent.chat_sync("Show me my tasks") + + assert result["content"] == "Handled: Show me my tasks" + assert result["metadata"]["intent"] == "task" + assert result["metadata"]["confidence"] == 0.87 + assert result["metadata"]["reasoning_trace"] == ["classified intent", "routed to tasks_agent"] + + def test_get_history_reflects_prior_turns(self): + from agents.smart_chat import SmartChatAgent + + agent = SmartChatAgent(repo=None, user_email="a@example.com", use_llm=True) + agent.chat_sync("Show me my tasks") + history = agent.get_history() + assert len(history) == 1 + assert history[0]["user_message"] == "Show me my tasks" + + def test_end_does_not_raise(self): + from agents.smart_chat import SmartChatAgent + + agent = SmartChatAgent(repo=None, user_email="a@example.com", use_llm=True) + agent.chat_sync("hi there, how are you") + agent.end() # must not raise