fix(assistant): keep proposals, selections and picks intact across a resume (#1212)
* fix(assistant): keep proposals, selections and picks intact across a resume PR3 of the assistant UI makeover (dev_docs/assistant_redesign_plan.md section 7): resume fidelity. Four ways the chat lost state that the user had every reason to think was still there. Approval cards ride on streamed staged_operation events, which are never persisted, so reopening a conversation rendered the tool trace and the answer but silently dropped the card. The proposal then sat in Granskning for its full 30-day expiry with nothing in the thread pointing at it. run-turn already stamps agent_metadata.conversation_id on every staged row, so both resume paths (the sheet's history and the /chat page) now re-attach the still-pending ones to the last assistant turn. Regenerate abandoned whatever the discarded turn had staged: the card left the screen, the operation stayed pending, and the regenerated turn usually staged a second proposal for the same booking, leaving two live proposals for one action. It now withdraws them through the same reject path the Avslå button uses, so the audit trail records why they went away. The sheet's remount key ignored intentArgs while some callers pass a CONSTANT contextRef with varying args: bulk-book always uses 'inbox:bulk' and carries the selected ids. Selecting A+B, collapsing, then selecting C+D reopened the A+B conversation while the user believed C+D were being booked. The key now includes a stable serialization of the args. Picking conversation A (slow) then B (fast) let A's late response overwrite B, leaving the user typing into a thread they did not choose. A sequence token now means only the newest pick may write state. Verified: 9540 unit tests pass (11 new), lint and tsc clean on every touched file, guards pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: record the resume-fidelity decisions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(assistant): render hydrated proposals with the same preview as live ones pending_operations.operation_type stores the bare action name ('categorize_transaction'), while the streamed card carries the MCP tool name ('gnubok_categorize_transaction') and ApprovalCard's PreviewBlock dispatches on that. Hydrated cards therefore fell through to the flat generic preview instead of the journal-line one, so a resumed proposal looked materially worse than the same proposal did live: the opposite of what this PR is for. Found by checking the query against prod rather than trusting the mock, which is also how the stored value space was confirmed: categorize_transaction, create_voucher and approve_supplier_invoice are what exist in the wild, and the four operation types that have a specialized renderer all stage unprefixed. The test fixture now uses the real stored shape so the mapping is actually covered rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(assistant): await proposal withdrawals, surface staged-query errors, share the type Review follow-ups on the resume-fidelity batch. All four findings were valid. The withdrawals were fire-and-forget and raced the replacement turn, so the new turn could stage a second proposal before the old one was rejected: the exact double-staging this change exists to prevent. They are now awaited, a 409 counts as withdrawn (someone else resolved it, which is all we need), and if any withdrawal genuinely fails the turn stays on screen with an error rather than hiding a card whose operation is still pending. Both staged-operation loaders ignored their error result, so a database or policy failure rendered the conversation as successful with the proposals silently missing: again the failure this query exists to prevent, reintroduced through the error path. Both now propagate, matching the sibling message query. StoredStagedOperation now lives in @/types: it is a persisted API contract that crosses a server page and three components, not an AgentChat detail. The unserializable-args fallback used a timestamp, which collides for two objects created in the same millisecond and changes on every render tick for the same object, remounting the sheet mid-session. A WeakMap gives each object one stable id for its lifetime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f0f3050f54
commit
4de648fb5d
@@ -0,0 +1,139 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { attachStagedOperations, toolNameFor } from '../AgentChat'
|
||||
import type { StoredStagedOperation } from '@/types'
|
||||
|
||||
/**
|
||||
* Approval cards ride on streamed `staged_operation` events, which are never
|
||||
* persisted. Without re-attaching them, reopening a conversation showed the
|
||||
* tool trace and the answer but no card, so an unapproved proposal waited out
|
||||
* its full 30-day expiry in Granskning with nothing in the thread pointing at
|
||||
* it. This is the reattachment.
|
||||
*/
|
||||
|
||||
const op = (overrides: Partial<StoredStagedOperation> = {}): StoredStagedOperation => ({
|
||||
id: 'op-1',
|
||||
// As actually stored: pending_operations.operation_type is the bare action
|
||||
// name, verified against prod (categorize_transaction / create_voucher /
|
||||
// approve_supplier_invoice are the values in the wild).
|
||||
operation_type: 'categorize_transaction',
|
||||
title: 'Kontering: Circle K, 689 kr',
|
||||
risk_level: 'low',
|
||||
preview_data: { lines: [] },
|
||||
...overrides,
|
||||
})
|
||||
|
||||
describe('attachStagedOperations', () => {
|
||||
it('returns the thread untouched when nothing is pending', () => {
|
||||
const messages = [
|
||||
{ role: 'user' as const, text: 'boka om' },
|
||||
{ role: 'assistant' as const, text: 'klart' },
|
||||
]
|
||||
expect(attachStagedOperations(messages, [])).toBe(messages)
|
||||
})
|
||||
|
||||
it('attaches a pending proposal to the last assistant turn', () => {
|
||||
const messages = [
|
||||
{ role: 'user' as const, text: 'boka om Circle K' },
|
||||
{ role: 'assistant' as const, text: 'Klart att godkänna.' },
|
||||
]
|
||||
|
||||
const [, assistant] = attachStagedOperations(messages, [op()])
|
||||
|
||||
expect(assistant!.staged).toHaveLength(1)
|
||||
expect(assistant!.staged![0]).toMatchObject({
|
||||
operation_id: 'op-1',
|
||||
risk_level: 'low',
|
||||
tool_name: 'gnubok_categorize_transaction',
|
||||
message: 'Kontering: Circle K, 689 kr',
|
||||
})
|
||||
})
|
||||
|
||||
it('maps the stored operation_type onto the tool name the preview dispatches on', () => {
|
||||
// pending_operations stores the bare action name; the live card carries the
|
||||
// MCP tool name, and ApprovalCard's PreviewBlock keys off that. Getting this
|
||||
// wrong is invisible in a mock but drops every hydrated card to the flat
|
||||
// generic preview instead of the journal-line one. These four are the
|
||||
// operation types that have a specialized renderer.
|
||||
for (const t of [
|
||||
'categorize_transaction',
|
||||
'create_invoice',
|
||||
'create_voucher',
|
||||
'correct_entry',
|
||||
]) {
|
||||
expect(toolNameFor(t)).toBe(`gnubok_${t}`)
|
||||
}
|
||||
})
|
||||
|
||||
it('leaves an already-prefixed operation type alone', () => {
|
||||
expect(toolNameFor('gnubok_create_voucher')).toBe('gnubok_create_voucher')
|
||||
})
|
||||
|
||||
it('keeps risk level for medium and high, and floors anything unknown to low', () => {
|
||||
const messages = [{ role: 'assistant' as const, text: 'svar' }]
|
||||
|
||||
const risks = (level: string | null) =>
|
||||
attachStagedOperations(messages, [op({ risk_level: level })])[0]!.staged![0]!.risk_level
|
||||
|
||||
expect(risks('high')).toBe('high')
|
||||
expect(risks('medium')).toBe('medium')
|
||||
// An unrecognized value must not silently render as a one-click low-risk
|
||||
// approval... but it also must not crash: low is the safe render, and the
|
||||
// server re-checks risk on commit regardless.
|
||||
expect(risks('nonsense')).toBe('low')
|
||||
expect(risks(null)).toBe('low')
|
||||
})
|
||||
|
||||
it('preserves staged cards already on the message', () => {
|
||||
const messages = [
|
||||
{
|
||||
role: 'assistant' as const,
|
||||
text: 'svar',
|
||||
staged: [
|
||||
{
|
||||
tool_use_id: 'tu_live',
|
||||
operation_id: 'op-live',
|
||||
risk_level: 'low' as const,
|
||||
message: 'redan här',
|
||||
},
|
||||
],
|
||||
},
|
||||
]
|
||||
|
||||
const out = attachStagedOperations(messages, [op({ id: 'op-2' })])
|
||||
|
||||
expect(out[0]!.staged!.map((s) => s.operation_id)).toEqual(['op-live', 'op-2'])
|
||||
})
|
||||
|
||||
it('appends a carrier turn when the thread has no assistant message', () => {
|
||||
// Defensive: a conversation whose assistant turn was never persisted still
|
||||
// has to show its proposal rather than swallow it.
|
||||
const out = attachStagedOperations([{ role: 'user', text: 'boka' }], [op()])
|
||||
|
||||
expect(out).toHaveLength(2)
|
||||
expect(out[1]!.role).toBe('assistant')
|
||||
expect(out[1]!.staged).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('attaches to the LAST assistant turn, not the first', () => {
|
||||
const messages = [
|
||||
{ role: 'assistant' as const, text: 'första' },
|
||||
{ role: 'user' as const, text: 'och sen?' },
|
||||
{ role: 'assistant' as const, text: 'andra' },
|
||||
]
|
||||
|
||||
const out = attachStagedOperations(messages, [op()])
|
||||
|
||||
expect(out[0]!.staged).toBeUndefined()
|
||||
expect(out[2]!.staged).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('gives every hydrated card a distinct tool_use_id key', () => {
|
||||
const out = attachStagedOperations(
|
||||
[{ role: 'assistant', text: 'svar' }],
|
||||
[op({ id: 'op-a' }), op({ id: 'op-b' })],
|
||||
)
|
||||
|
||||
const keys = out[0]!.staged!.map((s) => s.tool_use_id)
|
||||
expect(new Set(keys).size).toBe(2)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user