Skip to content

fix(core): recover from rejected Responses encrypted reasoning - #11596

Merged
DragonnZhang merged 1 commit into
mainfrom
dragon/fix-responses-encrypted-recovery
Sep 11, 2026
Merged

fix(core): recover from rejected Responses encrypted reasoning#11596
DragonnZhang merged 1 commit into
mainfrom
dragon/fix-responses-encrypted-recovery

Conversation

@DragonnZhang

@DragonnZhang DragonnZhang commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

When a Responses endpoint explicitly rejects replayed reasoning with HTTP 400 and invalid_encrypted_content, retry once with readable reasoning summaries in place of encrypted reasoning items. Preserve ordinary conversation, tool calls and their results, and the caller's original history. The first request remains unchanged; a second failure is surfaced normally. Recognize direct JSON errors and the observed single-frame Routify error envelope.

Why it's needed

A multi-turn session currently stops on this rejection even when the model already completed useful tool calls. We reproduced it against a real endpoint and verified that the rejected ciphertext matched what the endpoint had returned. The same ciphertext was accepted in a later resumed request. This change provides bounded recovery; it does not claim to fix or identify the upstream validation failure.

Reviewer Test Plan

How to verify

Use a controlled Responses endpoint that first returns encrypted reasoning and a file-reading tool call, then rejects the next request with invalid_encrypted_content. Confirm the CLI retries once, preserves the tool call/result pair, does not execute the tool twice, and completes. Verify a second rejection remains terminal, unrelated errors are not rewritten, and normal requests retain their encrypted reasoning. Repeat with direct JSON and the observed gateway envelope.

Evidence (Before & After)

Before: the controlled endpoint received one replay request and the CLI/pipeline stopped on HTTP 400; four new regression assertions failed. After: the bundled headless CLI completed with three total API requests, two model turns, one actual read_file execution, exit 0 and OK. The retry preserved tool history. All 314 focused pipeline, converter and reasoning-rejection tests passed; build, bundle, workspace typechecks and changed-file lint passed. The final integration typecheck reports TS2322 in unchanged ACP process-table code (string | NonSharedBuffer versus string).

Tested on

OS Status
🍏 macOS ✅ unit tests, build, bundled CLI and real-endpoint diagnosis
🪟 Windows ✅ downstream Astra code-mode desktop smoke
🐧 Linux ⚠️ not tested locally

Environment (optional)

Node.js 24.18.0; isolated CLI home and temporary test workspace; loopback mock endpoint with synthetic ciphertext. Real-endpoint diagnosis used Astra through Responses, recording ciphertext hashes rather than ciphertext or credentials.

Risk & Scope

  • Main risk or tradeoff: recovery loses the rejected encrypted reasoning state while preserving its readable summary. It occurs only after the explicit rejection and at most once per connection attempt.
  • Not validated / out of scope: upstream root cause, restricted-user/AP integration and full benchmark success. HTTP 200 mid-stream encrypted-content errors are not covered by this recovery.
  • Breaking changes / migration notes: none; no model-specific configuration or default stripping of reasoning.

Linked Issues

Related: #9452 (existing reasoning replay recovery).

中文说明

本 PR 的改动

当 Responses 服务明确以 HTTP 400 和 invalid_encrypted_content 拒绝历史 reasoning 时,用可读摘要替代加密 reasoning 项并重试一次。普通对话、工具调用及其结果、调用方原始历史均保留。首次请求不变;第二次失败正常抛出。支持直接 JSON 错误和实际观察到的 Routify 单帧错误包装。

为什么需要

当前多轮会话会因这类拒绝直接停止,即使模型已经完成了有效的工具调用。我们在真实接口复现过,确认被拒绝的密文与接口先前返回的内容一致;同一密文后来在恢复会话的请求中又被接受。此改动提供有限恢复能力,不代表已修复或定位上游校验故障。

审查者测试计划

使用可控的 Responses 接口:先返回加密 reasoning 和读文件工具调用,再以 invalid_encrypted_content 拒绝下一轮请求。确认 CLI 只重试一次,保留工具调用与结果,不重复执行工具,并正常完成。确认第二次拒绝仍然终止,其他错误不触发改写,正常请求仍保留加密 reasoning。分别验证直接 JSON 和实际网关包装格式。

修改前后证据

修改前:可控接口只收到一次历史回传请求,CLI/请求管线在 HTTP 400 后停止;四个新增回归断言失败。修改后:打包后的无界面 CLI 通过三次 API 请求、两轮模型调用、一次实际 read_file 执行完成任务,退出码为 0,返回 OK;重试保留工具历史。管线、转换器和 reasoning 拒绝处理的 314 个针对性测试全部通过;构建、打包、各工作区类型检查和修改文件 lint 通过。最后的集成类型检查在未改动的 ACP 进程表代码报告 TS2322(string | NonSharedBufferstring 类型不匹配)。

已测试平台

  • macOS:已通过单元测试、构建、打包 CLI 验证和真实接口诊断。
  • Windows:下游 Astra code mode 桌面 smoke 已通过。
  • Linux:未在本地测试。

环境

Node.js 24.18.0;隔离的 CLI 配置目录和临时测试工作区;使用模拟密文的本地可控接口。真实接口诊断通过 Responses 调用 Astra,仅记录密文哈希,不记录密文或凭证。

风险与范围

  • 主要取舍:恢复会丢失被拒绝的加密 reasoning 状态,保留可读摘要。仅在明确拒绝后发生,每次连接尝试最多恢复一次。
  • 未验证或不在范围内:上游根因、受限用户/AP 集成和完整 benchmark 成功。该恢复不覆盖 HTTP 200 流中途发生的加密内容错误。
  • 无破坏性变更或迁移要求;没有按模型写特殊配置,也不默认删除 reasoning。

关联问题

关联 #9452:已有的 reasoning 回传恢复逻辑。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on b0475ec and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— b0475ec 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@DragonnZhang

Copy link
Copy Markdown
Collaborator Author

E2E verification: VERIFIED_FIXED against a controlled Responses endpoint using the bundled headless CLI.

The endpoint emitted encrypted reasoning and a read_file call, then rejected the replay with HTTP 400 invalid_encrypted_content. The CLI recovered and returned OK with exit 0. There were three API requests, two model turns and one actual tool execution; the recovery preserved the tool call/result pair and did not execute the tool again. Direct JSON and the observed gateway envelope both passed transport-level checks. A second rejection remains terminal, and normal requests retain encrypted reasoning.

The installed older global CLI did not support Responses auth, so baseline reproduction used the real pipeline against a loopback HTTP server; final E2E verification used the newly built node dist/cli.js. Fixtures contained synthetic ciphertext and a dummy credential. Windows code-mode integration is being verified separately downstream.

This verifies the recovery behavior, not the cause of the upstream encryption validation failure. The live diagnosis found exact ciphertext round trips, including a failed request whose ciphertext was accepted later after resuming the session.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, Tested-on table filled in honestly (Windows pending, Linux not tested).

Problem: observed, not theoretical — you describe reproducing against a real endpoint and confirming the rejected ciphertext matched what that endpoint had issued, with the same ciphertext accepted later on a resumed request. That is the right kind of evidence, and it puts this in the same family as #9452, whose recovery already landed via #8169. The gap I'd like closed: there is no linked issue and the observed body is not attached anywhere. The whole fix hinges on two exact shapes — direct {"error":{"code":"invalid_encrypted_content"}} and one data:-framed routify_response envelope — and only you have seen them. If production drifts by a field, the classifier fails closed and the recovery is silently inert. Could you paste a redacted copy of the real body (or file an issue to hold it)?

Direction: aligned. This extends a recovery mechanism the project already decided it wants, rather than introducing a new posture — same "send unchanged first, recover at most once, leave persisted history alone" contract #9452 established. Routify is already a gateway this repo knows about (outbound-session-id.ts lists its hosts), so recognizing its envelope isn't scope creep. claude-code's CHANGELOG has no reference to encrypted-reasoning replay recovery, which is expected — this is wire-level recovery inside our own Responses adapter, not a user-facing feature they'd log. One sequencing question for the maintainer: #11567 is also working the #9452 family from the prevention angle. Different files, so no conflict, but worth deciding which lands first.

Size: core paths (packages/core/src/**), so here's the breakdown — 60 production lines (responses-pipeline.ts 27, responses-reasoning-rejection.ts 33), 165 test lines (115 + 50), 0 generated/schema. Well under the 500-line escalation threshold and under the 1000-line advisory. No maintainer escalation on size.

Approach: the scope feels right, and notably it doesn't add a parallel JSON parser — it reuses the existing envelope scanner (toEnvelopeText / readEnvelope / readErrorMember) and the existing downgradeRejectedReasoningItems rewrite. Cutting 80% of this wouldn't work: the classifier, the flag on the error, and the retry branch are each load-bearing. Nothing unrelated in the diff — no drive-by refactors, no formatting churn. The only thing I'd question is whether the Routify gateway branch needs to ship in the same PR as the direct-JSON case, or whether you've actually observed both.

Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths (openaiContentGenerator, streamingToolCallParser, geminiChat, acpConnection, shell*, mcp-*, LspServer, relaunch.ts, sandbox.ts). The module is entirely private: responses-reasoning-rejection.ts has exactly one importer (responses-pipeline.ts) and is not re-exported from the package index, so there is no public API surface change to track.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 必填章节齐全,中英双语,Tested-on 表格如实填写(Windows 待验证,Linux 未测试)。

问题: 是已观测到的 bug,不是理论性加固。你说明了在真实接口复现过,并确认被拒绝的密文与该接口先前下发的内容一致,同一密文后来在恢复会话的请求中又被接受。这正是我们需要的证据类型,也和 #9452 属于同一类问题——该恢复机制已由 #8169 落地。需要补上的缺口是:没有关联 issue,也没有附上实际观测到的错误体。整个修复依赖两种精确格式——直接的 {"error":{"code":"invalid_encrypted_content"}},以及单帧 data: 包装的 routify_response 结构——而目前只有你见过它们。如果生产环境的格式有任何字段差异,分类器会 fail closed,恢复逻辑就会静默失效。能否贴一份脱敏后的真实错误体(或开一个 issue 存档)?

方向: 对齐。这是对项目已经决定要有的恢复机制的扩展,而不是引入新姿态——沿用 #9452 确立的"首次原样发送、最多恢复一次、不改动持久化历史"契约。Routify 本就是本仓库已知的网关(outbound-session-id.ts 列有其域名),因此识别它的包装格式不算范围外扩。claude-code 的 CHANGELOG 没有关于 encrypted reasoning 回传恢复的记录,这在预期之内——这是我们自己的 Responses 适配层的协议级恢复,不是他们会记录的用户可见特性。有一个需要维护者决定的顺序问题:#11567 也在处理 #9452 这一族问题,走的是预防路线。文件不重叠,因此没有冲突,但值得确定哪个先合入。

规模: 触及核心路径(packages/core/src/**),明细如下——生产代码 60 行(responses-pipeline.ts 27 行,responses-reasoning-rejection.ts 33 行),测试 165 行(115 + 50),生成/schema 0 行。远低于 500 行的升级阈值,也低于 1000 行的大 PR 提示线。规模上无需升级给维护者。

方案: 范围合理,而且值得肯定的是它没有另起一套 JSON 解析器——复用了已有的信封扫描逻辑(toEnvelopeText / readEnvelope / readErrorMember)和已有的 downgradeRejectedReasoningItems 改写函数。砍掉 80% 行不通:分类器、错误上的标志位、重试分支三者都是必需的。diff 中没有无关改动——没有顺手重构,没有格式噪音。我唯一想问的是:Routify 网关分支是否必须和直接 JSON 分支在同一个 PR 里,还是说两种格式你都实际观测过?

风险: 无升级风险信号——改动文件均未命中与 revert 相关的高风险路径(openaiContentGeneratorstreamingToolCallParsergeminiChatacpConnectionshell*mcp-*LspServerrelaunch.tssandbox.ts)。该模块完全私有:responses-reasoning-rejection.ts 只有一个引用方(responses-pipeline.ts),也未从包入口 re-export,因此没有需要跟踪的公共 API 变更。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Code review

I read the diff against the surrounding module rather than in isolation, and traced every consumer. Short version: no blocking issues found. This is careful work that fits the mechanism it extends.

What I verified rather than assumed:

  • The retry cannot loop or double-execute. connectWithReasoningReplayRecovery runs the second connect outside the catch, so it is attempted exactly once and its own failure propagates unchanged. Your fetchMock call-count assertions (2 on recovery, 2 on second rejection, 1 when there is no reasoning to downgrade) pin this, and they're the right assertions.
  • Both entry points are covered. executeStream delegates to connectStream, which is the only caller of the recovery wrapper — so there is no second error path that sets reasoningIdRejection but forgets encryptedReasoningRejected. The single non-2xx site in connect is the only place either flag is written.
  • The classifier cannot throw. Unlike parseReasoningIdRejection, it has no try/catch, but it doesn't need one: readErrorResponseBody is typed Promise<string>, so the exotic-value defence that motivates the catch there doesn't apply, and the helpers it calls (toEnvelopeText, readEnvelope, normalizeEnvelope) are total.
  • The body cap lines up. MAX_ERROR_BODY_CHARS is CLASSIFIABLE_ERROR_BODY_CHARS + 1 = 64001, and your > MAX_BODY_CHARS (64000) guard rejects exactly the truncated case, so a capped body fails closed instead of being classified from a prefix. Same contract as the existing path.
  • Precedence is right. rejection wins over encryptedRejected, which is correct — the id rejection is the more specific diagnosis. namedIndex=all in the debug line is honest about the encrypted branch not having a named index.
  • The negative cases do real work. quoted request (a matching body under debug, not error), unknown code (the code string appearing only in message), contradictory gateway (success: true), duplicate error key, and multiple frames each pin a distinct way a looser classifier would be fooled. This is the part of the diff I'd have been most tempted to under-test, and it isn't under-tested.
  • No API surface change. responses-reasoning-rejection.ts has exactly one importer and is not re-exported from the package index; ResponsesApiError is module-local. Nothing downstream to chase.

Non-blocking, in descending order of how much I'd care:

  1. downgradeEncryptedReasoningItems fabricates a ReasoningIdRejection. It passes { namedIndex: items.findIndex(isReasoningItem), maxLength: null }, where namedIndex carries none of its documented meaning ("a 400 in which the endpoint named one replayed input[N].id") and is used purely as an is-there-any-reasoning-item existence check — the actual "downgrade everything" behaviour comes from maxLength: null making exceedsMax true for all. It is correct today and I walked the loop to confirm. The concern is that the coupling is invisible: someone later tightening downgradeRejectedReasoningItems to honour namedIndex literally would break the encrypted path, and the type's own doc comment would tell them they were right. Extracting the rewrite loop into a private rewriteReasoningItems(items, predicate) that both public functions call would remove the overload for about the same line count.
  2. Recovery is per-connect and history stays untouched (which is the correct bug(core): switching Responses models or endpoints can make a saved session unusable #9452 contract, and your expect(request).toEqual(original) assertion enforces it). The consequence worth naming: against an endpoint that rejects persistently, every subsequent turn re-sends the same ciphertext, pays a doubled round-trip, and loses reasoning continuity — with debugLogger.debug as the only signal. It degrades quietly rather than loudly. Not asking you to change it, but a maintainer reading this should know the failure mode is a slow session, not an error.
  3. readEnvelope(text, MAX_OBJECT_CANDIDATES, false) disables the raw-control-character repair the id path enables. For the direct shape, a proxy splicing a raw newline into error.message therefore fails closed and the recovery won't fire. Stricter is safe and I'd rather have it this way round — just confirm it's deliberate and not incidental.
  4. The new import entries aren't alphabetized with the existing ones. There's no sort-imports rule in eslint.config.js, so this won't fail lint; mentioning only because the surrounding lists were sorted.
sequenceDiagram
    participant P1 as Caller
    participant P2 as ResponsesPipeline
    participant P3 as Rejection Classifier
    participant P4 as Responses Endpoint
    P1->>P2: executeStream (request)
    P2->>P4: connect 1, input unchanged with encrypted reasoning
    P4-->>P2: HTTP 400 with an error body
    P2->>P3: parseReasoningIdRejection (400, body)
    P2->>P3: isEncryptedReasoningRejection (400, body)
    P3-->>P2: id rejection, encrypted rejection, or neither
    alt a reasoning id was named (takes precedence)
        P2->>P2: downgrade only items over the reported maximum
        P2->>P4: connect 2, outside the catch so at most once
    else encrypted content rejected (new in this PR)
        P2->>P2: downgrade every reasoning item to its summary text
        P2->>P4: connect 2, outside the catch so at most once
    else neither classified, or nothing in the body would change
        P2-->>P1: the original 400 surfaces unchanged
    end
    P4-->>P2: 200 SSE, or a second 400 that propagates unmodified
    P2-->>P1: streamed chunks, or the second error
Loading

Test evidence

This is an unattended CI run, so per the gate rules I did not build, run, or execute anything from this PR — no npm, no vitest, no checkout. The evidence below is the PR's own CI, read through the API for the reviewed commit. Nothing here is my re-run of your tests, and nothing below the line is your self-reported result presented as evidence.

Zero failures across all checks at the time of writing. Integration Tests (no-AK, No Sandbox) completed success, which is worth calling out directly: you reported a TS2322 in unchanged ACP process-table code from your local integration typecheck. CI does not reproduce it on this commit, so I'm treating that as environmental to your machine (the usual suspect is a shared or symlinked node_modules surfacing cross-package type drift) and not as a defect in this PR. Lint & Static and Test (ubuntu-latest, Node 22.x) were still in progress when I fetched — I don't poll, so they're listed as pending rather than guessed at. The finalize job updates the table below in place once CI settles.

Not verified, and why it matters: that the production error body actually matches either accepted shape. Your unit tests pin the classifier against bodies you wrote from your observation, which proves the classifier is self-consistent — it cannot prove the real Routify/OpenAI body has that exact field layout. That is the one load-bearing unknown in this PR, and no amount of CI green closes it. This is the same ask as in the gate comment: a redacted copy of the real body, or an issue holding it.

Also not verified: your claimed headless end-to-end run (three API requests, two model turns, one read_file execution, exit 0) and the 314 focused tests. Those are your results on macOS, attributed to you, not evidence I reproduced.

Final CI results for b0475ec (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle part of this: @qwen-code /verify — the retry path's load-bearingness is not observable from the diff, and a suite that passes identically with isEncryptedReasoningRejection stubbed to false would look exactly as green as this one. An A/B against the base build would prove the base surfaces the 400 while this branch completes, and that the read_file tool executes once rather than twice. You have write access, so this is a normal run rather than a sponsored one. What /verify cannot settle is the production body shape — that needs the real endpoint, so it stays an author-supplied artifact either way. /tmux adds nothing here: this is a wire-level recovery with no TUI surface.

Real-scenario testing (2c): N/A — unattended CI run, never attempted by design.

中文说明

代码审查

我是结合所在模块整体读这份 diff 的,并追踪了每一个引用方。结论:未发现阻塞性问题。 这是一份细致的改动,和它所扩展的既有机制契合。

我实际验证过(而非假设)的点:

  • 重试不会循环,也不会重复执行工具。 connectWithReasoningReplayRecovery 把第二次 connect 放在 catch 之外,因此只会尝试一次,其自身失败原样向上抛出。你的 fetchMock 调用次数断言(恢复时 2 次、二次拒绝时 2 次、无可降级 reasoning 时 1 次)正好钉住了这一点,断言选得对。
  • 两个入口都覆盖到了。 executeStream 委托给 connectStream,而后者是恢复包装的唯一调用方——所以不存在第二条只设置 reasoningIdRejection 却漏掉 encryptedReasoningRejected 的错误路径。connect 中唯一的非 2xx 分支是这两个标志位的唯一写入点。
  • 分类器不会抛异常。 它没有像 parseReasoningIdRejection 那样包 try/catch,但也不需要:readErrorResponseBody 的类型是 Promise<string>,因此那里用来防御奇异值的理由在此不成立,而它调用的辅助函数(toEnvelopeTextreadEnvelopenormalizeEnvelope)都是全函数。
  • body 长度上限对得上。 MAX_ERROR_BODY_CHARSCLASSIFIABLE_ERROR_BODY_CHARS + 1 = 64001,而你的 > MAX_BODY_CHARS(64000)判断恰好拒绝被截断的情况,因此达到上限的 body 会 fail closed,而不会基于前缀做分类。与既有路径的契约一致。
  • 优先级正确。 rejection 优先于 encryptedRejected,这是对的——id 拒绝是更具体的诊断。debug 日志里的 namedIndex=all 也如实反映了加密分支没有具名索引。
  • 反例测试是有效的。 quoted request(匹配内容出现在 debug 而非 error 下)、unknown code(code 字符串只出现在 message 里)、contradictory gatewaysuccess: true)、duplicate error keymultiple frames 各自钉住了一种会骗过宽松分类器的情况。这部分是我最担心测试不足的,实际并不不足。
  • 没有 API 面变更。 responses-reasoning-rejection.ts 只有一个引用方,也未从包入口 re-export;ResponsesApiError 是模块内私有。没有需要追查的下游。

非阻塞意见,按我的在意程度排序:

  1. downgradeEncryptedReasoningItems 伪造了一个 ReasoningIdRejection 它传入 { namedIndex: items.findIndex(isReasoningItem), maxLength: null },其中 namedIndex 完全不承载其文档含义("接口点名了某个回传的 input[N].id"),只被当作是否存在 reasoning 项的存在性检查——真正"全部降级"的行为来自 maxLength: nullexceedsMax 对所有项返回 true。今天是正确的,我逐步走过循环确认过。我担心的是这种耦合不可见:以后若有人把 downgradeRejectedReasoningItems 收紧为严格按 namedIndex 处理,就会破坏加密路径,而该类型的文档注释还会告诉他这么改是对的。把改写循环抽成一个私有的 rewriteReasoningItems(items, predicate)、让两个公开函数都调用它,行数差不多就能消除这个语义重载。
  2. 恢复是按 connect 粒度的,且历史保持不动(这是 bug(core): switching Responses models or endpoints can make a saved session unusable #9452 正确的契约,你的 expect(request).toEqual(original) 断言也强制了它)。值得点明的后果是:面对一个持续拒绝的接口,每一轮都会重发同样的密文、付出双倍往返、并丢失 reasoning 连续性——而唯一的信号是 debugLogger.debug。它是安静地退化,而不是显式报错。不是要求你改,但读到这里的人应该知道失败模式是"会话变慢"而非"报错"。
  3. readEnvelope(text, MAX_OBJECT_CANDIDATES, false) 关闭了 id 路径启用的原始控制字符修复。因此对直接 JSON 格式,若代理把原始换行拼进 error.message,就会 fail closed、恢复不触发。更严格是安全的,我也更希望是这样——只想确认这是有意为之,而非顺手写成。
  4. 新增的 import 条目没有和既有的按字母序排列。eslint.config.js 里没有 sort-imports 规则,所以不会导致 lint 失败;提一句只是因为周围的列表原本是有序的。

(时序图见上,中英文共用一份。)

测试证据

这是无人值守的 CI 运行,因此按门禁规则,我没有构建、运行或执行本 PR 的任何代码——没有 npm、没有 vitest、没有 checkout。下面的证据是本 PR 自己的 CI,通过 API 针对被审查的 commit 读取。这里没有我对你的测试的重跑,也没有把你的自述结果当作证据呈现。

截至撰写时,所有检查零失败。Integration Tests (no-AK, No Sandbox)success 完成,这一点值得直接点出:你报告本地集成类型检查在未改动的 ACP 进程表代码里出现 TS2322。CI 在此 commit 上没有复现,因此我把它判定为你本机环境问题(常见原因是共享或软链的 node_modules 导致跨包类型漂移),算本 PR 的缺陷。Lint & StaticTest (ubuntu-latest, Node 22.x) 在我拉取时仍在进行中——我不轮询,所以如实标为 pending,不猜测结果。CI 结束后 finalize 任务会就地更新下表。

未验证、且重要的部分:生产环境的错误体是否真的匹配两种被接受的格式之一。 你的单元测试是拿你依据观测写出的 body 来钉住分类器的,这证明了分类器自洽——但无法证明真实的 Routify/OpenAI body 就是那个字段布局。这是本 PR 唯一承重的未知项,CI 全绿也关不上它。这和门禁评论里的诉求是同一件事:一份脱敏的真实 body,或一个存档它的 issue。

同样未验证:你所述的无界面端到端运行(三次 API 请求、两轮模型调用、一次 read_file 执行、退出码 0)和 314 个针对性测试。这些是你在 macOS 上的结果,归属于你,不是我复现的证据。

CI 表格见上方标记区域(中英文共用一份)。

沙箱验证可以解决其中一部分:@qwen-code /verify —— 重试路径的承重性从 diff 上看不出来,而一个把 isEncryptedReasoningRejection 打桩为 false 后依然全绿的测试套件,看上去会和现在一样绿。对基线构建做 A/B 才能证明:基线会把 400 抛出去,而本分支能完成;并且 read_file 只执行一次而非两次。你有写权限,因此这是常规运行而非担保运行。/verify 无法解决的是生产 body 格式问题——那需要真实接口,无论如何都得由作者提供。/tmux 在此无增益:这是协议层恢复,没有 TUI 界面。

真实场景测试(2c):N/A —— 无人值守 CI 运行,按设计不尝试。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — correct, fail-closed and unusually well-tested for its size; the single thing holding it back from 5 is that the classifier's match to the real production error body rests on your word rather than an artifact anyone else can see.

Before reading the diff in detail I wrote down what I'd do from the title and the "why" alone: a classifier beside the existing parseReasoningIdRejection, a boolean on the pipeline's error type set at the one non-2xx site, a branch in the retry builder that downgrades every reasoning item (since nothing names one), and tests for fires-exactly-once, second-failure-surfaces, non-matching-bodies-don't-fire, tool-pairs-preserved, and caller-request-unmutated. That is what you built, down to reusing downgradeRejectedReasoningItems with maxLength: null rather than writing a second rewrite loop. So the approach matches my independent baseline and I didn't find a simpler path you missed. The one place I'd have gone differently is the fabricated namedIndex — I'd have extracted the loop into a shared private helper, for the maintainability reason in the review comment, not because today's behaviour is wrong.

What actually persuades me this should merge is the failure-mode asymmetry. If the classifier never matches production, you've added 60 lines of inert, fully unit-tested, fail-closed code and behaviour is byte-identical to today. If it does match, you've un-wedged sessions — and a wedged session is close to the worst failure mode this adapter has, because the offending ciphertext lives in persisted history, so every subsequent send rebuilds it and fails identically and retrying the turn doesn't help. Those two outcomes aren't symmetric, and the cheap one is safe.

I also want to be explicit about the checks I ran on myself here. You have nine open PRs, several of them core-scoped, so I deliberately asked whether I was evaluating this one on merit or being worn down by volume — it's focused, carries no unrelated edits, and adds no new abstraction, so the answer is merit. And I asked whether I was approving because I ran out of reasons to say no. The strongest case against is that this masks an upstream validation bug rather than fixing it, which you state plainly yourself. I don't think that's disqualifying: the recovery fires only after an explicit rejection, at most once per connect, leaves persisted history untouched, and writes a debug line, so it degrades a session's reasoning continuity without hiding anything from someone who goes looking. Bounded recovery in front of an unfixed upstream bug is the same posture #9452 already established and shipped.

Two things for whoever merges this, neither a blocker:

CI was still running when I wrote this (Lint & Static and Test (ubuntu-latest, Node 22.x) in progress, zero failures so far, integration and both Desktop Shell jobs already green), so I'm not approving in this run — approving against a result that doesn't exist yet would be attesting to nothing. Approval is deferred until CI lands green on b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f; the finalize job posts the commit-pinned approval once every check on that SHA completes, and withholds it if anything lands red or the head moves.

中文说明

Confidence: 4/5 —— 就其体量而言,正确、fail closed、测试覆盖出奇地充分;唯一让它到不了 5 分的,是分类器与真实生产错误体的匹配目前只基于你的陈述,而没有别人可以查看的凭据。

在细读 diff 之前,我先根据标题和"为什么需要"写下了我会怎么做:在既有的 parseReasoningIdRejection 旁边加一个分类器,在唯一的非 2xx 分支给管线的错误类型加一个布尔标志,在重试构造里加一个降级全部 reasoning 项的分支(因为没有任何一项被点名),以及针对"只触发一次""二次失败照常抛出""不匹配的 body 不触发""工具调用配对保留""调用方请求不被改写"的测试。你做的正是这些,连"复用 downgradeRejectedReasoningItems 并传 maxLength: null,而不是另写一套改写循环"这一点也一样。所以方案与我的独立基线一致,我也没找到你漏掉的更简路径。唯一我会做得不同的地方是那个伪造的 namedIndex——我会把循环抽成一个共享的私有辅助函数,理由是审查评论里说的可维护性,而不是因为今天的行为有错。

真正说服我这该合入的是失败模式的不对称性。如果分类器在生产环境永远匹配不上,你只是增加了 60 行惰性的、单元测试完整的、fail closed 的代码,行为与今天逐字节相同。如果它匹配上了,你就解开了被卡死的会话——而会话卡死几乎是这个适配层最糟的失败模式:出问题的密文存在持久化历史里,之后每次发送都会重建它并同样失败,重试这一轮也无济于事。这两种结果并不对称,而代价小的那一种是安全的。

我也想明确说明我对自己做的几项检查。你有九个开启中的 PR,其中数个涉及 core,所以我特意问了:我是在就这份 PR 本身的价值做判断,还是被数量磨平了标准?它聚焦、不夹带无关改动、不引入新抽象,所以答案是价值本身。我也问了:我是不是因为找不到拒绝的理由才批准?最强的反方意见是——它掩盖了上游的校验 bug 而没有修复它,这一点你自己也直说了。我不认为这足以否决:恢复只在明确拒绝之后触发,每次 connect 最多一次,不改动持久化历史,并写一条 debug 日志,因此它降低的是会话的 reasoning 连续性,而没有对愿意去查的人隐藏任何东西。在上游 bug 未修的前提下做有界恢复,与 #9452 已经确立并落地的姿态是同一个。

给合入者的两点,都不是阻塞项:

我写这段时 CI 仍在运行(Lint & StaticTest (ubuntu-latest, Node 22.x) 进行中,目前零失败,集成测试和两个 Desktop Shell 任务已绿),所以本次运行我不做批准——对一个尚不存在的结果做批准等于什么都没担保。批准将推迟到 CI 在 b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f 上全绿之后;finalize 任务会在该 SHA 的所有检查完成后发出绑定 commit 的批准,若有检查变红或 head 移动则不予批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f · re-run with @qwen-code /triage

@DragonnZhang

Copy link
Copy Markdown
Collaborator Author

Additional downstream Windows validation (2026-09-11):

The patch was cherry-picked onto the code-mode integration branch at 23253651ada512a7ee4668f926091297aee9774e, built as Qwen Code 0.23.3, and tested through the actual packaged CLI on an interactive Windows desktop with node-repl MCP 0.1.2 and CUA SDK 0.20.3.

  • Astra Responses: 210.25 seconds, 9 turns, exit 0. UIA input change/restoration, four inline images in persisted chat, and generated prototype startup were verified.
  • Qwen3.8 Chat Completions: 338.19 seconds, 8 turns, exit 0, no API errors; exec/MCP/UIA and PNG capture worked. The complete autonomous task did not pass: the prototype had a Python syntax error, restoration used the wrong input value (independently corrected), and image forwarding into model context could not be verified.
  • The integrated source passed 406 Responses/code-mode tests, build/bundle, and the packaged Responses exec/tool-result fixture. The downstream runtime passed 26 tests. Workspace typechecks passed; the existing final integration ACP string/Buffer error remains documented above.

The desktop smoke does not validate AP/rbagent isolation or demonstrate recovery from a live encrypted-content rejection; the forced rejection/retry path is covered by the deterministic tests and CLI fixture already reported. Temporary credentials, owned tasks/processes and hosts changes were cleaned up.

中文补充:修复已合入 code-mode 集成分支并使用实际打包 CLI 在 Windows 复测。Astra 的 UIA 编辑/恢复、截图进入上下文及原型启动均通过。Qwen3.8 的 Chat/API/工具调用链路通过,但原型语法、输入恢复及图片转发验证存在上述缺陷,不计为完整自主任务通过。此次为交互式管理员桌面 smoke,未验证 AP/rbagent 隔离,也未在真实服务上触发密文拒绝恢复;该恢复路径由确定性测试覆盖。临时凭据、任务、自有进程及 hosts 修改均已清理。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /review

@github-actions

Copy link
Copy Markdown
Contributor

Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Independent verification report — measured at head b0475ecbfc5c

结论 / Conclusion: mergeable at the head named above. Every number below was computed from a fresh read taken immediately before this comment was posted. Live-state fields at the read: head b0475ecbfc5c0dde9c395d9e5bbd9ae2479e5e0f, state=open merged=false, mergeable=true mergeable_state=blocked, and the rollup reads review_decision=null over REST while GraphQL reports reviewDecision=REVIEW_REQUIRED for the same head — both quoted because neither is a per-author verdict, which is what §5 turns on.

This is an independent verification pass, not a restatement of the bot's review. It ran because the gate conditions held at the time of the read: qwen-code-ci-bot APPROVED at head (review 5171161641, 2026-09-10T19:03:18Z, 63-char body carrying <!-- qwen-triage approve-on-green sha=b0475ecbfc5c -->, which equals the live head), a fully enumerated check-run census (102/102, total_count=102, complete) in which the 10 product lanes are 7 success / 3 skipped / 0 failure / 0 unfinished / 0 action_required / 0 unclassified, and this pass found no Critical.


1. Instrument — and why it is not a tmux E2E

The standing convention for this report is a tmux-driven TUI run. Three independent facts make that instrument the wrong one here, and naming the substitute is the honest move rather than quietly skipping the section:

  • The repository's own agent-quality lanes are all skipped at this headtmux-testing, publish-tmux, verify, publish-verify (4/4 skipped). CI itself ran no tmux scenario for this diff.
  • The changed path cannot be reached from a real interactive session. It executes only when an OpenAI-compatible Responses endpoint answers a request whose input already carries replayed reasoning items with HTTP 400 and error.code == "invalid_encrypted_content" (or the routify_response gateway envelope wrapping that error). The production endpoint does not return that, so a tmux session against it exercises zero changed lines.
  • The author states the same limit about their own downstream run: "The desktop smoke does not … demonstrate recovery from a live encrypted-content rejection; the forced rejection/retry path is covered by the deterministic tests and CLI fixture already reported."

So the decisive locally-executable question is whether those deterministic tests actually discriminate — and nobody had run them. ci-bot's stage-2 (5623690188) states verbatim: "This is an unattended CI run, so per the gate rules I did not build, run, or execute anything from this PR — no npm, no vitest, no checkout … Nothing here is my re-run of your tests." Stage-3 (5623713728) reasoned about the code and reported Confidence 4/5 but likewise executed nothing. I therefore ran the counterfactual.

NOT MEASURED, stated plainly rather than implied by omission: no live-CLI (tmux) arm against a stub Responses endpoint was run. Everything below is a deterministic-suite counterfactual on the real source blobs, not an end-to-end observation of a recovering session.

2. Executed two-arm counterfactual

Harness. A git worktree checked out at the PR head; packages/core built once inside that worktree; node_modules symlinked from the host checkout. The symlink cannot silently test the wrong tree, because both suites import only vitest, @google/genai, node:util and relative paths — never an @qwen-code/* specifier — so nothing resolves through a workspace package.

Blob identity, each verified three ways (worktree file via git hash-object == pulls/11596/files[].sha == the index line of the diff):

file head base 07b1cd033e28
responses-pipeline.ts (production) 52aaafd91864 28338b059c5
responses-reasoning-rejection.ts (production) 5a6da8eca861 b764c015299
responses-pipeline.test.ts 89b6f1d7c9b6 unchanged by this PR
responses-reasoning-rejection.test.ts e12e6e4a13f9 unchanged by this PR

Arm A — head production + head tests: 221 passed / 221, 2 files, exit 0.

Arm B — both production files reverted to base, both test files asserted byte-identical to arm A by blob sha: 203 passed / 18 FAILED, exit 1.

The 18 failures split into two classes, and the split matters because an export-level failure proves far less than a behavioural one:

  • 14 export-level, all inside isEncryptedReasoningRejection: (0 , isEncryptedReasoningRejection) is not a function.

  • 4 behavioural, all inside ResponsesPipeline > reasoning id rejection recovery:

    • recovers once from rejected encrypted replay on the direct {"error":{…}} body → expected Error: Responses API error 400: {"error":… to be undefined
    • the same test on the gateway SSE frame data: {"routify_response":{"success":false,"status":400,…}} → same assertion
    • preserves tool call/result pairs when recovering encrypted replay → same assertion
    • surfaces the second encrypted rejection without loopingexpected "spy" to be called 2 times, but got 1 times

    Read together: at base the 400 propagates to the caller as a thrown API error and no retry is issued at all. That is precisely the user-visible failure this PR removes, and the fourth case shows the retry bound is a real assertion rather than an artefact.

  • The 19th new test passes in both arms by construction and is a negative control, not a coverage gap. does not retry encrypted rejection without reasoning in the request asserts status: 400 plus exactly one fetch, which holds before and after the change — it pins that recovery does not fire when there is nothing to downgrade. (19 new tests = 14 recognizer + 5 pipeline; 18 failed, 1 passed. The denominator is 221 in both arms, which is itself the byte-identity witness.)

Two anti-vacuity witnesses:

  1. Reversibility. Restoring the head production files — shas re-verified as 52aaafd91864 / 5a6da8eca861, with the two test-file shas unchanged — and re-running returned 221/221 again.
  2. The compiled output was identical across all three runs. packages/core/dist was built once, before arm A, and never rebuilt; it still contains isEncryptedReasoningRejection. Arm B nevertheless failed 18 tests. So the suites execute the worktree's TypeScript source through vitest's transform, and the counterfactual cannot have been reading dist or the host checkout.

3. What I verified in the wiring, by reading head source

  • buildReasoningReplayRetry precedence is rejection ? downgradeRejectedReasoningItems(…) : encryptedRejected ? downgradeEncryptedReasoningItems(…) : apiRequest.input. A reasoning-id rejection therefore still wins over an encrypted-content rejection, and when neither flag is set the ternary yields apiRequest.input unchanged so the input === apiRequest.input guard declines the retry.
  • downgradeEncryptedReasoningItems delegates with namedIndex: items.findIndex(isReasoningItem) and maxLength: null, i.e. it downgrades every reasoning item rather than a named one — which matches the debug line's namedIndex=${rejection?.namedIndex ?? 'all'}.
  • The new flag is set at exactly one site, err.encryptedReasoningRejected = isEncryptedReasoningRejection(response.status, errBody) immediately above throw redactProxyError(err) — adjacent to the pre-existing err.reasoningIdRejection = rejection. The file has 6 throw redactProxyError sites in total (:670, :678, :714, :972, :1022, :1072), so the other five cannot raise this recovery. That is the same scope the pre-existing id-rejection flag has always had, and :714 is the non-2xx response handler where a 400 lands, so the scope is correct for this failure class rather than a regression.
  • The recovery connect is outside the catch, under a source comment that says it "is attempted exactly once and its own failure propagates unchanged rather than triggering another recovery". Arm B's expected "spy" to be called 2 times, but got 1 times and arm A's passing surfaces the second encrypted rejection without looping bound that from both directions.

4. Coverage and method

pulls/11596/files was asserted non-truncated before use as a cover instrument: len(files)=4 == changed_files=4, Σ+=216 == additions, Σ−=9 == deletions, every entry carrying a patch. Production cover is 2/2 files and test cover 2/2; no production file was sampled rather than read.

5. Disclosures — recorded, not blocking

  • 🔴 The re-review a maintainer asked for never produced a verdict. wenshao requested @qwen-code /review at 2026-09-10T19:18:34Z — 15 minutes after the approve-on-green pin. Run 34519636070 ("🧐 Qwen Pull Request Review", event=issue_comment) concluded failure at 20:35:05Z, and the automatic pull_request_target run 34514831033 concluded failure at 20:34:42Z. The bot then posted two qwen-review-fallback comments: "The review pipeline failed before a review could be posted… retry with @qwen-code /review." A command-triggered review is not listed under this PR's checks — the bot's own acknowledgement says so — which is why the 102/102 census above cannot see it. The re-examination a maintainer requested is therefore still outstanding at this head.
  • The approving row is an automation, not a code-review verdict. Its 63-char body is the sha-pinned approve-on-green finalize marker. The code review that does exist is ci-bot's stage-2/stage-3 comment pair at 18:41–18:42Z, which reported no blocking issues — both posted before the two failed re-runs.
  • "Green and complete" here means 7 success + 3 skipped, not 10 successes. The skipped product lanes are Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox). The one non-skipped failing lane in the whole census is review-pr (failure), which is bot automation, not a product lane.
  • mergeable_state=blocked and review_decision=null at the read, so nothing merges on this state alone.

6. What would change this conclusion

A head move past b0475ecbfc5c voids the counterfactual above (arm B would need re-running against the new base). A newer at-head CHANGES_REQUESTED, or a successful re-run of the maintainer-requested review that files Criticals, closes the approval leg while leaving the arm A/arm B measurements standing — those are claims about the tree at this head.

Approval posture, time-scoped to the read described above: as of the state read immediately before posting, no approval is submitted with this comment. The reason is the first disclosure: the review wenshao explicitly requested postdates the only approving row and failed without reaching a verdict, and the bot invites a retry, so the approval leg is open but unsettled at this head. This comment carries no approval.


独立验证报告(非机器人评审的复述)。该 diff 的改动面只在 OpenAI 兼容 Responses 端点以 HTTP 400 + error.code == "invalid_encrypted_content" 拒绝已回放的 reasoning 项时才会执行,真实端点不会返回该错误,因此 tmux 交互式 TUI 端到端在结构上不适用;本 PR 自身的 tmux-testing / publish-tmux / verify / publish-verify 四条 agent-quality lane 在此 head 也全部 skipped。作者对其下游 Windows 复测的自述同样排除了这一臂。故改用可本地执行且此前无人执行过的等价证据:ci-bot stage-2 明确声明"没有构建、运行或执行本 PR 的任何代码——没有 npm、没有 vitest、没有 checkout",于是我做了双臂反事实。四个 blob 的 git sha1 均以三路核对(worktree 重算 == files[].sha == diff 的 index 行)。**A 臂(head 生产码 + head 测试)221/221 全绿;B 臂(两个生产文件回退到 base 07b1cd033e28,两个测试文件经 blob sha 断言与 A 臂逐字节相同)203 通过 / 18 失败。**18 条失败中 14 条是导出级(isEncryptedReasoningRejection is not a function),另 4 条是行为级:三条 expected Error: Responses API error 400 … to be undefined,一条 expected "spy" to be called 2 times, but got 1 times —— 即 base 上该 400 直接抛给调用方且完全不重试,正是本 PR 消除的用户可见故障。第 19 条新测试在两臂都通过,是负对照(钉住"无可降级项时不触发恢复"),非覆盖缺口。两条反空洞性见证:还原 head 后重跑仍 221/221;且 dist 全程只构建过一次、从未重建,B 臂仍然失败 18 条 ⇒ 测试执行的是 worktree 的 TypeScript 源码,不可能读到 dist 或宿主 checkout。结论:在上述 head 可合入;本次不提交 approve,理由见第 5 节第一条(maintainer 于 19:18:34Z 显式请求的 /review 在两次运行中均以 failure 结束、未产出任何裁决,机器人并邀请重试,故 approve 这一腿在此 head 尚未定局)。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: test-efficacy probe — the harness could not validate its own control (harnessValidated: null, both probes inconclusive/no-output) because the probe worktree lacks the gitignored dist and packages/core/src/generated prerequisites that scripts/vitest-global-setup.js requires; the verifiers' hand-run mutations covered part of this ground instead.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — no check was cut short..

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查(原文为英文):test-efficacy probe — the harness could not validate its own control (harnessValidated: null, both probes inconclusive/no-output) because the probe worktree lacks the gitignored dist and packages/core/src/generated prerequisites that scripts/vitest-global-setup.js requires; the verifiers' hand-run mutations covered part of this ground instead.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)"none — no check was cut short.

未审查:反向审计——评审时间预算不足,未能开始第 4 轮。

— qwen3.8-max via Qwen Code /review (v0.23.3)

Comment on lines +487 to +492
const text = toEnvelopeText(responseBody.replace(/^data: ?/, ''));
if (!text) return false;
const envelope = readEnvelope(text, MAX_OBJECT_CANDIDATES, false);
if (!envelope || !isPlainObject(envelope.value)) return false;
const direct = readErrorMember(envelope.value);
if (direct) return direct['code'] === 'invalid_encrypted_content';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-1: This new classifier resolves its own error-envelope normalization inline instead of sharing one reader with its sibling parseReasoningIdRejection, so the two rejection classes in this module now recognize different subsets of the same HTTP 400 body. Two symptoms of that one root, both measured.

On the encrypted side, the function passes repairErrorMessage: false, has no analogue of matchQuotedRejection, and returns on any top-level error member. A user on the litellm-style proxy this module already has fixtures for (proxiedBody, responses-pipeline.test.ts:1994-2013, exercised at :2235 and :2249) gets the upstream rejection re-emitted as {"error":{"message":"litellm.BadRequestError: OpenAIException -<raw newline>{\"error\":{…\"code\":\"invalid_encrypted_content\"}}","type":null,"param":null,"code":"400"}}. readErrorMember finds the outer error, direct['code'] is "400", and line 492 returns false without ever reopening the message — and independently, readEnvelope(…, false) rejects the raw control character that the sibling classifier repairs via REPAIRABLE_CONTROLS. encryptedReasoningRejected therefore stays false, buildReasoningReplayRetry returns undefined, and because the rejected encrypted_content lives in persisted history (part.thoughtSignature, responses-converter.ts:540-553) every later send in that session rebuilds the same blob and 400s identically. The session stays permanently wedged — the outcome this recovery's own doc comment (responses-pipeline.ts:290-297) says it exists to prevent, and which the sibling id-rejection recovery would have handled on the same gateway. The early return also makes the routify_response branch below unreachable for a body carrying both a top-level error and a gateway envelope.

On the id side, the single SSE frame strip lives only here. A gateway that frames its non-2xx bodies as one data: frame — the behaviour your new comment states as fact, and which encryptedBodies[1] pins end to end — reports string_above_max_length as data: {…}\n\n; toEnvelopeText requires a leading {, sees d, returns undefined, and the issue-#9452 recovery never fires on that endpoint while the encrypted recovery added here does.

Both directions fail closed — nothing incorrect is ever produced or sent — so the cost is a lost recovery and a wedged session, not a wrong result.

Witness:

run against a byte-verified copy of b0475ecbfc (tsx, HEAD source unmodified):
  ENC litellm quoting upstream invalid_encrypted_content (valid JSON) => false
  ENC litellm quoting upstream, RAW \n spliced (attested proxy damage) => false
  ENC direct code + RAW \n inside its own error.message               => false
  ENC generic top-level error + routify envelope in one body          => false
  ID  proxied, no control char  => {"namedIndex":1,"maxLength":64}   <- the sibling RECOVERS
  ID  proxied, raw \n / \t / \r => {"namedIndex":2,"maxLength":100}
  ENC data: gateway\n\n          => true
  ID  data: directId\n\n         => undefined   <- same framing, opposite verdict
  ID  plain directId             => {"namedIndex":2,"maxLength":100}

variant A (repair flag false->true only): ENC direct + RAW \n => true; pinned-case deviations=0
variant B (repair + quoted reopen + gateway fall-through): all four targets => true;
  ENC unknown code => false, ENC quoted request (debug) => false, duplicate key / trailing
  garbage / multiple frames / oversized / array / malformed => false; deviations=0
shared readClassifiedErrorMember hoist, oracle = the repo's own real test file copied verbatim:
  WITNESS pristine: 1 failed | 2 passed (3)  ->  patched: 3 passed (3)
  REAL SUITE pristine: 90 passed (90)        ->  patched: 90 passed (90)
  object budget binary-searched: PRISTINE direct max filler=28, gateway=-1 (never classifies)
                                 PATCHED  direct=28, gateway=26

The fix spans both classifiers, so it is not a one-click suggestion: hoist one shared envelope reader in this module that applies the transport normalizations once (single SSE frame stripping, the documented error.message control-character repair) and resolves the recognized wrappers in order — top-level error, then a quoted object inside that member's own message, then routify_response.error_detail.error gated on success === false && status === 400 — returning the authoritative structured error or undefined. parseReasoningIdRejection and isEncryptedReasoningRejection then each match only their own code, and the next gateway or framing shape learned benefits every rejection class at once instead of being copied a third time.

function readClassifiedErrorMember(envelope: Envelope): Record<string, unknown> | undefined {
  const direct = readErrorMember(envelope.value);
  if (direct) {
    if (isPlainObject(direct)) return direct;
  }
  const quoted = matchQuotedError(direct, MAX_OBJECT_CANDIDATES - envelope.objects);
  if (quoted) return quoted;
  const gateway = envelope.value['routify_response'];
  if (!isPlainObject(gateway) || gateway['success'] !== false || gateway['status'] !== 400) {
    return undefined;
  }
  return readErrorMember(gateway['error_detail']);
}

Placement of the frame strip matters and was measured: put it inside toEnvelopeText after that function's own MAX_BODY_CHARS check, not before it. Before, the encrypted path stays identical to HEAD but the id path's raw ceiling widens by 6 characters — a framed body of raw length exactly 64001, i.e. one readErrorResponseBody capped and may have truncated, then classifies instead of failing closed, and all 221 existing tests still pass because both id-path oversized controls (responses-pipeline.test.ts:2199, :2750) feed unframed bodies. After, both paths are fixed with no exposure on either and 223 tests green.

Three existing facts the shared reader must not violate: the object budget is shared across nesting levels rather than per level, so a nested reopen must subtract envelope.objects the way matchQuotedRejection(error, MAX_OBJECT_CANDIDATES - envelope.objects) does at :113 (measured, the gateway shape costs 2 more objects than the direct one, not 3, because error exists in both — do not hardcode a remainder); only the recognized top-level error.message may be reopened, and only strictly, since "Text the endpoint merely quoted somewhere else (a debug field, an example, a request it echoed back) is never evidence about the request we sent" (:68-70), pinned by ['quoted request', …] and ['unknown code', …] which must both stay false; and the strip may remove only the literal data: prefix and only one frame, since String.prototype.trim "would quietly accept, and then act on, bodies that are not JSON at all" (:293-297), pinned by 'a vertical tab precedes the top-level object', ['multiple frames', …] and ['trailing garbage', …].

The tests that must go red if this fix is reverted: a positive case in describe('isEncryptedReasoningRejection') for a proxiedBody-style litellm envelope whose quoted upstream error carries code: 'invalid_encrypted_content', and the same with a raw \n spliced into the outer error.message — both assert true and both return false today; plus a positive case in describe('parseReasoningIdRejection') asserting parseReasoningIdRejection(400, `data: ${rejection('input[1].id', MAX_64)}\n\n`) equals { namedIndex: 1, maxLength: 64 }, which returns undefined today. Please confirm each by mutation: drop the shared reader back to the inline per-classifier normalization and check that both go red.

中文说明

这个新的分类器把错误包体的规范化写在了自己内部,而没有和同文件里的 parseReasoningIdRejection 共用一个读取器,于是本模块中两类拒绝识别现在对同一个 HTTP 400 响应体认出了不同的子集。下面是同一个根因的两个症状,均已实测。

加密这一侧:该函数传入 repairErrorMessage: false,没有 matchQuotedRejection 的对应实现,并且只要存在顶层 error 成员就直接返回。当用户走本模块已有 fixture 的 litellm 式代理时(proxiedBodyresponses-pipeline.test.ts:1994-2013,在 :2235:2249 被使用),上游拒绝会被重新包装成 {"error":{"message":"litellm.BadRequestError: OpenAIException -<原始换行>{\"error\":{…\"code\":\"invalid_encrypted_content\"}}","type":null,"param":null,"code":"400"}}readErrorMember 找到的是外层 errordirect['code']"400",第 492 行于是返回 false,被引用的内层 JSON 永远不会被打开;同时 readEnvelope(…, false) 会因为代理拼进消息里的原始控制字符而拒绝整个包体,而同文件的姊妹分类器会通过 REPAIRABLE_CONTROLS 修复它。结果 encryptedReasoningRejected 保持 false,buildReasoningReplayRetry 返回 undefined,而被拒绝的 encrypted_content 存在于持久化历史中(part.thoughtSignatureresponses-converter.ts:540-553),因此该会话后续每一次发送都会重建同一份密文并以同样方式 400。会话就此永久卡死——这正是本恢复逻辑自己的文档注释(responses-pipeline.ts:290-297)声称要防止的结果,而同一个网关下姊妹的 id 拒绝恢复本来是可以处理它的。这个提前返回还导致下面的 routify_response 分支对同时携带顶层 error 和网关包装的响应体不可达。

id 这一侧:单帧 SSE 剥离只写在了这里。若某网关把非 2xx 响应体包装成单个 data: 帧——也就是你新增注释所断言的行为,encryptedBodies[1] 也端到端钉住了它——那么 string_above_max_length 会以 data: {…}\n\n 形式到达;toEnvelopeText 要求首字符是 {,看到 d 便返回 undefined,于是 issue-#9452 的恢复在该网关上永远不触发,而这里新增的加密恢复却会触发。

两个方向都是 fail closed——绝不会产生或发出错误结果——所以代价是丢失一次恢复、会话卡死,而不是给出错误结果。

实测证据(针对 b0475ecbfc 的逐字节校验副本运行,HEAD 源码未修改):litellm 引用式包体(合法 JSON 与拼入原始换行两种)均返回 false,直接式包体在自身 error.message 含原始换行时返回 false,同时携带通用顶层 error 与 routify 包装的包体返回 false;而 id 分类器对同样的代理损伤(原始 \n/\t/\r)全部成功恢复。data: 帧下加密侧为 true、id 侧为 undefined。仅把修复开关从 false 改为 true(变体 A)即可让直接式含原始换行的包体恢复,且 14 个既有钉住用例零偏差;再加上引用式重开与网关回落(变体 B)后四个目标形态全部为 true,各反例仍为 false,零偏差。共用 readClassifiedErrorMember 的提升实现以仓库自带真实测试文件逐字节副本作为判据:见证用例由 1 failed | 2 passed 翻转为 3 passed,真实套件两侧均 90 passed;对象预算经二分实测——原始 direct 上限 28、gateway 为 -1(从不识别),修补后 direct 28、gateway 26。

修复跨越两个分类器,因此不是一键 suggestion:在本模块中提升出一个共用的包体读取器,把传输层规范化只做一次(单帧 SSE 剥离、已文档化的 error.message 控制字符修复),并按顺序解析已识别的包装——顶层 error、该成员自身 message 内被引用的对象、以及在 success === false && status === 400 门控下的 routify_response.error_detail.error——返回权威的结构化错误或 undefined。此后 parseReasoningIdRejectionisEncryptedReasoningRejection 各自只匹配自己的 code,下次再学到新的网关或分帧形态时,所有拒绝类别一次性受益,而不必再抄第三遍。

帧剥离的放置位置有影响,且已实测:应放在 toEnvelopeText 自身的 MAX_BODY_CHARS 检查之后,而不是之前。放在之前时,加密侧与 HEAD 完全一致,但 id 侧的原始长度上限会放宽 6 个字符——原始长度恰为 64001(即 readErrorResponseBody 截断过、可能被截断)的带帧包体会被识别而不是 fail closed,而全部 221 个既有测试仍然通过,因为 id 侧两个超长控制用例(responses-pipeline.test.ts:2199:2750)喂的都是不带帧的包体。放在之后,则两侧都被修复、任一侧都无暴露,223 个测试全绿。

共用读取器不得违反三条既有事实:对象预算是跨嵌套层级共享而非每层独享,因此嵌套重开必须像 :113matchQuotedRejection(error, MAX_OBJECT_CANDIDATES - envelope.objects) 那样减去 envelope.objects(实测网关形态比直接形态多消耗 2 个对象而非 3 个,因为 error 在两种形态中都存在——不要硬编码余量);只有被识别的顶层 error.message 可以被重开,且必须严格读取,因为「接口只是在别处引用的文本(debug 字段、示例、回显的请求)永远不能作为关于我们所发请求的证据」(:68-70),由 ['quoted request', …]['unknown code', …] 钉住,二者必须继续返回 false;剥离只能移除字面的 data: 前缀且只能一帧,因为 String.prototype.trim 「会悄悄接受并进而处理根本不是 JSON 的包体」(:293-297),由 'a vertical tab precedes the top-level object'['multiple frames', …]['trailing garbage', …] 钉住。

若撤销此修复必须变红的测试:在 describe('isEncryptedReasoningRejection') 中新增一个正例,使用 proxiedBody 式的 litellm 包装、其被引用的上游错误携带 code: 'invalid_encrypted_content',以及在外层 error.message 中拼入原始 \n 的同一形态——两者都断言 true,而今天都返回 false;另在 describe('parseReasoningIdRejection') 中新增正例,断言 parseReasoningIdRejection(400, `data: ${rejection('input[1].id', MAX_64)}\n\n`) 等于 { namedIndex: 1, maxLength: 64 },今天返回 undefined。请分别用变异验证:把共用读取器改回各分类器内部的规范化写法,确认两者都变红。

— qwen3.8-max via Qwen Code /review (v0.23.3)

`{"error":{},"error":${JSON.stringify(error)}}`,
400,
],
['oversized', direct.padEnd(64001, ' '), 400],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-2: Nothing in the repository discriminates that the raw-length guard at responses-reasoning-rejection.ts:485 runs before the data: frame strip on :487, and that ordering is load-bearing. This row is the only oversized negative, and it carries no data: prefix, so the strip is a no-op on it and toEnvelopeText's own length check rejects the body for exactly the same reason the guard would have — the two guards are indistinguishable on every input the suite supplies. Deleting the guard, or moving the strip above it, leaves 221 of 221 tests green across both touched files.

So an editor who drops || responseBody.length > MAX_BODY_CHARS from :485, or who hoists the strip while trying to share it with the sibling classifier, gets a fully green suite and a real behaviour change: a framed error body whose raw length is exactly 64001 — i.e. one readErrorResponseBody capped at MAX_ERROR_BODY_CHARS and therefore may have truncated — strips to 63995, parses as a complete frame, and classifies as a recoverable rejection. Measured end to end, the pipeline goes from fetchCalls=1 with the original 400 surfaced to fetchCalls=2 with a recovery retry issued over a body its own reader flagged as possibly truncated, which is precisely what the cap coupling exists to prevent ("a body that hits the cap is returned one character over the classifier's own ceiling, so recovery fails closed on it rather than acting on a prefix", responses-pipeline.ts:740-748). That length is reachable rather than synthetic: readErrorResponseBody returns text.slice(0, MAX_ERROR_BODY_CHARS) (:805, :834), so 64001 is exactly the length a possibly-truncated body arrives at, and raw 64002-64006 can never reach the classifier from the pipeline. The mutation widens the raw ceiling by exactly 6 characters (len('data: ')) and leaves the unframed ceiling untouched, which is why this row cannot see it. The id path's two oversized controls (responses-pipeline.test.ts:2199, :2750) also feed unframed bodies, so neither pins the ordering there either.

The guard itself is present and correct at this commit, so this is a mutation that survives rather than a behaviour the diff broke.

Witness:

HEAD (unmodified PR code, isolated verbatim copy):
  responses-reasoning-rejection.test.ts 90 passed; responses-pipeline.test.ts 131 passed
MUTATION A (:485 -> `if (status !== 400) return false;`):
  2 files, 221 passed (221)   <- guard deleted, suite fully green
MUTATION B (strip hoisted above the guard):
  2 files, 221 passed (221)   <- reorder, suite fully green
HEAD + proposed row:  91 passed (91)                      <- the row passes today
MUTATION A + proposed row: does not recover on oversized framed
  -> AssertionError: expected true to be false | 1 failed, 90 passed
MUTATION B + proposed row: does not recover on oversized framed
  -> AssertionError: expected true to be false | 1 failed, 90 passed

isEncryptedReasoningRejection(400, ('data: ' + direct).padEnd(64001, ' ')):
  HEAD: false | MUT-A: true | MUT-B: true   (raw=64001 stripped=63995)
raw-length sweep, framed body (T = classified):
  HEAD  63998:T 63999:T 64000:T 64001:F 64002:F
  MUT-B 63998:T ...  64006:T 64007:F        (unframed ceiling unchanged on both arms)
end to end (probe it(...) in a verbatim copy of responses-pipeline.test.ts):
  HEAD  framed oversized raw=64001 fetchCalls=1 drained=error status 400  <- fails closed
  MUT-A framed oversized raw=64001 fetchCalls=2 drained=ok(recovered)     <- acts on a capped body
  MUT-B framed oversized raw=64001 fetchCalls=2 drained=ok(recovered)
  control: all plain oversized raw=64001 fetchCalls=1 | all framed normal len=151 fetchCalls=2
Suggested change
['oversized', direct.padEnd(64001, ' '), 400],
['oversized', direct.padEnd(64001, ' '), 400],
['oversized framed', ('data: ' + direct).padEnd(64001, ' '), 400],

A framed pipeline-level analogue of responses-pipeline.test.ts:2199 (does not retry an oversized rejection even when redaction would shrink it below the limit) would pin the same ordering end to end; the encrypted path has no such control today.

The row depends on MAX_BODY_CHARS = 64_000 (responses-reasoning-rejection.ts:50) staying exactly one below MAX_ERROR_BODY_CHARS = CLASSIFIABLE_ERROR_BODY_CHARS + 1 with CLASSIFIABLE_ERROR_BODY_CHARS = 64_000 (responses-pipeline.ts:159, :164) — the +1 is what makes a body at the reader's cap land one character over the classifier's ceiling, and padEnd(64001, …) in both test files depends on that exact value.

The test that must go red if the guard is removed is the new does not recover on oversized framed case: delete || responseBody.length > MAX_BODY_CHARS from responses-reasoning-rejection.ts:485 and confirm the whole suite is green today at 221/221 (that is the defect), then confirm it fails once the row is added.

中文说明

仓库中没有任何测试能够区分 responses-reasoning-rejection.ts:485 的原始长度守卫是在 :487data: 帧剥离之前执行的,而这个顺序是承重的。当前这一行是唯一的超长反例,它不带 data: 前缀,所以剥离对它是空操作,而 toEnvelopeText 自身的长度检查会以与守卫完全相同的理由拒绝该包体——在套件提供的每一个输入上,两道守卫都无法区分。删除守卫、或把剥离移到它上面,两个被改动文件的全部 221 个测试仍然全绿。

因此,若有人从 :485 删掉 || responseBody.length > MAX_BODY_CHARS,或在试图与姊妹分类器共用剥离时把它上移,他会得到全绿的套件和一个真实的行为变化:一个原始长度恰为 64001 的带帧错误包体——也就是被 readErrorResponseBodyMAX_ERROR_BODY_CHARS 截断过、因而可能被截断的那种——剥离后变成 63995,作为完整帧解析成功,并被判定为可恢复的拒绝。端到端实测:管线从 fetchCalls=1(原始 400 被抛出)变成 fetchCalls=2(对一个被自身读取器标记为可能截断的包体发起恢复重试),而这正是上限耦合要防止的结果(「命中上限的包体会被返回为比分类器自身上限多一个字符,因此恢复会 fail closed,而不是基于前缀行事」,responses-pipeline.ts:740-748)。该长度是可达的而非人造的:readErrorResponseBody 返回 text.slice(0, MAX_ERROR_BODY_CHARS):805:834),所以 64001 恰恰是可能被截断的包体到达时的长度,而原始长度 64002-64006 从管线永远到不了分类器。该变异把原始长度上限恰好放宽 6 个字符(len('data: ')),且不改变不带帧的上限,这正是当前这一行看不见它的原因。id 路径的两个超长控制用例(responses-pipeline.test.ts:2199:2750)同样喂不带帧的包体,因此在那一侧也没有钉住这个顺序。

守卫本身在本次提交中是存在且正确的,所以这是一个存活下来的变异,而不是本 diff 破坏的行为。

实测证据:HEAD(未修改的 PR 代码,隔离的逐字节副本)下 responses-reasoning-rejection.test.ts 90 通过、responses-pipeline.test.ts 131 通过;变异 A(把 :485 改为 if (status !== 400) return false;)与变异 B(把剥离上移到守卫之上)都让 2 个文件 221 个测试全绿;HEAD 加上新增行是 91 通过;变异 A/B 加上新增行都出现 does not recover on oversized framed -> AssertionError: expected true to be false,1 失败 90 通过。直接调用 isEncryptedReasoningRejection(400, ('data: ' + direct).padEnd(64001, ' ')):HEAD 为 false,两个变异均为 true(raw=64001,stripped=63995)。原始长度扫描(带帧,T 表示被识别):HEAD 在 64001 处变为 F,变异 B 一直到 64006 都是 T、64007 才 F,不带帧的上限两侧一致。端到端(在 responses-pipeline.test.ts 逐字节副本中插入探针用例):HEAD 带帧超长 raw=64001 为 fetchCalls=1 drained=error status 400(fail closed),两个变异均为 fetchCalls=2 drained=ok(recovered)(对已被截断标记的包体行事);两个对照分别是所有不带帧超长 raw=64001 为 fetchCalls=1、所有带帧正常 len=151 为 fetchCalls=2

另外,为 responses-pipeline.test.ts:2199does not retry an oversized rejection even when redaction would shrink it below the limit)补一个带帧的管线级同类用例,可以端到端钉住同一顺序;加密路径目前没有这样的控制用例。

该用例依赖 MAX_BODY_CHARS = 64_000responses-reasoning-rejection.ts:50)恰好比 MAX_ERROR_BODY_CHARS = CLASSIFIABLE_ERROR_BODY_CHARS + 1 小一,其中 CLASSIFIABLE_ERROR_BODY_CHARS = 64_000responses-pipeline.ts:159:164)——正是这个 +1 让命中读取器上限的包体落在分类器上限之上一个字符,两个测试文件中的 padEnd(64001, …) 都依赖这个确切数值。

若移除守卫必须变红的测试就是新增的 does not recover on oversized framed 用例:从 responses-reasoning-rejection.ts:485 删掉 || responseBody.length > MAX_BODY_CHARS,先确认今天整个套件在 221/221 全绿(这就是缺陷),再确认加上该行后它会失败。

— qwen3.8-max via Qwen Code /review (v0.23.3)

@LaZzyMan LaZzyMan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DragonnZhang
DragonnZhang added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 7ea4645 Sep 11, 2026
170 of 171 checks passed
@DragonnZhang
DragonnZhang deleted the dragon/fix-responses-encrypted-recovery branch September 11, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants