refactor(app-shell): AiChatPage narrows the chat hook's messages through toRuntimeMessages (#4437) - #4445
Merged
Conversation
…ugh toRuntimeMessages (#4437) `useObjectChat` returns `ObjectChatMessage[]` — the shape both of its modes really produce (#4424 / PR #4436) — which keeps the authored `'tool'` role and the legacy tool states the runtime type folds. This page wants the runtime shape and said so five times with `as ChatMessage[]`, plus one `as unknown as` double cast. The narrowing was real; what was missing is that a cast erases the whole difference rather than the intentional part of it. One conversion now sits where the hook's values enter the page's runtime-typed world, memoized on `messages` exactly as the plugin's own three renderers do (#4399 / PR #4416). All five sites read it and no cast replaces them; the double cast turned out to be unnecessary once the value is honestly typed. Per-site fold sensitivity was measured, not assumed: - sanitizeChatMessagesForCache — AFFECTED, in the direction it wants. Its parameter declares `role: 'user' | 'assistant' | 'system'`, so it has always asked for folded roles and the cast is what let an unfolded `'tool'` past that declaration. Measured: the entry was cached as `role: 'tool'`, which `readMessageCache`'s own validator rejects on the way back in, and its tool invocations were dropped entirely because sanitize gates them on `role === 'assistant'` — including the re-serialized draft envelope behind "Review N changes / Publish". - isConversationZh — UNAFFECTED. Reads `role === 'user'` and the text; the fold can neither create nor destroy a user turn. - deriveBoundPackageId — UNAFFECTED. Walks every message irrespective of role and reads keys the adapter spreads through. Nothing on screen moves today: neither a `'tool'` role nor a legacy tool state is producible on this page's own paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. 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 #4437
useObjectChatreturnsObjectChatMessage[]— the shape both of its modes really produce (#4424 / PR #4436) — which is wide where local mode is wide: it keeps the authored'tool'role and the legacy'partial-call'/'call'/'result'tool states.AiChatPagewants the runtime shape, and said so five times:sanitizeChatMessagesForCache(...)messages as ChatMessage[]isConversationZh(...)messages as ChatMessage[]useHitlInChat({ messages })messages as ChatMessage[]deriveBoundPackageId(...)messages as unknown as readonly PackageBearingMessage[]ChatbotEnhancedmessagespropmessages as ChatMessage[]The narrowing was real and every cast was legal — that is the card's premise, and it survived re-verification at the branch point. What was missing is that a cast erases the whole difference rather than the intentional part of it: a new authored role, or a newly required runtime key, kept compiling at all five sites and surfaced as rendering behaviour.
The conversion
One conversion where the hook's values enter the page's runtime-typed world, memoized on
messagesexactly as the plugin's own three renderers do (#4399 / PR #4416):All five sites read it. No cast replaces them, and no new
asis introduced — theas unknown asdouble cast at row 7 turned out to be unnecessary once the value is honestly typed, which is itself a small piece of evidence that the double cast existed to paper over the mis-declaration rather than over a real structural gap.Placement is right after the
useObjectChatdestructure, above the first consumer. Identity is neither more nor less stable than before: the hook rebuildsapiMessagesevery render, so both the oldmessages as ChatMessage[]and the new memo change identity on exactly the same renders.Per-site fold sensitivity — measured, not assumed
The card's binding clause. Each site was called twice — once on the honest pre-fold array (what the casts handed it), once on
toRuntimeMessages(...)output — over a thread carrying a'tool'role and two legacy states. The fold itself, measured:sanitizeChatMessagesForCacheisConversationZhtrue/true; en-lastfalse/false; zh-only-on-the-tool-turnfalse/falsederiveBoundPackageIdapp.crm2/app.crm2; tool-role-lastapp.crm/app.crmNeither insensitive result is vacuous by construction: the zh probe includes a variant that answers
trueon both sides and one where the Chinese text sits only on the message whose role the fold rewrites; the package probe puts the'tool'message last (the derivation is newest-wins), so a role-dependence would show as its packageId appearing or disappearing.Both are insensitive structurally, not incidentally.
isConversationZhreadsrole === 'user'and the text — the fold maps'tool'to'assistant', so it can neither create nor destroy a user turn, andcontent/partsride the pass-through untouched.deriveBoundPackageIdwalks every message irrespective of role and readsdraftReview/builderHandoff, whichtoRuntimeToolInvocationspreads through.The one affected site, and why the change is the one it wants
sanitizeChatMessagesForCachedeclares its parameter's role as'user' | 'assistant' | 'system'. It has always asked for folded roles — the cast is precisely what let an unfolded'tool'past that declaration, so routing through the conversion makes the call type-checked rather than cast-asserted. Three measured deltas, all on the'tool'-role message:role:'tool'->'assistant'.readMessageCache's own validator accepts only user/assistant/system, so the pre-fold entry was written and then silently dropped on restore — the message disappeared on a cache-fallback reload.role === 'assistant'), present post-fold — including the re-serialized draft envelope{status:'drafted', drafted:[…], packageId:'app.crm'}, i.e. the "Review N changes / Publish" affordance survives the reload it exists to survive only post-fold.'call'->'input-available'(that one was already serialized; only the spelling changed, and the restore path names the v6 states while catching legacy only via a default branch).So the site's own semantics — cache round-trip fidelity, expressed in its own parameter type — want the folded value. Adapting at the last moment per call site was the alternative the card offered; it would have preserved a write the cache's reader rejects.
Reachability, stated so the table is not over-read: neither a
'tool'role nor a legacy state is producible on this page's own paths today.initialMessagescomes fromhydratedMessagesToChatMessages, whose roles areHydratedUIMessage['role'](user/assistant/system) and whose states come frompartToolState(v6 only); API mode's values come frommapMessages, also v6. The rendered-output bar is therefore zero at runtime right now, and this delta is a latent-correctness fix that becomes live the day such a value reaches the page.Tests
packages/app-shell/src/console/ai/__tests__/AiChatPage.runtimeMessageSeam.test.tsx— 8 tests, three groups, driving the real page with a faked hook that hands out the honest unfolded shape:'tool'message reaches theChatbotEnhancedelement asrole: 'assistant'with its legacy state folded to'output-available', anddraftReview/pendingActionIdsurvive the conversion (the pass-through);useHitlInChatreceives the same converted array (it reads the runtime-onlypendingActionId);messages, and that the conversion happens exactly once.Reverse verification — direction predicted first
The card's premise is that the casts are legal, so "restore a cast and watch
tsc" cannot be the probe: the prediction at every site is that it compiles. What has to red is a behavioural pin or the net. Predictions were written down before running; all three matched.ChatbotEnhancedprop)TSC=0, empty output.AssertionError: expected [ 'user', 'tool' ] to deeply equal [ 'user', 'assistant' ]. Net red. 2 failed / 6 passedTSC=0.AssertionError: expected 'tool' to be 'assistant'. Net red. 2 failed / 6 passedisConversationZh)TSC=0. 1 failed / 115 passed over the wholeai/suite — the single failure is the source netProbe C is the honest negative and the reason the net exists rather than being assumed. Stated plainly: of the five sites, only two can red behaviourally. Rows 5 and 7 are fold-insensitive (that is the measurement), and row 6 reads a key the pass-through preserves either way — a cast coming back at any of those three is invisible to every runtime assertion and to
tsc, because a cast is exactly the instruction to stop checking. The net is what makes them non-silent; nothing else in this PR can.Baseline restored after each probe, tree verified clean.
Verification
pnpm exec vitest run packages/app-shell/src/console/ai/— 116 passed (18 files), up from 108 passed (17 files) onorigin/main: +8 tests in +1 file, nothing existing changed.pnpm exec vitest run packages/app-shell/— 3377 passed | 1 skipped (355 files), 0 failed.tsccommands for app-shell (tsc --noEmitandtsc -p tsconfig.test.json) — green, afterpnpm --filter '@object-ui/app-shell^...' build(the package-leveltscresolves@object-ui/*throughdist/*.d.ts, absent in a fresh worktree; only vitest and the root tsconfig alias tosrc).eslinton both touched files — 0 errors. Warning count is 26 before and 26 after onAiChatPage.tsx(measured by running eslint on theorigin/maincopy of the file), i.e. no new finding on a touched line.check-changeset-presence/check-changeset-no-major/check-changeset-fixed/check-control-bytes/check-phantom-dependencies— all exit 0.Changeset:
@object-ui/app-shellpatch. Internal page wiring — nothing published moves: no export is added, removed or retyped, andtoRuntimeMessageswas already exported from@object-ui/plugin-chatbot's barrel by PR #4416. Nevermajor(fixed group;check-changeset-no-major).Observed and deliberately left alone
AiChatPage.tsx:862carries a structurally identicalas unknown as readonly PackageBearingMessage[]onhydratedMessagesToChatMessages(messages). It is not one of the card's five — that value is the page's own locally-produced runtime array, not the hook'smessages— so it is out of this card's surface and untouched. Noting it because the same simplification would now apply there; not a defect, so not filed.Generated by Claude Code