feat(acp): LLM-based message rewrite middleware with custom prompts - #3191
Merged
Conversation
Contributor
📋 Review SummaryThis PR introduces a Message Rewrite Middleware that intercepts ACP model outputs and uses an LLM to rewrite technical raw outputs into business-friendly structured analysis. The implementation is well-architected with modular components (TurnBuffer, LlmRewriter, MessageRewriteMiddleware) and integrates into both interactive (Session) and non-interactive (CLI/eval) modes. Overall code quality is high with good error handling and clear separation of concerns. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
zhangxy-zju
changed the title
Contributor
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. |
tanzhenxin
previously requested changes
Apr 13, 2026
tanzhenxin
left a comment
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
LLM-based message rewrite middleware with clean separation of concerns (TurnBuffer / LlmRewriter / MessageRewriteMiddleware). Silent degradation design is correct — original messages always flow through first.
Issues
-
Workspace trust bypass in
loadRewriteConfig(security) —loadRewriteConfigreadssettings.workspace?.originalSettingsdirectly, bypassing theisTrustedcheck thatmergeSettingsenforces. A malicious.qwen/settings.jsonin a cloned repo can enable the rewriter with a custom prompt that accesses conversation content including internal reasoning. Suggestion: putmessageRewritein the settings schema and read fromsettings.merged, or checksettings.isTrustedbefore accepting workspace values. -
No timeout protection in interactive path (correctness) — The PR claims "30s timeout protection" but this only exists in the non-interactive path. In the Session path,
BaseEmitter.sendUpdatecallsinterceptUpdate(update)without a signal, soflushTurn(undefined)makes the LLM rewrite call with no abort signal. If the call hangs, the session blocks indefinitely. Suggestion: addAbortSignal.timeout(30000)insideflushTurnitself. -
History replay gets rewritten (correctness) —
BaseEmitter.sendUpdateroutes every emission through the rewriter, including history replay viaHistoryReplayer. Old assistant messages get fresh synthetic rewrite chunks appended on session load. Suggestion: scope rewriting to live model output only — bypass during replay or install the rewriter after replay completes.
Verdict
REQUEST_CHANGES — Security hole in config loading must be fixed, interactive path needs timeout protection, and history replay should not be rewritten.
Collaborator
Author
|
Thanks for the thorough review @tanzhenxin! All 3 issues have been addressed in commit a457805: 1. Workspace trust bypass (security) 2. No timeout in interactive path (correctness) 3. History replay gets rewritten (correctness) |
yiliang114
reviewed
Apr 14, 2026
yiliang114
reviewed
Apr 14, 2026
yiliang114
reviewed
Apr 14, 2026
yiliang114
requested changes
Apr 14, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for addressing the earlier trust-check, interactive-timeout, and history-replay issues. Those look fixed in the latest commits.
I still found three blocking correctness issues in the remaining rewrite flow: target filtering is missing in the non-interactive path, the non-interactive/cron path no longer enforces the advertised 30s timeout, and the ACP async rewrite lifecycle still only tracks the latest pending rewrite. Details are in the inline comments.
I re-ran npm run typecheck and cd packages/cli && npx vitest run src/config/settings.test.ts src/acp-integration/session/Session.test.ts src/nonInteractiveCli.test.ts.
Collaborator
Author
|
手动测试结果：
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an optional, configurable “message rewrite” pipeline for ACP and non-interactive CLI flows, where accumulated assistant output (messages and/or thoughts) is rewritten by an LLM using a custom prompt and emitted as an additional assistant message.
Changes:
- Added rewrite primitives (
TurnBuffer,LlmRewriter) and an ACPMessageRewriteMiddlewareto accumulate turn output and emit rewritten assistant text with metadata. - Integrated rewrite support into ACP session updates (installed after history replay) and into non-interactive CLI (including cron-triggered turns).
- Added unit tests for rewrite config loading, buffering, and middleware behavior.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/nonInteractiveCli.ts | Adds non-interactive + cron rewrite accumulation and async rewrite emission. |
| packages/cli/src/acp-integration/session/types.ts | Extends SessionContext with optional messageRewriter. |
| packages/cli/src/acp-integration/session/rewrite/types.ts | Introduces rewrite config + turn content types. |
| packages/cli/src/acp-integration/session/rewrite/index.ts | Exports rewrite module entrypoints. |
| packages/cli/src/acp-integration/session/rewrite/config.ts | Loads rewrite config from trusted workspace/user settings. |
| packages/cli/src/acp-integration/session/rewrite/config.test.ts | Tests for trusted/untrusted config resolution. |
| packages/cli/src/acp-integration/session/rewrite/TurnBuffer.ts | Implements per-turn accumulation and flushing. |
| packages/cli/src/acp-integration/session/rewrite/TurnBuffer.test.ts | Tests for buffering/flush semantics and tool-call marking. |
| packages/cli/src/acp-integration/session/rewrite/MessageRewriteMiddleware.ts | Implements ACP update interception and background rewrite emission. |
| packages/cli/src/acp-integration/session/rewrite/MessageRewriteMiddleware.test.ts | Tests pass-through, target filtering, tool-call boundary, metadata. |
| packages/cli/src/acp-integration/session/rewrite/LlmRewriter.ts | Implements LLM call with prompt/promptFile and turn-to-turn context. |
| packages/cli/src/acp-integration/session/emitters/BaseEmitter.ts | Routes session updates through the rewrite middleware when configured. |
| packages/cli/src/acp-integration/session/Session.ts | Installs rewriter post-history replay; triggers rewrite at turn end; waits in interactive path. |
| packages/cli/src/acp-integration/acpAgent.ts | Installs rewriter after replayHistory during session creation. |
| package-lock.json | Lockfile metadata change. |
Comments suppressed due to low confidence (1)
packages/cli/src/acp-integration/session/Session.ts:604
- Cron prompt execution kicks off rewrite work via messageRewriter.flushTurn(ac.signal), but this path never awaits waitForPendingRewrites(). This can allow rewrites to be emitted after the cron prompt has finished (or interleaved with later prompts), and contradicts the intent of ensuring pending rewrites aren’t lost at turn boundaries. Add an await this.messageRewriter.waitForPendingRewrites() before #executeCronPrompt returns (e.g., in a finally block).
if (usageMetadata) {
// Kick off rewrite in background (non-blocking)
if (this.messageRewriter) {
this.messageRewriter.flushTurn(ac.signal);
}
const durationMs = Date.now() - streamStartTime;
await this.messageEmitter.emitUsageMetadata(
usageMetadata,
'',
durationMs,
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add MessageRewriteMiddleware that intercepts ACP messages and appends
LLM-rewritten versions with _meta.rewritten=true at turn boundaries.
Original messages pass through unmodified. At the end of each turn
(before tool calls or at response end), accumulated thought/message
chunks are sent to LLM for rewriting into business-friendly text.
- TurnBuffer: accumulates chunks per turn
- LlmRewriter: calls LLM with configurable prompt
- MessageRewriteMiddleware: orchestrates intercept → buffer → rewrite → emit
- BaseEmitter.sendUpdate: routes through middleware when configured
- Session: initializes middleware from settings.messageRewrite config
Enable via settings.json:
{
"messageRewrite": {
"enabled": true,
"target": "both",
"prompt": "custom system prompt for rewriter"
}
}
Rewritten messages carry _meta.rewritten=true for frontend to
prioritize display. Original messages remain for debugging.
- Session.ts cron 路径添加 messageRewriter.flushTurn() 调用 - nonInteractiveCli.ts cron 路径添加 turnBuffer 累积 + flush + rewrite - 提取 loadRewriteConfig() 共享函数，消除两处重复配置读取 - 主路径和 cron 路径添加 turnBuffer.markToolCall() - rewrite 调用添加 30s 超时保护（AbortSignal.timeout） - 修复 import 语句被 const 声明分割的问题
- 默认 prompt 改为通用英文版（适配任意 coding agent，不绑定数据分析场景） - 支持 promptFile 配置项，从文件加载自定义 prompt（优先于 inline prompt） - 上下文连贯性：lastOutput 记录上一轮改写结果，拼接到下一轮输入， 避免连续 turn 间信息重复 - 修复 CLI 非交互模式 async rewrite 丢失：void doRewrite() 改为 pendingRewrites 数组 + emitResult 前 Promise.allSettled - 增加 debug logging：REWRITE INPUT/OUTPUT 完整内容 + prev_output 长度
…ewrite - Remove `async` field from MessageRewriteConfig - MessageRewriteMiddleware.flushTurn() always fires in background - nonInteractiveCli.ts main & cron paths always push to pendingRewrites - No user-facing latency from rewrite calls
1. loadRewriteConfig: skip workspace settings when !isTrusted, preventing untrusted repos from enabling rewriter with a custom prompt 2. MessageRewriteMiddleware.flushTurn: always enforce 30s timeout internally, even when caller provides no AbortSignal (interactive path) 3. Install rewriter AFTER history replay completes (Session.installRewriter), so historical messages are never rewritten on session load
1. nonInteractiveCli: apply rewriteConfig.target filter to accumulation (main path and cron path), matching MessageRewriteMiddleware behavior 2. nonInteractiveCli: add 30s AbortSignal.timeout to rewrite calls in both main and cron paths 3. MessageRewriteMiddleware: replace single pendingRewrite slot with pendingRewrites array + Promise.allSettled, ensuring all rewrites complete before session exits
…eMiddleware - TurnBuffer: flush, reset, isEmpty, markToolCall, whitespace filtering (12 tests) - loadRewriteConfig: isTrusted gating, workspace/user precedence (5 tests) - MessageRewriteMiddleware: target filtering, tool_call boundary flush, pendingRewrites queue, rewrite metadata (9 tests)
LLM sometimes outputs "（空字符串）" or similar text instead of actual empty string when instructed to "return empty string". Add regex patterns to catch common variants and treat them as null (skip rewrite output).
…ig.model 1. nonInteractiveCli: rewrite promises now return data only, adapter emission happens synchronously via emitSettledRewrites() at safe boundaries (before next turn starts, before cron next turn, before final result). Prevents concurrent startAssistantMessage corruption. 2. LlmRewriter: use rewriteConfig.model when set, fallback to config.getModel(). Previously model field was defined but ignored.
zhangxy-zju
force-pushed
the
feat/acp-message-rewriter
branch
from
April 14, 2026 06:56
aa95e83 to
ecd57e2
Compare
This reverts commit ecd57e2.
Allow configuring how many previous rewrite outputs are included as context when rewriting a new turn: - contextTurns: 1 (default) = last rewrite only - contextTurns: 0 = no context - contextTurns: N = last N rewrites - contextTurns: "all" = all previous rewrites
- Rename target value 'both' → 'all' for future extensibility (e.g. 'tool') - Add LlmRewriter tests: contextTurns (0/1/N/all), model override, filtering - Total: 35 tests across 4 test files
Non-interactive mode (qwen -p "..." --output-format json) consumers are scripts/programs that don't need user-friendly rewrites. Additionally, the JSON output adapter doesn't support _meta fields, so rewritten text was silently mixed into normal assistant messages without any marker. Rewrite middleware is now ACP-only (Session path).
Explain the feature purpose (business-oriented output customization), mark it as a temporary solution, and reference the hook-based alternative (#3266) for future discussion.
Collaborator
|
@zhangxy-zju Even though it's just a temporary fix, this user scenario is really valuable. We should think about creating something like an output-style solution down the road. It's worth looking into more deeply. Thanks again for your contribution! |
This was referenced Apr 15, 2026
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…wenLM#3191) * feat(acp): LLM-based message rewrite middleware Add MessageRewriteMiddleware that intercepts ACP messages and appends LLM-rewritten versions with _meta.rewritten=true at turn boundaries. Original messages pass through unmodified. At the end of each turn (before tool calls or at response end), accumulated thought/message chunks are sent to LLM for rewriting into business-friendly text. - TurnBuffer: accumulates chunks per turn - LlmRewriter: calls LLM with configurable prompt - MessageRewriteMiddleware: orchestrates intercept → buffer → rewrite → emit - BaseEmitter.sendUpdate: routes through middleware when configured - Session: initializes middleware from settings.messageRewrite config Enable via settings.json: { "messageRewrite": { "enabled": true, "target": "both", "prompt": "custom system prompt for rewriter" } } Rewritten messages carry _meta.rewritten=true for frontend to prioritize display. Original messages remain for debugging. * fix: TypeScript 编译错误修复 + 优化默认改写 prompt（参考竞品风格） * fix: 从 user/workspace originalSettings 读取 messageRewrite 配置（绕过 schema 校验） * feat: 非交互 CLI 模式也支持 message rewrite（eval 可用） * fix: 禁用 rewriter LLM 的 thinking，过滤 thought 部分只取纯文本输出 * fix: cron 路径补齐 message rewrite flush + 代码质量优化 - Session.ts cron 路径添加 messageRewriter.flushTurn() 调用 - nonInteractiveCli.ts cron 路径添加 turnBuffer 累积 + flush + rewrite - 提取 loadRewriteConfig() 共享函数，消除两处重复配置读取 - 主路径和 cron 路径添加 turnBuffer.markToolCall() - rewrite 调用添加 30s 超时保护（AbortSignal.timeout） - 修复 import 语句被 const 声明分割的问题 * feat: rewrite 支持 async/sync 模式（默认 async，不增加执行时间） * feat: rewrite prompt 通用化 + 上下文连贯 + promptFile + async 修复 - 默认 prompt 改为通用英文版（适配任意 coding agent，不绑定数据分析场景） - 支持 promptFile 配置项，从文件加载自定义 prompt（优先于 inline prompt） - 上下文连贯性：lastOutput 记录上一轮改写结果，拼接到下一轮输入， 避免连续 turn 间信息重复 - 修复 CLI 非交互模式 async rewrite 丢失：void doRewrite() 改为 pendingRewrites 数组 + emitResult 前 Promise.allSettled - 增加 debug logging：REWRITE INPUT/OUTPUT 完整内容 + prev_output 长度 * refactor: remove sync rewrite mode, always use async (non-blocking) rewrite - Remove `async` field from MessageRewriteConfig - MessageRewriteMiddleware.flushTurn() always fires in background - nonInteractiveCli.ts main & cron paths always push to pendingRewrites - No user-facing latency from rewrite calls * fix: address review feedback — trust check, timeout, history replay 1. loadRewriteConfig: skip workspace settings when !isTrusted, preventing untrusted repos from enabling rewriter with a custom prompt 2. MessageRewriteMiddleware.flushTurn: always enforce 30s timeout internally, even when caller provides no AbortSignal (interactive path) 3. Install rewriter AFTER history replay completes (Session.installRewriter), so historical messages are never rewritten on session load * fix: address second round review — target filter, timeout, rewrite queue 1. nonInteractiveCli: apply rewriteConfig.target filter to accumulation (main path and cron path), matching MessageRewriteMiddleware behavior 2. nonInteractiveCli: add 30s AbortSignal.timeout to rewrite calls in both main and cron paths 3. MessageRewriteMiddleware: replace single pendingRewrite slot with pendingRewrites array + Promise.allSettled, ensuring all rewrites complete before session exits * test: add unit tests for TurnBuffer, loadRewriteConfig, MessageRewriteMiddleware - TurnBuffer: flush, reset, isEmpty, markToolCall, whitespace filtering (12 tests) - loadRewriteConfig: isTrusted gating, workspace/user precedence (5 tests) - MessageRewriteMiddleware: target filtering, tool_call boundary flush, pendingRewrites queue, rewrite metadata (9 tests) * fix: config.test.ts use unknown cast for LoadedSettings stub (fix tsc --build) * fix: filter LLM literal "empty string" responses in rewriter output LLM sometimes outputs "（空字符串）" or similar text instead of actual empty string when instructed to "return empty string". Add regex patterns to catch common variants and treat them as null (skip rewrite output). * revert: remove LLM empty-string pattern defense, rely on prompt fix instead * fix: prevent async rewrite from corrupting adapter state + honor config.model 1. nonInteractiveCli: rewrite promises now return data only, adapter emission happens synchronously via emitSettledRewrites() at safe boundaries (before next turn starts, before cron next turn, before final result). Prevents concurrent startAssistantMessage corruption. 2. LlmRewriter: use rewriteConfig.model when set, fallback to config.getModel(). Previously model field was defined but ignored. * docs: add messageRewrite configuration guide to settings.md * Revert "docs: add messageRewrite configuration guide to settings.md" This reverts commit ecd57e2. * feat: add contextTurns config for rewrite history context Allow configuring how many previous rewrite outputs are included as context when rewriting a new turn: - contextTurns: 1 (default) = last rewrite only - contextTurns: 0 = no context - contextTurns: N = last N rewrites - contextTurns: "all" = all previous rewrites * refactor: rename target 'both' to 'all' + add LlmRewriter unit tests - Rename target value 'both' → 'all' for future extensibility (e.g. 'tool') - Add LlmRewriter tests: contextTurns (0/1/N/all), model override, filtering - Total: 35 tests across 4 test files * refactor: remove message rewrite from non-interactive CLI mode Non-interactive mode (qwen -p "..." --output-format json) consumers are scripts/programs that don't need user-friendly rewrites. Additionally, the JSON output adapter doesn't support _meta fields, so rewritten text was silently mixed into normal assistant messages without any marker. Rewrite middleware is now ACP-only (Session path). * revert: restore package-lock.json and nonInteractiveCli.ts to main state * docs: add README for message rewrite middleware Explain the feature purpose (business-oriented output customization), mark it as a temporary solution, and reference the hook-based alternative (QwenLM#3266) for future discussion. * docs: move temporary-solution notice to top of README * docs: simplify temporary-solution notice in rewrite README
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduce a configurable Message Rewrite Middleware for ACP that transforms raw model output into user-friendly formats using LLM-based rewriting with custom prompts.
rewriteConfig.targetPromise.allSettled, wait before session exitsCloses #3189
Configuration
Add
messageRewritetosettings.json(user or project level):{ "messageRewrite": { "enabled": true, "target": "all", "promptFile": ".qwen/rewrite-prompt.txt", "model": "qwen3-plus", "contextTurns": 1 } }enabledfalsetarget"message","thought", or"all""all"promptpromptFilepromptmodelcontextTurns"all"0= none,1= last one, N = last N,"all"= full history1Original messages are always sent as-is. Rewritten versions are appended with
_meta.rewritten: true, allowing clients to choose which to display.Test plan