fix(app-shell): keep the approval envelope + pending-action id in the chat cache - #9450
Merged
Merged
Conversation
… chat cache (objectui#9232)
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
…placing the draft envelope
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-tesla
marked this pull request as ready for review
September 14, 2026 01:58
os-tesla
marked this pull request as draft
September 14, 2026 02:13
Collaborator
Author
Contract reviewHead reviewed: ① Derived judgments
② Semver
③ Boundary flags
VerdictPASS.
|
Collaborator
Author
|
Clause-② carriers stripped — PASS. Review record: comment
⭐ Platform fact, new and worth carrying: converting the PR to draft and disabling auto-merge did NOT evict its merge-queue entry. ⇒ With the PASS on record and both carriers clear, the entry is now legitimately landable; the PR is returned to ready. Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9232
sanitizeChatMessagesForCacherebuilds each tool part field by field —type, toolCallId, toolName, state, errorText?, output?— and wrote neither the AI SDKapprovalenvelope nor the ObjectStackpendingActionId.statesurvived, so a cachedapproval-requestedcame back with the awaiting-approval card lit and nothing behind it:useHitlInChatindexes decisions onpendingActionId, so pressing Approve could only answer "No pending-action id found for this tool call". Theoutputre-serialization coveredreplayOutcome/draftReview/proposedPlanonly, 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). PR #9229 merged 2026-09-12, so the divergence was live onmain, not prospective. The cache fallback is not a corner —readMessageCacheis what renders the thread whenever the server returns no messages.Correction in the second commit — read this first
The first commit put the pending arm first in the
cachedOutputchain, arguing the four detectors were "disjoint by construction, so the order is unobservable". That argument was wrong, and a pre-existing pin caught it:AiChatPage.runtimeMessageSeam.test.tsxwent red on CI shard 3.The detectors are disjoint over one result. But
draftReviewandpendingActionIdare independent keys on an invocation, not two readings of one result, and a turn can carry both — that fixture does. Pending-first therefore did not reorder equals; it stopped the draft envelope reaching the cache for such a turn. That is 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 PR's own stale-cache reasoning refused to accept from a key bump. The precedence was a choice described as a non-choice.The second commit fixes the mechanism rather than the symptom.
AiChatPage.runtimeMessageSeam.test.tsxis unedited and passes.What changed
outputholds 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.The two carriers are not redundant — each reaches where the other cannot, and that is the design:
outputis the only carrier that survives API mode's SDK store.useObjectChat'saiInitialMessagesrebuilds each part from{type, toolCallId, toolName, input, output, errorText, state}and drops every other key, after whichextractToolInvocationsre-derives the id by re-parsing the result. A pending-only turn lands here — and that is the only shape API mode can produce, sincedetectDraftResultanddetectPendingApprovalread oneparseResultEnvelope(result)and require differentstatusvalues.output. Local mode keeps it, becausenormalizeMessagespassestoolInvocationsthrough verbatim, andhydratedMessagesToChatMessageslifts it on the way back.approvalcontinues to ride the part key on both directions of the server path, wherepartApprovalnarrows it straight back.No second dialect.
hydratedMessagesToChatMessagesconsultsdetectPendingApprovalfirst and only falls back to the part key. On the server path the envelope always answers, so that line behaves exactly as objectui#8442 left it; the fallback exists solely for the cache, which is the only writer of that key.Clause-②: yes — re-declared, overturning the claim's prediction
The claim declared
Clause-②: noon the basis thatsanitizeChatMessagesForCacheis not exported. That is still true, and for the first commit the declaration was right. The second commit changes the behaviour ofhydratedMessagesToChatMessages, whichpackages/app-shell/src/index.tsdoes re-export. The signature and return type are unchanged and the change is additive — it recovers an id in a case that previously yielded none — but it moves a published function, so the declaration is re-made asyesandneeds:contract-reviewis attached to both the card and this PR. The diff is the fact; the prediction was made before the diff existed.check-governed-queue-guard.mjs --teston all four paths: NOT GOVERNED, none of 5 governed surfaces matched.Stale cache: read-side tolerance, deliberately — not a version bump, not a discard
Entries already in a user's
localStoragelack the new fields. The decision is to keep and read them, recorded onreadMessageCacheand pinned by two tests:partApprovalreturns undefined for a missing envelope,detectPendingApprovalreturns undefined for a result that is not one.runtimeMessageson every render, so the first server-backed load restores the id through the objectui#8442 path and writes it back in the new shape.Tolerance does not invent: an old entry keeps
approval-requestedwith no id — today's behaviour — and no id is fabricated.Evidence
Readings at
95f2ff0, tree clean. Repo root, path-filtered, per AGENTS.md; heavy runs throughscripts/pm/os-verify-lock.sh, and every verdict is the wrapper'sVERDICT command-exitline or the tool's own summary line.Scope actually run — stated plainly, because the first round's scope is how this got through.
pnpm test --shard=3/4, the exact CI invocation:Test Files 776 passed (776),Tests 10360 passed | 1 skipped (10361), exit 0.AiChatPage.runtimeMessageSeam.test.tsxhas exactly 8 tests. Vitest partitions by position in the discovered file list, so a file set that differs anywhere reshuffles membership — local shard 3 is not CI's shard 3. Reported rather than presented as "shard green, therefore fixed".pnpm exec vitest run packages/app-shell/src/console/ai/__tests__/AiChatPage.runtimeMessageSeam.test.tsx->Test Files 1 passed (1),Tests 8 passed (8), unedited.Test Files 4 passed (4),Tests 79 passed (79).Typecheck —
pnpm --filter @object-ui/app-shell run type-check(tsc --noEmit && tsc -p tsconfig.test.json): exit 0, afterpnpm --filter '@object-ui/app-shell^...' build(exit 0) supplied the workspacedist/*.d.ts. Run before that build it reports 26TS2307, which is a precondition failure and NOT MEASURED, never a red verdict.Gates — eleven, each exit 0:
check-control-bytes,check-changeset-presence,check-changeset-claims,check-changeset-fixed,check-changeset-no-major,check-changeset-overwrite,check-test-path-roots,check-vi-mock-override-shape,check-vi-mock-specifiers,check-vi-mock-inherit,check-new-cross-file-line-citations. Plus a control-byte self-scan over all four changed files: no hits.Lint — a declared narrowing with its three readings. (1) Population: ESLint's own configured file count is 4954. (2) The narrowed run covered 3 files (
--format json): 0 errors, 36 warnings. (3) Immutability:eslint.config.jssetslanguageOptionstoecmaVersion+globalsonly — noparserOptions.project, noprojectService— so no rule here is type-aware and a three-file change cannot move any untouched file's verdict. Every warning is pre-existing: theAiChatPage.tsxhunk spans lines 244-255 and the nearest warnings sit at 191 and 302, whileuseChatConversation.tscarries the same fivereact-hooks/*warnings as before, shifted only by added comment lines. Repo-widepnpm lintis CI's run.Ablation — three legs, each predicted in writing before it ran. The fix has two carriers, so one blanket mutation would have proved nothing about either; the legs separate them.
outputarmoutputpin. The driven pending-only test stays green1 failed | 45 passed (46)toolPart?.pendingActionId1 failed | 45 passed (46),AssertionError: expected undefined to be 'pa_9232'urlsassertions3 failed | 43 passed (46), includingAssertionError: expected [] to deeply equal [ Array(1) ]Leg A is the honest one and its prediction says so in advance: at the hydration boundary the part key alone still carries the id, so these tests cannot see
output's loss.output's necessity is for the API-mode SDK store rebuild, which no test here crosses — it is argued from the source, not measured. Leg B is the one that matters this round: it proves the new carrier is load-bearing rather than decoration.Mutation proved on disk, never off an editor exit code: anchor greps went 1 to 0 for each deleted anchor, and blob hashes moved (
2459da19to69e417d5/95924f2f/e12412ccon the writer;5fa6843dto84493432on the reader, unchanged in leg A as expected since leg A does not touch it). Restore proved by state after every leg:git diff HEADempty, 0 lines. The script carriestrap ... EXIT INT TERMwith absolute repo-root paths, restores withgit checkout HEAD -- FILE, and refuses to start unless both files are already atHEAD— which it did refuse, correctly, on the first attempt before the patch was committed.Not measured: browser behaviour, and
output's necessity for the API-mode store (see leg A).Acceptance notes
A targeted dedup search was run before recording these: REST
/search/*is refused for this session by the egress proxy (HTTP 403, "sessions are bound to their configured repositories"), so one MCPsearch_issuescall was used instead and is declared here. Its first hit was objectui#9232 itself, the lit control proving the query reaches this surface; no open card names either observation.approvalenvelope is currently write-only on every path.extractToolInvocationsdoes not liftpart.approval, andaiInitialMessagesdrops it re-entering the SDK store; grep finds no consumer oftool.approvalanywhere. No observable symptom, so declared-but-unconsumed rather than a defect. Noted, not filed. Successor: objectui#8426, which the@object-ui/typesdocstring names as the follow-up that makes the envelope load-bearing. This PR writes it anyway, for parity with what the server path persists.isAwaitingApprovallights Approve / Reject without requiringpendingActionId. An invocation with the state but no id renders live buttons whose only outcome is the handled error indecide(). That handler is deliberate and user-facing rather than a crash, so this is UX polish, not a defect. Noted, not filed. Successor: objectui#2477, the openpm:queueumbrella for Console AI chat UX follow-ups..github/workflowsreads a PR label. The reading still holds mechanically, but it was the wrong frame: the Clause-② flip makesneeds:contract-reviewowed as a review routing signal to a human, not as a gate input. It is attached to both the card and this PR, and both were read back to confirm.Produced by Claude Code, seat session
https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa. The session id identifies the SEAT, not this individual change.Generated by Claude Code