Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 23e122b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c5591d81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const mediaClientError = | ||
| error.kind === 'status' && | ||
| error.statusCode === 400 && | ||
| countMediaParts(request.content.messages) > 0; |
There was a problem hiding this comment.
Let retryable 400s use normal backoff before stripping
When a provider or custom adapter marks a generic HTTP 400 as retryable: true and the request contains media, this recovery runs before runAttempts reaches shouldRetry, records a strip, and makes every later backoff attempt omit valid media; if the transient retry then succeeds, the model answers without the image even though the provider explicitly requested a retry. The status contract preserves retryability independently of the HTTP code, so exclude retryable errors here or let normal retries run first.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
| const mediaClientError = | ||
| error.kind === 'status' && | ||
| error.statusCode === 400 && | ||
| countMediaParts(request.content.messages) > 0; |
There was a problem hiding this comment.
Check the media that actually reached the provider
When AgentMediaResolverService converts a daemon or inline media part to text before sending it—for an unsupported model, an unreadable or invalid file, or the media budget—runAttempts still passes this policy the pre-agent-media recovery request, so this count is nonzero although the wire request contained no media. An unrelated 400 therefore records and persists a media-strip rewrite and can change the next request's text without emitting the media warning; base this gate on the resolved attempt that actually failed.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
…without wire media
|
已处理 Codex 自动审查的两条 P2 意见(commit 484a988):
验证: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484a988942
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ? new Map() | ||
| : await service.displayPaths(current.content.messages); | ||
| const messages = await service.resolve(current.content.messages, model, request, control.signal); | ||
| resolvedHasMedia = countMediaParts(messages) > 0; |
There was a problem hiding this comment.
Measure media after protocol lowering
When an Anthropic request contains audio_url, or an OpenAI Responses request contains video_url, this flag is true even though encoding later converts those parts to text (packages/agent-core-v2/src/human/llm/builtin/protocol/anthropic/legalize.ts:17-22 and packages/agent-core-v2/src/human/llm/builtin/protocol/openai-responses/lower.ts:87-94). If that media-free wire request gets an unrelated non-retryable 400, the new generic branch still records and persists a media strip before retrying. Fresh evidence after the earlier review is that the new resolver-output check runs before protocol legalization/lowering; measure the encoded request, or only parts the selected protocol will actually transmit, instead.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
|
已处理第二轮 Codex 审查的 P2 意见(commit f8c5963): 媒体判定收窄到图片:协议编码层会把 Anthropic 的 新增测试 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c59631b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const mediaClientError = | ||
| error.kind === 'status' && | ||
| error.statusCode === 400 && | ||
| !error.retryable && | ||
| countMediaParts(request.content.messages) > 0 && |
There was a problem hiding this comment.
Persist a generic strip only after its retry succeeds
When an image-bearing request hits an unrelated generic 400 and the stripped retry also fails, accepting this proposal has already made AgentLoopRequestFactory.markMediaRecovery store the stripped ledger, and the failed outcome never rolls it back. createMediaRecoveryStore.read reapplies that ledger to later steps or turns for the same model and history revision, so once the unrelated failure clears a valid image is silently omitted without another warning. Keep this recovery tentative and persist it only after the media-free retry succeeds.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
|
已处理第三轮 Codex 审查的 P1 意见,并简化了整体设计(commit 23e122b): 问题:通用 400 的误剥离会被 最终方案:通用 400 的剥离只在当次请求内生效、永不持久化——在
|
Requirement or Bug
Resolve #4863
一次正常的读图操作(
ReadMediaFile读取一张很小的 PNG)之后,服务端返回了通用 400Invalid request Error。由于错误未被识别为与媒体相关,会话不做任何恢复:该步直接失败,之后的重试、后续 turn、甚至手动触发的 full compaction 都把同一条图片消息原样重发、以同样的 400 失败,整个会话不可恢复直到崩溃。本 PR 让这类"携带媒体的请求被未分类客户端 400 拒绝"的场景可以自动剥离媒体重试一次,会话得以继续。Bug Reproduction Steps
See linked issue(issue 中包含 debug 包时间线、三次 400 的 requestId 与 wire.jsonl 证据)。
Root Cause
这是根因修复(针对客户端恢复缺口;服务端为何返回通用 400 属于网关行为,客户端无法依赖其消息内容)。
时序如下:
ReadMediaFile工具结果以kimi-file://图片消息进入上下文;请求时AgentMediaResolverService.resolve将其解析为 data URL / 上传引用,这一步正常。Invalid request Error。toLlmStatusErrorMessage(src/human/llm/protocol/rules/status.ts)中它不匹配image_format、request_structure等任何消息模式,被归为kind: 'status'。createMediaDegradeRecovery(src/human/llm/builtin/policy/media-degrade.ts)只对request_too_large和image_format(含unsupported media type for base64 image/invalid data url for image消息模式)提出剥离/降级,通用 400 不在范围内,于是当步失败、无剥离重试。full_compaction(droppedCount=0)把同一媒体原样重发 → 同样 400 → 会话死亡。Code Changes
在恢复策略中新增一个分支:未分类的客户端 400(
kind: 'status'、statusCode === 400、非 retryable)且当前请求携带媒体部件时,走与 image-format 拒绝相同的剥离重试路径,但标记为专用 actionstripped-client-error。propose({ error, request, appliedRecoveries }) imageFormat = ...(不变) + mediaClientError = error.kind === 'status' + && error.statusCode === 400 + && !error.retryable + && countMediaParts(request.content.messages) > 0 - if (error.kind !== 'request_too_large' && !imageFormat) return undefined + if (error.kind !== 'request_too_large' && !imageFormat && !mediaClientError) return undefined ... - return stripEntry(messages, appliedRecoveries, replacement) + strip = stripEntry(...) + if (strip !== undefined && mediaClientError) + return { ...strip, action: 'stripped-client-error' } // 不持久化 + return strip同时两处配套:
core/request.ts的恢复存储在写入时按 action 过滤:stripped-client-error永远不会被持久化到跨请求恢复存储(在 store 边界过滤,而不是只在事件处跳过——否则后续任意 media-degrade 事件都会把整个 ledger 连同它一起写入)。isStrippedRecord同时识别stripped与stripped-client-error,避免混合错误序列(通用 400 剥离后重试又遇 413)产生无意义的无重写恢复事件。因果说明:
retryable错误:可重试的 400 应保留媒体走正常 backoff,剥离会让后续 backoff 请求永久丢失图片。countMediaParts === 0不再命中新分支,stripEntry也因无媒体且已剥离返回undefined,错误照常上浮。does not resend for an unrelated 400(无媒体)语义不变。Behavior Changes and Affected Users
Invalid request Error)retryable的客户端 400受影响模块与测试覆盖:
src/human/llm/builtin/policy/media-degrade.ts(新分支与剥离记账)、src/agent/loop/core/request.ts(存储边界 action 过滤)— 由test/agent/llmRequester/llmRequesterService.test.ts新增七个用例覆盖:calls.value === 2,媒体数[1, 0]);retryable的 400 + 携带媒体 → 不剥离,媒体原样保留;[1, 0, 1]);[1, 0, 1]);llmRequesterService.test.ts75/75 通过;modelRequester.test.ts14/14 通过;包级 typecheck 通过。sessionMediaStore.test.ts与fullCompaction.test.ts各有 4/1 个失败,已在干净基线上复现确认为 Windows 环境既有失败,与本改动无关。Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.