From 967af26ca265c797d3c10004d60305dbd0c8de89 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 00:15:49 +0000 Subject: [PATCH 1/2] fix(app-shell): keep the approval envelope + pending-action id in the chat cache (objectui#9232) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sanitizeChatMessagesForCache` rebuilds each tool part field by field and wrote neither the AI SDK `approval` envelope nor the ObjectStack `pendingActionId`. `state` survived, so a cached `approval-requested` came back with the awaiting-approval card lit and nothing behind it: `useHitlInChat` indexes on `pendingActionId`, so Approve could only answer "No pending-action id found for this tool call". The mirror of objectui#8442, which closed the same gap on the way OUT of persisted server history. The id travels asymmetrically, and the fix follows that rather than flattening it. `approval` is a persisted PART key on the server path, so the cache writes it as one and `partApproval` reads it straight back. `pendingActionId` is never a part key anywhere, so it round-trips through a new `pendingApprovalToCachedResult` inverse that re-mints the minimal `{ status: 'pending_approval', pendingActionId }` envelope `detectPendingApproval` re-parses — one parse for one contract, no second reader on the cache side. Entries written by the old shape are kept and read, not discarded: both readers of the new keys already answer "absent" rather than throwing, a version bump would blank the transcript and cards the old writer did keep, and old entries self-heal on the next server-backed load. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa --- .changeset/9232-cache-approval-envelope.md | 13 + .../__tests__/useChatConversation.test.tsx | 275 ++++++++++++++++++ .../src/hooks/useChatConversation.ts | 115 +++++++- 3 files changed, 396 insertions(+), 7 deletions(-) create mode 100644 .changeset/9232-cache-approval-envelope.md diff --git a/.changeset/9232-cache-approval-envelope.md b/.changeset/9232-cache-approval-envelope.md new file mode 100644 index 0000000000..42af96cce1 --- /dev/null +++ b/.changeset/9232-cache-approval-envelope.md @@ -0,0 +1,13 @@ +--- +'@object-ui/app-shell': patch +--- + +A pending approval now survives the AI-chat localStorage cache, so a cache-fallback reload keeps an Approve / Reject the operator can actually complete. + +`sanitizeChatMessagesForCache` rebuilds each tool part field by field, and neither the AI SDK `approval` envelope nor the ObjectStack `pendingActionId` was among the fields it wrote. `state` was, so a cached `approval-requested` came back with the awaiting-approval card lit and nothing behind it: `useHitlInChat` indexes decisions on `pendingActionId`, so pressing Approve could only answer "No pending-action id found for this tool call". The `output` re-serialization covered `replayOutcome` / `draftReview` / `proposedPlan` only, so the id was not recoverable from the cached output either. + +This is the mirror of objectui#8442, which closed the same gap in the other direction (`hydratedMessagesToChatMessages`, the way OUT of persisted server history). Until now the two hydration sources disagreed on `main`: the server path yielded an invocation `useHitlInChat` could index, the cache-fallback path yielded one carrying `approval-requested` and nothing to decide with. The cache fallback is not a corner — it is what renders the thread whenever the server returns no messages. + +The id travels asymmetrically and the fix follows that asymmetry rather than flattening it. `approval` is a persisted PART key on the server path, so the cache writes it as one and the hydration mapper reads it straight back. `pendingActionId` is never a part key anywhere — it exists in rehydrated history only inside the tool RESULT — so it round-trips through a new `pendingApprovalToCachedResult` inverse that re-mints the minimal `{ status: 'pending_approval', pendingActionId }` envelope `detectPendingApproval` re-parses. One parse for one contract: no second reader is added on the cache side. The envelope's operator-facing prose and proposed arguments are not re-serialized, matching the leanness the draft and plan inverses already take. + +**Cached entries written by the previous version are kept and read, not discarded.** No cache-key version bump: both readers of the new keys already answer "absent" rather than throwing, so an old entry restores exactly as it does today, and a discard would blank the transcript, the draft card and the plan card that the old writer did keep — on the one path that renders when the server has nothing. Old entries self-heal, because the first server-backed load after this ships restores the id through the objectui#8442 path and rewrites the cache in the new shape. diff --git a/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx b/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx index 89a9fb834d..6c3d942cab 100644 --- a/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx +++ b/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx @@ -12,6 +12,13 @@ import { renderHook, waitFor, act } from '@testing-library/react'; // via these mappers — round-tripping our cache through them is the real // production path (live render → cache → cache-fallback reload). import { uiMessageToChatMessage } from '@object-ui/plugin-chatbot'; +// objectui#9232 — the cache round trip is only proven by driving the affordance +// it exists to restore, so the test mounts the real HITL hook over the restored +// messages and presses Approve. `hydratedMessagesToChatMessages` is the +// cache-fallback read this page really performs (AiChatPage feeds it +// `initialMessages` from `useChatConversation`). +import { useHitlInChat } from '@object-ui/plugin-chatbot'; +import { hydratedMessagesToChatMessages } from '../../console/ai/AiChatPage'; import { purgeChatCaches, @@ -1071,3 +1078,271 @@ describe('useChatConversation — clears chat cache on logout / user switch', () expect(localStorage.getItem(`${CACHE_PREFIX}:u1`)).toBeNull(); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// objectui#9232 — the cache WRITE half of the approval round trip. +// +// objectui#8442 stopped `hydratedMessagesToChatMessages` dropping the approval +// envelope and `pendingActionId` on the way OUT of persisted history. +// `sanitizeChatMessagesForCache` is the way IN, and it rebuilt each tool part +// field by field without either key — while keeping `state`. So a cached +// `approval-requested` came back with the Approve / Reject affordance lit and +// nothing behind it: `useHitlInChat` indexes on `pendingActionId`, so `decide` +// could only answer "No pending-action id found for this tool call". +// +// These tests DRIVE the round trip rather than reading the call graph: live +// mapper → sanitize → real `writeConversationMessagesCache` → JSON in +// localStorage → back out → the hydration mapper → `useHitlInChat.decide()`, +// and the assertion is the REST call the operator's Approve actually makes. +// ───────────────────────────────────────────────────────────────────────────── +describe('sanitizeChatMessagesForCache — a pending approval survives the cache round trip (objectui#9232)', () => { + const PENDING_ACTION_ID = 'pa_9232'; + const CONVERSATION_ID = 'conv-hitl'; + + /** + * The framework HITL envelope as `action-tools.ts` really returns it — note + * the operator-facing `message` and the proposed args, neither of which the + * detector reads and neither of which the cache is asked to keep. + */ + const pendingOutput = { + status: 'pending_approval', + pendingActionId: PENDING_ACTION_ID, + message: 'Delete task “Q3 rollout”?', + toolName: 'action_delete_task', + args: { recordId: 'task_77', hard: true }, + }; + + /** The AI SDK UI message the live stream produces for a HITL-gated tool. */ + const liveUiMessage = { + id: 'a1', + role: 'assistant' as const, + parts: [ + { type: 'text', text: 'This needs your approval.' }, + { + type: 'tool-action_delete_task', + toolCallId: 'tc-approve', + toolName: 'action_delete_task', + state: 'approval-requested' as const, + input: { recordId: 'task_77' }, + output: pendingOutput, + approval: { id: 'req_1' }, + }, + ], + }; + + /** Write through the real cache helper and read back what localStorage holds. */ + function throughLocalStorage(cached: HydratedUIMessage[]): HydratedUIMessage[] { + writeConversationMessagesCache(CONVERSATION_ID, cached); + const raw = localStorage.getItem(`${MESSAGE_PREFIX}:${CONVERSATION_ID}`); + expect(raw).toBeTruthy(); + return JSON.parse(raw as string) as HydratedUIMessage[]; + } + + /** + * Mount `useHitlInChat` over restored messages and press Approve, exactly as + * `` does. Returns the REST + * calls the decision made — an empty list means the affordance was dead. + */ + async function pressApprove(messages: ReturnType) { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse({ status: 'executed' })); + vi.stubGlobal('fetch', fetchMock); + const { result } = renderHook(() => + useHitlInChat({ messages, apiBase: API_BASE }), + ); + await act(async () => { + await result.current.decide('tc-approve', true); + }); + return { + urls: fetchMock.mock.calls.map((c) => String(c[0])), + decision: result.current.decisions['tc-approve'], + }; + } + + beforeEach(() => { + localStorage.clear(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + localStorage.clear(); + }); + + /** Live render → cache write → the bytes localStorage actually holds. */ + function cacheTheLiveApproval(): HydratedUIMessage[] { + // Baseline / lit control: the LIVE render really does hold the id, so a + // zero further down means the cache lost it — not that it never existed. + const live = uiMessageToChatMessage(liveUiMessage); + expect(live.toolInvocations?.[0]?.pendingActionId).toBe(PENDING_ACTION_ID); + expect(live.toolInvocations?.[0]?.state).toBe('approval-requested'); + return throughLocalStorage(sanitizeChatMessagesForCache([live])); + } + + it('re-mints the MINIMAL pending envelope into the cached tool part', () => { + const toolPart = cacheTheLiveApproval()[0]?.parts.find( + (p) => p.type === 'tool-action_delete_task', + ); + expect(toolPart?.output).toEqual({ + status: 'pending_approval', + pendingActionId: PENDING_ACTION_ID, + }); + // Leanness, same bargain the draft/plan inverses strike: the operator-facing + // prose and the proposed arguments are NOT re-serialized into the cache. + expect(JSON.stringify(toolPart?.output)).not.toContain('Q3 rollout'); + expect(JSON.stringify(toolPart?.output)).not.toContain('task_77'); + // The state that makes the card render survived the rebuild all along. + expect(toolPart?.state).toBe('approval-requested'); + }); + + // THE load-bearing test. Deliberately holds ONE claim — that the restored + // message is something `useHitlInChat` can index and decide on — so that the + // assertion which fails when the fix is removed is the DRIVEN one (the REST + // call the operator's Approve makes), not an earlier shape check standing in + // front of it. + it('restores a message useHitlInChat can index, and Approve reaches the pending action', async () => { + const restored = hydratedMessagesToChatMessages(cacheTheLiveApproval()); + + const { urls, decision } = await pressApprove(restored); + expect(urls).toEqual([`${API_BASE}/pending-actions/${PENDING_ACTION_ID}/approve`]); + expect(decision?.state).toBe('success'); + }); + + it('writes the AI SDK approval envelope as a part key, where the hydration mapper reads it', () => { + // The envelope rides the PART on both directions of the server path, so the + // cache writes it as a part key too. The input shape here is the one + // AiChatPage re-caches after a HYDRATED load (`hydratedMessagesToChatMessages` + // lifts `approval`; the live `extractToolInvocations` does not) — which is + // exactly the loop that would otherwise erase it on the next cache write. + const cached = sanitizeChatMessagesForCache([ + { + id: 'a1', + role: 'assistant', + content: 'This needs your approval.', + toolInvocations: [ + { + toolCallId: 'tc-approve', + toolName: 'action_delete_task', + state: 'approval-requested', + approval: { id: 'req_1', reason: 'destructive' }, + pendingActionId: PENDING_ACTION_ID, + }, + ], + }, + ]); + const persisted = throughLocalStorage(cached); + const toolPart = persisted[0]?.parts.find((p) => p.type === 'tool-action_delete_task'); + expect(toolPart?.approval).toEqual({ id: 'req_1', reason: 'destructive' }); + + const restored = hydratedMessagesToChatMessages(persisted); + expect(restored[0]?.toolInvocations?.[0]?.approval).toEqual({ + id: 'req_1', + reason: 'destructive', + }); + }); + + it('leaves a tool with no pending approval exactly as it was', () => { + // The new arm is reachable only through `pendingActionId`; a plain tool must + // not grow an `output` (and an `approval`-less part must not grow that key). + const cached = sanitizeChatMessagesForCache([ + { + id: 'a1', + role: 'assistant', + content: 'Counted.', + toolInvocations: [ + { toolCallId: 'tc1', toolName: 'aggregate_data', state: 'output-available' }, + ], + }, + ]); + const toolPart = cached[0]?.parts.find((p) => p.type === 'tool-aggregate_data'); + expect('output' in (toolPart as object)).toBe(false); + expect('approval' in (toolPart as object)).toBe(false); + }); + + it('re-mints the pending envelope ahead of a co-occurring draft envelope', () => { + // Not producible by the live mapper: all four detectors read ONE + // `parseResultEnvelope(result)` and discriminate on its single `status`, so + // `pending_approval` and `drafted` cannot both match one result. The arm + // order is therefore unobservable in production — pinned anyway so the + // precedence is a decision rather than an accident of where the arm landed. + // An undecided action is the only one of the four whose loss leaves a live + // control wired to nothing; the others describe something already done. + const cached = sanitizeChatMessagesForCache([ + { + id: 'a1', + role: 'assistant', + toolInvocations: [ + { + toolCallId: 'tc-approve', + toolName: 'apply_blueprint', + state: 'approval-requested', + pendingActionId: PENDING_ACTION_ID, + draftReview: { items: [{ type: 'object', name: 'task' }] }, + }, + ], + }, + ]); + const toolPart = cached[0]?.parts.find((p) => p.type === 'tool-apply_blueprint'); + expect(toolPart?.output).toEqual({ + status: 'pending_approval', + pendingActionId: PENDING_ACTION_ID, + }); + // And minting it cannot resurrect a Publish button over a rolled-back + // publish (objectui#5695): `detectDraftResult` is silent over this status. + const restored = hydratedMessagesToChatMessages( + JSON.parse(JSON.stringify(cached)) as HydratedUIMessage[], + ); + expect(restored[0]?.toolInvocations?.[0]?.draftReview).toBeUndefined(); + }); + + // ── Stale entries: the decision is READ-SIDE TOLERANCE, not a version bump ── + // + // Entries written by the OLD code are one format behind. They are kept and + // read, because (1) nothing can break on them — both readers of the new keys + // answer "absent" rather than throwing; (2) a `:v2` key or a discard would + // blank the transcript, the draft card and the plan card that the old writer + // DID keep, on the one path that renders when the server has nothing; and + // (3) they self-heal on the next server-backed load, which rewrites the cache + // through the new writer. See the block comment on `readMessageCache`. + describe('an entry written by the OLD cache shape', () => { + /** Byte-for-byte what the pre-objectui#9232 writer produced for this tool. */ + const staleEntry: HydratedUIMessage[] = [ + { + id: 'a1', + role: 'assistant', + parts: [ + { type: 'text', text: 'This needs your approval.' }, + { + type: 'tool-action_delete_task', + toolCallId: 'tc-approve', + toolName: 'action_delete_task', + state: 'approval-requested', + }, + ], + }, + ]; + + it('still restores, and never fabricates an id it does not have', () => { + const restored = hydratedMessagesToChatMessages(throughLocalStorage(staleEntry)); + // Tolerated, not discarded: everything the old writer kept still renders. + expect(restored).toHaveLength(1); + expect(restored[0]?.content).toBe('This needs your approval.'); + const tool = restored[0]?.toolInvocations?.[0]; + expect(tool?.toolCallId).toBe('tc-approve'); + expect(tool?.state).toBe('approval-requested'); + // No invention: absent stays absent on both halves. + expect(tool?.pendingActionId).toBeUndefined(); + expect(tool?.approval).toBeUndefined(); + }); + + it('degrades to the pre-fix decision error, making no REST call', async () => { + // The NEGATIVE half of the load-bearing assertion. Its lit control is the + // first test in this block, which differs in exactly one way — whether the + // cached part carries the re-minted envelope — and DOES reach + // `/pending-actions/pa_9232/approve`. A zero here with that control dark + // would be a void reading. + const restored = hydratedMessagesToChatMessages(throughLocalStorage(staleEntry)); + const { urls, decision } = await pressApprove(restored); + expect(urls).toEqual([]); + expect(decision?.state).toBe('error'); + }); + }); +}); diff --git a/packages/app-shell/src/hooks/useChatConversation.ts b/packages/app-shell/src/hooks/useChatConversation.ts index c1f5b084fb..46cb7a1ea6 100644 --- a/packages/app-shell/src/hooks/useChatConversation.ts +++ b/packages/app-shell/src/hooks/useChatConversation.ts @@ -180,6 +180,25 @@ interface CacheableChatToolInvocation { /** objectui#5695 — the confirm-replay verdict, so the 确认修改 card's terminal * state (已生效 / 已暂存为草稿 / 未生效) survives a cache-fallback reload. */ replayOutcome?: { kind: 'published' | 'drafted' | 'failed'; outcome?: string; error?: string; packageId?: string; dispatchError?: boolean }; + /** + * objectui#9232 — the two halves of an ACTIONABLE approval, mirroring + * objectui#8442 on the way IN to the cache. They travel differently and that + * asymmetry is the whole design (see `sanitizeChatMessagesForCache`): + * + * * `approval` is the AI SDK envelope and rides the persisted PART, so it + * is written back as a part key and read back as one (`partApproval`); + * * `pendingActionId` is the ObjectStack `pending_actions` row id, is + * never persisted as a part key on ANY path, and is therefore restored + * by re-minting the result envelope `detectPendingApproval` re-parses. + */ + approval?: { + id: string; + approved?: boolean; + reason?: string; + isAutomatic?: boolean; + signature?: string; + }; + pendingActionId?: string; } interface CacheableChatMessage { @@ -265,6 +284,29 @@ function replayOutcomeToCachedResult( }; } +/** + * Rebuild the MINIMAL HITL envelope `mapMessages.detectPendingApproval` + * re-parses (objectui#9232). Inverse of that detector, exactly as + * `draftReviewToCachedResult` is the inverse of `detectDraftResult`. + * + * This exists because the id travels asymmetrically. The AI SDK `approval` + * envelope is a persisted PART key, so the cache can write it and the + * hydration mapper reads it straight back; `pendingActionId` is not a part key + * on any path — it lives only inside the tool RESULT — so the only way to + * round-trip it is to re-mint the envelope the one detector re-parses. A + * second hand-rolled reader on the cache side would be the second dialect of + * one contract that Commandment #0.1 refuses. + * + * The two keys the detector reads are the only two written: the framework's + * real envelope also carries a human `message` and the proposed arguments, and + * none of that is re-serialized — the same leanness the draft/plan inverses + * take, and the reason a cached thread does not grow a copy of every pending + * action's payload. + */ +function pendingApprovalToCachedResult(pendingActionId: string): Record { + return { status: 'pending_approval', pendingActionId }; +} + const CACHE_PREFIX = 'objectstack:ai-chat-conversation-id'; const MESSAGE_CACHE_PREFIX = 'objectstack:ai-chat-messages'; @@ -317,6 +359,39 @@ export function purgeChatCaches(): void { } } +/** + * ## Cache-format compatibility — objectui#9232 chose READ-SIDE TOLERANCE + * + * objectui#9232 added two keys to what `sanitizeChatMessagesForCache` writes, + * so entries already in a user's `localStorage` are a short format behind. + * Three routes were on the table — tolerate them, bump `MESSAGE_CACHE_PREFIX` + * to a `:v2` key, or discard on read. This path deliberately TOLERATES, and + * the reasoning is pinned in `useChatConversation.test.tsx` rather than left to + * be re-derived: + * + * 1. Nothing can break on an old entry. The cached payload is a list of OPEN + * records (`HydratedUIMessagePart`), and both readers of the new keys + * already answer "absent" rather than throwing — `partApproval` returns + * undefined for a missing envelope, `detectPendingApproval` returns + * undefined for a result that is not one. An old entry restores exactly + * as it does today. + * 2. A version bump or a discard would throw away everything the old writer + * got RIGHT — the transcript text, the draft "Review N changes / Publish" + * card, the ADR-0038 chip, the proposed plan, the objectui#5695 replay + * verdict — to recover one affordance. This cache is only ever read when + * the server returned no messages, so the discarded thread is the only + * thread the operator would have seen: a strictly worse trade. + * 3. Old entries self-heal. The cache is rewritten from `runtimeMessages` on + * every render, so the first server-backed load after this ships restores + * the id through the objectui#8442 path and writes it back in the new + * shape. The stale window closes by itself, and only stays open on a + * conversation the server cannot serve at all — where there is nothing + * better to restore from anyway. + * + * What tolerance does NOT do is invent: an old entry keeps `approval-requested` + * with no id, which is precisely today's behaviour, and no id is fabricated to + * paper over it. See the objectui#9232 block in the tests. + */ function readMessageCache(conversationId: string): HydratedUIMessage[] { try { const raw = localStorage.getItem(messageCacheKey(conversationId)); @@ -374,19 +449,45 @@ export function sanitizeChatMessagesForCache( // earlier cache shape never kept. `output` (not a custom part field) // is used because the AI SDK preserves it through `useChat` init, // exactly as the server-backed tool-result merge relies on. - const cachedOutput = tool.replayOutcome - ? replayOutcomeToCachedResult(tool.replayOutcome) - : tool.draftReview - ? draftReviewToCachedResult(tool.draftReview) - : tool.proposedPlan - ? proposedPlanToCachedResult(tool.proposedPlan) - : undefined; + // + // objectui#9232 — the pending-approval arm. `state` already survived + // this rebuild, so a cached `approval-requested` came back carrying + // nothing to decide with: the operator got Approve / Reject buttons + // whose only possible outcome was "No pending-action id found for + // this tool call". That is the exact divergence objectui#8442 closed + // on the way OUT of server history, reopened on the way IN to the + // cache. + // + // It is FIRST in the chain on purpose. The four detectors all read + // one `parseResultEnvelope(result)` and discriminate on its single + // `status` field (`pending_approval` / `drafted` / + // `blueprint_proposed` / the replay pair), so the arms are disjoint + // by construction and the order is unobservable today. The position + // fixes what happens if that ever stops being true: the other three + // restore a card describing something that already HAPPENED, while + // this one restores the operator's ability to ACT, and losing it is + // the only one of the four that leaves a live control wired to + // nothing. Minting this envelope cannot resurrect a Publish button + // over a rolled-back publish (objectui#5695's hazard) either — + // `detectDraftResult` is silent over a `pending_approval` status. + const cachedOutput = tool.pendingActionId + ? pendingApprovalToCachedResult(tool.pendingActionId) + : tool.replayOutcome + ? replayOutcomeToCachedResult(tool.replayOutcome) + : tool.draftReview + ? draftReviewToCachedResult(tool.draftReview) + : tool.proposedPlan + ? proposedPlanToCachedResult(tool.proposedPlan) + : undefined; parts.push({ type: `tool-${tool.toolName}`, toolCallId: tool.toolCallId, toolName: tool.toolName, state: tool.state ?? (tool.errorText ? 'output-error' : 'output-available'), ...(tool.errorText ? { errorText: tool.errorText } : {}), + // The SDK envelope rides the PART, both here and on the server + // path — `partApproval` narrows it straight back off this key. + ...(tool.approval ? { approval: tool.approval } : {}), ...(cachedOutput !== undefined ? { output: cachedOutput } : {}), }); } From 95f2ff0c5910998bf5d8e498aa90274f33c04645 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 01:11:34 +0000 Subject: [PATCH 2/2] fix(app-shell): carry the pending-action id on a part key, not by displacing the draft envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Patch round on objectui#9232. The first version put the pending-approval arm FIRST in the `cachedOutput` chain, on the argument that the four detectors are "disjoint by construction, so the order is unobservable". That argument was wrong and `AiChatPage.runtimeMessageSeam.test.tsx` already pinned the counter-example: the detectors are disjoint over one RESULT, but `draftReview` and `pendingActionId` are independent KEYS on an invocation and a turn can carry both. Pending-first therefore stopped the draft envelope reaching the cache for such a turn — the same "Review N changes / Publish" loss on a cache-fallback reload that the other three arms exist to prevent, and the same loss this card's own stale-cache reasoning refused to accept from a key bump. `output` holds one envelope, so the three existing arms keep it exactly as before and the pending envelope is minted only when none of them claims it. The id is no longer hostage to that slot: it is also written as a cache-side part key, and `hydratedMessagesToChatMessages` consults `detectPendingApproval` FIRST and only falls back to the key. On the server path the envelope always answers, so that line behaves exactly as objectui#8442 left it and no second dialect of the contract is introduced. The two carriers are not redundant — each reaches where the other cannot. `output` is the only one that survives API mode's SDK store, because `aiInitialMessages` rebuilds each part from {type,toolCallId,toolName,input, output,errorText,state} and drops every other key; the part key is the only one left when a richer envelope has taken `output`, and local mode keeps it because `normalizeMessages` passes `toolInvocations` through verbatim. The falsified precedence pin is replaced by a both-at-once pin that drives the round trip: the draft card comes back AND Approve reaches the pending action. The over-reaching prose claim is replaced by the narrow claim that is true, as an executable pin with lit controls: over ONE result the two envelopes are mutually exclusive. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa --- .changeset/9232-cache-approval-envelope.md | 4 +- .../app-shell/src/console/ai/AiChatPage.tsx | 13 ++- .../__tests__/useChatConversation.test.tsx | 91 ++++++++++++++----- .../src/hooks/useChatConversation.ts | 78 ++++++++++------ 4 files changed, 134 insertions(+), 52 deletions(-) diff --git a/.changeset/9232-cache-approval-envelope.md b/.changeset/9232-cache-approval-envelope.md index 42af96cce1..c46b805d95 100644 --- a/.changeset/9232-cache-approval-envelope.md +++ b/.changeset/9232-cache-approval-envelope.md @@ -8,6 +8,8 @@ A pending approval now survives the AI-chat localStorage cache, so a cache-fallb This is the mirror of objectui#8442, which closed the same gap in the other direction (`hydratedMessagesToChatMessages`, the way OUT of persisted server history). Until now the two hydration sources disagreed on `main`: the server path yielded an invocation `useHitlInChat` could index, the cache-fallback path yielded one carrying `approval-requested` and nothing to decide with. The cache fallback is not a corner — it is what renders the thread whenever the server returns no messages. -The id travels asymmetrically and the fix follows that asymmetry rather than flattening it. `approval` is a persisted PART key on the server path, so the cache writes it as one and the hydration mapper reads it straight back. `pendingActionId` is never a part key anywhere — it exists in rehydrated history only inside the tool RESULT — so it round-trips through a new `pendingApprovalToCachedResult` inverse that re-mints the minimal `{ status: 'pending_approval', pendingActionId }` envelope `detectPendingApproval` re-parses. One parse for one contract: no second reader is added on the cache side. The envelope's operator-facing prose and proposed arguments are not re-serialized, matching the leanness the draft and plan inverses already take. +The id travels asymmetrically and the fix follows that asymmetry rather than flattening it. `approval` is a persisted PART key on the server path, so the cache writes it as one and the hydration mapper reads it straight back. `pendingActionId` is never a part key on the server path — it exists in rehydrated history only inside the tool RESULT — so the cache re-mints the minimal `{ status: 'pending_approval', pendingActionId }` envelope `detectPendingApproval` re-parses, through a new `pendingApprovalToCachedResult` inverse. The envelope's operator-facing prose and proposed arguments are not re-serialized, matching the leanness the draft and plan inverses already take. + +The id is ALSO written as a cache-side part key, and the two carriers are not redundant — each reaches where the other cannot. `output` holds one envelope, and an invocation can carry several affordances at once (`draftReview` and `pendingActionId` are independent keys, not two readings of one result). So a turn carrying both keeps its DRAFT envelope in `output` — the pending arm is last in the chain, and the "Review N changes / Publish" card is never displaced — while the id rides the part key. In the other direction, `output` is the only carrier that survives API mode's SDK store, because `useObjectChat`'s `aiInitialMessages` rebuilds each part from `{type,toolCallId,toolName,input,output,errorText,state}` and drops every other key; a pending-only turn, the shape that path can actually produce, lands there. `hydratedMessagesToChatMessages` consults `detectPendingApproval` first and only falls back to the part key, so the framework envelope still decides wherever it has anything to say and no second dialect of it is introduced. **Cached entries written by the previous version are kept and read, not discarded.** No cache-key version bump: both readers of the new keys already answer "absent" rather than throwing, so an old entry restores exactly as it does today, and a discard would blank the transcript, the draft card and the plan card that the old writer did keep — on the one path that renders when the server has nothing. Old entries self-heal, because the first server-backed load after this ships restores the id through the objectui#8442 path and rewrites the cache in the new shape. diff --git a/packages/app-shell/src/console/ai/AiChatPage.tsx b/packages/app-shell/src/console/ai/AiChatPage.tsx index 030288650a..5fa6843de4 100644 --- a/packages/app-shell/src/console/ai/AiChatPage.tsx +++ b/packages/app-shell/src/console/ai/AiChatPage.tsx @@ -241,7 +241,18 @@ export function hydratedMessagesToChatMessages(messages: HydratedUIMessage[]): C // Without the id, `useHitlInChat` never indexes the invocation and the // operator's Approve / Reject has nothing to call. const approval = partApproval(part); - const pendingActionId = detectPendingApproval(result)?.pendingActionId; + // objectui#9232 — the envelope still decides wherever it has anything + // to say; the part key is only consulted when it does not. That order + // is what keeps this a single contract rather than two dialects: on the + // SERVER path the id exists only inside the result, so `??` never + // reaches its right-hand side and this line behaves exactly as + // objectui#8442 left it. The fallback exists for the CACHE path, where + // `output` holds one envelope and a turn carrying both a draft and a + // pending approval has to put its draft there — leaving the part key as + // the only place the id can ride. `sanitizeChatMessagesForCache` is the + // only writer of that key. + const pendingActionId = + detectPendingApproval(result)?.pendingActionId ?? partString(part, 'pendingActionId'); toolInvocations.push({ toolCallId, toolName, diff --git a/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx b/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx index 6c3d942cab..93aca68856 100644 --- a/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx +++ b/packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx @@ -17,7 +17,11 @@ import { uiMessageToChatMessage } from '@object-ui/plugin-chatbot'; // messages and presses Approve. `hydratedMessagesToChatMessages` is the // cache-fallback read this page really performs (AiChatPage feeds it // `initialMessages` from `useChatConversation`). -import { useHitlInChat } from '@object-ui/plugin-chatbot'; +import { + useHitlInChat, + detectDraftResult, + detectPendingApproval, +} from '@object-ui/plugin-chatbot'; import { hydratedMessagesToChatMessages } from '../../console/ai/AiChatPage'; import { @@ -1257,40 +1261,81 @@ describe('sanitizeChatMessagesForCache — a pending approval survives the cache expect('approval' in (toolPart as object)).toBe(false); }); - it('re-mints the pending envelope ahead of a co-occurring draft envelope', () => { - // Not producible by the live mapper: all four detectors read ONE - // `parseResultEnvelope(result)` and discriminate on its single `status`, so - // `pending_approval` and `drafted` cannot both match one result. The arm - // order is therefore unobservable in production — pinned anyway so the - // precedence is a decision rather than an accident of where the arm landed. - // An undecided action is the only one of the four whose loss leaves a live - // control wired to nothing; the others describe something already done. - const cached = sanitizeChatMessagesForCache([ + // ── The both-at-once turn ──────────────────────────────────────────────── + // + // objectui#9232 first shipped with the pending arm FIRST in the chain, on the + // argument that the arms were "disjoint by construction, so the order is + // unobservable". That argument was wrong, and a pre-existing pin + // (`AiChatPage.runtimeMessageSeam.test.tsx`) caught it: the detectors are + // disjoint over one RESULT, but `draftReview` and `pendingActionId` are + // independent KEYS on an invocation and a turn can carry both. Pending-first + // therefore stopped the draft envelope reaching the cache for such a turn — + // the exact "Review N changes / Publish" loss the other arms exist to + // prevent. The tests below pin both halves of the answer. + describe('a turn carrying BOTH a draft envelope and a pending approval', () => { + const both = [ { id: 'a1', - role: 'assistant', + role: 'assistant' as const, + content: 'Staged the changes; deleting the old object needs your approval.', toolInvocations: [ { toolCallId: 'tc-approve', toolName: 'apply_blueprint', state: 'approval-requested', pendingActionId: PENDING_ACTION_ID, - draftReview: { items: [{ type: 'object', name: 'task' }] }, + draftReview: { items: [{ type: 'object', name: 'lead' }], packageId: 'app.crm' }, }, ], }, - ]); - const toolPart = cached[0]?.parts.find((p) => p.type === 'tool-apply_blueprint'); - expect(toolPart?.output).toEqual({ - status: 'pending_approval', - pendingActionId: PENDING_ACTION_ID, + ]; + + it('keeps BOTH: the draft card renders and Approve reaches the pending action', async () => { + const persisted = throughLocalStorage(sanitizeChatMessagesForCache(both)); + + // `output` is the DRAFT envelope — the richer affordance keeps the one + // slot it can ride in, exactly as before objectui#9232. + const toolPart = persisted[0]?.parts.find((p) => p.type === 'tool-apply_blueprint'); + expect(toolPart?.output).toMatchObject({ + status: 'drafted', + packageId: 'app.crm', + drafted: [{ type: 'object', name: 'lead' }], + }); + // …and the id rides the part key, which is the carrier left over. + expect(toolPart?.pendingActionId).toBe(PENDING_ACTION_ID); + + const restored = hydratedMessagesToChatMessages(persisted); + const tool = restored[0]?.toolInvocations?.[0]; + + // Half one: the draft card comes back. + expect(tool?.draftReview).toEqual({ + items: [{ type: 'object', name: 'lead' }], + packageId: 'app.crm', + }); + // Half two: DRIVEN, not read off the call graph — the same drive the + // pending-only round trip gets. Approve must reach the pending action. + const { urls, decision } = await pressApprove(restored); + expect(urls).toEqual([`${API_BASE}/pending-actions/${PENDING_ACTION_ID}/approve`]); + expect(decision?.state).toBe('success'); }); - // And minting it cannot resurrect a Publish button over a rolled-back - // publish (objectui#5695): `detectDraftResult` is silent over this status. - const restored = hydratedMessagesToChatMessages( - JSON.parse(JSON.stringify(cached)) as HydratedUIMessage[], - ); - expect(restored[0]?.toolInvocations?.[0]?.draftReview).toBeUndefined(); + }); + + // The claim the first version of objectui#9232 asserted in prose and got + // wrong by over-reaching. Pinned in the narrow form that is actually true, + // because the arm ORDER now rests on it: over one RESULT the two envelopes + // are mutually exclusive, which is why a pending-only turn (the shape API + // mode can produce) still has `output` free to carry its id. + it('a single tool result cannot yield both a draft and a pending approval', () => { + const drafted = { status: 'drafted', drafted: [{ type: 'object', name: 'lead' }] }; + const pending = { status: 'pending_approval', pendingActionId: PENDING_ACTION_ID }; + + // Lit controls first: each detector really does fire on its own envelope, + // so the two zeros below are readings and not two dead detectors. + expect(detectDraftResult(drafted)).toBeDefined(); + expect(detectPendingApproval(pending)).toBeDefined(); + + expect(detectPendingApproval(drafted)).toBeUndefined(); + expect(detectDraftResult(pending)).toBeUndefined(); }); // ── Stale entries: the decision is READ-SIDE TOLERANCE, not a version bump ── diff --git a/packages/app-shell/src/hooks/useChatConversation.ts b/packages/app-shell/src/hooks/useChatConversation.ts index 46cb7a1ea6..2459da192d 100644 --- a/packages/app-shell/src/hooks/useChatConversation.ts +++ b/packages/app-shell/src/hooks/useChatConversation.ts @@ -450,34 +450,52 @@ export function sanitizeChatMessagesForCache( // is used because the AI SDK preserves it through `useChat` init, // exactly as the server-backed tool-result merge relies on. // - // objectui#9232 — the pending-approval arm. `state` already survived - // this rebuild, so a cached `approval-requested` came back carrying - // nothing to decide with: the operator got Approve / Reject buttons - // whose only possible outcome was "No pending-action id found for - // this tool call". That is the exact divergence objectui#8442 closed - // on the way OUT of server history, reopened on the way IN to the - // cache. + // objectui#9232 — the pending-approval arm, and it is LAST on + // purpose. `state` already survived this rebuild, so a cached + // `approval-requested` came back carrying nothing to decide with: + // the operator got Approve / Reject buttons whose only possible + // outcome was "No pending-action id found for this tool call". That + // is the divergence objectui#8442 closed on the way OUT of server + // history, reopened on the way IN to the cache. // - // It is FIRST in the chain on purpose. The four detectors all read - // one `parseResultEnvelope(result)` and discriminate on its single - // `status` field (`pending_approval` / `drafted` / - // `blueprint_proposed` / the replay pair), so the arms are disjoint - // by construction and the order is unobservable today. The position - // fixes what happens if that ever stops being true: the other three - // restore a card describing something that already HAPPENED, while - // this one restores the operator's ability to ACT, and losing it is - // the only one of the four that leaves a live control wired to - // nothing. Minting this envelope cannot resurrect a Publish button - // over a rolled-back publish (objectui#5695's hazard) either — - // `detectDraftResult` is silent over a `pending_approval` status. - const cachedOutput = tool.pendingActionId - ? pendingApprovalToCachedResult(tool.pendingActionId) - : tool.replayOutcome - ? replayOutcomeToCachedResult(tool.replayOutcome) - : tool.draftReview - ? draftReviewToCachedResult(tool.draftReview) - : tool.proposedPlan - ? proposedPlanToCachedResult(tool.proposedPlan) + // ⚠️ `output` holds ONE envelope, and an invocation can carry more + // than one affordance at a time — `draftReview` and `pendingActionId` + // are independent KEYS on the invocation, not two readings of one + // result. Putting the pending arm first therefore did not merely + // reorder equals: it stopped the draft envelope reaching the cache + // for any turn carrying both, which is the same loss (the "Review N + // changes / Publish" card on a cache-fallback reload) that the other + // three arms exist to prevent, and which + // `AiChatPage.runtimeMessageSeam.test.tsx` already pinned. So the + // three existing arms keep `output` exactly as before and the + // pending envelope is minted only when none of them claims it. + // + // Losing the id is not the price of that, because the id does NOT + // depend on `output` alone here — it is also written as a part key + // below. The two carriers are not redundant; each reaches where the + // other cannot, and that is the whole design: + // + // * `output` is the only carrier that survives API mode's SDK + // store, because `useObjectChat`'s `aiInitialMessages` rebuilds + // each part from `{type,toolCallId,toolName,input,output, + // errorText,state}` and drops every other key, after which + // `extractToolInvocations` re-derives the id by re-parsing the + // result. A pending-only turn — the only shape API mode can + // actually produce, since `detectDraftResult` and + // `detectPendingApproval` read ONE `parseResultEnvelope(result)` + // and require different `status` values — lands here. + // * the PART KEY is the only carrier left when a richer envelope + // has taken `output`. Local mode keeps it, because + // `normalizeMessages` passes `toolInvocations` through verbatim, + // and `hydratedMessagesToChatMessages` lifts it on the way back. + const cachedOutput = tool.replayOutcome + ? replayOutcomeToCachedResult(tool.replayOutcome) + : tool.draftReview + ? draftReviewToCachedResult(tool.draftReview) + : tool.proposedPlan + ? proposedPlanToCachedResult(tool.proposedPlan) + : tool.pendingActionId + ? pendingApprovalToCachedResult(tool.pendingActionId) : undefined; parts.push({ type: `tool-${tool.toolName}`, @@ -488,6 +506,12 @@ export function sanitizeChatMessagesForCache( // The SDK envelope rides the PART, both here and on the server // path — `partApproval` narrows it straight back off this key. ...(tool.approval ? { approval: tool.approval } : {}), + // The id as a part key — the carrier that survives a taken + // `output`. It is a CACHE-side supplement, never a rival reading of + // the framework envelope: `hydratedMessagesToChatMessages` consults + // `detectPendingApproval` FIRST and only falls back here, so the + // one contract still decides wherever it has anything to say. + ...(tool.pendingActionId ? { pendingActionId: tool.pendingActionId } : {}), ...(cachedOutput !== undefined ? { output: cachedOutput } : {}), }); }