Skip to content

Commit a457805

Browse files
committed
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
1 parent 61c803e commit a457805

File tree

5 files changed

+38
-20
lines changed

5 files changed

+38
-20
lines changed

packages/cli/src/acp-integration/acpAgent.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,9 @@ class QwenAgent implements Agent {
514514
await session.replayHistory(conversation.messages);
515515
}
516516

517+
// Install rewriter AFTER history replay to avoid rewriting historical messages
518+
session.installRewriter();
519+
517520
return session;
518521
}
519522

packages/cli/src/acp-integration/session/Session.ts

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,8 @@ export class Session implements SessionContext {
128128
private readonly planEmitter: PlanEmitter;
129129
private readonly messageEmitter: MessageEmitter;
130130

131-
// Message rewrite middleware (optional)
132-
readonly messageRewriter?: MessageRewriteMiddleware;
131+
// Message rewrite middleware (optional, installed after history replay)
132+
messageRewriter?: MessageRewriteMiddleware;
133133

134134
// Implement SessionContext interface
135135
readonly sessionId: string;
@@ -144,17 +144,6 @@ export class Session implements SessionContext {
144144
this.sessionId = id;
145145
this.runtimeBaseDir = Storage.getRuntimeBaseDir();
146146

147-
// Initialize message rewrite middleware if configured
148-
const rewriteConfig = loadRewriteConfig(settings);
149-
if (rewriteConfig?.enabled) {
150-
debugLogger.info('Message rewrite middleware enabled');
151-
this.messageRewriter = new MessageRewriteMiddleware(
152-
config,
153-
rewriteConfig,
154-
(update) => this.sendUpdate(update),
155-
);
156-
}
157-
158147
// Initialize modular components with this session as context
159148
this.toolCallEmitter = new ToolCallEmitter(this);
160149
this.planEmitter = new PlanEmitter(this);
@@ -170,6 +159,22 @@ export class Session implements SessionContext {
170159
return this.config;
171160
}
172161

162+
/**
163+
* Install the message rewrite middleware if configured.
164+
* Must be called AFTER history replay to avoid rewriting historical messages.
165+
*/
166+
installRewriter(): void {
167+
const rewriteConfig = loadRewriteConfig(this.settings);
168+
if (rewriteConfig?.enabled) {
169+
debugLogger.info('Message rewrite middleware enabled');
170+
this.messageRewriter = new MessageRewriteMiddleware(
171+
this.config,
172+
rewriteConfig,
173+
(update) => this.sendUpdate(update),
174+
);
175+
}
176+
}
177+
173178
/**
174179
* Replays conversation history to the client using modular components.
175180
* Delegates to HistoryReplayer for consistent event emission.

packages/cli/src/acp-integration/session/rewrite/MessageRewriteMiddleware.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,15 @@ export class MessageRewriteMiddleware {
109109
this.turnIndex++;
110110
const turnIdx = this.turnIndex;
111111

112+
// Always enforce a 30s timeout, combined with caller's signal if provided
113+
const timeoutSignal = AbortSignal.timeout(30_000);
114+
const rewriteSignal = signal
115+
? AbortSignal.any([signal, timeoutSignal])
116+
: timeoutSignal;
117+
112118
this.pendingRewrite = (async () => {
113119
try {
114-
const rewritten = await this.rewriter.rewrite(content, signal);
120+
const rewritten = await this.rewriter.rewrite(content, rewriteSignal);
115121
if (!rewritten) {
116122
debugLogger.info(`Turn ${turnIdx}: no rewrite output`);
117123
return;

packages/cli/src/acp-integration/session/rewrite/config.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,20 @@ import type { MessageRewriteConfig } from './types.js';
99

1010
/**
1111
* Reads messageRewrite configuration from user/workspace originalSettings.
12-
* Workspace settings take precedence over user settings.
12+
* Workspace settings are only used when the workspace is trusted,
13+
* preventing untrusted repos from enabling the rewriter with a custom prompt.
1314
*/
1415
export function loadRewriteConfig(
1516
settings: LoadedSettings,
1617
): MessageRewriteConfig | undefined {
1718
const userOriginal = settings.user?.originalSettings as
1819
| Record<string, unknown>
1920
| undefined;
20-
const workspaceOriginal = settings.workspace?.originalSettings as
21-
| Record<string, unknown>
22-
| undefined;
21+
const workspaceOriginal = settings.isTrusted
22+
? (settings.workspace?.originalSettings as
23+
| Record<string, unknown>
24+
| undefined)
25+
: undefined;
2326
return (workspaceOriginal?.['messageRewrite'] ??
2427
userOriginal?.['messageRewrite']) as MessageRewriteConfig | undefined;
2528
}

packages/cli/src/acp-integration/session/types.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,9 @@ export interface SessionUpdateSender {
3030
export interface SessionContext extends SessionUpdateSender {
3131
readonly sessionId: string;
3232
readonly config: Config;
33-
/** Optional message rewrite middleware for ACP message transformation */
34-
readonly messageRewriter?: MessageRewriteMiddleware;
33+
/** Optional message rewrite middleware for ACP message transformation.
34+
* Installed after history replay to avoid rewriting historical messages. */
35+
messageRewriter?: MessageRewriteMiddleware;
3536
}
3637

3738
/**

0 commit comments

Comments
 (0)