diff --git a/src/agent_manager/api/routes/conversations.py b/src/agent_manager/api/routes/conversations.py index 1dfbc37f..d202538e 100644 --- a/src/agent_manager/api/routes/conversations.py +++ b/src/agent_manager/api/routes/conversations.py @@ -22,6 +22,8 @@ ConversationSummary, CreateConversationRequest, CreateConversationResponse, + MessageFeedbackRequest, + MessageFeedbackResponse, MessageOut, PaginatedConversationsResponse, SendMessageRequest, @@ -84,6 +86,7 @@ async def list_messages( content=message.content, status=message.status, created_at=message.created_at, + feedback=message.feedback, ) for message in messages ] @@ -105,6 +108,28 @@ async def get_usage( ) +@router.post( + "/conversations/{conversation_id}/messages/{message_id}/feedback", + response_model=MessageFeedbackResponse, +) +async def set_message_feedback( + conversation_id: str, + message_id: str, + body: MessageFeedbackRequest, + service: Service, + caller: Caller, +) -> MessageFeedbackResponse: + with as_http_error(): + updated = await service.set_message_feedback( + conversation_id, message_id, body.feedback, caller + ) + if updated is None: + from fastapi import HTTPException + + raise HTTPException(status_code=404, detail="message not found") + return MessageFeedbackResponse(message_id=updated.message_id, feedback=updated.feedback) + + @router.post("/conversations/{conversation_id}/messages", response_model=SendMessageResponse) async def send_message( conversation_id: str, diff --git a/src/agent_manager/api/schemas.py b/src/agent_manager/api/schemas.py index 7cfb2d70..2b620f69 100644 --- a/src/agent_manager/api/schemas.py +++ b/src/agent_manager/api/schemas.py @@ -59,6 +59,16 @@ class MessageOut(BaseModel): content: str status: str created_at: datetime + feedback: str | None = None + + +class MessageFeedbackRequest(BaseModel): + feedback: str = Field(..., pattern="^(thumbs_up|thumbs_down)$") + + +class MessageFeedbackResponse(BaseModel): + message_id: str + feedback: str | None = None class SendMessageRequest(BaseModel): diff --git a/src/agent_manager/api/static/widget/api/AgentChatClient.ts b/src/agent_manager/api/static/widget/api/AgentChatClient.ts index 504d86b8..e3af509b 100644 --- a/src/agent_manager/api/static/widget/api/AgentChatClient.ts +++ b/src/agent_manager/api/static/widget/api/AgentChatClient.ts @@ -164,6 +164,21 @@ export class AgentChatClient { }; } + async setMessageFeedback( + conversationId: string, + messageId: string, + feedback: "thumbs_up" | "thumbs_down", + ): Promise<{ message_id: string; feedback: string | null }> { + const response = await this.request( + `/conversations/${conversationId}/messages/${messageId}/feedback`, + { + method: "POST", + body: JSON.stringify({ feedback }), + }, + ); + return await response.json(); + } + async *streamMessage( conversationId: string, message: string, diff --git a/src/agent_manager/api/static/widget/react/AgentChatApp.tsx b/src/agent_manager/api/static/widget/react/AgentChatApp.tsx index 0c528f9f..77c9140d 100644 --- a/src/agent_manager/api/static/widget/react/AgentChatApp.tsx +++ b/src/agent_manager/api/static/widget/react/AgentChatApp.tsx @@ -5,6 +5,8 @@ import { CopyIcon, HistoryIcon, SquarePenIcon, + ThumbsDownIcon, + ThumbsUpIcon, XIcon, } from "lucide-react"; import { type Ref, useCallback, useEffect, useMemo, useRef, useState } from "react"; @@ -71,6 +73,7 @@ const toEntries = (message: ChatMessage): MessageEntry[] => { runId: message.run_id ?? undefined, role: message.role === "user" ? "user" : "ai", text: message.content, + feedback: message.feedback, }; return message.role === "user" && message.status === "cancelled" ? [entry, { id: newId(), role: "ai", text: "", status: "cancelled" }] @@ -709,6 +712,7 @@ export function AgentChatApp({ void decideApproval(activeId, entry, approval, decision) } @@ -716,6 +720,9 @@ export function AgentChatApp({ void cancelApproval(activeId, entry.id, approval) } onEdit={() => editMessage(entry)} + onFeedback={(messageId, feedback) => + void conversation.setMessageFeedback(activeId, messageId, feedback) + } editable={canEdit} /> ))} @@ -797,15 +804,19 @@ function Launcher({ function ChatMessage({ entry, + conversationId, onApproval, onCancelApproval, onEdit, + onFeedback, editable, }: { entry: MessageEntry; + conversationId?: string; onApproval: (approval: PendingApproval, decision: ApprovalDecision) => void; onCancelApproval: (approval: PendingApproval) => void; onEdit: () => void; + onFeedback: (messageId: string, feedback: "thumbs_up" | "thumbs_down") => void; editable: boolean; }) { const from = entry.role === "user" ? "user" : "assistant"; @@ -869,7 +880,7 @@ function ChatMessage({ {entry.text} ) : null} - {entry.text.trim() ? : null} + {entry.text.trim() ? : null} )} @@ -957,10 +968,60 @@ function ThinkingDots() { ); } -function MessageActions({ text }: { text: string }) { +function MessageActions({ + text, + feedback, + messageId, + conversationId, + onFeedback, +}: { + text: string; + feedback?: "thumbs_up" | "thumbs_down"; + messageId?: string; + conversationId?: string; + onFeedback?: (messageId: string, feedback: "thumbs_up" | "thumbs_down") => void; +}) { + const handleThumbsUp = useCallback(() => { + if (!messageId || !conversationId || !onFeedback) return; + const next = feedback === "thumbs_up" ? null : "thumbs_up"; + if (next) { + void onFeedback(messageId, next); + } + }, [messageId, conversationId, onFeedback, feedback]); + + const handleThumbsDown = useCallback(() => { + if (!messageId || !conversationId || !onFeedback) return; + const next = feedback === "thumbs_down" ? null : "thumbs_down"; + if (next) { + void onFeedback(messageId, next); + } + }, [messageId, conversationId, onFeedback, feedback]); + return (
+ {messageId && conversationId && onFeedback ? ( +
+ + +
+ ) : null}
); } diff --git a/src/agent_manager/api/static/widget/react/useConversation.ts b/src/agent_manager/api/static/widget/react/useConversation.ts index 02849442..e3adef7c 100644 --- a/src/agent_manager/api/static/widget/react/useConversation.ts +++ b/src/agent_manager/api/static/widget/react/useConversation.ts @@ -56,6 +56,11 @@ export interface Conversation { listThreads(limit?: number, cursor?: string | null): Promise; switchTo(conversationId: string): void; startNew(): void; + setMessageFeedback( + conversationId: string, + messageId: string, + feedback: "thumbs_up" | "thumbs_down", + ): Promise<{ message_id: string; feedback: string | null }>; } /** A stored conversation the server will not serve us: gone (404), or owned by @@ -199,6 +204,12 @@ export function useConversation( [endpoint], ); + const setMessageFeedback = useCallback( + (conversationId: string, messageId: string, feedback: "thumbs_up" | "thumbs_down") => + client.setMessageFeedback(conversationId, messageId, feedback), + [client], + ); + return useMemo( () => ({ peekId, @@ -213,6 +224,7 @@ export function useConversation( listThreads, switchTo, startNew, + setMessageFeedback, }), [ peekId, @@ -227,6 +239,7 @@ export function useConversation( listThreads, switchTo, startNew, + setMessageFeedback, ], ); } diff --git a/src/agent_manager/api/static/widget/types.ts b/src/agent_manager/api/static/widget/types.ts index 13339a69..9e315f0b 100644 --- a/src/agent_manager/api/static/widget/types.ts +++ b/src/agent_manager/api/static/widget/types.ts @@ -45,6 +45,7 @@ export interface ChatMessage { content: string; status: string; created_at?: string; + feedback?: "thumbs_up" | "thumbs_down"; } export type BudgetSeverity = "normal" | "warning" | "critical"; @@ -72,6 +73,7 @@ export interface MessageEntry { approvalSubmitting?: boolean; approvalCancelling?: boolean; approvalError?: string; + feedback?: "thumbs_up" | "thumbs_down"; } export interface PendingApproval { diff --git a/src/agent_manager/application/conversation_service.py b/src/agent_manager/application/conversation_service.py index 6e22a3db..2e1f1334 100644 --- a/src/agent_manager/application/conversation_service.py +++ b/src/agent_manager/application/conversation_service.py @@ -167,6 +167,16 @@ async def usage(self, conversation_id: str, principal: Principal) -> TokenBudget used = await self._repository.get_token_usage(conversation_id) return TokenBudgetUsage.from_totals(used, self._max_tokens) + async def set_message_feedback( + self, + conversation_id: str, + message_id: str, + feedback: str, + principal: Principal, + ) -> ConversationMessage | None: + await self._authorize(conversation_id, principal) + return await self._repository.update_message_feedback(message_id, feedback) + async def list_conversations( self, principal: Principal, page: PageRequest | None = None ) -> Page[ConversationSession]: diff --git a/src/agent_manager/domain/models.py b/src/agent_manager/domain/models.py index 831d09c9..6539293d 100644 --- a/src/agent_manager/domain/models.py +++ b/src/agent_manager/domain/models.py @@ -108,6 +108,7 @@ class ConversationMessage: status: str = "succeeded" error_type: str | None = None metadata: dict[str, Any] = field(default_factory=dict) + feedback: str | None = None @dataclass(frozen=True) diff --git a/src/agent_manager/domain/repository.py b/src/agent_manager/domain/repository.py index 02e5816a..0fed34f8 100644 --- a/src/agent_manager/domain/repository.py +++ b/src/agent_manager/domain/repository.py @@ -192,3 +192,15 @@ async def add_message(self, conversation_id: str, role: Role, content: str) -> N async def list_messages(self, conversation_id: str, limit: int | None = None) -> list[Message]: """Messages oldest-first. With `limit`, the most recent `limit`, still oldest-first.""" + + @abstractmethod + async def update_message_feedback( + self, + message_id: str, + feedback: str, + ) -> ConversationMessage | None: + """Persist a 👍/👎 vote on a single assistant message. + + Returns the updated message, or ``None`` if the message does not exist. + """ + ... diff --git a/src/agent_manager/infrastructure/persistence/memory_repository.py b/src/agent_manager/infrastructure/persistence/memory_repository.py index fcf08ad2..ab2c0a81 100644 --- a/src/agent_manager/infrastructure/persistence/memory_repository.py +++ b/src/agent_manager/infrastructure/persistence/memory_repository.py @@ -301,6 +301,20 @@ async def list_messages(self, conversation_id: str, limit: int | None = None) -> msgs = await self.list_conversation_messages(conversation_id, limit) return [Message(role=m.role, content=m.content, created_at=m.created_at) for m in msgs] + async def update_message_feedback( + self, + message_id: str, + feedback: str, + ) -> ConversationMessage | None: + for _sid, messages in list(self._messages.items()): + for index, message in enumerate(messages): + if message.message_id == message_id: + metadata = {**message.metadata, "feedback": feedback} + updated = replace(message, feedback=feedback, metadata=metadata) + messages[index] = updated + return updated + return None + async def get_snapshot(self, session_id: str) -> ConversationSnapshot | None: return self._snapshots.get(session_id) diff --git a/src/agent_manager/infrastructure/persistence/sql_repository.py b/src/agent_manager/infrastructure/persistence/sql_repository.py index 30b3560e..e9512504 100644 --- a/src/agent_manager/infrastructure/persistence/sql_repository.py +++ b/src/agent_manager/infrastructure/persistence/sql_repository.py @@ -383,6 +383,23 @@ async def list_messages(self, conversation_id: str, limit: int | None = None) -> Message(role=row.role, content=row.content, created_at=row.created_at) for row in rows ] + async def update_message_feedback( + self, + message_id: str, + feedback: str, + ) -> ConversationMessage | None: + async with self._sessions() as session: + row = await session.get(ConversationMessageRow, message_id) + if row is None: + return None + metadata = dict(row.metadata_json or {}) + metadata["feedback"] = feedback + row.metadata_json = metadata + session.add(row) + await session.commit() + await session.refresh(row) + return _message(row) + async def get_snapshot(self, session_id: str) -> ConversationSnapshot | None: async with self._sessions() as session: row = await session.get(ConversationSnapshotRow, session_id) @@ -601,6 +618,9 @@ def _session(row: ConversationSessionRow) -> ConversationSession: def _message_row(message: ConversationMessage) -> ConversationMessageRow: + metadata = dict(message.metadata) + if message.feedback is not None: + metadata["feedback"] = message.feedback return ConversationMessageRow( message_id=message.message_id, session_id=message.session_id, @@ -621,12 +641,13 @@ def _message_row(message: ConversationMessage) -> ConversationMessageRow: latency_ms=message.latency_ms, status=message.status, error_type=message.error_type, - metadata_json=dict(message.metadata), + metadata_json=metadata, created_at=message.created_at, ) def _message(row: ConversationMessageRow) -> ConversationMessage: + metadata = dict(row.metadata_json or {}) return ConversationMessage( message_id=row.message_id, session_id=row.session_id, @@ -647,7 +668,8 @@ def _message(row: ConversationMessageRow) -> ConversationMessage: latency_ms=row.latency_ms, status=row.status, error_type=row.error_type, - metadata=dict(row.metadata_json or {}), + metadata=metadata, + feedback=metadata.get("feedback"), created_at=ensure_utc(row.created_at) or row.created_at, ) @@ -666,6 +688,7 @@ def _message_json(row: ConversationMessageRow) -> dict[str, Any]: "status": row.status, "created_at": (ensure_utc(row.created_at) or row.created_at).isoformat(), "metadata": dict(row.metadata_json or {}), + "feedback": (row.metadata_json or {}).get("feedback"), } diff --git a/tests/agent_manager/test_api.py b/tests/agent_manager/test_api.py index 408abb58..fb9025c0 100644 --- a/tests/agent_manager/test_api.py +++ b/tests/agent_manager/test_api.py @@ -1434,3 +1434,50 @@ def test_tool_error_text_is_sanitized_in_stream_message() -> None: assert response.status_code == 200 assert "Tool execution failed" in response.text assert "localhost" not in response.text + + +def test_set_message_feedback_returns_updated_message(client: TestClient) -> None: + cid = client.post("/conversations").json()["conversation_id"] + sent = client.post(f"/conversations/{cid}/messages", json={"message": "hello"}) + assert sent.status_code == 200 + messages = client.get(f"/conversations/{cid}/messages").json() + assistant = next(m for m in messages if m["role"] == "assistant") + message_id = assistant["message_id"] + + response = client.post( + f"/conversations/{cid}/messages/{message_id}/feedback", + json={"feedback": "thumbs_up"}, + ) + assert response.status_code == 200 + body = response.json() + assert body["message_id"] == message_id + assert body["feedback"] == "thumbs_up" + + +def test_set_message_feedback_returns_404_for_missing_message(client: TestClient) -> None: + cid = client.post("/conversations").json()["conversation_id"] + + response = client.post( + f"/conversations/{cid}/messages/no-such-id/feedback", + json={"feedback": "thumbs_up"}, + ) + assert response.status_code == 404 + + +def test_another_caller_cannot_set_feedback_on_anothers_conversation( + client: TestClient, +) -> None: + u1 = bearer("u1") + cid = client.post("/conversations", headers=u1).json()["conversation_id"] + client.post(f"/conversations/{cid}/messages", json={"message": "hello"}, headers=u1) + messages = client.get(f"/conversations/{cid}/messages", headers=u1).json() + assistant = next(m for m in messages if m["role"] == "assistant") + message_id = assistant["message_id"] + + u2 = bearer("u2") + response = client.post( + f"/conversations/{cid}/messages/{message_id}/feedback", + json={"feedback": "thumbs_up"}, + headers=u2, + ) + assert response.status_code == 403 diff --git a/tests/agent_manager/test_service.py b/tests/agent_manager/test_service.py index 073b6aa4..5d87a6d0 100644 --- a/tests/agent_manager/test_service.py +++ b/tests/agent_manager/test_service.py @@ -479,3 +479,37 @@ async def test_adopting_merges_into_conversations_the_account_already_had() -> N "signed-in", "pre-login", } + + +async def test_set_message_feedback_persists_and_returns_message() -> None: + service, _ = _service() + await service.create(ALICE, session_id="s1") + await service.send("s1", "hello", ALICE) + history = await service.history("s1", ALICE) + assistant_message = next(m for m in history if m.role == Role.ASSISTANT) + + updated = await service.set_message_feedback( + "s1", assistant_message.message_id, "thumbs_up", ALICE + ) + + assert updated is not None + assert updated.feedback == "thumbs_up" + assert updated.metadata.get("feedback") == "thumbs_up" + + +async def test_set_message_feedback_returns_none_for_missing_message() -> None: + service, _ = _service() + await service.create(ALICE, session_id="s1") + + updated = await service.set_message_feedback("s1", "no-such-id", "thumbs_up", ALICE) + + assert updated is None + + +async def test_set_message_feedback_is_authorized() -> None: + service, _ = _service() + await service.create(ALICE, session_id="s1") + await service.send("s1", "hello", ALICE) + + with pytest.raises(ConversationAccessDenied): + await service.set_message_feedback("s1", "no-such-id", "thumbs_up", BOB)