Conversation
|
Thanks — the overall direction looks good, but I still see a few blockers before approving.
The service currently authorizes the caller against but the repository update is performed only by and in SQL: That creates a broken object-level authorization gap. A caller who owns conversation A could potentially submit a message ID that belongs to conversation B, pass authorization on A, and still update the message from B. Please scope the update to both: at the repository boundary itself, rather than authorizing one object and mutating another by a global identifier.
The feature is described as feedback on assistant messages, but the backend currently allows feedback to be written to any message. The repository/service contract should enforce that the target message is: This should be a domain/application invariant, not something we rely on the widget to enforce.
The feedback actions are rendered only when: History-loaded messages have So the likely behavior is: The user should be able to rate the answer immediately after receiving it. Please propagate the persisted assistant message ID through the final stream contract, or otherwise reconcile the newly streamed entry with the persisted message before rendering feedback actions.
That means: until the conversation is reloaded. Please update the local entry state after a successful response, or use an optimistic update with rollback on failure.
Right now most layers use: and the allowed values are only enforced at the API schema using a regex. I would prefer a shared domain type / enum such as: so invalid feedback values cannot flow through the application or repository layers. This is less important than the authorization/UI issues, but it would make the contract cleaner.
The backend quality gate passes, but the UI job fails on: Because of that, the widget unit tests and Playwright tests are skipped. Please rebuild/commit the widget bundle and add regression coverage for the new feedback behavior. At minimum I would expect UI coverage for:
Once the mutation is correctly scoped, the assistant-only invariant is enforced server-side, and the live widget state is wired correctly, I think this will be in a much stronger state. |
|
I welcome this initiative @Karn2898 |
Adds 👍/👎 feedback buttons on assistant messages in the widget, persisted alongside the conversation in
agent_manager.What changed
src/agent_manager/domain/models.py— addsfeedback: str | None = NonetoConversationMessage.src/agent_manager/domain/repository.py— addsupdate_message_feedbackto theRepositoryport.src/agent_manager/infrastructure/persistence/sql_repository.py— implementsupdate_message_feedbackby writing{"feedback": "thumbs_up"|"thumbs_down"}into the message'smetadata_jsoncolumn.src/agent_manager/infrastructure/persistence/memory_repository.py— same for the in-memory adapter.src/agent_manager/application/conversation_service.py— addsset_message_feedback(conversation_id, message_id, feedback, principal)which authorizes the caller and delegates to the repository.src/agent_manager/api/schemas.py— addsMessageFeedbackRequest/MessageFeedbackResponseand addsfeedbacktoMessageOut.src/agent_manager/api/routes/conversations.py— addsPOST /conversations/{conversation_id}/messages/{message_id}/feedback.src/agent_manager/api/static/widget/types.ts— addsfeedback?: "thumbs_up" | "thumbs_down"toMessageEntryandChatMessage.src/agent_manager/api/static/widget/api/AgentChatClient.ts— addssetMessageFeedback.src/agent_manager/api/static/widget/react/useConversation.ts— exposessetMessageFeedbackon theConversationhook.src/agent_manager/api/static/widget/react/AgentChatApp.tsx— adds 👍/👎 buttons toMessageActions, rendered only on assistant messages, wired toconversation.setMessageFeedback.Tests
tests/agent_manager/test_service.py— 3 tests: persist+return, missing message returnsNone, authorization enforced.tests/agent_manager/test_api.py— 3 tests: happy-path 200, 404 for missing message, 403 for wrong caller.All 955 tests pass, lint clean, typecheck clean.