fix(core): separate hook context from transcript display - #7948
Conversation
|
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @destire-mio, thanks for picking this up — the linked issue (#7940) is well-specified and the fix direction looks right.
Before we can review the code, the PR body needs to follow the PR template. Right now it uses custom headings (Summary, Root cause, Compatibility, Validation) instead of the required ones. Specifically, we need:
## What this PR doesand## Why it's needed— your Summary and Root cause content maps well to these, just needs the right headings.## Reviewer Test Planwith### How to verify,### Evidence (Before & After), and### Tested on— this is the big one. How should a reviewer confirm the fix? What does the JSONL look like before vs. after? Which OSes did you test on?## Risk & Scope— main risk/tradeoff, what's not validated, breaking changes.## Linked Issues—Closes #7940is great, just put it under this heading.<details><summary>中文说明</summary>— a Chinese translation of the above.
Once the body follows the template, re-run with @qwen-code /triage and we'll pick it right back up.
中文说明
@destire-mio 你好，感谢认领这个 issue（#7940）！修复方向看起来是对的。
在进入代码审查之前，PR 正文需要使用PR 模板中规定的标题格式。目前使用的是自定义标题（Summary、Root cause、Compatibility、Validation），需要改为：
## What this PR does和## Why it's needed—— 现有的 Summary 和 Root cause 内容可以直接对应，只需更换标题。## Reviewer Test Plan（含### How to verify、### Evidence (Before & After)、### Tested on）—— 这是关键部分：reviewer 如何验证修复？修复前后的 JSONL 长什么样？在哪些操作系统上测试过？## Risk & Scope—— 主要风险/权衡、未验证的部分、破坏性变更。## Linked Issues——Closes #7940放在这个标题下即可。<details><summary>中文说明</summary>—— 上述内容的中文翻译。
更新正文后，使用 @qwen-code /triage 重新触发审查即可。
— Qwen Code · qwen3.8-max-preview
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: You are review agent reverse-audit — Reverse audit agen... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
中文说明
已审查。 未审查：You are review agent reverse-audit — Reverse audit agen...——它被指向 diff 的行却从未打开：有工具调用，但没有一次读取 diff。
— qwen3.7-max via Qwen Code /review
|
The current head addresses the previous template request and is mergeable. The only failing check is SDK Java 11. This PR does not modify Could a maintainer please rerun the failed job and refresh or dismiss the stale |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| it('projects Qwen user transcript records without hook context', () => { | ||
| const extractQwenRecordText = ( | ||
| QwenAgent.prototype as unknown as QwenHistoryInternals |
There was a problem hiding this comment.
[Suggestion] This test file lives under packages/desktop/, which is excluded from the npm workspaces test collection (!packages/desktop in the root package.json), so the new extractQwenRecordText projection behaviour ships without any test that gates it in the npm-based CI jobs. — Concrete cost: a regression in extractQwenRecordText (e.g. failing to strip the <qwen:user-prompt-submit-context> tag, or returning hook context in the user-visible Desktop transcript) would pass undetected, because this test never runs in npm test or in the CI jobs that run the npm workspace test suites; the behaviour (clean transcript text in Desktop) would silently break. Consider wiring the packages/desktop bun test script into a CI job (the suite passes locally via bun test), so this regression is caught.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Thanks, this is a valid concern. packages/desktop is intentionally isolated from the root npm workspace and synchronized with the standalone OpenWork repository, and the focused Bun test suite passes locally. Since adding Desktop to the required CI matrix is a repository-wide policy change, I’d prefer to handle it in a dedicated follow-up unless maintainers want it included in this PR.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
| : '', | ||
| ) | ||
| .join(''); | ||
| const text = projection.displayText ?? fallbackText; |
There was a problem hiding this comment.
[Suggestion] The new fallbackText mapping — the tag-stripping fallback for records that have no systemPayload.displayText — is never exercised by a test. The only new DataProcessor test feeds a record with systemPayload.displayText, so projection.parts is empty and fallbackText is discarded by the ??; the not.toContain('hook-only context') assertion passes via the displayText branch, not the fallback.
Concrete cost: every sibling consumer this PR adds (transcript-replay, resumeHistoryUtils, qwenTranscriptText, JSONLAdapter) tests the 'strips a complete final tag-only context part' case, but DataProcessor does not. A regression reverting the fallback to record.message.parts would re-leak hook context into insight analysis (the bug #7940 fixes) and stay green.
Suggested test (mirrors the sibling consumers):
it('strips a complete final tag-only context part without metadata', () => {
const tag =
'<qwen:user-prompt-submit-context>\nhook-only context\n</qwen:user-prompt-submit-context>';
const records = [
{ type: 'user', message: { parts: [{ text: 'user prompt' }, { text: tag }] } },
];
const result = formatRecordsForAnalysis(records as never[]);
expect(result).toContain('[User]: user prompt');
expect(result).not.toContain('hook-only context');
});中文说明
新增的 fallbackText 映射（针对没有 systemPayload.displayText 的记录做标签删除的回退分支）没有任何测试覆盖。DataProcessor 唯一的新测试传入的是带 systemPayload.displayText 的记录，因此 projection.parts 为空，fallbackText 会被 ?? 丢弃；not.toContain('hook-only context') 断言是经由 displayText 分支通过的，而非回退删除。
具体代价：本 PR 新增的其它所有消费端（transcript-replay、resumeHistoryUtils、qwenTranscriptText、JSONLAdapter）都测试了'删除完整且位于最后的纯标签上下文 part'场景，唯独 DataProcessor 没有。一旦有回归把回退改回 record.message.parts，Hook 上下文就会重新泄漏进 insight 分析（即 #7940 要修的 bug），而测试仍会通过。
— qwen3.8-max-preview via Qwen Code /review
| if (payload && typeof payload.displayText === 'string') { | ||
| return payload.displayText; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The webui contract 'an empty displayText is meaningful — do not fall back to model-facing parts' has no test in this package. JSONLAdapter.test.ts only feeds a non-empty displayText ('raw @file prompt'); a grep for an empty displayText across packages/webui returns nothing, while the vscode sibling (qwenTranscriptText.test.ts) asserts exactly this case (displayText: '' → '').
Concrete cost: the guard below is correct today (typeof payload.displayText === 'string' admits ''). But if a maintainer simplifies it to a truthy check (if (payload.displayText)), an empty displayText falls through to the parts-join and the webui chat renders the model-expanded prompt plus the hook context — the precise leak this PR exists to prevent — and JSONLAdapter.test.ts stays green. The vscode suite would catch it; the webui suite would not.
Suggested test (mirrors the vscode test):
it('does not fall back to model-facing parts for empty display metadata', () => {
const messages = [
{
type: 'user',
message: { parts: [{ text: 'expanded model prompt' }] },
systemPayload: { displayText: '', hookContext: 'hook-only context' },
},
];
const [adapted] = adaptJSONLMessages(messages as JSONLMessage[]);
expect(adapted?.content).toBe('');
});中文说明
webui 的'空 displayText 有意义——不得回退到模型侧 parts'这一约定在本包没有测试。JSONLAdapter.test.ts 只传入非空 displayText（'raw @file prompt'）；在整个 packages/webui 中搜索空 displayText 没有任何结果，而 vscode 的同类测试（qwenTranscriptText.test.ts）恰好断言了这一场景（displayText: '' → ''）。
具体代价：此处的守卫目前是正确的（typeof payload.displayText === 'string' 接受 ''）。但若有人将其简化为真值判断（if (payload.displayText)），空 displayText 就会回退到 parts 拼接，webui 聊天将渲染出模型扩展后的 prompt 加 Hook 上下文——正是本 PR 要防止的泄漏——而 JSONLAdapter.test.ts 仍会通过。vscode 的测试能抓到，webui 的测试抓不到。
— qwen3.8-max-preview via Qwen Code /review
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
Review: separate hook context from transcript displayWhat it doesWraps The chokepoint choice is good: 1.
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /resolve |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and locally).
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and locally)。
— qwen3.8-max-preview via Qwen Code /review
| const isUserPromptPayload = | ||
| payload && typeof payload['hookContext'] === 'string'; |
There was a problem hiding this comment.
[Critical] Compatibility regression: this discriminator (and the matching gates in the desktop and webui duplicates) requires hookContext to treat displayText as the user-prompt projection, and the tag-strip fallback below requires systemPayload === undefined. Released v0.21.2/v0.21.3 (PR #7956, commit 953c9d817) wrote user-prompt records with systemPayload: { displayText } and no hookContext, with the tagged <qwen:user-prompt-submit-context> block still in message.parts. Those records satisfy neither branch, so the projection returns the raw model-facing parts — leaking the literal XML block into TUI resume, ACP/export replay, insights, session-reference, goal-evidence, and conversation-branches. — Failure scenario: a user who ran v0.21.2/v0.21.3 with a UserPromptSubmit hook (e.g. any session with QWEN.md context) resumes that session → the user turn renders the expanded model prompt plus the raw <qwen:user-prompt-submit-context>…</…> block, the exact content this feature exists to keep out of display surfaces. The consumers this PR replaced (extractUserRecordDisplayText, transcript-replay's withUserPromptDisplayText) honored the single-field displayText. Note the new test does not trust bare displayText on plain user records asserts this leaky output as intended — but released single-field records are byte-identical to that "plain" shape. — Suggested fix: also accept the released single-field shape, using the tagged part as pairing evidence (a released user-prompt record carries the complete tagged context part; notification/cron/mid-turn labels never do) — e.g. treat displayText as authoritative when typeof payload['displayText'] === 'string' and either hookContext is a string or the parts end in a complete tagged context part; add a regression fixture shaped like the v0.21.2 record (displayText without hookContext). The fix must also land in the desktop (qwen-agent.ts) and webui (userTranscriptDisplay.ts) duplicates, which do not import core.
中文说明
[Critical] 兼容性回退：该判别条件（以及 desktop 与 webui 中对应的两份副本）要求存在 hookContext 才会把 displayText 视作用户 prompt 投影，而下方的 tag-strip 回退又要求 systemPayload === undefined。已发布的 v0.21.2/v0.21.3（PR #7956，commit 953c9d817）写入的 user-prompt 记录只有 systemPayload: { displayText }、没有 hookContext，且带标签的 <qwen:user-prompt-submit-context> 块仍留在 message.parts 中。这类记录两个分支都不满足，因此投影会返回原始的模型 parts——把字面 XML 块泄漏到 TUI resume、ACP/export replay、insights、session-reference、goal-evidence 和 conversation-branches。— 失败场景：用户在 v0.21.2/v0.21.3 上使用带 UserPromptSubmit hook 的会话（例如任何含 QWEN.md 上下文的会话）并 resume 该会话时，用户轮次会渲染出扩展后的模型 prompt 加上原始 <qwen:user-prompt-submit-context>…</…> 块，正是本功能要从展示界面排除的内容。本 PR 替换掉的消费端（extractUserRecordDisplayText、transcript-replay 的 withUserPromptDisplayText）原本会采纳单字段 displayText。注意新测试 does not trust bare displayText on plain user records 把这种泄漏结果断言为预期行为——但已发布的单字段记录与该“plain”形状在字节上完全一致。— 建议修复：同时接受已发布的单字段形状，以带标签的 part 作为配对证据（已发布的 user-prompt 记录带有完整的末尾标签 part；notification/cron/mid-turn 标签则从不带）——例如当 typeof payload['displayText'] === 'string' 且 hookContext 为字符串或 parts 以完整标签 part 结尾时，将 displayText 视为权威；并补一个形如 v0.21.2 记录（有 displayText、无 hookContext）的回归 fixture。修复还必须同步到 desktop（qwen-agent.ts）与 webui（userTranscriptDisplay.ts）两份不依赖 core 的副本。
— qwen3.8-max-preview via Qwen Code /review
| const displayText = | ||
| payload && asString(payload.hookContext) !== undefined | ||
| ? asString(payload.displayText) | ||
| : undefined; | ||
| if (displayText !== undefined) return displayText; |
There was a problem hiding this comment.
[Critical] Same compatibility regression as the core projectUserTranscriptForDisplay, in this standalone desktop duplicate (desktop has no @qwen-code/qwen-code-core dependency, so a core-only fix will NOT reach this path). The metadata gate requires hookContext; the tag-strip gate below requires systemPayload === undefined. Released v0.21.2/v0.21.3 single-field { displayText } records satisfy neither, so extractQwenRecordText returns the raw parts — including the <qwen:user-prompt-submit-context> block — joined with \n\n into the desktop history UI. — Failure scenario: the desktop app loads a v0.21.2/v0.21.3 session that used a UserPromptSubmit hook → the user turn displays the expanded model prompt plus the raw XML context block. The desktop test at qwen-agent-slash-history.test.ts:265-272 asserts .toContain('trusted context') for a single-field record, so it blesses the leak and stays green even after a core fix. — Suggested fix: mirror the core fix here (accept single-field displayText when the parts end in a complete tagged context part), or have desktop delegate to a shared projection; update the test assertion accordingly.
中文说明
[Critical] 与 core projectUserTranscriptForDisplay 相同的兼容性回退，出现在这份独立的 desktop 副本中（desktop 不依赖 @qwen-code/qwen-code-core，因此仅修 core 不会覆盖此路径）。metadata 分支要求 hookContext；下方的 tag-strip 分支要求 systemPayload === undefined。已发布的 v0.21.2/v0.21.3 单字段 { displayText } 记录两者都不满足，于是 extractQwenRecordText 返回原始 parts——包括 <qwen:user-prompt-submit-context> 块——以 \n\n 拼接进 desktop 历史界面。— 失败场景：desktop 加载一个用过 UserPromptSubmit hook 的 v0.21.2/v0.21.3 会话时，用户轮次会显示扩展后的模型 prompt 加上原始 XML 上下文块。qwen-agent-slash-history.test.ts:265-272 的 desktop 测试对单字段记录断言 .toContain('trusted context')，因此它把该泄漏断言为正确，即便修了 core 也仍然通过。— 建议修复：在此处同步 core 的修复（当 parts 以完整标签 part 结尾时接受单字段 displayText），或让 desktop 委托给共享投影；并相应更新测试断言。
— qwen3.8-max-preview via Qwen Code /review
| if ( | ||
| payload && | ||
| typeof payload.hookContext === 'string' && | ||
| typeof payload.displayText === 'string' | ||
| ) { | ||
| return payload.displayText; | ||
| } |
There was a problem hiding this comment.
[Critical] Same compatibility regression as the core projectUserTranscriptForDisplay, in this standalone webui duplicate (webui imports only @qwen-code/sdk, never core, so a core-only fix will NOT reach this path). The metadata gate requires both hookContext and displayText strings; the tag-strip gate below requires systemPayload === undefined. Released v0.21.2/v0.21.3 single-field { displayText } records satisfy neither, so getUserTranscriptDisplayText returns the raw concatenated model-facing parts — including the <qwen:user-prompt-submit-context> block — and the ?? extractContent fallback in JSONLAdapter.ts/ChatViewer.tsx never engages because the return is a non-null string. — Failure scenario: a v0.21.2/v0.21.3 session opened in the webui ChatViewer renders the internal hook-context XML in the user message. JSONLAdapter.test.ts ('keeps tag-like parts when another system payload is present') feeds a single-field record and asserts .toContain('user-authored text'), blessing the leak; a probe confirmed the output flips to clean and this test flips pass→fail under a fix. — Suggested fix: mirror the core fix here (accept single-field displayText when the parts end in a complete tagged context part) and update the two JSONLAdapter.test.ts cases, or have webui consume a shared projection.
中文说明
[Critical] 与 core projectUserTranscriptForDisplay 相同的兼容性回退，出现在这份独立的 webui 副本中（webui 只依赖 @qwen-code/sdk，从不依赖 core，因此仅修 core 不会覆盖此路径）。metadata 分支要求 hookContext 与 displayText 同时为字符串；下方的 tag-strip 分支要求 systemPayload === undefined。已发布的 v0.21.2/v0.21.3 单字段 { displayText } 记录两者都不满足，于是 getUserTranscriptDisplayText 返回原始拼接的模型 parts——包括 <qwen:user-prompt-submit-context> 块——而 JSONLAdapter.ts/ChatViewer.tsx 中的 ?? extractContent 回退永远不会触发，因为返回值是非空字符串。— 失败场景：在 webui ChatViewer 中打开一个 v0.21.2/v0.21.3 会话时，用户消息会渲染出内部 hook 上下文 XML。JSONLAdapter.test.ts（'keeps tag-like parts when another system payload is present'）传入单字段记录并断言 .toContain('user-authored text')，把泄漏断言为正确；探针确认在修复后输出会变为干净值且该测试由通过变为失败。— 建议修复：在此处同步 core 的修复（当 parts 以完整标签 part 结尾时接受单字段 displayText）并更新 JSONLAdapter.test.ts 的两个用例，或让 webui 使用共享投影。
— qwen3.8-max-preview via Qwen Code /review
| const prefix = `${USER_PROMPT_SUBMIT_CONTEXT_OPEN}\n`; | ||
| const suffix = `\n${USER_PROMPT_SUBMIT_CONTEXT_CLOSE}`; | ||
| if (!trimmed.startsWith(prefix) || !trimmed.endsWith(suffix)) { |
There was a problem hiding this comment.
[Suggestion] The tightened matcher now requires newline delimiters between the tags and the body (rejecting the newline-less input the old version accepted), but no test pins this: dropping the \n from both prefix and suffix (restoring the old loose matching) leaves all 26 matcher tests green. — Concrete cost: a future change that loosens the delimiter requirement would silently re-accept user-authored text that merely starts/ends with the tag strings, and nothing in CI would catch it. — Suggested fix: add a negative case to the isUserPromptSubmitContextPartText tests asserting a complete tag block without the newline delimiters returns false.
中文说明
[Suggestion] 收紧后的匹配器现在要求标签与正文之间有换行分隔符（会拒绝旧版本接受的无换行输入），但没有测试固定这一点：把 prefix 和 suffix 中的 \n 去掉（恢复旧的宽松匹配）后，全部 26 个匹配器测试仍然通过。— 具体代价：未来任何放宽分隔符要求的改动都会静默地重新接受仅仅以标签字符串开头/结尾的用户自撰文本，而 CI 不会捕获。— 建议修复：在 isUserPromptSubmitContextPartText 测试中补一个反例，断言一个不带换行分隔符的完整标签块返回 false。
— qwen3.8-max-preview via Qwen Code /review
| it('projects Qwen user transcript records without hook context', () => { | ||
| const extractQwenRecordText = ( | ||
| QwenAgent.prototype as unknown as QwenHistoryInternals | ||
| ).extractQwenRecordText; |
There was a problem hiding this comment.
[Suggestion] This is the only test in the diff covering the desktop extractQwenRecordText hook-context stripping, but it is never collected by this repo's test command — packages/desktop is excluded from the root npm workspaces (!packages/desktop) and is a separate bun-based workspace; no npm CI job runs it (.github/workflows/ci.yml has only a check:desktop-isolation step). — Concrete cost: if the desktop projection regresses (e.g. stops preferring displayText, or leaks the <qwen:user-prompt-submit-context> tag), this new test would catch it — but it does not run in this repo's CI, so the regression would ship undetected unless the separate openwork pipeline that owns packages/desktop runs bun test. — Suggested fix: confirm the desktop suite runs in the openwork/desktop CI pipeline; if not, add a job that runs bun test there so this coverage gates the change.
中文说明
[Suggestion] 这是本 diff 中唯一覆盖 desktop extractQwenRecordText hook 上下文剥离的测试，但它永远不会被本仓库的测试命令收集——packages/desktop 被排除在根 npm workspaces 之外（!packages/desktop），是一个独立的基于 bun 的 workspace；没有 npm CI job 运行它（.github/workflows/ci.yml 只有 check:desktop-isolation 一步）。— 具体代价：如果 desktop 投影回退（例如不再优先使用 displayText，或泄漏 <qwen:user-prompt-submit-context> 标签），这个新测试本可以捕获——但它不在本仓库 CI 中运行，因此除非拥有 packages/desktop 的独立 openwork 流水线运行 bun test，否则回退会未被发现地发布。— 建议修复：确认 desktop 套件在 openwork/desktop CI 流水线中运行；若没有，则在那里添加运行 bun test 的 job，使该覆盖能门禁此改动。
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and locally).
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and locally)。
— qwen3.8-max-preview via Qwen Code /review
| if (record.systemPayload === undefined && hasFinalHookContextPart) { | ||
| return { displayText: undefined, parts: parts.slice(0, -1) }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The tag-strip fallback tests the raw record.systemPayload === undefined, but the metadata branch one line above uses the object-checked payload (isObjectRecord(record.systemPayload) ? record.systemPayload : undefined). A non-object, non-undefined systemPayload (e.g. null) falls through both branches: payload is undefined so displayText is not honored, and record.systemPayload === undefined is false so the final hook-context part is not stripped — the reserved <qwen:user-prompt-submit-context> tag leaks into the clean display projection, contradicting the function's own doc comment ("For tag-only third-party records with no metadata, only that complete final part is removed"). A probe confirmed the output flips from leaky to clean under the fix. — Failure scenario: a third-party- or hand-edited JSONL user record with "systemPayload": null (JSON serializers commonly emit null for absent fields; validateTranscriptRecord spreads ...value through unsanitized) whose final part is a canonical wrapped context block and parts.length > 1 → the raw tag text surfaces in conversation-branches lastUserText, derived session titles, and injected session references. The same pattern is mirrored in packages/webui/src/adapters/userTranscriptDisplay.ts (~L65) and packages/desktop/packages/shared/src/agent/qwen-agent.ts (~L1004); the fix should land in all three.
| if (record.systemPayload === undefined && hasFinalHookContextPart) { | |
| return { displayText: undefined, parts: parts.slice(0, -1) }; | |
| } | |
| if (payload === undefined && hasFinalHookContextPart) { | |
| return { displayText: undefined, parts: parts.slice(0, -1) }; | |
| } |
中文说明
[Suggestion] tag-strip 回退判断的是原始的 record.systemPayload === undefined，而上一行的 metadata 分支使用的是经过对象检查的 payload（isObjectRecord(record.systemPayload) ? record.systemPayload : undefined）。当 systemPayload 是非对象且非 undefined 的值（例如 null）时会同时绕过两个分支：payload 为 undefined，因此 displayText 不被采纳；而 record.systemPayload === undefined 为 false，因此末尾的 hook-context part 不会被移除——保留的 <qwen:user-prompt-submit-context> 标签会泄漏进干净展示投影，与函数自身的文档注释（“对于无 metadata 的仅带标签的第三方记录，只移除完整的末尾 part”）相矛盾。探针确认修复后输出由泄漏变为干净。— 失败场景：第三方或手工编辑的 JSONL user 记录带有 "systemPayload": null（JSON 序列化器常把缺失字段写为 null；validateTranscriptRecord 会原样 ...value 透传而不做清理），且其末尾 part 是规范的带标签上下文块、parts.length > 1 时 → 原始标签文本会出现在 conversation-branches 的 lastUserText、派生的会话标题以及注入的 session reference 中。同样的模式也存在于 packages/webui/src/adapters/userTranscriptDisplay.ts（约 L65）与 packages/desktop/packages/shared/src/agent/qwen-agent.ts（约 L1004）；修复应同步到这三处。
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and by this review's focused suite runs). Test Plan (not a blocker): src/core/client.test.ts — no such file or directory; src/core/prompts.test.ts — no such file or directory; src/services/chatRecordingService.test.ts — no such file or directory; src/utils/transcript-records.test.ts — no such file or directory; src/hooks/user-prompt-submit-context.test.ts — no such file or directory; and 12 more.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by CI's linux Test job and by this review's focused suite runs)。 Test Plan（非阻断）：src/core/client.test.ts — no such file or directory; src/core/prompts.test.ts — no such file or directory; src/services/chatRecordingService.test.ts — no such file or directory; src/utils/transcript-records.test.ts — no such file or directory; src/hooks/user-prompt-submit-context.test.ts — no such file or directory; and 12 more。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| if (!replacedText && displayText.length > 0) { | ||
| projected.push({ text: displayText }); | ||
| } | ||
| replacedText = true; |
There was a problem hiding this comment.
[Suggestion] The test-efficacy probe (mutant analysis, harness validated) found this in-loop replacement guard's false side is unexercised: forcing !replacedText && displayText.length > 0 to true leaves every test in transcript-replay.test.ts green. No added test feeds a record with multiple text parts, where replacedText is already true when a later text part is reached. — Concrete cost: a regression that injects displayText into every text part (dropping the !replacedText half) would pass the whole added suite while duplicating display text across multi-part user records. Fix: add a replaceTextPartsForDisplay case with two text parts asserting the replacement happens exactly once, at the first text position.
中文说明
[Suggestion] 测试有效性探针（变异分析，harness 已验证）发现这个循环内替换分支的“假”侧没有任何测试覆盖：将 !replacedText && displayText.length > 0 强制为 true，transcript-replay.test.ts 中所有测试仍然通过。新增测试没有喂入含多个 text part 的记录（即后续 text part 到达时 replacedText 已为 true）。— 具体代价：一个把 displayText 注入到每个 text part（去掉 !replacedText 这一半）的回退，会让整个新增套件全部通过，同时在多 part 用户记录里重复插入展示文本。修复：补一个含两个 text part 的 replaceTextPartsForDisplay 用例，断言替换只在第一个 text 位置发生一次。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| if (!replacedText && displayText.length > 0) { | ||
| projected.push({ text: displayText }); | ||
| } | ||
| return projected; |
There was a problem hiding this comment.
[Suggestion] Same probe: this trailing-append guard's false side is unexercised — no test covers the path where no text part was replaced (replacedText stays false) while displayText is empty. Forcing the condition true leaves every test green. — Concrete cost: a regression that always appends { text: displayText } (e.g. a spurious empty { text: '' } part when displayText is empty) would not fail any test in this diff. Fix: add a case with empty displayText and no text parts asserting no extra part is appended.
中文说明
[Suggestion] 同一探针：这个末尾追加分支的“假”侧没有被覆盖——没有测试覆盖“没有 text part 被替换（replacedText 保持 false）且 displayText 为空”的路径。将该条件强制为 true，所有测试仍通过。— 具体代价：一个总是追加 { text: displayText } 的回退（例如 displayText 为空时追加多余的空 { text: '' } part）不会让本 diff 的任何测试失败。修复：补一个“空 displayText + 无 text part”的用例，断言不追加多余 part。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #7948 does not currently have merge conflicts with main. |
|
Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by this review's workspace runs). Not reviewed: build-and-test — packages/desktop bun workspace: excluded from the npm workspace graph and no CI job collects packages/desktop/packages/shared/**; bun unavailable on this runner, so the desktop projection mirror was verified by static trace and extracted-code probes only. Test Plan (not a blocker): src/core/client.test.ts — no such file or directory; src/core/prompts.test.ts — no such file or directory; src/services/chatRecordingService.test.ts — no such file or directory; src/utils/transcript-records.test.ts — no such file or directory; src/hooks/user-prompt-submit-context.test.ts — no such file or directory; and 13 more.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the unit-test ground was covered by this review's workspace runs)。 未审查：build-and-test — packages/desktop bun workspace: excluded from the npm workspace graph and no CI job collects packages/desktop/packages/shared/**; bun unavailable on this runner, so the desktop projection mirror was verified by static trace and extracted-code probes only。 Test Plan（非阻断）：src/core/client.test.ts — no such file or directory; src/core/prompts.test.ts — no such file or directory; src/services/chatRecordingService.test.ts — no such file or directory; src/utils/transcript-records.test.ts — no such file or directory; src/hooks/user-prompt-submit-context.test.ts — no such file or directory; and 13 more。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| it('stores hook display provenance in systemPayload only when provided', async () => { | ||
| const taggedParts: Part[] = [ | ||
| { text: 'my prompt' }, | ||
| it('preserves model-bound parts and records clean display text', async () => { |
There was a problem hiding this comment.
[Suggestion] The rewritten test deleted the only assertion pinning "no systemPayload when no prompt payload is provided" (expect(plain.systemPayload).toBeUndefined() from the old stores hook display provenance in systemPayload only when provided test); no remaining test covers the absent branch of ...(promptPayload ? { systemPayload: promptPayload } : {}) in recordUserMessage. — Failure scenario: a future regression that records a systemPayload for plain, hook-free prompts (the conditional spread dropped, or payload construction hoisted out of the additionalContext guard) passes CI silently; downstream that flips projectUserTranscriptForDisplay off its tag-strip branch (payload === undefined is required), so metadata-free records with a trailing tagged part keep the raw <qwen:user-prompt-submit-context> block in every display consumer (resume, ACP replay, insights, webui, vscode, desktop). A review probe confirmed the class: forcing systemPayload: {} on payload-free records passed all 70 tests in this file; restoring the deleted assertion caught it. — Suggested fix: keep a negative case:
chatRecordingService.recordUserMessage([{ text: 'plain prompt' }]);
await chatRecordingService.flush();
expect((vi.mocked(jsonl.writeLine).mock.calls.at(-1)![1] as ChatRecord).systemPayload).toBeUndefined();中文说明
重写后的测试删除了唯一钉住「未提供 prompt payload 时不写入 systemPayload」的断言（旧测试 stores hook display provenance in systemPayload only when provided 中的 expect(plain.systemPayload).toBeUndefined()）；recordUserMessage 中 ...(promptPayload ? { systemPayload: promptPayload } : {}) 的缺省分支不再有任何测试覆盖。— 失败场景：未来某个回归让普通无 Hook 的 prompt 也写入 systemPayload（例如条件展开被删、payload 构造被提出 additionalContext 判断之外），CI 会静默通过；下游这会使 projectUserTranscriptForDisplay 偏离 tag-strip 分支（该分支要求 payload === undefined），导致带尾部标签 part 的无 metadata 记录在所有展示消费端（resume、ACP replay、insights、webui、vscode、desktop）保留原始 <qwen:user-prompt-submit-context> 块。评审探针确认了该回归类：对无 payload 记录强制写入 systemPayload: {} 后，本文件全部 70 个测试仍然通过；恢复被删断言后则能捕获。— 建议修复：保留一个负向用例（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| return typeof value === 'string' ? value : undefined; | ||
| } | ||
|
|
||
| // Keep these in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE in Qwen core. |
There was a problem hiding this comment.
[Suggestion] The sync comment names only the two tag constants, but the obligation covers the entire mirrored projection algorithm — isQwenUserPromptContextPart duplicates core's isUserPromptSubmitContextPartText (trim semantics, \n-padded prefix/suffix, body tag-rejection), and projectQwenUserRecordText duplicates projectUserTranscriptForDisplay's decision table (parts.length > 1 guard, hookContext-or-final-tag pairing evidence, authoritative-displayText rule, payload === undefined strip fallback). Desktop has no core dependency, so the duplication is structurally forced — but the comment understates the sync surface. — Failure scenario: a future core change to projection semantics (new tag variant, changed pairing-evidence rule, changed fallback) lands without a matching desktop edit; nothing in either workspace flags the divergence; desktop then renders the raw <qwen:user-prompt-submit-context> block in the transcript UI or drops user-visible text while CLI/VS Code/webui render correctly — desktop's own test asserts its own frozen copy, so both workspaces stay green.
| // Keep these in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE in Qwen core. | |
| // Keep the tag constants and the projection semantics (isQwenUserPromptContextPart / projectQwenUserRecordText) in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE, isUserPromptSubmitContextPartText, and projectUserTranscriptForDisplay in Qwen core. |
中文说明
同步注释只点名了两个标签常量，但同步义务实际覆盖整个镜像的投影算法——isQwenUserPromptContextPart 复制了 core 的 isUserPromptSubmitContextPartText（trim 语义、带 \n 的前/后缀、body 内标签拒绝），projectQwenUserRecordText 复制了 projectUserTranscriptForDisplay 的决策表（parts.length > 1 守卫、hookContext 或末尾标签 part 的配对证据、权威的 displayText 规则、payload === undefined 的仅剥离回退）。desktop 不依赖 core，重复不可避免——但注释低估了同步面。— 失败场景：未来 core 投影语义变更（新标签变体、配对证据规则或回退策略变化）落地而 desktop 未同步；两个 workspace 都没有任何机制发现分歧；desktop 将在历史界面渲染原始 <qwen:user-prompt-submit-context> 块或丢失用户可见文本，而 CLI/VS Code/webui 渲染正常——desktop 自己的测试只断言自己的冻结副本，两个 workspace 都会保持绿色。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| // Keep these in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE in core. |
There was a problem hiding this comment.
[Suggestion] Same hazard as the desktop copy: this webui adapter is a third full implementation of core's projection algorithm (isHookContextPart mirrors isUserPromptSubmitContextPartText; getUserTranscriptDisplayText mirrors projectUserTranscriptForDisplay's decision table), but the sync comment names only the two tag constants. Webui imports only @qwen-code/sdk, so the duplication is structurally forced — but nothing keeps the copies aligned. — Failure scenario: a future core change to projection semantics lands without a matching webui edit; no test in either package flags it; webui then renders the raw <qwen:user-prompt-submit-context> block in the ChatViewer or drops user-visible text while CLI/VS Code render correctly. If the desktop comment fix lands without this one, this copy stays silently divergent.
| // Keep these in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE in core. | |
| // Keep the tag constants and the projection semantics (isHookContextPart / getUserTranscriptDisplayText) in sync with USER_PROMPT_SUBMIT_CONTEXT_OPEN/CLOSE, isUserPromptSubmitContextPartText, and projectUserTranscriptForDisplay in core. |
中文说明
与 desktop 副本相同的隐患：该 webui 适配器是 core 投影算法的第三份完整实现（isHookContextPart 镜像 isUserPromptSubmitContextPartText；getUserTranscriptDisplayText 镜像 projectUserTranscriptForDisplay 的决策表），但同步注释只点名了两个标签常量。webui 只依赖 @qwen-code/sdk，重复不可避免——但没有任何机制保持副本一致。— 失败场景：未来 core 投影语义变更落地而 webui 未同步；两个包的测试都不会报警；webui 将在 ChatViewer 中渲染原始 <qwen:user-prompt-submit-context> 块或丢失用户可见文本，而 CLI/VS Code 渲染正常。如果只修 desktop 注释而不修这里，这份副本会静默失同步。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| cron, and mid-turn records can also have `displayText`, but those values are | ||
| compact display labels and must not be substituted for their model-bound text | ||
| without that evidence. |
There was a problem hiding this comment.
[Suggestion] This sentence is ambiguous and contradicts first-party behavior: notification/cron/mid-turn records can never carry the pairing evidence defined above (hooks never fire for Cron/Notification — verified in client.ts's hook gate), yet ACP replay and CLI resume substitute their displayText label unconditionally, while webui, insight DataProcessor, and session-reference keep model text. "Substitute X for Y" is also a classically inverted construction, so both readings are available to an implementer. — Failure scenario: a third-party/offline consumer (this block's stated audience) implementing the rule as written never sees evidence on cron/notification records, so it renders their model-bound parts — the raw <task-notification /> envelope or the full internal cron prompt — where first-party surfaces show the compact label; the opposite reading contradicts webui's policy. Either way, following the spec diverges from at least one first-party surface. — Suggested fix: separate the two regimes.
| cron, and mid-turn records can also have `displayText`, but those values are | |
| compact display labels and must not be substituted for their model-bound text | |
| without that evidence. | |
| cron, and mid-turn records can also have `displayText`, but those values are | |
| compact display labels. Subtype-aware consumers render those records using the | |
| display label; the pairing-evidence rule above governs only user-prompt records. |
中文说明
该句存在歧义且与第一方行为矛盾：notification/cron/mid-turn 记录永远不可能携带上文定义的配对证据（client.ts 的 Hook 门控已核实 Cron/Notification 从不触发 Hook），但 ACP replay 与 CLI resume 会无条件用 displayText 标签替换正文，而 webui、insight DataProcessor、session-reference 保留模型正文。"substitute X for Y" 又是经典的可反向解读结构，实现者两种读法都成立。— 失败场景：按本段目标读者（第三方/离线消费者）照字面实现规则时，cron/notification 记录永远没有证据，于是渲染其模型正文——原始 <task-notification /> 封装或完整 cron 内部 prompt——而第一方界面显示的是紧凑标签；反向读法又与 webui 策略矛盾。无论哪种读法，照规范实现都会与至少一个第一方界面不一致。— 建议修复：把两种机制分开表述（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.21.7)
|
|
||
| # Core Mandates | ||
|
|
||
| - **UserPromptSubmit Context:** Text inside a \`<qwen:user-prompt-submit-context>\` tag is model context added by a configured \`UserPromptSubmit\` hook, not user input. |
There was a problem hiding this comment.
[Suggestion] The new Core Mandate hardcodes the tag literal instead of deriving it from USER_PROMPT_SUBMIT_CONTEXT_OPEN in utils/transcript-records.ts; prompts.test.ts:82 asserts the same literal and every snapshot bakes it in — a literal-against-literal island. — Failure scenario: a future tag rename follows the constants everywhere (wrapping, projection, tests) except the system prompt; the shipped test stays green while the model receives hook-injected context inside the new tag with no mandate identifying it as non-user input, so a malicious UserPromptSubmit hook's additionalContext can be treated as the user's own instructions — the exact misattribution this PR exists to prevent. A review probe confirmed it: renaming only the two constants left prompts.test.ts green while the prompt named a tag the writer no longer writes. — Suggested fix: import the constant and interpolate:
import { USER_PROMPT_SUBMIT_CONTEXT_OPEN } from '../utils/transcript-records.js';
// in the mandate template:
- **UserPromptSubmit Context:** Text inside a \`${USER_PROMPT_SUBMIT_CONTEXT_OPEN}\` tag is model context added by a configured \`UserPromptSubmit\` hook, not user input.(or at minimum have prompts.test.ts assert the sentence constructed from the constant, and update snapshots)
中文说明
新增的 Core Mandate 把标签字面量硬编码，而不是从 utils/transcript-records.ts 的 USER_PROMPT_SUBMIT_CONTEXT_OPEN 派生；prompts.test.ts:82 断言的也是同一字面量，所有快照同样固化了它——形成一个「字面量对字面量」的孤岛。— 失败场景：未来重命名标签时，常量驱动的各处（包裹、投影、测试）都会跟随，唯独系统提示词不跟随；现有测试仍然绿色，但模型会收到包在新标签里的 Hook 注入内容，而提示词中没有任何条款说明它不是用户输入——恶意 UserPromptSubmit Hook 的 additionalContext 就可能被当作用户本人的指令，正是本 PR 要防止的错误归因。评审探针已确认：只重命名两个常量，prompts.test.ts 依然通过，而提示词描述的标签已不是写入端实际使用的标签。— 建议修复：导入常量并插值（见上方代码块），或至少让 prompts.test.ts 断言由常量构造出的句子并更新快照。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| USER_PROMPT_SUBMIT_CONTEXT_CLOSE_TAG, | ||
| wrapUserPromptSubmitContext, | ||
| isUserPromptSubmitContextPartText, | ||
| stripTrailingUserPromptSubmitContextPart, | ||
| } from './hooks/user-prompt-submit-context.js'; |
There was a problem hiding this comment.
[Suggestion] The rewires in this PR removed the last production consumer of stripTrailingUserPromptSubmitContextPart (resume history and ACP replay now route through projectUserTranscriptForDisplay), but the function remains publicly exported here and via the ./userPromptSubmitContext subpath with zero production callers. Worse, its decision table already differs from the wired projection today: the exported strip removes a trailing tagged part by part shape alone, while the projection strips only when systemPayload is absent — a record with any payload object plus a trailing tag is stripped by the export but kept raw by every wired display surface. — Failure scenario: a future change to the strip policy lands in projectUserTranscriptForDisplay (which every consumer calls) while the exported duplicate and its passing tests silently diverge; the green tests give false confidence that strip behavior is still covered, and a new caller (e.g. an extension author) importing the public variant gets semantics matching no wired consumer. — Suggested fix: delete the helper, this export, and its describe block in user-prompt-submit-context.test.ts — or, if deliberately kept as public API, add a comment naming that contract and reconcile its strip gate with the projection's.
中文说明
本 PR 的改造移除了 stripTrailingUserPromptSubmitContextPart 的最后一批生产消费端（resume history 与 ACP replay 现在都经过 projectUserTranscriptForDisplay），但该函数仍通过此处 barrel 及 ./userPromptSubmitContext 子路径公开导出，且没有任何生产调用者。更糟的是它的决策表与线上投影今天就已不同：导出版仅凭 part 形状删除尾部标签 part，而投影只在 systemPayload 缺失时才剥离——带任意 payload 对象且有尾部标签的记录，导出版会剥离，而所有线上展示面都会保留原文。— 失败场景：未来剥离策略的修改落在 projectUserTranscriptForDisplay（所有消费端实际调用的函数）上，而这份公开副本和它通过的测试静默失同步；绿色测试制造「剥离行为仍有覆盖」的假象，新调用者（如扩展作者）导入这个公开变体得到的语义与任何线上消费端都不一致。— 建议修复：删除该辅助函数、此导出以及 user-prompt-submit-context.test.ts 中对应的 describe 块——或者，若确有意保留为公开 API，请加注释说明该契约，并让其剥离条件与投影保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| .flatMap((record) => record.message?.parts ?? []) | ||
| .flatMap((record) => { | ||
| if (type !== 'user') return record.message?.parts ?? []; | ||
| const projection = projectUserTranscriptForDisplay(record); |
There was a problem hiding this comment.
[Suggestion] The changed extractText fallback branch (projection.parts — tag-strip and legacy passthrough) has no test at the conversation-branches level; the only new integration test covers the displayText branch (its record carries a full payload). — Failure scenario: a future edit that keeps the displayText branch but reverts the fallback to record.message?.parts ?? [] passes every test in this file — a review probe confirmed it: with the regression in place the full suite still passed 21/21 while the raw <qwen:user-prompt-submit-context> block leaked into firstUserTextAfterBranchPoint/lastUserText branch summaries shown to users, for payload-less records with a trailing tag part (the third-party shape the projection's docstring explicitly designs for). — Suggested fix: add a case with a user record whose parts are [{text: 'user text'}, {text: '<qwen:user-prompt-submit-context>\nhook-only context\n</qwen:user-prompt-submit-context>'}] and no systemPayload, asserting the branch summaries equal 'user text' and contain no tag content.
中文说明
extractText 改动的回退分支（projection.parts——标签剥离与旧式直通路径）在 conversation-branches 层级没有任何测试；唯一的新集成测试只覆盖 displayText 分支（其记录带完整 payload）。— 失败场景：未来某次修改保留了 displayText 分支、却把回退改回 record.message?.parts ?? []，本文件所有测试仍会通过——评审探针已确认：回归存在时完整套件仍 21/21 通过，而对带尾部标签 part 的无 payload 记录（投影 docstring 明确支持的第三方形状），原始 <qwen:user-prompt-submit-context> 块会泄漏进展示给用户的分支摘要 firstUserTextAfterBranchPoint/lastUserText。— 建议修复：新增一个用例，用户记录 parts 为 [{text: 'user text'}, {text: '<qwen:user-prompt-submit-context>\nhook-only context\n</qwen:user-prompt-submit-context>'}] 且无 systemPayload，断言分支摘要等于 'user text' 且不含标签内容。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const content: string[] = []; | ||
| for (const part of record.message?.parts ?? []) { | ||
| const parts = projection?.parts ?? record.message?.parts ?? []; |
There was a problem hiding this comment.
[Suggestion] The projection fallback in evidenceContent (and the independently wired twin in evidencePreview below) is untested at the goal-evidence level — the new goal tests cover only the displayText branch and the mid-turn passthrough. — Failure scenario: a future edit that keeps the displayText branch but reverts this fallback to record.message?.parts ?? [] passes every test in this file — a review probe confirmed it: with the regression applied to both functions the suite still passed 18/18, while for a payload-less real_user record with a trailing complete tag part the raw <qwen:user-prompt-submit-context> block flowed into cited content delivered to the Goal verifier as user_input-kind evidence, contradicting the Core Mandate this PR adds. — Suggested fix: add a goal-evidence test for a real_user record with no systemPayload and a trailing tagged part, asserting validated content and catalog preview equal the user text and contain no tag block.
中文说明
evidenceContent 中的投影回退（以及下方 evidencePreview 中独立接线的孪生分支）在 goal-evidence 层级没有测试——新增的 goal 测试只覆盖 displayText 分支与 mid-turn 直通。— 失败场景：未来某次修改保留 displayText 分支、却把该回退改回 record.message?.parts ?? []，本文件所有测试仍会通过——评审探针已确认：对两个函数同时施加该回归后套件仍 18/18 通过，而对带完整尾部标签 part 的无 payload real_user 记录，原始 <qwen:user-prompt-submit-context> 块会流入作为 user_input 类证据交给 Goal verifier 的引用内容，与本 PR 新增的 Core Mandate 相悖。— 建议修复：新增 goal-evidence 测试：无 systemPayload、带尾部标签 part 的 real_user 记录，断言校验内容与 catalog preview 等于用户文本且不含标签块。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const parts = projection?.parts ?? record.message?.parts ?? []; | ||
| for (const part of parts) { | ||
| if (part.thought !== true && typeof part.text === 'string') { | ||
| append(part.text); |
There was a problem hiding this comment.
[Suggestion] Same untested projection fallback as in evidenceContent above — evidencePreview is wired independently, so a partial revert can break one without the other; the existing goal tests pin neither fallback (a review probe reverted both and the suite still passed 18/18). — Failure scenario: a revert of just this fallback leaks the raw <qwen:user-prompt-submit-context> block into the catalog preview shown to the Goal worker for payload-less records with a trailing tag part. — Suggested fix: the same single test (payload-less tagged real_user record) can pin both functions by asserting on both the validated content and the preview.
中文说明
与上方 evidenceContent 相同的未测试投影回退——evidencePreview 是独立接线的，因此部分回退可能只破坏其中一个而另一个正常；现有 goal 测试对两个回退都没有钉住（评审探针把两者同时回退后套件仍 18/18 通过）。— 失败场景：只回退这一处，就会让带尾部标签 part 的无 payload 记录在展示给 Goal worker 的 catalog preview 中泄漏原始 <qwen:user-prompt-submit-context> 块。— 建议修复：同一个测试（无 payload、带标签 part 的 real_user 记录）可以同时钉住两个函数——同时断言校验内容与 preview。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| projection carried by `submitted_prompt`; ACP, headless, `serve`, SDK, remote | ||
| input, and other paths without that provenance record the expanded pre-hook | ||
| prompt instead. |
There was a problem hiding this comment.
[Suggestion] This overclaims the ACP/serve/SDK record shape. The spec says every UserQuery with added hook context preserves the tagged part and adds {displayText, hookContext}, but ACP-session turns record the raw pre-expansion input BEFORE the hook fires (Session.ts:3178, the only recordUserMessage call in that path) and send through GeminiChat.sendMessageStream directly, bypassing GeminiClient.sendMessageStream — the only place userPromptRecordPayload is written. Verified at the reviewed commit: headless genuinely matches the claim, TUI matches via submitted_prompt, but an ACP/serve/SDK JSONL user record has neither systemPayload nor tagged part. — Failure scenario: in a serve/ACP/SDK session whose hook injects additionalContext, the model sees the hook context but the JSONL record holds only the raw input text, while this spec tells offline/third-party consumers both are present — a consumer built on the spec finds no provenance on ACP/serve-originated sessions and misattributes or discards those turns. — Suggested fix: scope the sentences to the surfaces that write the payload, or extend the ACP path to write it.
| projection carried by `submitted_prompt`; ACP, headless, `serve`, SDK, remote | |
| input, and other paths without that provenance record the expanded pre-hook | |
| prompt instead. | |
| projection carried by `submitted_prompt`, and headless runs record the | |
| expanded pre-hook prompt the same way. ACP, `serve`, and SDK turns record the | |
| raw pre-hook input before the hook fires and carry no display provenance. |
中文说明
此处对 ACP/serve/SDK 的记录形状表述过度。规范声称每个带 Hook 上下文的 UserQuery 都会保留标签 part 并附加 {displayText, hookContext}，但 ACP 会话轮次在 Hook 触发之前就已记录原始未展开输入（Session.ts:3178，该路径唯一的 recordUserMessage 调用），且直接经 GeminiChat.sendMessageStream 发送，绕过了 GeminiClient.sendMessageStream——写入 userPromptRecordPayload 的唯一位置。已在被审提交上核实：headless 与描述相符，TUI 通过 submitted_prompt 相符，但 ACP/serve/SDK 的 JSONL 用户记录既无 systemPayload 也无标签 part。— 失败场景：在 Hook 注入 additionalContext 的 serve/ACP/SDK 会话中，模型看到了 Hook 上下文，但 JSONL 记录只有原始输入文本，而本规范告诉离线/第三方消费者两者都存在——按规范实现的消费者在 ACP/serve 来源的会话中找不到任何来源信息，会错误归因或直接丢弃这些轮次。— 建议修复：把表述限定到实际写入 payload 的界面，或扩展 ACP 路径使其写入 payload（见上方 suggestion，为前一种方案）。
— qwen3.8-max via Qwen Code /review (v0.21.7)
Local verification report — PR #7948Verified on a real local environment: both arms built from source and driven through the real Verdict: LGTM — recommend merge. The fix is real, reproducible, and backward compatible. One pre-existing residual gap is noted below (not a regression; suggested as a follow-up). Environment
1. Recorded JSONL — the hook fired for real, model history is unchangedThe hook's stdin capture confirms a genuine
2. Consumer matrix — this is what the PR actually buysOne real recorded session, replayed through each consumer's real built module in each arm.
Seven surfaces are fixed. Three were already clean on the merge-base ( The sharpest one is The same polluted title showed up in the VS Code panel ( Backward compatibility confirmed: PR-arm consumers reading merge-base-written records ( 3. Real TUI resumeSession submitted through the interactive TUI (so
4. Cross-copy consistencyThe projection logic exists in four places: core ( All copies agree on every decision in all 12 cases. The only differences are each product's pre-existing join separator ( Also worth noting: the new matcher is stricter than the one it replaces. Given a forged block whose body contains a nested open tag, the merge-base matcher returns 5. Test suites and repo health
For the desktop suite I verified the shipped code path instead:
6. FindingsF1 — Residual gap (pre-existing, not a regression): two
Both are F2 — Nit: four copies of the tag constants with no sync test. The F3 — Note, intentional: F4 — Note: 中文完整版本地验证报告 — PR #7948在真实本地环境完成验证：两个 arm 均从源码构建，通过真实 结论：LGTM，建议合入。 修复真实、可复现、且向后兼容。下面记录了一个既有的遗留缺口（不是本 PR 引入的回归，建议作为 follow-up）。 环境
1. 记录的 JSONL —— hook 真实触发，模型历史保持不变hook 的 stdin 捕获证明这是一次真实的 两个 arm 的 2. 消费端矩阵 —— 这才是本 PR 的真正价值同一个真实录制的会话，分别送进两个 arm 各自真实构建出的消费端模块。 七个消费端被修复。三个在 merge-base 上本来就是干净的（ 其中最严重的是 同样被污染的标题也出现在 VS Code 面板里：merge-base 上 向后兼容已确认： 用 PR arm 的消费端去读 merge-base 写出的记录（只有 3. 真实 TUI Resume先通过交互式 TUI 提交（这样才会走到 4. 多份拷贝的一致性投影逻辑一共存在于四处：core（ 12 个用例中所有拷贝的判定完全一致。 唯一的差异是各产品原有的拼接分隔符（ 另外值得一提：新的匹配器比它替换掉的那个更严格。对于 body 中含有嵌套 open tag 的伪造块，merge-base 的匹配器会返回 5. 测试套件与仓库健康度
desktop 套件我改用另一种方式验证了实际发布的代码路径：把 把 PR head 合并进当前的 6. 发现的问题F1 —— 遗留缺口（既有问题，非本 PR 回归）：两个
两处都是 F2 —— Nit：tag 常量有四份拷贝且没有同步测试。 目前唯一防止漂移的只有 F3 —— 说明（属于有意设计）： F4 —— 说明：只有 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 118 passed · 0 failed · 118 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：118 通过 · 0 失败 · 118 总计 Verification reportPR 7948 Deep Verification —
|
| cell | environment | observable oracle | result |
|---|---|---|---|
| head writer | HEAD dist CLI | recorded user record | parts ["user prompt", TAGGED]; systemPayload {displayText:"user prompt", hookContext:"hook-only context"} |
| base writer (control) | base-tree dist (3edecac, rebuilt, realpath-verified) |
recorded user record | parts identical; systemPayload {displayText:"user prompt"} — hookContext absent (predicted pre-PR shape) |
| model-bound, both arms | fake server request capture | last user message parts | byte-identical across arms after scratch-path normalization: ["user prompt", TAGGED] |
17/17 scripted writer assertions passed (05-writer-assertions.mjs), including the control cell asserting base lacks hookContext.
Consumer flip matrix (real consumers, both arms — 02-consumer-ab.mjs)
9 fixtures × 5 consumers. Leak-shape fixtures (F1 paired, F2 displayText-only era, F3 tag-only, F5 empty displayText, F8 image+text):
| consumer | base leaks (of 5) | head leaks (of 5) | flip |
|---|---|---|---|
CLI insight DataProcessor (real dist) |
5/5 | 0/5 | 5 fixed |
WebUI adaptJSONLMessages (real source) |
5/5 | 0/5 | 5 fixed |
VS Code record text (head: qwenRecordToText; base: faithful replica of deleted contentToText) |
5/5 | 0/5 | 5 fixed |
| ACP transcript replay (real dist) | 0/5 | 0/5 | already clean on base (first-round fix); documented divergences below |
core projectUserTranscriptForDisplay |
absent on base (new) | 9/9 compat cells as designed | — |
Head-arm compat details, all asserted: empty displayText is authoritative ("", no fallback to parts); user-authored tag-like prose is never matched; a sole tagged part with no metadata is preserved; legacy bare-injection records keep their text; image parts survive alongside displayText. Base control cells all reproduced the predicted leak/consumers-absent behavior (36/36 base assertions passed).
Documented intentional divergences (not regressions): on F5 (synthetic empty displayText + non-empty parts) base ACP shows the stripped parts while head shows nothing — "displayText is authoritative even when empty" is pinned by the PR's own unit test and the shape is not writer-producible; both arms preserve a single tagged part (F7) via the same length <= 1 guard lineage.
Witness for both tables: 01-writer-consumer-ab-head-vs-base.png.
Corrections
None required (no prior-round report present; no inaccurate bot claims in scope).
Findings
Ordered by severity. No blocking findings.
1. Two projection guards survive mutation — coverage gaps, behavior verified correct (Info, non-blocking)
Mutation matrix: 11 mutants over the PR's production hunks, including one positive control (PC1, whole projection removed); baselines green (17/17, 318/318, 70/70); 9/11 killed, incl. the positive control (4 tests red), proving the harness and suite are live (02-mutation-matrix.png).
| mutant | guard | transcript-records suite | cli+acp consumer suites (alias core src) | classification |
|---|---|---|---|---|
M1 drop parts.length > 1 |
sole tagged part preserved | killed | — | pinned |
| M2 drop final-tag pairing evidence | displayText-only era pairing |
killed | — | pinned |
M3 drop payload === undefined on tag-only strip |
strip only metadata-free records | survived | survived | coverage gap |
| M4 drop nested-tag body check | forged/nested tag rejection | survived | survived | coverage gap |
M5 empty displayText truthy fallback |
empty-displayText authority | killed | — | pinned |
M6 keep text parts with displayText |
text-part omission | killed (2 tests) | — | pinned |
M7 matcher without \n shape |
exact wrapped shape | killed | — | pinned |
| PC1 projection passthrough | positive control | killed (4 tests) | — | live |
| C1 telemetry post-hook prompt | pre-hook span attribute | killed (2 tests) | — | pinned |
| C2 memory recall post-hook prompt | pre-hook recall | killed (2 tests) | — | pinned |
| R1 drop systemPayload persistence | provenance written to JSONL | killed (2 tests) | — | pinned |
Survivor classification (both are coverage gaps, not dead code and not defects):
- M3: the clause decides outcomes for records carrying a non-user-prompt
systemPayload(e.g.{}) plus a complete final tagged part — head keeps the tagged part visible (driven through the real dist:{parts:[user, tagged], displayText: undefined}, conservative keep); the mutant would strip it. The writer never produces that shape, and the conservative direction matches the documented rule ("metadata-free records" only), but nothing asserts it. - M4: the body re-tag check rejects forged parts like
OPEN…OPEN…CLOSE…CLOSE. Unreachable via this writer (getAdditionalContext()escapes</>— verified intypes.ts), reachable only via hand-written third-party transcripts; no fixture exercises it.
Completeness reporting only — per the matrix both guards are correct and conservative; suggested follow-up fixtures (a systemPayload: {} + trailing-tag record; a nested-tag forged part) are named in the table above.
2. Tag matcher exists in three copies (Info, non-blocking)
Core holds the canonical matcher (utils/transcript-records.ts, Node-free subpath used by acp-bridge); WebUI (adapters/userTranscriptDisplay.ts) and Desktop (qwen-agent.ts) each carry a hand-copied matcher under "keep in sync" comments. This is the PR's disclosed tradeoff (browser bundles avoid the core barrel). I verified the WebUI copy behaviorally through the consumer matrix (F6/F7 semantics identical to core) and the Desktop copy by inspection (logic byte-equivalent); the Desktop suite that pins it could not run here (see Not covered). Drift risk is the residual cost; no action required for this PR.
Not covered
- Desktop bun suite (
qwen-agent-slash-history.test.ts): not runnable in this container — the desktop bun workspace is not installed (Cannot find module 'date-fns/locale/en-US'). Proven environmental via A/A control: the identical command on the base tree fails with the identical error. The Desktop consumer change is therefore covered only by inspection + the identical-logic WebUI arm. - Live TUI resume rendering (reviewer plan step 4's CLI leg): projection logic covered by the
resumeHistoryUtilsgate (154-test cli suite incl. the PR's new fixtures) and the consumer matrix; no interactive TUI session was driven. - Per-commit attribution: the depth-2 checkout exposes only the merge commit locally (
git rev-list HEAD^1..HEAD^2= 1) while the metadata lists 22 commits — verification is of the aggregateHEAD^1..HEADdiff. - Full integration lanes (
test:integration:no-ak:sandbox:none, credentialed suites) and repo-wide test sweep: out of scope; PR CI covers them. The focused suites run here are the ones the diff touches. - Windows/Linux-specific paths: nothing OS-specific in the diff; N/A in container either way.
- The ACP-replay arm's F5 divergence from base (see table) is a designed semantic change, not covered by a dedicated E2E — pinned by the PR's unit test
uses display metadata even when the display text is empty.
Methodology
One container (node:22-bookworm, Node v22.23.2), PR code executed freely. Writer A/B: 01-writer-ab.mjs drove each tree's compiled CLI headlessly against an in-harness fake OpenAI server capturing every request body; hooks configured via project settings.json under a scratch QWEN_HOME; JSONL located under $QWEN_HOME/projects/<slug>/chats/. Base control: git worktree at HEAD^1 (3edecac) rebuilt (core, cli, acp-bridge) against the shared root node_modules (PR leaves lockfile untouched — dependency-pure control); internal @qwen-code/* links re-pointed via a sparse tmp/base-tree/node_modules overlay, and every harness dependency realpath asserted into the base tree before trust (readlink -f → tmp/base-tree/packages/core|acp-bridge). Consumer A/B: 02-consumer-ab.mjs (tsx) fed 9 fixtures through real dist/source consumers per arm with containment oracles; base VS Code arm uses a faithful replica of the deleted private contentToText (byte-identical at base in both deleted copies). Mutation matrix: 03-mutation-matrix.mjs applied/restored single-point mutants around vitest runs; survivors re-tested against aliased consumer suites (03b-survivor-consumers.mjs) and driven through the real dist. Gates: focused suites per package + root typecheck. Raw logs per cell live in logs/; harness scripts in this directory.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. Verified against merged #7956: this is its documented follow-up rollout, not a duplicate — main only has the single-field {displayText?} payload; the hookContext field, the trust-rule hardening (bare displayText no longer trusted without pairing evidence, empty displayText authoritative, strict non-nesting tag matcher), and the consumer rollout (insight DataProcessor, session-reference-service, goal-evidence, conversation-branches, webui, vscode, desktop, Core Mandates tag instruction) are genuinely new and absent from main. Context still reaches the model (tagged part appended to request, recorded verbatim); resume/replay lose/duplicate nothing; tags can't be forged (angle brackets escaped). CI green on current head (ubuntu Test, Desktop Shell, web-shell E2E all success). Non-blocking P2/P3: background-agent-resume initialPrompt still reads raw model-bound parts (pre-existing gap, route through the projection in a follow-up); displayText semantics shift (submittedPrompt preferred, empty=authoritative) is a user-visible change worth a release note; tag matcher now duplicated 3x across core/desktop/webui (drift risk — consider a shared Node-free subpath); web-shell transcriptToMessages.ts still has no projection handling (file a follow-up).
|
Relationship to #7956 (for future readers): this is NOT a duplicate — it is the documented follow-up to the already-merged #7956, which introduced the qwen:user-prompt-submit-context tag mechanism, a single-field {displayText?} payload, and the clean projection only for CLI resume + ACP replay. #7956's own design doc deferred the remaining consumers to follow-ups. What this PR adds on top of #7956 (all verified absent from main):
So it does not re-solve the original issue — #7956 already solved the core separation; this completes the consumer rollout and hardens the schema/semantics. ~1200 of the ~2000 added lines are tests covering that rollout. |
|
Released in v0.21.8. |
6 participants
What this PR does
This PR separates model-facing UserPromptSubmit hook context from user-visible transcript text. It wraps the sanitized hook context in a dedicated <qwen:user-prompt-submit-context> part, persists clean displayText and hookContext provenance while retaining the exact model-bound message parts, and makes transcript consumers use the clean projection across CLI resume, insight generation, session references, goal evidence, conversation branches, ACP replay, WebUI, VS Code, and Desktop. Telemetry and automatic-memory recall now use the pre-hook user prompt instead of the previous post-hook prompt, so UserPromptSubmit-added context is excluded.
Why it's needed
UserPromptSubmit additionalContext was appended to the user request and that already-augmented request was then recorded. Consumers that concatenated every user message part therefore displayed hook-injected context as if the user had typed it when resuming a session or analyzing JSONL. Fixing only one UI would leave the persisted provenance ambiguous and duplicate brittle stripping logic, while removing the context from the recorded message would make model-history resume inaccurate. This change keeps the model and display views separate.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Automated transcript and resume regression evidence is provided instead of screenshots.
Observed focused results after the latest main merge and review follow-up: Core 563 tests passed; CLI 149; ACP 23; WebUI 8; VS Code 13; Desktop 43; no-AK integration smoke 52. The full build, typecheck, bundle, pre-commit Prettier, and ESLint checks also passed. The credentialed CLI integration suite was attempted locally; its model-backed cases require the repository OPENAI_API_KEY/OPENAI_BASE_URL/OPENAI_MODEL secrets and therefore could not be validated in this fork environment.
Tested on
Environment (optional)
macOS 26.3.1 on arm64, Node.js v26.5.0, npm 11.17.0, and Bun 1.3.9 for the Desktop regression suite.
Risk & Scope
Linked Issues
Closes #7940
中文说明
本 PR 做了什么
本 PR 将模型使用的 UserPromptSubmit Hook 上下文与用户可见的 transcript 文本分离。它把经过清理的 Hook 上下文包裹在独立的 <qwen:user-prompt-submit-context> part 中，在保留模型实际收到的完整 message parts 的同时记录干净的 displayText 与 hookContext 来源信息，并让 CLI Resume、Insight 生成、Session Reference、Goal Evidence、Conversation Branch、ACP Replay、WebUI、VS Code 和 Desktop 等 transcript 消费端统一使用干净投影。Telemetry 与自动记忆召回现在由 Hook 注入后的 prompt 改为使用注入前的用户 prompt，因此不再包含 UserPromptSubmit 新增的上下文。
为什么需要
此前 UserPromptSubmit 的 additionalContext 会被追加进 user request，随后这份已经扩展过的 request 又被直接写入记录。任何拼接全部 user message parts 的消费者，都会在恢复会话或分析 JSONL 时把 Hook 注入内容显示成用户亲自输入的文本。只修一个 UI 会继续留下含义不清的持久化数据，并在不同产品里复制脆弱的字符串删除逻辑；如果直接从记录消息中移除上下文，又会导致模型历史恢复不准确。因此本改动把模型视图和展示视图明确分离。
审阅者测试计划
如何验证
证据（修改前与修改后）
这里使用自动化 transcript 与 Resume 回归证据代替截图。
在最新 main 合并及本轮审查修复后，已观察到的定向测试结果：Core 563 个测试通过；CLI 149；ACP 23；WebUI 8；VS Code 13；Desktop 43；无 AK integration smoke 52。全仓 Build、Typecheck、Bundle、Pre-commit Prettier 与 ESLint 检查也均已通过。完整的凭据型 CLI integration suite 已在本机尝试运行；其中依赖模型的用例需要仓库的 OPENAI_API_KEY/OPENAI_BASE_URL/OPENAI_MODEL secrets，因此无法在当前 fork 环境完成验证。
测试平台
环境（可选）
macOS 26.3.1 arm64，Node.js v26.5.0，npm 11.17.0；Desktop 回归测试使用 Bun 1.3.9。
风险与范围
关联 Issue
Closes #7940