fix: surface and render OpenAI and Claude reasoning summaries - #2645
fix: surface and render OpenAI and Claude reasoning summaries#2645hqhq1025 wants to merge 3 commits into
Conversation
0b82b9c to
c87bef5
Compare
|
Self-review completed after marking ready. Fixed two issues before merge:
Rebased onto the latest |
|
Thanks — the gap is real (on main, Conclusion: PASS — all P3, including one worth handling. P3-1 (worth handling): explicit P3-2: the Anthropic response path for AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS，全 P3（其中 P3-1 建议处理）。问题真实（main 上 buildProviderOptions 在用户未选 thinking 级别时对 openai/anthropic/claude-subscription 一律不发 reasoning 参数 → 模型按默认运行、reasoning 不出现在 transcript；display 链路本就存在——SDK reasoning_summary_text.delta → reasoning-delta → adapter thinking 事件 → 既有 thinking_delta/AssistantMessage.thinking），改动补齐请求端默认，非并行实现。已按固定版本 SDK 源码验证 wire 行为：reasoningSummary:'auto' 发 reasoning:{effort,summary:'auto'} 且 store:false 时仍带 reasoning.encrypted_content（与 #2328/#2518 加密链共存）；thinking:{type:'adaptive',display:'summarized'} 由 SDK 原样透传。无 schema/协议改动，旧数据/旧客户端不受影响，未被 main 后续提交取代。P3-1（建议处理）：显式 off 在 Claude effort 模型上被静默升级为开启可见 adaptive thinking，与 PR 自身政策矛盾（PR 自带测试断言 off→{thinking:{type:'adaptive',display:'summarized'}}）——旧行为是发空配置；仓库惯例（kimi-coding-plan 分支"必须大声失败、不得静默升级"）是拒绝而非升级。当前仅直接 runtime 调用方可达（UI 对 effort 模型不提供 off 选项），建议显式拒绝或如实文档化。P3-2：Anthropic 响应路径无 dedicated 测试（只有请求体断言）；P3-3：默认开启的 wire 参数对第三方 relay 无能力协商（代理旧版 API 可能 400 或静默丢字段），合并前建议对真实 relay 实测；P3-4：分支落后 main ~530 commits（CONFLICTING），"3325 passed" 是对旧树的验证，rebase 后重跑全套。 |
Astro-Han
left a comment
There was a problem hiding this comment.
The main design is sound: this completes the existing provider-options → SDK → thinking-display chain rather than creating another transcript path, and the provider/wire split has broad focused coverage.
This head is now conflicting with current main in two reasoning-contract test files, so its old-base green checks are not integration evidence; please rebase and rerun the focused Runtime tests, typecheck, and CI. One additional low-severity edge remains for custom Chat relays: an explicit unsupported setting can be silently replaced by the GPT-family default. From first principles, an invalid explicit choice must not become a different valid choice; defaulting should apply only when no choice was supplied.
Reviewed with Codex using two independent reviewer agents and an external DeepSeek review; I verified the latest head, current-main conflicts, provider-option paths, existing discussion, and CI state.
中文
主方案是合理的：它补齐现有 provider-options → SDK → thinking-display 链路，没有建立第二条 transcript 路径；provider/wire 分流也有较完整的聚焦测试。
但当前 head 已与最新 main 在两个 reasoning contract 测试文件中冲突，旧 base 上的全绿检查不能证明可集成；请先 rebase，再重跑 Runtime 聚焦测试、typecheck 与 CI。另有一个低严重度边界：自定义 Chat relay 的显式不支持选项可能被静默替换成 GPT family 默认值。按第一性原理，无效的显式选择不应变成另一个有效选择；只有调用方没有提供选择时才应使用默认值。
本次由 Codex 配合两个独立 reviewer agent 和外部 DeepSeek 审查；我核验了最新 head、current-main 冲突、provider option 路径、已有讨论和 CI 状态。
|
/agentic_review |
Code Review by Qodo
1. Copilot omits default effort
|
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head c87bef5a51f8d74e70d763e8d96f7f7cfd93fd5f against current main@d62857a8357e9160926726a2a13096bc2dc2b91d.
The bug still exists on current main: recognized GPT Responses routes do not request reasoningSummary, and recognized Claude Anthropic routes do not request summarized display by default. The PR's requested default-medium/summarized policy matches issue #2644, and its request-shape plus real SSE boundary coverage is directionally strong.
Blocking rebase/authority conflict. This head is based before the current Responses routing and replay contract. Current buildFamilyWire distinguishes native OpenAI from open-responses, verifies reasoningReplay, keys provider options by the actual Open Responses provider name, preserves serviceTier, and enables forceReasoning for encrypted-content replay. The PR's version destructures only { adapter, wire } and returns a single openai namespace for every Responses wire. It is now text-conflicting, and resolving that mechanically would either drop current replay/service-tier behavior or silently send summary options to a namespace the Open Responses adapter does not read.
Rebase and implement visibility at the current resolved-wire seam: retain the existing reasoningReplay and open-responses branches, add OpenAI reasoningSummary: 'auto' only where the native OpenAI Responses schema consumes it, and preserve provider-native options/names for Open Responses relays. Re-run the real Responses SSE test and the provider contract sweep on the rebased exact head.
Required conclusions:
- Optimal for the actual problem: the policy is sound, but this revision is no longer an optimal/current implementation.
- Production code to delete: none identified beyond avoiding a parallel/legacy Responses branch during rebase.
- Tests to delete/replace: none identified; port the strong request-shape and SSE tests to the current routing matrix.
- Deeper refactor: no new abstraction; adapt the change to the existing current-main wire/replay authority.
- Ready to merge: no; it is conflicting and predates material routing contracts.
- Residual risks/gaps: provider-visible reasoning changes user-visible behavior and token/cost characteristics, so independent human review and exact-head CI remain required after rebase.
c87bef5 to
ebfab4b
Compare
|
Rebased onto current This revision addresses the review findings while preserving the current runtime routing contracts:
Validation on the exact head:
Ready for re-review. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for pushing this through several rounds — the OpenAI Responses side looks right to me, and the earlier points from @jackwener and the automated pass have all been addressed on this head. The provider-options → SDK → thinking-display chain is the correct place to fix this, and I like that you didn't invent a second transcript path.
Four independent reviewers looked at this. Three found nothing blocking. The fourth, reviewing blind, found something the other three missed, and I've since verified it myself against the pinned SDK and the model catalog. Two P1s, both on the Anthropic side. Details inline.
The root cause is one confusion, not two bugs: thinkingOptions.efforts is a UI fact — which effort tiers to show in a picker. supportsAdaptiveThinking is a wire fact — whether the model accepts thinking: {type: 'adaptive'}. This PR uses the first as a proxy for the second. They diverge in both directions, which is why there are two failure modes rather than one.
What I'd suggest: derive one normalized Claude thinking-mode decision — adaptive / legacy enabled+budget / unsupported — from the model id, and add request-body tests for Sonnet 4.5, Opus 4.5, a current adaptive model, and a dotted/slashed relay alias. The existing tests cover Haiku 4.5 and Opus 4.8, which happen to sit on the two ends that already work.
Not approving on this head, but that's a mechanical note as much as a judgement: reviewDecision is still CHANGES_REQUESTED from the earlier review, so merge is gated on that being lifted regardless of what I say here.
3549efa to
0bfc703
Compare
|
Pushed revision What changed since the prior revision:
Validation on this revision:
The PR description now includes matched before/after screenshots. Please re-review this revision; merge readiness still depends on green CI and a fresh human approval. |
aa09557 to
68b45d9
Compare
|
CI update for |
ARE404
left a comment
There was a problem hiding this comment.
Exact-head review of 68b45d94f1fa0c3fe4bbb1aae3eca277ff9ba425 against merge-base cd0163a1effc27aec61cc331f799959e30fd7446.
Result: NO-GO (COMMENT). The provider request-shape matrix is substantially improved and all expected exact-head checks are terminal green (test, dependency audit, Windows package), but the shared display change introduces one privacy-boundary regression and one ordinary-text rendering regression. The PR also remains DIRTY/conflicting, and the repository's existing CHANGES_REQUESTED decision remains in force.
Standards axis
- The collapsed reasoning preview bypasses the display redaction boundary (P1, inline).
- The PR description omits the mandatory yes/no generative-tooling declaration required by
CONTRIBUTING.md; the sole commit also has noGenerated-by:trailer, which is additionally required if material AI-authored content is present. - Nonblocking smell: default OpenAI effort resolution is repeated across direct Codex, direct OpenAI, native Responses, and compatible Chat branches. A single resolver would reduce future off/default drift.
Spec axis
- The shared KaTeX scanner does not preserve paired currency, despite the PR's explicit ordinary-currency guarantee (P2, inline).
- The resolved-wire/provider coverage and the Claude family-capability split otherwise match the stated request policy on this head.
No merge action was taken.
68b45d9 to
c4b7d44
Compare
There was a problem hiding this comment.
I re-checked every open point on c4b7d443 rather than on the heads they were raised against. Five are closed; two still apply, and I have replied in their existing threads instead of opening new ones.
Closed on this head
- The Claude thinking-options gap is no longer there — the catalog now covers the effort models in generated metadata plus the Sonnet 4.5 / Opus 4.1 / Haiku 4.5 overrides, and the factory normalizes the namespace before falling back to it (
model-factory.ts:407-412). adaptiveis no longer sent to models the pinned SDK says cannot accept it;model-factory.ts:414-416picksadaptiveorenabledwith a 1024 budget by capability, covered atmodel-factory-thinking.test.ts:100-120and:459-477.- An explicit unsupported relay level is no longer silently replaced with
medium— the default only applies when the level is omitted (model-factory.ts:701-703,:721-724), asserted atmodel-factory-thinking.test.ts:502-507. - Both automated findings on
model-factory.tswere real on the older commit and are fixed here: Copilot Responses now goes through the shared wire branch at:679-714, and namespaced Claude ids normalize viaclaudeFamilyIdat:407-409before the metadata lookup.
Still open — collapsed reasoning preview bypasses secret redaction (P1), and paired dollar signs are parsed as inline math (P2). Both in the threads above.
test, package and audit are all terminal green on this head.
中文
所有未决点我都对着 c4b7d443 重新核了一遍，不是沿用它们各自提出时的 head。五条已闭：Claude thinking-options 缺失（catalog 已覆盖 + model-factory.ts:407-412 先归一化命名空间再回退）、往不支持的模型发 adaptive（:414-416 按 capability 选择）、显式非法 relay level 被替成 medium（:701-703/:721-724 仅在省略时套默认）、两条自动化发现（旧 commit 上是真问题，当前 head 已修）。
两条仍成立：折叠态 reasoning 预览绕过脱敏（P1）、成对美元号被当作行内公式（P2）。都已回在原有线程里，没有另开新线。
此 head 上 test、package、audit 均已终态绿。
c4b7d44 to
e14eb41
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Rendering reasoning through the same Markdown pipeline as answers is the right direction, and routing the preview text separately so the collapsed summary stays plain is a nice touch. The redaction path still holds — Markdown redacts text and settledText itself, so moving the call out of DeepThinking does not widen anything.
Three things, two of which I would like fixed before this merges.
[P2] Display math written across several lines is left on screen as raw LaTeX
protectMathOutsideCode splits the source on newlines and hands each line to protectMathInLine, and every reader — readDelimitedMath and findClosingDollar — searches only within that one line. A delimiter pair that opens on one line and closes on another is never recognised.
I ran the functions from this branch directly against a few inputs:
inline \(..\) TOKENIZED
single-line $$ TOKENIZED
single-line \[ TOKENIZED
multi-line $$ LEFT LITERAL
multi-line \[ LEFT LITERAL
The two that fail are the ordinary way a model emits display math:
$$
E = mc^2
$$
That reaches the reader as the literal characters, and worse than untouched — the body between the delimiters is still Markdown, so underscores become emphasis and backslashes get eaten. The three new tests all keep their display math on one line, which is why this passes. Please handle the multi-line form and add a case for it; the single-line paths look right and I have no concerns there.
[P2] Dropping white-space: pre-wrap from the reasoning body re-opens the problem #1748 caused
The rule this replaces carried its own history: it said the Astryx atoms own no white-space, so the inherited white-space: normal collapses every newline in thinking text, and that pre-wrap was restored after the #1748 migration dropped it. The new rule sets white-space: normal again and relies on Markdown to supply the structure.
That holds only for reasoning that is actually Markdown. The Markdown parser treats a single newline as a soft break and joins the lines unless the line ends in two spaces or a blank line separates the paragraphs. Plenty of reasoning text is plain prose with hard single newlines and neither of those, and all of it will now run together into one block. Before, pre-wrap guaranteed the layout regardless of what the model emitted.
Please check this against real reasoning output that is not Markdown-structured. If it reads badly, keeping pre-wrap on the container while still rendering Markdown inside it would preserve both.
[P3] @ai-sdk/anthropic/internal is the first internal-subpath import in the tree
getModelCapabilities comes from a subpath the package does export, so this is legitimate, and @ai-sdk/anthropic is pinned to an exact version, so nothing moves under you today. But it is the first place we reach into an internal entrypoint, and that name is the usual signal that its contents are not covered by the package's compatibility promise. Worth a comment at the import saying what it is for and what to do if it disappears, so whoever next bumps that version knows this is load-bearing.
I have not raised anything about the KaTeX dependency itself — pinned exactly, the stylesheet is imported alongside it, and the math container styles look sound. One thing I could not check from the diff: please confirm the KaTeX web fonts are actually bundled into the desktop renderer. When they are missing, KaTeX silently falls back to a serif face with the wrong metrics, so it looks merely ugly rather than broken. A screenshot of a rendered formula would settle it and would also cover the before/after evidence this change needs.
简体中文
把 reasoning 走与答案相同的 Markdown 管线是对的方向，把预览文本单独抽出来、让折叠态摘要保持纯文本也处理得很好。脱敏路径没有被削弱——Markdown 组件自身会对 text 和 settledText 调用脱敏，所以把这个调用从 DeepThinking 里移走并没有扩大暴露面。
三点，其中两点希望在合并前修掉。
[P2] 跨行书写的展示式公式会以 LaTeX 原文留在界面上
protectMathOutsideCode 按换行切分源文本、逐行交给 protectMathInLine，而所有读取函数（readDelimitedMath、findClosingDollar）都只在当前这一行内查找。一对在某行开启、在另一行闭合的定界符永远不会被识别。
我直接用这个分支上的函数跑了几组输入：
inline \(..\) TOKENIZED
single-line $$ TOKENIZED
single-line \[ TOKENIZED
multi-line $$ LEFT LITERAL
multi-line \[ LEFT LITERAL
失败的这两种，恰恰是模型输出展示式公式最常见的写法：
$$
E = mc^2
$$
它会以字面字符呈现给读者，而且比"原样不动"更糟：定界符之间的正文仍会走 Markdown，于是下划线变成强调、反斜杠被吞掉。新增的三个测试的展示式公式都写在一行内，所以这个问题没有被暴露。请支持跨行形式并补一条对应测试；单行的几条路径看下来是对的，我没有疑虑。
[P2] 去掉 reasoning 正文的 white-space: pre-wrap，会重新打开 #1748 造成的那个问题
被替换掉的那条规则自带历史说明：Astryx 的原子组件不负责 white-space，因此继承来的 white-space: normal 会把 thinking 文本里的每一个换行都折叠掉；而 pre-wrap 正是在 #1748 迁移把它弄丢之后被恢复回来的。新规则重新设成了 white-space: normal，改为依赖 Markdown 来提供结构。
但这只有在 reasoning 本身确实是 Markdown 时才成立。 Markdown 解析器把单个换行当作软换行，除非行尾有两个空格、或者段落之间隔了空行，否则会把相邻行拼成一行。而相当多的 reasoning 是带硬换行的纯散文，两个条件都不满足，这些内容现在会全部挤成一整块。在此之前，pre-wrap 不管模型输出什么都能保证排版。
请拿真实的、非 Markdown 结构的 reasoning 输出验证一下。如果阅读效果变差，可以在容器上保留 pre-wrap、同时在其内部渲染 Markdown，两者可以兼得。
[P3] @ai-sdk/anthropic/internal 是本仓库第一处 internal 子路径导入
getModelCapabilities 来自该包确实导出的子路径，所以这个用法是合法的；而且 @ai-sdk/anthropic 锁的是精确版本，当下不会有东西在脚下移动。但这是我们第一次伸进 internal 入口，而这个命名通常意味着其内容不在该包的兼容性承诺范围内。建议在 import 处加一句注释，写明它的用途、以及万一它消失了该怎么办，好让下一个升级这个版本的人知道这里是承重的。
关于 KaTeX 依赖本身我没有提出问题——版本锁死、样式表随之引入、公式容器的样式也看着合理。有一点我从 diff 里看不出来：请确认 KaTeX 的 web 字体确实被打进了桌面端渲染进程。 字体缺失时 KaTeX 会静默回退到衬线字体并带着错误的度量，表现为"只是有点丑"而不是明显坏掉。一张公式渲染后的截图就能说明问题，同时也正好满足这个改动需要补的前后对比证据。
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
e14eb41 to
e4b3549
Compare
|
Addressed all three points from the latest review on exact head
Exact-head local verification:
The branch is rebased onto GitHub CI is running on this head. The PR is not being merged; @jackwener still needs to re-review and clear the existing |
jackwener
left a comment
There was a problem hiding this comment.
Re-review of exact head e4b3549b3c411cdbf669e12efd86b706fdf2ca4d: the earlier Anthropic blockers are fixed. Sonnet 4.5 now uses legacy enabled thinking with the required 1,024-token budget, while Opus 4.5 no longer receives unsupported adaptive thinking and Opus 4.8 retains summarized adaptive thinking.
This revision is not ready to merge because three current behavior gaps remain, detailed inline. Exact-head test, package, and audit are green. I also verified the root build, 234 UI tests, 94 focused Runtime/UI tests, the active Claude request-shape sweep, and production Markdown rendering probes.
| const familyModelId = claudeFamilyId(modelId); | ||
| if (!familyModelId.startsWith('claude-')) return undefined; | ||
| const effectiveOptions = thinkingOptions ?? thinkingOptionsForModel('anthropic', familyModelId); | ||
| const supportsThinking = |
There was a problem hiding this comment.
[P2] Include the shipped OpenCode Sonnet 4 route in the thinking-support decision. Selecting the active opencode/claude-sonnet-4 model with no explicit level reaches this function without thinkingOptions, so supportsThinking is false and the SDK capability lookup below never runs. A real getAIModel() + generateText() capture sent /zen/v1/messages with no thinking; positive controls on the same connection sent legacy enabled/1,024 for Sonnet 4.5 and summarized adaptive for Opus 4.8. A sweep of all 13 active shipped reasoning-capable Claude models on the Anthropic wire found exactly this active gap. Please make one classifier own both whether a shipped Claude family supports thinking and which wire mode it uses, or add Sonnet 4 to the canonical metadata seam, and cover every active Anthropic-wire Claude model plus an unknown non-Claude negative control.
| return undefined; | ||
| } | ||
| const lineEnd = line.indexOf('\n', index + 1); | ||
| const close = findClosingDollar(line, index + 1, lineEnd < 0 ? line.length : lineEnd); |
There was a problem hiding this comment.
[P2] Avoid enabling ambiguous single-dollar math on the shared technical Markdown surface. Ordinary shell text such as Use $HOME/$USER for paths. reaches this parser, pairs the two dollar signs, turns HOME/ into KaTeX, and leaves USER as bare text; a production MarkdownBody render reproduced the corruption and removed both $ characters. The numeric currency exception does not cover shell variables, and local heuristics cannot make $...$ unambiguous in general technical prose. Please retain explicit \(...\), \[...\], and $$...$$ delimiters while removing global single-dollar parsing, or scope it behind a narrow opt-in. Add rendered controls for $HOME/$USER, $PATH:$HOME, paired currency, inline code, and explicit TeX.
| pattern: TOKEN_PATTERN, | ||
| render: (match, key) => { | ||
| const displayMode = match[1] === 'D'; | ||
| const formula = decodeFormula(match[2] ?? ''); |
There was a problem hiding this comment.
[P2] Do not let source text impersonate the internal math-token protocol. Model or imported transcript text containing the private-use token shape \uE000MAKAMATHIFFFFFFEND\uE001 matches this plugin, then decodeFormula() calls String.fromCodePoint(0xFFFFFF) and throws RangeError: Invalid code point 16777215. A production MarkdownBody render reproduces the crash; trust: false and throwOnError: false cannot help because it occurs before KaTeX. The stored content is retried on reload, so the top-level renderer fallback repeats for the affected session. Please keep a per-render token-to-formula Map in a namespace proven absent from the source, render only keys created during that invocation, and leave unknown token-shaped source literal. Add malformed/raw-sentinel negative controls alongside valid inline and multiline display math.
Summary
Reasoning-capable GPT and Claude models now request user-visible thinking by default, and the existing reasoning disclosure renders that content through the shared safe Markdown and KaTeX pipeline.
The implementation follows the resolved wire and recognized model family rather than the provider label. It preserves explicit-off behavior, encrypted reasoning replay, provider-native namespaces, code and currency literals, and display redaction.
The latest revision also:
$$...$$and\[...\]display math outside code fences;Closes #2644
Request policy
OpenAI / GPT
reasoning: { effort: "medium", summary: "auto" }for recognized GPT reasoning models.reasoning_effort: "medium"and keep the existingreasoning_contentresponse transport.effort: "none"and omits summary.Claude / Anthropic
thinking: { type: "adaptive", display: "summarized" }.thinking: { type: "disabled" }where supported.Shared display path
All provider-visible reasoning uses the existing common pipeline:
reasoning-deltaModelAdapterMakathinkingeventthinking_delta/thinking_completeAssistantMessage.thinkingThe disclosure supports safe Markdown and KaTeX for
\(...\),\[...\],$...$, and$$...$$, including multiline display forms. Inline and fenced code remain literal, ordinary currency is preserved, collapsed previews are redacted, and plain reasoning keeps intentional single-line breaks.Visual comparison
Same fixture content, viewport, state, and zoom level.
Before
After
Verification
Current exact-head local verification:
KaTeX_Main-Regularin WOFF2, WOFF, and TTF formats.git diff --check: passed.Existing end-to-end evidence retained from the prior revision:
reasoningSummary: "auto"and persisted non-empty thinking text.GitHub CI is rerunning on the rebased exact head.
AI use
Select exactly one:
Tool(s) and scope: Codex contributed implementation, tests, review remediation, and verification. Every commit in this pull request carries a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?