Jakob Wennberg
1dce9227a4
* feat(assistant): make the thumbs actually report something The thumbs up/down under an assistant answer shipped wired to nothing. They lit up, the vote died in component state, and the code said so in a comment nobody reading the UI could see. An affordance that looks like it reports something and does not is worse than no affordance: it spends the user's goodwill once, and silently. They now post to a new /api/agent/feedback, which emits the SAME agent.feedback event the gnubok_feedback MCP tool emits, with actorType 'user'. The product team already queries event_log for that type, so chat votes land in the backlog they read rather than in a second place someone has to remember to look at. event_log takes the payload as jsonb and already treats agent.* as telemetry, so there is no migration. The conversation id is caller-supplied, so it gets the same ownership check /api/agent/invoke got: without it a member could file feedback against a colleague's thread and the backlog would carry conversations the reporter never saw. Mutation-checked, three tests fail when the guard is removed. The pressed state is set only after the server accepts the vote, so the button never claims a report that never arrived, and a vote does not toggle off: it is append-only telemetry, and offering an undo we cannot honour would be a control that lies. Changing your mind sends the other sentiment instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(assistant): drop the unused free-text field from the feedback route compliance-swarm flagged the `comment` field as an undocumented PII exposure: free text embedded verbatim into the agent.feedback payload and written to event_log under telemetry retention, with no data-classification decision, next to a PostHog policy that treats the same class of data differently. The finding is right, and the field was worse than it looked: no caller ever sent one. The UI posts sentiment and a turn index. So this is dead API surface whose only effect was to accept whatever a user might type, in an accounting product, into a 180-day log: client names, personnummer, case details. Removed rather than documented. A comment box is a reasonable thing to want, but it needs its own classification and redaction decision made with the UI in front of it, not inherited from an unused parameter. The test now asserts the property instead of the field's absence: a caller that posts a comment anyway must not get it stored anywhere in the payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
72 lines
2.4 KiB
TypeScript
72 lines
2.4 KiB
TypeScript
import { describe, it, expect, vi } from 'vitest'
|
|
import { sendFeedback } from '../feedback-client'
|
|
|
|
/**
|
|
* The thumbs shipped wired to nothing: they lit up and the vote died in
|
|
* component state. These pin the contract the button now depends on, in
|
|
* particular that a failed send reports false, because the pressed state is
|
|
* set from this return value and must never claim a report that never arrived.
|
|
*/
|
|
|
|
describe('sendFeedback', () => {
|
|
it('posts the vote to the feedback endpoint', async () => {
|
|
const fetchImpl = vi.fn().mockResolvedValue({ ok: true })
|
|
|
|
const ok = await sendFeedback({
|
|
conversationId: 'c1',
|
|
sentiment: 'negative',
|
|
messageIndex: 3,
|
|
fetchImpl: fetchImpl as unknown as typeof fetch,
|
|
})
|
|
|
|
expect(ok).toBe(true)
|
|
const [url, init] = fetchImpl.mock.calls[0]!
|
|
expect(url).toBe('/api/agent/feedback')
|
|
expect(init.method).toBe('POST')
|
|
expect(JSON.parse(init.body)).toEqual({
|
|
conversation_id: 'c1',
|
|
sentiment: 'negative',
|
|
message_index: 3,
|
|
})
|
|
})
|
|
|
|
it('omits the message index rather than sending null for it', async () => {
|
|
// The route validates message_index as an integer when present; sending
|
|
// null would fail validation and lose the vote for no reason.
|
|
const fetchImpl = vi.fn().mockResolvedValue({ ok: true })
|
|
await sendFeedback({
|
|
conversationId: 'c1',
|
|
sentiment: 'positive',
|
|
fetchImpl: fetchImpl as unknown as typeof fetch,
|
|
})
|
|
expect(JSON.parse(fetchImpl.mock.calls[0]![1].body)).toEqual({
|
|
conversation_id: 'c1',
|
|
sentiment: 'positive',
|
|
})
|
|
})
|
|
|
|
it('reports failure for a non-2xx instead of assuming it landed', async () => {
|
|
const fetchImpl = vi.fn().mockResolvedValue({ ok: false, status: 404 })
|
|
expect(
|
|
await sendFeedback({
|
|
conversationId: 'c1',
|
|
sentiment: 'positive',
|
|
fetchImpl: fetchImpl as unknown as typeof fetch,
|
|
}),
|
|
).toBe(false)
|
|
})
|
|
|
|
it('reports failure instead of throwing when the request dies', async () => {
|
|
// Offline, or the request cut off by a navigation. An unhandled rejection
|
|
// here would surface as an error in the middle of reading an answer.
|
|
const fetchImpl = vi.fn().mockRejectedValue(new TypeError('Failed to fetch'))
|
|
await expect(
|
|
sendFeedback({
|
|
conversationId: 'c1',
|
|
sentiment: 'negative',
|
|
fetchImpl: fetchImpl as unknown as typeof fetch,
|
|
}),
|
|
).resolves.toBe(false)
|
|
})
|
|
})
|