Repository navigation
Remove the chat surface from the AI service - #218
Conversation
The chat surface is retired on the frontend (sprintstart-frontend#206) — /chat redirects to the buddy and nothing calls /api/v1/chat any more — so the service side goes in one piece rather than as a route that outlives the UI that stopped asking for it. What goes: the /api/v1/chat route with its SSE path, ChatAgent and ChatOrchestrator, the chat CLI (scripts/chat_cli.py, `sprintstart chat`) that was their only non-HTTP caller, ChatRequest/HistoryEntry and the legacy-field validator in schemas.py (the `prompt`/`context` acceptance existed for pre-rename clients), and the chat-only docstrings that referenced any of it. What stays, and why: the buddy runs on the same retrieval stack and the same SSE event shapes, so `agents/__init__.py` stays as the package root, `ChatFilters` still describes the buddy request's filters, and BuddyStreamEvent is untouched. The shared `chat_stream` client primitive keeps its tests — only its docstrings stopped claiming a chat agent. The README is deliberately left alone: it has an open rewrite (#216), and folding prose about the service's entry points into a removal would make both harder to review.
The golden-retrieval and image-ingest integration tests spoke HTTP to /api/v1/chat because that was the one entry point exercising the full stack. With the endpoint gone, the same coverage calls the retrieval layer directly (rag.retriever.retrieve) — the pipeline the buddy itself runs — so the assertions still watch real behaviour instead of a second, thinner client path. The source-exclusion regression test moves to the buddy route, which is where exclusions are applied now, and gains a case proving a disabled connector's material cannot reach the buddy's context. The chat route's own test file and the agent/orchestrator unit tests go with the code they tested; the dependency-graph test keeps its subject and drops the chat route from its docstring.
|
A note for whoever merges or rebases this after the README work: PR #216 ( Two caveats from the implementation run, for the merge window:
|
- Drop rag.prompt.build_messages, dead since the /chat route went - Re-word stale chat references in the CLI client, chroma_store and tests - Rename test_empty_source_systems_mean_all_as_they_did_for_chat
- Add route-level tests that another project's chunks and chunks with no project are neither seen by the model nor cited (search_docs + grep, with and without filters), and that an empty project_ids admits nothing - Drop the chat() helper from tests/api/test_client.py, the last caller of /api/v1/chat
kiranfin
left a comment
There was a problem hiding this comment.
Review
The removal itself is clean: chat route, agent, orchestrator, factory, CLI, ChatRequest/HistoryEntry and build_messages are gone with no dangling imports, the buddy path is untouched, and the gates reproduce locally (ruff format/check clean, pyright src/ 0 errors, pytest 1043 passed / 7 skipped). Re-pointing the golden and image integration tests also fixes something the description doesn't mention: they were posting to /chat without a projectId, so they could only ever have 422'd. The new source-exclusion test does its job: dropping exclusions=source_state.get_exclusions() from the buddy route makes it fail. What's left is about tests that were deleted along with test_chat.py without being moved over, and about the documentation scope from #207.
🟡 Worth a look
- Deferring the README to #216 leaves the documented API wrong after both PRs merge —
README.md:80,README.md:91-110
#207's scope explicitly includes updatingREADME.mdtogether withAGENTS.md. This PR leaves it to #216, but #216's current head (a235e12) still documents the removed surface:uv run python scripts/sprintstart.py chat(README:71), a "Chat agent" node in the architecture diagram (README:86), "Chat and the buddy are agentic" (README:98),agents/ chat agent and its tools(README:125), and indocs/api.mdthePOST /chatrow (:26) plus a whole "Chat stream events" section (:63-65). So whichever PR merges second, the published docs will describe an endpoint and a CLI command that no longer exist. Either remove the/api/v1/chatrow, the "Chat SSE stream" section and/chatfrom theprojectIdlist here, or get an explicit commitment on #216 (a checkbox or comment) that its rebase drops them, and link it from this PR. ChatFiltersrequest validation lost its only tests, even though the schema stays —src/api/schemas.py:262-286,src/api/schemas.py:1411
The description says the deleted tests went "with their subjects", butChatFiltersis kept and is now validated only throughBuddyAgentRequest.filters.test_chat_accepts_a_bitbucket_source_filter,test_chat_accepts_a_notion_source_filterandtest_chat_still_rejects_an_unknown_source_filter(GITLAB→ 422) were the only tests of that contract at the request boundary, and nothing intests/referencesChatFiltersor a rejected source system anymore. The per-source exclusion combined with narrowing (test_chat_applies_source_exclusions_with_retrieval_filters, which usesset_sources_enabled) is also only covered below the route now. Moving these to/onboarding/buddy/agentis cheap.- The re-pointed golden/image tests run unscoped retrieval, which no production caller does —
tests/rag/test_golden.py:36-63,:81-83,:106,tests/api/test_ingest_image.py:127-132
The corpus is ingested withoutproject_idsand queried withretrieve(..., filters=None), somatches_retrieval_filtersreturnsTruewithout checking anything. The docstring says this is "the pipeline the buddy itself runs", but under the buddy's fail-closedproject_idsscope this corpus would be invisible, and the buddy also addsdrop_test_materialand its own_TOP_K/_MIN_SCORE(src/onboarding/buddy_agent.py:75-78,:207). As written, the golden suite can pass while the buddy finds nothing for the same questions. Ingesting with aproject_idsvalue and retrieving withRetrievalFilters(project_ids=frozenset({...}))would make these tests check what production actually does. Otherwise, please change the docstring so it doesn't claim parity.
🟢 Nits / cleanup
- Chat references that still describe the deleted agent in the present tense:
src/onboarding/buddy_agent.py:7-8("They are the chat agent's own tools … loses nothing chat had"),:193("as the chat agent does"),:265-268("Chat'sretrieve… what chat had"),src/rag/retriever.py:7("every caller (chat, agent tools, and onboarding)"), andtests/agents/test_evidence.py:3. agents.tools.base.Invocation(src/agents/tools/base.py:20-23) was only ever emitted byChatAgent.run. It's now dead and only re-exported fromsrc/agents/tools/__init__.py. Removing it fits "No dead imports ofchat_agent/orchestrator".test_a_disabled_connector_is_neither_searched_nor_citedonly checks citations. The responsemessagesinclude thetoolresult the model saw, so also asserting that no message mentionsexcluded.mdwould back up the "cannot reach the buddy's context" claim in the description, not just "not cited".
Overall, the deletion is careful and the service builds cleanly without the chat surface. None of the points above blocks the code: 1 needs either a fix here or a recorded commitment on #216, and 2–3 can be addressed here or explicitly accepted as follow-ups. Approving on the condition that merge order is respected: sprintstart-backend dev still calls /api/v1/chat from ChatAiClient.kt:71, and pushes to dev publish the image, so this should only be merged after sprintstart-backend#283 and sprintstart-frontend#318 have landed.
The AI-service leg of retiring the chat surface:
/api/v1/chat,ChatAgent,ChatOrchestratorand the chat CLI are removed in one piece. The buddy is untouched — same retrieval stack, sameBuddyStreamEventcontract.Closes #207. Part of the coordinated retirement across the repos — sprintstart-frontend#206 retires
/chatin the UI and SprintStartProject/sprintstart-backend#283 deletes the chat tables — so this should not merge ahead of those.What goes
POST /api/v1/chatand the SSE streaming path behind itChatAgent,ChatOrchestratorand their promptsscripts/chat_cli.pyand thesprintstart chatcommandChatRequest/HistoryEntryand the legacyprompt/contextfield acceptanceWhat stays, and why
BuddyStreamEventuntouchedChatFilters— it describes the buddy request's filterschat_streamLLM primitive and its contract testsTests
rag.retriever.retrievetests/api/test_chat.py,tests/agents/test_chat_agent.py,tests/agents/test_orchestrator.pyGates
uv run ruff format --check .— cleanuv run ruff check .— cleanuv run python -m pyright src/— 0 errors, 0 warningsuv run python -m pytest— 1037 passed, 7 skipped