diff --git a/application/alembic/versions/0016_conversation_visibility.py b/application/alembic/versions/0016_conversation_visibility.py new file mode 100644 index 00000000..403759b8 --- /dev/null +++ b/application/alembic/versions/0016_conversation_visibility.py @@ -0,0 +1,59 @@ +"""0016 conversation visibility — separate "persisted" from "shown in sidebar". + +Adds ``conversations.visibility`` (``listed`` | ``hidden``) so a conversation +can be stored without surfacing in the owner's sidebar. Until now display was +inferred from identity columns: ``(api_key IS NULL OR agent_id IS NOT NULL)``. +The backfill reproduces that exact predicate so existing sidebars are +unchanged; new rows set the value explicitly at write time. + +Revision ID: 0016_conversation_visibility +Revises: 0015_token_usage_model_id +""" + +from typing import Sequence, Union + +from alembic import op + + +revision: str = "0016_conversation_visibility" +down_revision: Union[str, None] = "0015_token_usage_model_id" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.execute( + "ALTER TABLE conversations " + "ADD COLUMN visibility TEXT NOT NULL DEFAULT 'listed';" + ) + # NOT VALID first so the ACCESS EXCLUSIVE lock doesn't scan the table; + # VALIDATE then runs under a weaker lock that allows concurrent reads/writes. + op.execute( + "ALTER TABLE conversations " + "ADD CONSTRAINT conversations_visibility_chk " + "CHECK (visibility IN ('listed', 'hidden')) NOT VALID;" + ) + op.execute( + "ALTER TABLE conversations VALIDATE CONSTRAINT conversations_visibility_chk;" + ) + # Preserve current sidebars: the old display heuristic showed a + # conversation when (api_key IS NULL OR agent_id IS NOT NULL); hide the rest. + op.execute( + "UPDATE conversations SET visibility = 'hidden' " + "WHERE NOT (api_key IS NULL OR agent_id IS NOT NULL);" + ) + # Matches the sidebar query: user_id + visibility, newest first. + op.execute( + "CREATE INDEX conversations_user_listed_idx " + 'ON conversations (user_id, date DESC) ' + "WHERE visibility = 'listed';" + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS conversations_user_listed_idx;") + op.execute( + "ALTER TABLE conversations " + "DROP CONSTRAINT IF EXISTS conversations_visibility_chk;" + ) + op.execute("ALTER TABLE conversations DROP COLUMN IF EXISTS visibility;") diff --git a/application/api/answer/routes/answer.py b/application/api/answer/routes/answer.py index f1d25a1c..1e869c47 100644 --- a/application/api/answer/routes/answer.py +++ b/application/api/answer/routes/answer.py @@ -8,6 +8,7 @@ from application.api import api from application.api.answer.routes.base import answer_ns, BaseAnswerResource +from application.api.answer.services.persistence_policy import resolve_persistence from application.api.answer.services.stream_processor import StreamProcessor logger = logging.getLogger(__name__) @@ -117,6 +118,12 @@ class AnswerResource(Resource, BaseAnswerResource): if error := self.check_usage(processor.agent_config): return error + should_persist, visibility = resolve_persistence( + display_flag=data.get("save_conversation"), + api_key=data.get("api_key"), + is_shared_usage=processor.is_shared_usage, + persist_flag=data.get("persist"), + ) stream = self.complete_stream( question=data["question"], agent=agent, @@ -125,7 +132,8 @@ class AnswerResource(Resource, BaseAnswerResource): decoded_token=processor.decoded_token, isNoneDoc=data.get("isNoneDoc"), index=None, - should_save_conversation=data.get("save_conversation", True), + should_persist=should_persist, + visibility=visibility, agent_id=processor.agent_id, is_shared_usage=processor.is_shared_usage, shared_token=processor.shared_token, diff --git a/application/api/answer/routes/base.py b/application/api/answer/routes/base.py index 2f95d10c..4b4a5d03 100644 --- a/application/api/answer/routes/base.py +++ b/application/api/answer/routes/base.py @@ -183,7 +183,8 @@ class BaseAnswerResource: decoded_token: Dict[str, Any], isNoneDoc: bool = False, index: Optional[int] = None, - should_save_conversation: bool = True, + should_persist: bool = True, + visibility: str = "listed", attachment_ids: Optional[List[str]] = None, agent_id: Optional[str] = None, is_shared_usage: bool = False, @@ -204,7 +205,8 @@ class BaseAnswerResource: decoded_token: Decoded JWT token isNoneDoc: Flag for document-less responses index: Index of message to update - should_save_conversation: Whether to persist the conversation + should_persist: Whether to persist the conversation + visibility: ``listed`` (sidebar) or ``hidden`` for a new conversation attachment_ids: List of attachment IDs agent_id: ID of agent used is_shared_usage: Flag for shared agent usage @@ -232,7 +234,7 @@ class BaseAnswerResource: # mid-stream still leaves the question queryable. Continuations # reuse the original placeholder. reserved_message_id: Optional[str] = None - wal_eligible = should_save_conversation and not _continuation + wal_eligible = should_persist and not _continuation if wal_eligible: try: reservation = self.conversation_service.save_user_question( @@ -244,6 +246,7 @@ class BaseAnswerResource: agent_id=agent_id, is_shared_usage=is_shared_usage, shared_token=shared_token, + visibility=visibility, model_id=model_id or self.default_model_id, request_id=request_id, index=index, @@ -489,7 +492,7 @@ class BaseAnswerResource: continuation = getattr(agent, "_pending_continuation", None) if continuation: # First-turn pause needs a conversation row to attach to. - if not conversation_id and should_save_conversation: + if not conversation_id and should_persist: try: provider = ( get_provider_from_model_id( @@ -531,6 +534,7 @@ class BaseAnswerResource: agent_id=agent_id, is_shared_usage=is_shared_usage, shared_token=shared_token, + visibility=visibility, ) ) except Exception as e: @@ -678,7 +682,7 @@ class BaseAnswerResource: # Title-gen only; agent stream tokens live on ``agent.llm``. llm._token_usage_source = "title" - if should_save_conversation: + if should_persist: if reserved_message_id is not None: self.conversation_service.finalize_message( reserved_message_id, @@ -717,6 +721,7 @@ class BaseAnswerResource: shared_token=shared_token, attachment_ids=attachment_ids, metadata=query_metadata if query_metadata else None, + visibility=visibility, ) # Persist compression metadata/summary if it exists and wasn't saved mid-execution compression_meta = getattr(agent, "compression_metadata", None) @@ -823,7 +828,7 @@ class BaseAnswerResource: # else journal ``error`` so a reconnecting client sees a # failed terminal state instead of a blank "success". finalized_complete = False - if should_save_conversation and response_full: + if should_persist and response_full: try: if isNoneDoc: for doc in source_log_docs: @@ -903,6 +908,7 @@ class BaseAnswerResource: shared_token=shared_token, attachment_ids=attachment_ids, metadata=query_metadata if query_metadata else None, + visibility=visibility, ) # No journal row to gate, but flag the save as # successful for symmetry with the WAL path. diff --git a/application/api/answer/routes/stream.py b/application/api/answer/routes/stream.py index 1e7c4c69..18abb158 100644 --- a/application/api/answer/routes/stream.py +++ b/application/api/answer/routes/stream.py @@ -8,6 +8,7 @@ from application.api import api from application.api.answer.routes.base import answer_ns, BaseAnswerResource +from application.api.answer.services.persistence_policy import resolve_persistence from application.api.answer.services.stream_processor import StreamProcessor logger = logging.getLogger(__name__) @@ -135,6 +136,12 @@ class StreamResource(Resource, BaseAnswerResource): if error := self.check_usage(processor.agent_config): return error + should_persist, visibility = resolve_persistence( + display_flag=data.get("save_conversation"), + api_key=data.get("api_key"), + is_shared_usage=processor.is_shared_usage, + persist_flag=data.get("persist"), + ) return Response( self.complete_stream( question=data["question"], @@ -144,7 +151,8 @@ class StreamResource(Resource, BaseAnswerResource): decoded_token=processor.decoded_token, isNoneDoc=data.get("isNoneDoc"), index=data.get("index"), - should_save_conversation=data.get("save_conversation", True), + should_persist=should_persist, + visibility=visibility, attachment_ids=data.get("attachments", []), agent_id=processor.agent_id, is_shared_usage=processor.is_shared_usage, diff --git a/application/api/answer/services/conversation_service.py b/application/api/answer/services/conversation_service.py index 68d317cc..cedb94cc 100644 --- a/application/api/answer/services/conversation_service.py +++ b/application/api/answer/services/conversation_service.py @@ -78,6 +78,7 @@ class ConversationService: shared_token: Optional[str] = None, attachment_ids: Optional[List[str]] = None, metadata: Optional[Dict[str, Any]] = None, + visibility: str = "listed", ) -> str: """Save or update a conversation in Postgres. @@ -182,6 +183,7 @@ class ConversationService: if (resolved_agent_id and is_shared_usage) else None ), + visibility=visibility, ) conv_pg_id = str(conv["id"]) append_payload = dict(message_payload) @@ -202,6 +204,7 @@ class ConversationService: shared_token: Optional[str] = None, model_id: Optional[str] = None, request_id: Optional[str] = None, + visibility: str = "listed", status: str = "pending", index: Optional[int] = None, ) -> Dict[str, str]: @@ -257,6 +260,7 @@ class ConversationService: if (resolved_agent_id and is_shared_usage) else None ), + visibility=visibility, ) conv_pg_id = str(conv["id"]) diff --git a/application/api/answer/services/persistence_policy.py b/application/api/answer/services/persistence_policy.py new file mode 100644 index 00000000..c943a2ac --- /dev/null +++ b/application/api/answer/services/persistence_policy.py @@ -0,0 +1,45 @@ +"""Resolve whether an answer is persisted and whether it lists in the sidebar. + +Persistence (is a row written at all?) and visibility (does it show in the +owner's sidebar?) used to be one decision driven by the request-level +``save_conversation`` flag plus an identity heuristic. They are now separate: +conversations persist by default everywhere, while ``save_conversation`` only +controls sidebar visibility. +""" + +from typing import Optional, Tuple + +VISIBILITY_LISTED = "listed" +VISIBILITY_HIDDEN = "hidden" + + +def resolve_persistence( + *, + display_flag: Optional[bool], + api_key: Optional[str] = None, + is_shared_usage: bool = False, + persist_flag: Optional[bool] = None, +) -> Tuple[bool, str]: + """Resolve ``(should_persist, visibility)`` for an answer request. + + Args: + display_flag: Request-level ``save_conversation`` (``None`` if the + caller omitted it). Forces visibility on/off when provided. + api_key: Present for programmatic/agent callers (including the + OpenAI-compatible ``/v1`` endpoint), absent for first-party + session users. + is_shared_usage: True for shared-agent/widget traffic. + persist_flag: Explicit persistence opt-out (``False`` to skip writing + a row, e.g. stateless tool rounds that would orphan one). ``None`` + keeps the always-persist default. + + Returns: + ``(should_persist, visibility)`` where ``visibility`` is + ``"listed"`` or ``"hidden"``. + """ + should_persist = True if persist_flag is None else bool(persist_flag) + # Only genuine first-party interactive turns default to the sidebar. + default_display = api_key is None and not is_shared_usage + display = default_display if display_flag is None else bool(display_flag) + visibility = VISIBILITY_LISTED if display else VISIBILITY_HIDDEN + return should_persist, visibility diff --git a/application/api/user/conversations/routes.py b/application/api/user/conversations/routes.py index 239aba20..b723e6a6 100644 --- a/application/api/user/conversations/routes.py +++ b/application/api/user/conversations/routes.py @@ -76,7 +76,7 @@ class DeleteAllConversations(Resource): @conversations_ns.route("/get_conversations") class GetConversations(Resource): @api.doc( - description="Retrieve a list of the latest 30 conversations (excluding API key conversations)", + description="Retrieve a list of the latest 30 sidebar conversations (visibility = listed)", ) def get(self): decoded_token = request.decoded_token diff --git a/application/api/v1/routes.py b/application/api/v1/routes.py index 1b4bfb72..e0084187 100644 --- a/application/api/v1/routes.py +++ b/application/api/v1/routes.py @@ -15,6 +15,7 @@ from typing import Any, Dict, Generator, Optional from flask import Blueprint, jsonify, make_response, request, Response from application.api.answer.routes.base import BaseAnswerResource +from application.api.answer.services.persistence_policy import resolve_persistence from application.api.answer.services.stream_processor import StreamProcessor from application.api.v1.translator import ( translate_request, @@ -157,7 +158,14 @@ def chat_completions(): if usage_error: return usage_error - should_save_conversation = bool(internal_data.get("save_conversation", False)) + # v1 always persists (unless the translator opted out for a stateless + # tool round); the agent owner's sidebar only lists it on explicit + # ``docsgpt.save_conversation: true``. + should_persist, visibility = resolve_persistence( + display_flag=internal_data.get("save_conversation"), + api_key=internal_data.get("api_key"), + persist_flag=internal_data.get("persist"), + ) # Only strip leaked reasoning from content for structured requests -- the # only path where models echo reasoning into content -- so legitimate # answers that mention the marker text are never corrupted. @@ -174,7 +182,8 @@ def chat_completions(): processor, model_name, continuation, - should_save_conversation, + should_persist, + visibility, strip_reasoning_leak, ), mimetype="text/event-stream", @@ -191,7 +200,8 @@ def chat_completions(): processor, model_name, continuation, - should_save_conversation, + should_persist, + visibility, strip_reasoning_leak, ) @@ -222,7 +232,8 @@ def _stream_response( processor: StreamProcessor, model_name: str, continuation: Optional[Dict], - should_save_conversation: bool, + should_persist: bool, + visibility: str, strip_reasoning_leak: bool = False, ) -> Generator[str, None, None]: """Generate translated SSE chunks for streaming response.""" @@ -237,7 +248,8 @@ def _stream_response( agent_id=processor.agent_id, model_id=processor.model_id, model_user_id=processor.model_user_id, - should_save_conversation=should_save_conversation, + should_persist=should_persist, + visibility=visibility, _continuation=continuation, ) @@ -285,7 +297,8 @@ def _non_stream_response( processor: StreamProcessor, model_name: str, continuation: Optional[Dict], - should_save_conversation: bool, + should_persist: bool, + visibility: str, strip_reasoning_leak: bool = False, ) -> Response: """Collect full response and return as single JSON.""" @@ -298,7 +311,8 @@ def _non_stream_response( agent_id=processor.agent_id, model_id=processor.model_id, model_user_id=processor.model_user_id, - should_save_conversation=should_save_conversation, + should_persist=should_persist, + visibility=visibility, _continuation=continuation, ) diff --git a/application/api/v1/translator.py b/application/api/v1/translator.py index 94b4f9e7..349e1883 100644 --- a/application/api/v1/translator.py +++ b/application/api/v1/translator.py @@ -263,13 +263,15 @@ def translate_request( # rebuilt from the resent messages instead of server-side state. "messages": messages, } - # Stateful continuations (with a conversation_id) default to True so the - # final turn / WAL row is persisted. Stateless continuations (no - # conversation_id, e.g. opencode) default to False — otherwise every - # tool round persists an orphan conversation with an empty question. An - # explicit ``docsgpt.save_conversation`` override is honoured either way. + # Persistence: stateful continuations (carrying a conversation_id) + # persist the final turn; stateless ones (no conversation_id, e.g. + # opencode) skip it, else every tool round writes an orphan conversation + # with an empty question. ``docsgpt.persist`` overrides. Visibility is + # separate — hidden unless ``docsgpt.save_conversation`` opts in. + docsgpt_ext = data.get("docsgpt", {}) + result["persist"] = bool(docsgpt_ext.get("persist", bool(conversation_id))) result["save_conversation"] = bool( - data.get("docsgpt", {}).get("save_conversation", bool(conversation_id)) + docsgpt_ext.get("save_conversation", False) ) # Carry tools forward for next iteration if data.get("tools"): @@ -303,8 +305,8 @@ def translate_request( "question": question, "api_key": api_key, "history": json.dumps(history), - # Conversations are NOT persisted by default on the v1 endpoint. - # Callers opt in via ``docsgpt.save_conversation: true``. + # v1 conversations always persist; ``save_conversation`` only controls + # whether they list in the agent owner's sidebar (default hidden). "save_conversation": bool(docsgpt.get("save_conversation", False)), } diff --git a/application/storage/db/models.py b/application/storage/db/models.py index 8cccf97d..b0c96d7f 100644 --- a/application/storage/db/models.py +++ b/application/storage/db/models.py @@ -326,6 +326,9 @@ conversations_table = Table( Column("is_shared_usage", Boolean, nullable=False, server_default="false"), Column("shared_token", Text), Column("shared_with", ARRAY(Text), nullable=False, server_default="{}"), + # "listed" shows in the owner's sidebar; "hidden" persists silently + # (agent/API/OpenAI-compat traffic). See migration 0016. + Column("visibility", Text, nullable=False, server_default="listed"), Column("compression_metadata", JSONB), Column("date", DateTime(timezone=True), nullable=False, server_default=func.now()), Column("created_at", DateTime(timezone=True), nullable=False, server_default=func.now()), diff --git a/application/storage/db/repositories/conversations.py b/application/storage/db/repositories/conversations.py index 2e7c2c35..b8e3ee25 100644 --- a/application/storage/db/repositories/conversations.py +++ b/application/storage/db/repositories/conversations.py @@ -45,7 +45,7 @@ class MessageUpdateOutcome(str, Enum): def _message_row_to_dict(row) -> dict: """Like ``row_to_dict`` but renames the DB column ``message_metadata`` back to the public API key ``metadata`` so callers keep the Mongo-era - shape. See migration 0016 for the column rename rationale.""" + shape. See migration 0001_initial for the column rename rationale.""" out = row_to_dict(row) if "message_metadata" in out: out["metadata"] = out.pop("message_metadata") @@ -149,6 +149,7 @@ class ConversationsRepository: api_key: str | None = None, is_shared_usage: bool = False, shared_token: str | None = None, + visibility: str = "listed", legacy_mongo_id: str | None = None, ) -> dict: """Create a new conversation. @@ -162,6 +163,7 @@ class ConversationsRepository: values: dict = { "user_id": user_id, "name": name, + "visibility": visibility, } # ``agent_id`` may arrive as a Mongo ObjectId during the dual-write # window; resolve to a UUID (or drop silently if not yet backfilled). @@ -247,15 +249,16 @@ class ConversationsRepository: return row_to_dict(row) if row is not None else None def list_for_user(self, user_id: str, limit: int = 30) -> list[dict]: - """List conversations for a user, most recent first. + """List a user's sidebar conversations, most recent first. - Mirrors the Mongo query: either no api_key or agent_id exists. + Only ``visibility = 'listed'`` rows surface; agent/API/OpenAI-compat + traffic persists as ``'hidden'`` and is excluded. """ result = self._conn.execute( text( "SELECT * FROM conversations " "WHERE user_id = :user_id " - "AND (api_key IS NULL OR agent_id IS NOT NULL) " + "AND visibility = 'listed' " "ORDER BY date DESC LIMIT :limit" ), {"user_id": user_id, "limit": limit}, @@ -302,7 +305,7 @@ class ConversationsRepository: " LIMIT 1 " ") mt ON TRUE " "WHERE c.user_id = :user_id " - "AND (c.api_key IS NULL OR c.agent_id IS NOT NULL) " + "AND c.visibility = 'listed' " "ORDER BY c.date DESC LIMIT :limit" ), { diff --git a/tests/api/answer/routes/test_base.py b/tests/api/answer/routes/test_base.py index 0f1b48ca..63745692 100644 --- a/tests/api/answer/routes/test_base.py +++ b/tests/api/answer/routes/test_base.py @@ -139,7 +139,7 @@ class TestCompleteStreamMethod: conversation_id=None, user_api_key=None, decoded_token=decoded_token, - should_save_conversation=False, + should_persist=False, ) ) @@ -171,7 +171,7 @@ class TestCompleteStreamMethod: conversation_id=None, user_api_key=None, decoded_token=decoded_token, - should_save_conversation=False, + should_persist=False, ) ) @@ -197,7 +197,7 @@ class TestCompleteStreamMethod: conversation_id=None, user_api_key=None, decoded_token=decoded_token, - should_save_conversation=False, + should_persist=False, ) ) @@ -239,7 +239,7 @@ class TestCompleteStreamMethod: conversation_id=None, user_api_key=None, decoded_token=decoded_token, - should_save_conversation=True, + should_persist=True, ) ) @@ -299,7 +299,7 @@ class TestCompleteStreamMethod: conversation_id=None, # caller had no conv yet user_api_key=None, decoded_token={"sub": "user-prop"}, - should_save_conversation=True, + should_persist=True, ) ) @@ -330,7 +330,7 @@ class TestCompleteStreamMethod: conversation_id="conv-1", user_api_key=None, decoded_token={"sub": "user123"}, - should_save_conversation=True, + should_persist=True, ) ) diff --git a/tests/api/answer/services/test_persistence_policy.py b/tests/api/answer/services/test_persistence_policy.py new file mode 100644 index 00000000..d3bde387 --- /dev/null +++ b/tests/api/answer/services/test_persistence_policy.py @@ -0,0 +1,44 @@ +"""Unit tests for ``resolve_persistence`` (persist + sidebar visibility).""" + +import pytest + +from application.api.answer.services.persistence_policy import ( + VISIBILITY_HIDDEN, + VISIBILITY_LISTED, + resolve_persistence, +) + + +@pytest.mark.unit +class TestResolvePersistence: + def test_first_party_default_lists(self): + persist, visibility = resolve_persistence(display_flag=None) + assert persist is True + assert visibility == VISIBILITY_LISTED + + def test_api_key_caller_defaults_hidden(self): + persist, visibility = resolve_persistence(display_flag=None, api_key="k-1") + assert persist is True + assert visibility == VISIBILITY_HIDDEN + + def test_shared_usage_defaults_hidden(self): + _, visibility = resolve_persistence(display_flag=None, is_shared_usage=True) + assert visibility == VISIBILITY_HIDDEN + + def test_explicit_display_true_lists_api_key_caller(self): + _, visibility = resolve_persistence(display_flag=True, api_key="k-1") + assert visibility == VISIBILITY_LISTED + + def test_explicit_display_false_hides_first_party(self): + persist, visibility = resolve_persistence(display_flag=False) + # Still persisted, just not shown in the sidebar. + assert persist is True + assert visibility == VISIBILITY_HIDDEN + + def test_persist_opt_out(self): + persist, _ = resolve_persistence(display_flag=None, persist_flag=False) + assert persist is False + + def test_persist_defaults_true_for_hidden_api_caller(self): + persist, _ = resolve_persistence(display_flag=False, api_key="k-1") + assert persist is True diff --git a/tests/api/answer/test_base_routes.py b/tests/api/answer/test_base_routes.py index 1c4a7f9a..8da90cc3 100644 --- a/tests/api/answer/test_base_routes.py +++ b/tests/api/answer/test_base_routes.py @@ -98,7 +98,7 @@ class TestCompleteStreamToolCalls: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) tool_chunks = [s for s in stream if '"type": "tool_calls"' in s] @@ -124,7 +124,7 @@ class TestCompleteStreamToolCalls: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) thought_chunks = [s for s in stream if '"type": "thought"' in s] @@ -159,7 +159,7 @@ class TestCompleteStreamStructuredOutput: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) structured_chunks = [ @@ -192,7 +192,7 @@ class TestCompleteStreamMetadata: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) # Should not crash, metadata handled silently @@ -225,7 +225,7 @@ class TestCompleteStreamIsNoneDoc: user_api_key=None, decoded_token={"sub": "u"}, isNoneDoc=True, - should_save_conversation=False, + should_persist=False, ) ) # Verify stream completes without error @@ -256,7 +256,7 @@ class TestCompleteStreamErrorType: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) error_chunks = [s for s in stream if '"type": "error"' in s] @@ -281,7 +281,7 @@ class TestCompleteStreamErrorType: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) custom_chunks = [s for s in stream if '"type": "custom_event"' in s] @@ -426,7 +426,7 @@ class TestCompleteStreamCompressionMetadata: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -470,7 +470,7 @@ class TestCompleteStreamCompressionMetadata: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -501,7 +501,7 @@ class TestCompleteStreamLogTruncation: conversation_id=None, user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=False, + should_persist=False, ) ) @@ -545,7 +545,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) @@ -586,7 +586,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", isNoneDoc=True, ) @@ -628,7 +628,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) @@ -688,7 +688,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) # Drain the early ``message_id`` event, then close before @@ -770,7 +770,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) next(gen) # message_id frame @@ -850,7 +850,7 @@ class TestCompleteStreamGeneratorExit: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) next(gen) # message_id frame @@ -951,7 +951,7 @@ class TestCompleteStreamWalAcceptance: conversation_id=None, user_api_key=None, decoded_token={"sub": "u-acceptance"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -1023,7 +1023,7 @@ class TestCompleteStreamWalAcceptance: conversation_id=None, user_api_key=None, decoded_token={"sub": "u-tap"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -1103,7 +1103,7 @@ class TestCompleteStreamWalAcceptance: conversation_id=str(conv_id), user_api_key=None, decoded_token={"sub": user_id}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", _continuation={ "messages": [], @@ -1153,7 +1153,7 @@ class TestCompleteStreamWalAcceptance: conversation_id=None, user_api_key=None, decoded_token={"sub": "u-request-id"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -1215,7 +1215,7 @@ class TestStreamingHeartbeatSeed: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) @@ -1270,7 +1270,7 @@ class TestStreamingHeartbeatSeed: conversation_id="conv1", user_api_key=None, decoded_token={"sub": "u"}, - should_save_conversation=True, + should_persist=True, model_id="gpt-4", ) ) diff --git a/tests/e2e/specs/tier-a/chat-turn.spec.ts b/tests/e2e/specs/tier-a/chat-turn.spec.ts index b8108730..f2871304 100644 --- a/tests/e2e/specs/tier-a/chat-turn.spec.ts +++ b/tests/e2e/specs/tier-a/chat-turn.spec.ts @@ -248,7 +248,7 @@ test.describe('tier-a · chat turn save', () => { } }); - test('save_conversation=false skips all four tables', async ({ browser }) => { + test('save_conversation=false persists a hidden conversation (kept out of the sidebar)', async ({ browser }) => { const { sub, token } = await newUserContext(browser); // Sanity: the user has nothing yet. @@ -266,23 +266,30 @@ test.describe('tier-a · chat turn save', () => { isNoneDoc: true, }); expect(result.status).toBe(200); - // When save_conversation=false the route emits `{"type":"id","id":"None"}` - // (stringified None) — no real UUID persisted. - expect(result.conversationId).toBe('None'); + // save_conversation now controls sidebar visibility, not persistence: + // the turn persists, so a real conversation UUID is emitted. + expect(result.conversationId).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, + ); - // Key invariant: conversations + conversation_messages must be empty - // for this user. token_usage still fires (it's per-LLM-call, not per - // conversation); user_logs also fires (audit log). We only assert on - // the two "persistent chat state" tables. + // The conversation + its messages persist... expect( await countRows('conversations', { sql: 'user_id = $1', params: [sub], }), - ).toBe(0); - + ).toBe(1); const msgs = await fetchMessagesForUser(sub); - expect(msgs).toHaveLength(0); + expect(msgs.length).toBeGreaterThan(0); + + // ...but it is hidden, so it never surfaces in the sidebar + // (list_for_user filters visibility = 'listed'). + expect( + await countRows('conversations', { + sql: "user_id = $1 AND visibility = 'listed'", + params: [sub], + }), + ).toBe(0); }); test('silent-break: two concurrent /stream calls on the same conversation_id — neither 500s and positions stay unique', async ({ diff --git a/tests/e2e/specs/tier-b/agent-preview.spec.ts b/tests/e2e/specs/tier-b/agent-preview.spec.ts index 0b86f0fd..dd1a536d 100644 --- a/tests/e2e/specs/tier-b/agent-preview.spec.ts +++ b/tests/e2e/specs/tier-b/agent-preview.spec.ts @@ -1,27 +1,26 @@ /** - * Tier-B · agent preview does NOT persist conversations + * Tier-B · agent preview persists a HIDDEN conversation * * Covers B14. The preview flow in `frontend/src/agents/agentPreviewSlice.ts` - * calls `handleFetchAnswerSteaming(..., save_conversation=false)` — see the - * `false` positional arg at line 126. That flag flows through - * `POST /stream` (application/api/answer/routes/stream.py:55-59,142) into - * `BaseAnswerResource.complete_stream(should_save_conversation=...)` - * (application/api/answer/routes/base.py:174,290,389,479), where the - * branches that insert `conversations` / `conversation_messages` rows are - * gated on `should_save_conversation`. + * calls `handleFetchAnswerSteaming(..., save_conversation=false)`. As of the + * conversation-visibility change, `save_conversation` no longer gates + * persistence — it controls sidebar visibility only. A preview turn therefore + * persists a `conversations` row with `visibility = 'hidden'` (decided by + * `resolve_persistence` in + * application/api/answer/services/persistence_policy.py and threaded through + * `BaseAnswerResource.complete_stream(visibility=...)`). * - * The load-bearing assertion is: after a preview stream completes, the - * `conversations` table still has zero rows for this user. If a - * regression drops the `save_conversation` flag (or flips the default), - * preview traffic would silently pollute the user's sidebar history. + * The load-bearing assertion is: after a preview stream completes, the row + * exists but is `hidden`, so it is excluded from the sidebar query + * (`ConversationsRepository.list_for_user`, which filters `visibility = + * 'listed'`). If a regression flips the preview default to `listed`, preview + * traffic would pollute the user's sidebar history. * * API-driven: the frontend preview UI ultimately POSTs /stream with * `save_conversation:false` and `conversation_id:null`; we exercise that - * contract directly to keep the spec fast and isolated from the - * AgentPreview modal's rendering quirks. A standard chat POST - * (save_conversation defaulted to true) is included as a control — it - * MUST write a conversation row, so "preview wrote zero" is a meaningful - * contrast rather than "nothing ever writes". + * contract directly to keep the spec fast and isolated from the AgentPreview + * modal's rendering quirks. A standard chat POST (save_conversation omitted) + * is the control — it writes a `listed` row that DOES surface in the sidebar. */ import * as playwright from '@playwright/test'; @@ -58,6 +57,16 @@ async function countUserConversations(userSub: string): Promise { return Number(rows[0]?.n ?? 0); } +/** Conversations that surface in the sidebar (the `list_for_user` filter). */ +async function countListedConversations(userSub: string): Promise { + const { rows } = await pg.query<{ n: string }>( + `SELECT count(*)::text AS n FROM conversations + WHERE user_id = $1 AND visibility = 'listed'`, + [userSub], + ); + return Number(rows[0]?.n ?? 0); +} + async function countConversationMessages(userSub: string): Promise { const { rows } = await pg.query<{ n: string }>( `SELECT count(*)::text AS n @@ -71,10 +80,9 @@ async function countConversationMessages(userSub: string): Promise { /** * POST /stream with save_conversation=false and drain the SSE body. The - * stream emits `{type:"id", id:"None"}` in preview mode (preview has no - * conversation_id to echo back) — we can't use `streamOnce` here because - * that helper insists on a UUID-shaped id. Instead, just drain the - * response and assert 200. + * stream emits `{type:"id", id:""}` once the (hidden) conversation is + * persisted. Sidebar membership is asserted via the DB helpers below rather + * than the stream id, so we just drain the response and assert 200. */ async function drainStream( api: import('@playwright/test').APIRequestContext, @@ -96,7 +104,7 @@ test.describe('tier-b · agent preview', () => { await resetDb(); }); - test('preview stream with save_conversation=false writes NO conversations row', async ({ + test('preview stream with save_conversation=false persists a hidden conversation excluded from the sidebar', async ({ browser, }) => { const { context, sub, token } = await newUserContext(browser); @@ -118,14 +126,19 @@ test.describe('tier-b · agent preview', () => { chunks: '0', }); - // Core assertion: preview MUST NOT persist to conversations. + // The turn persists (a row + messages exist)... expect( await countUserConversations(sub), - 'preview stream must not write a conversations row', - ).toBe(0); + 'preview stream persists a conversation row', + ).toBe(1); expect( await countConversationMessages(sub), - 'preview stream must not write conversation_messages rows', + 'preview stream persists conversation_messages rows', + ).toBeGreaterThan(0); + // ...but it is hidden, so the sidebar query returns nothing. + expect( + await countListedConversations(sub), + 'preview conversation must not surface in the sidebar', ).toBe(0); } finally { await api.dispose(); @@ -133,7 +146,7 @@ test.describe('tier-b · agent preview', () => { } }); - test('control: a non-preview stream (save_conversation omitted) DOES write a conversations row', async ({ + test('control: a non-preview stream (save_conversation omitted) writes a LISTED conversations row', async ({ browser, }) => { const { context, sub, token } = await newUserContext(browser); @@ -142,10 +155,10 @@ test.describe('tier-b · agent preview', () => { const agentId = await createDraftAgent(sub, 'non-preview-target'); expect(await countUserConversations(sub)).toBe(0); - // Standard chat path — save_conversation defaults to true at the - // backend (stream_model field default + get() fallback in stream.py). - // This is the "control" that makes the preview assertion meaningful: - // without it, a spec where nothing ever writes could pass trivially. + // Standard chat path — save_conversation omitted, so a first-party + // interactive turn defaults to `listed` (resolve_persistence). This is + // the "control" that makes the preview assertion meaningful: without + // it, a spec where nothing ever surfaces could pass trivially. const conversationId = await streamOnce(api, { question: 'Hello persistence', conversation_id: null, @@ -157,8 +170,9 @@ test.describe('tier-b · agent preview', () => { /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, ); - // Exactly one conversation row landed. + // Exactly one conversation row landed, and it surfaces in the sidebar. expect(await countUserConversations(sub)).toBe(1); + expect(await countListedConversations(sub)).toBe(1); expect(await countConversationMessages(sub)).toBeGreaterThan(0); } finally { await api.dispose(); @@ -166,7 +180,7 @@ test.describe('tier-b · agent preview', () => { } }); - test('multiple preview turns still produce zero conversations rows', async ({ + test('multiple preview turns persist hidden rows that never surface in the sidebar', async ({ browser, }) => { const { context, sub, token } = await newUserContext(browser); @@ -186,10 +200,11 @@ test.describe('tier-b · agent preview', () => { }); } - // Still zero — no preview turn ever persists, regardless of how - // many times the user clicks "Test message". - expect(await countUserConversations(sub)).toBe(0); - expect(await countConversationMessages(sub)).toBe(0); + // Each turn persisted its own hidden conversation... + expect(await countUserConversations(sub)).toBe(3); + // ...yet the sidebar stays empty no matter how many "Test message" + // clicks the user makes. + expect(await countListedConversations(sub)).toBe(0); } finally { await api.dispose(); await context.close(); diff --git a/tests/storage/db/repositories/test_conversations.py b/tests/storage/db/repositories/test_conversations.py index 7f5dadbd..ddcb2135 100644 --- a/tests/storage/db/repositories/test_conversations.py +++ b/tests/storage/db/repositories/test_conversations.py @@ -75,14 +75,28 @@ class TestListForUser: assert len(results) == 2 assert all(r["user_id"] == "alice" for r in results) - def test_excludes_api_key_without_agent(self, pg_conn): + def test_excludes_hidden_conversations(self, pg_conn): repo = _repo(pg_conn) repo.create("alice", "normal") - repo.create("alice", "api-only", api_key="key-1") + repo.create("alice", "hidden-one", visibility="hidden") results = repo.list_for_user("alice") assert len(results) == 1 assert results[0]["name"] == "normal" + def test_lists_api_key_conversation_when_listed(self, pg_conn): + # Visibility is explicit now: an api_key conversation created as + # ``listed`` surfaces (the old api_key/agent_id heuristic is gone). + repo = _repo(pg_conn) + repo.create("alice", "api-listed", api_key="key-1", visibility="listed") + results = repo.list_for_user("alice") + assert len(results) == 1 + assert results[0]["name"] == "api-listed" + + def test_create_defaults_to_listed(self, pg_conn): + repo = _repo(pg_conn) + conv = repo.create("alice", "default-vis") + assert conv["visibility"] == "listed" + class TestRename: def test_renames(self, pg_conn): diff --git a/tests/test_v1_translator.py b/tests/test_v1_translator.py index 1249ff27..f1eace7d 100644 --- a/tests/test_v1_translator.py +++ b/tests/test_v1_translator.py @@ -214,7 +214,8 @@ class TestTranslateRequest: result = translate_request(data, "test-key") assert result["question"] == "What's 2+2?" assert result["api_key"] == "test-key" - # Conversations are not persisted by default on the v1 endpoint. + # v1 conversations persist; ``save_conversation`` only controls whether + # they list in the sidebar (default off). assert result["save_conversation"] is False history = json.loads(result["history"]) assert len(history) == 1 @@ -265,7 +266,7 @@ class TestTranslateRequest: ], } result = translate_request(data, "key") - assert result["save_conversation"] is True + assert result["persist"] is True def test_stateless_continuation_does_not_persist_by_default(self): """A stateless continuation (no conversation_id, e.g. an OpenAI client @@ -282,7 +283,25 @@ class TestTranslateRequest: ], } result = translate_request(data, "key") - assert result["save_conversation"] is False + assert result["persist"] is False + + def test_stateful_continuation_persist_override_via_docsgpt(self): + """``docsgpt.persist=false`` forces no persistence even with a + conversation_id.""" + data = { + "conversation_id": "conv-1", + "docsgpt": {"persist": False}, + "messages": [ + {"role": "user", "content": "Search for X"}, + { + "role": "assistant", + "tool_calls": [{"id": "c1", "type": "function", "function": {"name": "search", "arguments": "{}"}}], + }, + {"role": "tool", "tool_call_id": "c1", "content": "done"}, + ], + } + result = translate_request(data, "key") + assert result["persist"] is False def test_continuation_honours_explicit_save_conversation_override(self): """An explicit docsgpt.save_conversation=false on the continuation wins."""