fix(core): isolate image payload eviction state - #9423
Conversation
|
Thanks for the clean split from #8938 — reviewing the image-history part on its own works well. Template looks good ✓ Problem: this one is real, not theoretical. The eviction path shipped via #6045/#6380 has two structural gaps traceable in the current code: Direction: aligned — state isolation for a shipped core feature, and making fork snapshot copies independent became required the moment eviction turned into an in-place rewrite. No CHANGELOG signal needed for an internal correctness fix. Size: core paths touched ( Approach: the three changes remain mutually load-bearing — in-place marker rewrite (fixes durable coverage), below-threshold marker resolution through the store (fixes unresolved requests), and Part cloning in Risk: Moving on to code review. 🔍 中文说明感谢从 #8938 干净地拆分出来——图片历史部分独立评审很合适。 模板完整 ✓ 问题:这是真实问题,不是理论加固。#6045/#6380 引入的淘汰路径在当前代码里有两个结构性缺口:旧版 方向:对齐——这是已上线核心功能的状态隔离修复;一旦淘汰改为原地改写,fork 快照副本的隔离就成为必需。内部正确性修复无需 CHANGELOG 信号。 规模:触及核心路径( 方案:三处改动依然互为前提——原地改写标记(修复持久历史覆盖)、低于阈值时经 store 解析标记(修复未解析请求)、 风险: 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (this pass: head 982ea51)The round-1 design assessment stands — in-place Part rewrite gives durable-history coverage for merged turns, store-backed resolution fixes below-threshold requests, and Part cloning at the capture boundary closes the aliasing the rewrite creates. What this re-run verified is the two Criticals raised in later rounds and fixed since:
Around the fix, re-verified on this head:
Testing evidence (this PR's own CI — unattended run, no PR code executed here)All checks on the reviewed commit are complete; the Linux unit suite — the PR gate — is green:
Both Sandboxed verification would settle the residual claim end-to-end: 中文说明代码审查(本轮:head 982ea51):第一轮的设计判断依然成立——原地改写 Part 让合并轮次也能覆盖持久历史、store 解析修复低于阈值的请求、捕获边界克隆 Part 关闭原地改写带来的别名风险。本轮重点核实后续两轮提出、此后已修复的两个 Critical:
周边复核:reattach 只落到复制出的 测试证据:无人值守 CI 运行,此处未执行任何 PR 代码。评审提交上所有检查已完成,PR 门禁 Linux 单测套件为绿色;两个 沙箱验证可端到端坐实残余声明: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the two later-round Criticals are fixed exactly as suggested and pinned by regression tests, CI is green on the reviewed head, and what remains is deferred Suggestion-level debt. Stepping back: this PR went through four honest rounds, and it shows — in a good way. Each round found one real Critical (eviction leaking into un-cloned consumers, then an unbounded current-turn exemption, then marker resurrection through loose text matching), and each was answered with the suggested fix and a test that targets the exact witness, not a generic happy path. The round-3 resurrection probe reproduced the bug through the real module and demonstrably flips under the anchored matcher now on this head, and I re-checked the anchor against the marker producer: genuine markers match, bare id echoes cannot. The round-4 pass found nothing new. The five open threads are all Suggestion-level perf/hygiene items the author deferred to follow-ups with reasons I find fair; none of them changes what this PR ships. The design still beats my original copy-on-write proposal, every edit in the diff is load-bearing, and the high-risk-path requirement — full-depth review plus CI evidence — is met: the Linux unit suite and both workflow runs are green on the exact reviewed commit. Approving. 中文说明退一步整体看:这个 PR 经历了四轮认真的评审,效果是正面的——每一轮各发现一个真实 Critical(淘汰泄漏进未克隆的消费方、当前轮豁免无上限、宽松文本匹配导致标记复活),每一次作者都按建议修复,并附上针对原探针场景的测试,而不是泛泛的正向用例。第三轮的复活探针通过真实模块复现了问题,在当前提交的锚定匹配下明确翻转;我也重新核对了锚定模式与标记生成方:真实标记可匹配、裸 id 复述无法匹配。第四轮审查未发现新问题。五个未结线程均为建议级的性能/卫生项,作者已附理由延后跟进,我认为理由成立,且均不影响本 PR 交付的内容。设计依然优于我最初提出的 copy-on-write 方案,diff 中每处改动都不可或缺,高风险路径的要求——完整深度审查加 CI 证据——已满足:Linux 单测套件与两个工作流在评审提交上均为绿色。批准。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Approving — no blockers.
Verified against the head commit 3198feb:
replaceImagePayloadsInPlacenow mutates sharedPartobjects in place (part.text = …; part.inlineData = undefined) instead of replacing array slots. BecauseextractCuratedHistory/appendCuratedContentpreservePartreferences while only rebuilding thepartsarray for merged user turns, this is what actually lands markers in durable history for the merged-turn case the old slot replacement missed.getRequestHistorynow callsbuildReattachPartsunconditionally, so a later below-threshold request resolvesImage #…markers through the store instead of sending unresolved marker text. Emptyreplacedis handled, and the current-turn exemption correctly scopescollectReferencedImageIdsto the last user turn.copyHistoryContainersclonesPartobjects (clonePart), and bothsaveCacheSafeParamsandgetCacheSafeParamsrun through it, so a fork snapshot captured before eviction keeps its bytes after the main conversation mutates parts in place.
CI is green on this commit (Test (ubuntu-latest, Node 22.x), web-shell E2E Smoke, Secret scan, Desktop Shell, Dependency CVE audit). The new tests fail against the base eviction code, so the suite pins the change rather than passing vacuously.
Non-blocking nit: clonePart hand-rolls the functionResponse.parts cast; reusing getFunctionResponseParts from compactionInputSlimming.ts (the documented canonical carrier accessor) would keep the two walkers in sync.
中文:批准,无阻塞项。已在 3198feb 上核对三处改动——原地改写共享 Part 对象使合并轮次的标记真正写入持久历史;buildReattachParts 无条件调用使低于阈值的请求能经 store 解析标记;copyHistoryContainers 克隆 Part 使 fork 快照与后续淘汰隔离。CI 全绿,新增测试在基线淘汰代码上会失败,确实钉住了改动。仅一处非阻塞建议:clonePart 可复用 getFunctionResponseParts 作为规范访问器。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 356 tests pass — this review observed 20508, 1578, 21727, 1597, 494 passed.
中文说明
Test Plan(非阻断):356 tests pass — this review observed 20508, 1578, 21727, 1597, 494 passed。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Review closeout for the current head
Verification: 357 focused tests passed; Prettier, ESLint, core typecheck, and the full repository build passed. |
doudouOUC
left a comment
There was a problem hiding this comment.
Re-check: Previous blocker R1-1
R1-1 (Critical): In-place Part mutation propagating to un-cloned history consumers
Verdict: Fixed by this diff.
The initial fix (commit 608bd71faa) patched buildAgentHistory (skillReviewAgentPlanner.ts) and the AgentTool fork seeding (agent.ts) individually. The final approach (commit ff0a28d4a7) reverted those individual patches and instead fixed the source: getHistoryShallow now returns cloned Part objects via copyContentContainer/copyPartContainer (geminiChat.ts). This protects ALL consumers of getHistoryShallow — including skill-review agents, AgentTool forks, and any future caller — rather than just the two explicitly patched consumers.
Additionally:
clonePart/copyHistoryContainersinforkedAgent.tsprotects CacheSafeParams (used bysaveCacheSafeParams/getCacheSafeParams)getHistoryForForkWindowalso usescopyContentContainer, so AgentTool fork seeding is covered
Status of previous suggestions
| Suggestion | Status |
|---|---|
R1-2 (SHA-256 overhead in buildReattachParts) |
Partially addressed — early return `if (replaced.length === 0 && (!store |
R1-3 (duplicate walk in collectInlineImageIds) |
Partially addressed — collectInlineImageIds and countAllInlineImages now share the inlineImageParts generator, eliminating the duplicated loop. |
R1-4 (store-based marker resolution duplicates prepareImagePayloadsForRequest) |
Still valid — buildReattachParts and prepareImagePayloadsForRequest both resolve markers from the store. However, they serve different contexts (request-time via getRequestHistory vs fork-time) and merging them would add coupling. Low risk. |
R1-5 (hand-rolled cast in clonePart) |
Fixed — now uses getFunctionResponseParts from compactionInputSlimming.ts |
| R1-6 (stale doc comment) | Fixed — doc comment updated to reflect the new contract |
R1-7 (missing test for nested functionResponse.parts path) |
Partially addressed — "does not reattach an image already inline in a tool response" test was added. The specific nested functionResponse.parts walk in collectInlineImageIds is tested indirectly. |
R1-8 (part.inlineData = undefined keeps key present) |
Fixed — now uses delete part.inlineData |
| R1-9 (removed warning about not mutating leaf payloads) | Fixed — warning about leaf payloads was added back to the doc comment |
New findings
None. The diff is clean, the tests (357 passing) pin the behavior, and the build compiles cleanly (the audio-capture build failure is a pre-existing Windows Python infrastructure issue, unrelated to this PR).
Verification
- Build:
packages/corebuilds successfully (other packages fail due to pre-existing Windows Python/node-gyp issue) - Tests: 3 test files, 357 tests — all pass
- Typecheck: PR author reports both
npm run typecheckandnpm run buildpass locally on macOS
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 357 tests pass — this review observed 20509, 1578, 21724, 1597, 494, 3749, 539 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/services/image-payload-references.ts:92 — [probe] inline images are re-hashed (SHA-256) on every request in marker-bearing sessions (memoize with a WeakMap)packages/core/src/services/image-payload-references.ts:106 — [review] unresolvable markers are dropped silently — no log, counter, or telemetry anywhere in the modulepackages/core/src/services/image-payload-references.ts:119 — [review] the store-miss marker guards (session-resume scenario) have no testpackages/core/src/services/image-payload-references.ts:114 — [probe] the '0 = restore none' doc no longer describes the send path (cap-0 exemption is deliberate)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):357 tests pass — this review observed 20509, 1578, 21724, 1597, 494, 3749, 539 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Review closeout for current head b55aaf3:
Regression evidence: the new cap test fails when reattachment is made unbounded and passes with the fix. Verification: 358 focused tests, Prettier, ESLint, core typecheck, and the full repository build passed. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-4 copyPartContainer duplicate of clonePart (second location) — already reported at geminiChat.ts:1243 (comment 3809531532)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/services/image-payload-references.ts:244 — [probe] inline images are re-hashed (SHA-256) on every request once any eviction has run (memoize per inlineData leaf object)packages/core/src/services/image-payload-references.ts:73 — [review] zero logging at eviction/reattach/failed resolution — cross-instance image loss is silent
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Round 3 closeout for head
Current verification: 359 focused tests, Prettier, ESLint, core typecheck, and the full repository build pass. I am closing the non-blocking threads so the review state matches the round-3 convergence posture. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- SHA-256 re-hashing of inline images on every request (rediscovered by 4 dimensions) — already reported (comment 3807124658), deferred rounds 2-3
- clonePart/copyPartContainer byte-for-byte duplication (rediscovered by 3 dimensions) — already reported (comments 3809531530/3809531532), author deferred to a follow-up
- whole-history regex scan before the early-return guard (rediscovered) — already reported (comment 3809531525), author deferred to a follow-up
- zero logging at eviction/reattach/failed store resolution (rediscovered) — deferred under the convergence posture in rounds 2 and 3
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 359 tests pass — this review observed 20511, 1578, 21724, 1597, 494, 3749, 539 passed.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):359 tests pass — this review observed 20511, 1578, 21724, 1597, 494, 3749, 539 passed。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
There was a problem hiding this comment.
APPROVE — all previous Critical/Suggestion threads are resolved, CI is green (13/13 pass), MERGEABLE, and my earlier re-check confirmed R1-1 was fixed by the getHistoryShallow deep-clone approach. No new findings in the final diff. Ready for merge.
|
Released in v0.21.15. |
What this PR does
Keeps image-payload eviction consistent across durable chat history, outgoing requests, and cached fork snapshots. When the inline-image threshold is reached, shared image parts are rewritten to stable text markers in the durable history, while fork snapshots retain isolated copies. Later requests can resolve those stored markers even when the request is below the eviction threshold, retain an image explicitly referenced by the current user outside the recency cap, and avoid attaching duplicate bytes when the same image is already inline.
This is the image-history portion split out of #8938 so the upstream placeholder defense can be reviewed independently.
Why it's needed
Copying only history containers left the underlying image parts shared in some paths and detached in others. That could either leave large base64 payloads in durable history after eviction or let a later below-threshold request send only an unresolved marker. Cached fork snapshots also need their own part objects so eviction in the main conversation cannot mutate an already captured fork context.
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/services/image-payload-references.test.ts src/utils/forkedAgent.cache.test.ts src/core/geminiChat.test.ts; 359 tests pass locally.npm run typecheckandnpm run build; both pass locally.Evidence (Before & After)
N/A (non-UI runtime state-isolation fix).
Tested on
Environment (optional)
Node.js 22 workspace with package unit tests and the repository build/typecheck gates.
Risk & Scope
Linked Issues
Split from #8938.
中文说明
本 PR 做了什么
让图片 payload 的淘汰在持久聊天历史、发出的请求和缓存的 fork 快照之间保持一致。达到内联图片阈值后,共享的图片 Part 会在持久历史中原地改写为稳定文本标记,而 fork 快照保留隔离副本。后续请求即使低于淘汰阈值,也能从存储中解析这些标记;当前用户显式引用的图片不受最近图片数量上限影响;若相同图片已经以内联形式存在,则不会重复附加字节。
这是从 #8938 拆出的图片历史部分,使上游占位响应防御可以独立评审。
为什么需要
只复制历史容器会导致底层图片 Part 在部分路径中仍然共享、在另一些路径中又脱离持久历史。结果可能是淘汰后持久历史仍保留大段 base64 payload,或后续低于阈值的请求只发送无法解析的标记。缓存的 fork 快照也需要独立 Part 对象,避免主会话的淘汰操作修改已经捕获的 fork 上下文。
评审测试计划
如何验证
cd packages/core && npx vitest run src/services/image-payload-references.test.ts src/utils/forkedAgent.cache.test.ts src/core/geminiChat.test.ts;本地 359 个测试通过。npm run typecheck和npm run build;本地均通过。前后对比证据
N/A(非 UI 的运行时状态隔离修复)。
测试环境
macOS ✅;Windows / Linux 未本地测试,交由 CI 验证。
环境说明
Node.js 22 workspace,运行 package 单元测试及仓库 build/typecheck 门禁。
风险与范围
关联 Issue
从 #8938 拆出。