feat(acp): LLM-based message rewrite middleware with custom prompts#3191
Open
zhangxy-zju wants to merge 22 commits intomainfrom
Open
feat(acp): LLM-based message rewrite middleware with custom prompts#3191zhangxy-zju wants to merge 22 commits intomainfrom
zhangxy-zju wants to merge 22 commits intomainfrom
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
requested changes
Apr 13, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
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
packages/cli/src/acp-integration/session/rewrite/MessageRewriteMiddleware.ts
Outdated
Show resolved
Hide resolved
yiliang114
requested changes
Apr 14, 2026
Collaborator
yiliang114
left a comment
yiliang114
left a comment
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.
packages/cli/src/acp-integration/session/rewrite/MessageRewriteMiddleware.ts
Show resolved
Hide resolved
packages/cli/src/acp-integration/session/rewrite/LlmRewriter.ts
Outdated
Show resolved
Hide resolved
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
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).
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