Skip to content

Commit c7935dc

Browse files
committed
fix: harden background questions, session prompts, and ACP execution
Gate AskUserQuestion background mode on the TaskList, TaskOutput, and TaskStop controls in both engines, re-evaluating the policy after the active tool set changes so a retained tool instance cannot keep advertising a mode it can no longer run. Stop rebuilding the system prompt when session instructions change mid-session, so an AGENTS.md edit no longer rewrites a bound agent's prompt. Let an ACP session fall back to local process execution when the client advertises no terminal or the spawn is not Bash-shaped, accept stdio MCP servers that declare no runtime identity, and prune a staged runtime entry when its removal teardown throws so the same runtime id can be registered again.
1 parent f62da4c commit c7935dc

20 files changed

Lines changed: 648 additions & 80 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Run shell commands locally when an ACP client provides no terminal, accept stdio MCP servers in ACP sessions, and let a reloaded ACP session bind its runtime again.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Prevent AskUserQuestion from starting background tasks when task controls are unavailable.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Keep the system prompt unchanged for the rest of a session when AGENTS.md is edited.

packages/acp-server/src/acp-terminal/acpTerminalRunner.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,18 +47,16 @@ class AcpProcessService implements IHostProcessService {
4747
private readonly sessionId: string,
4848
private readonly cwd: string,
4949
private readonly connection: IAcpConnection,
50+
private readonly local: IHostProcessService,
5051
) {}
5152

5253
async spawn(
5354
command: string,
5455
args: readonly string[] = [],
5556
options?: HostProcessOptions,
5657
): Promise<IHostProcess> {
57-
if (!this.connection.terminalEnabled) {
58-
throw new Error('ACP terminal capability is unavailable');
59-
}
60-
if (!isBashToolInvocation(args, options)) {
61-
throw new Error('ACP runtime only supports interactive Bash tool processes');
58+
if (!this.connection.terminalEnabled || !isBashToolInvocation(args, options)) {
59+
return this.local.spawn(command, args, { ...options, cwd: options?.cwd ?? this.cwd });
6260
}
6361

6462
const handle = await this.connection.get().createTerminal({
@@ -178,6 +176,7 @@ class AcpSessionRuntime implements Runtime {
178176
cwd: string,
179177
connection: IAcpConnection,
180178
environment: IHostEnvironment,
179+
local: IHostProcessService,
181180
) {
182181
this.identity = {
183182
workspaceId,
@@ -205,7 +204,7 @@ class AcpSessionRuntime implements Runtime {
205204
dirname: (p: string) => path.dirname(p),
206205
};
207206
this.fs = new AcpHostFileSystem({ sessionId } as unknown as ISessionContext, connection);
208-
this.process = new AcpProcessService(sessionId, cwd, connection);
207+
this.process = new AcpProcessService(sessionId, cwd, connection, local);
209208
}
210209

211210
dispose(): void {}
@@ -219,13 +218,21 @@ class AcpWorkspaceRuntimeAttachment implements RuntimeProviderAttachment {
219218
private readonly host: RuntimeProviderHost,
220219
private readonly connection: IAcpConnection,
221220
private readonly environment: IHostEnvironment,
221+
private readonly local: IHostProcessService,
222222
) {}
223223

224224
bindSession(sessionId: string, cwd: string): string {
225225
const runtimeId = AcpRuntimeProviderFactory.runtimeId(sessionId);
226226
if (this.sessions.has(sessionId)) return runtimeId;
227227
const registration = this.host.registerRuntime(
228-
new AcpSessionRuntime(this.workspace.id, sessionId, cwd, this.connection, this.environment),
228+
new AcpSessionRuntime(
229+
this.workspace.id,
230+
sessionId,
231+
cwd,
232+
this.connection,
233+
this.environment,
234+
this.local,
235+
),
229236
);
230237
this.sessions.set(sessionId, registration);
231238
return runtimeId;
@@ -241,7 +248,7 @@ class AcpWorkspaceRuntimeAttachment implements RuntimeProviderAttachment {
241248
async dispose(): Promise<void> {
242249
const registrations = [...this.sessions.values()];
243250
this.sessions.clear();
244-
for (const registration of registrations.reverse()) await registration.remove();
251+
for (const registration of registrations.toReversed()) await registration.remove();
245252
}
246253
}
247254

@@ -253,14 +260,21 @@ export class AcpRuntimeProviderFactory implements RuntimeProviderFactory {
253260
constructor(
254261
private readonly connection: IAcpConnection,
255262
private readonly environment: IHostEnvironment,
263+
private readonly local: IHostProcessService,
256264
) {}
257265

258266
static runtimeId(sessionId: string): string {
259267
return `acp:${sessionId}`;
260268
}
261269

262270
async attach(workspace: RuntimeProviderContext, host: RuntimeProviderHost): Promise<RuntimeProviderAttachment> {
263-
const attachment = new AcpWorkspaceRuntimeAttachment(workspace, host, this.connection, this.environment);
271+
const attachment = new AcpWorkspaceRuntimeAttachment(
272+
workspace,
273+
host,
274+
this.connection,
275+
this.environment,
276+
this.local,
277+
);
264278
this.attachments.set(workspace.id, attachment);
265279
return {
266280
dispose: async () => {

packages/acp-server/src/convert.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,14 @@ export function acpMcpServersToConfigRecord(
176176
const out: Record<string, McpServerConfig> = {};
177177
for (const server of servers) {
178178
if (!('type' in server)) {
179-
throw new Error(`ACP stdio MCP server ${server.name} does not declare a runtime identity`);
179+
out[server.name] = {
180+
transport: 'stdio',
181+
command: server.command,
182+
args: server.args,
183+
env: namedPairsToRecord(server.env),
184+
runtime_id: 'local',
185+
};
186+
continue;
180187
}
181188
if (server.type === 'http' || server.type === 'sse') {
182189
out[server.name] = {

packages/acp-server/src/start.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
IAgentRuntimeBindingService,
2626
IAppendLogStore,
2727
IHostEnvironment,
28+
IHostProcessService,
2829
ISessionContext,
2930
ISessionIndexMirror,
3031
IWorkspaceInstanceManager,
@@ -141,7 +142,11 @@ export async function runAcpServerWithStream(
141142
// `IAcpConnection.get()`.
142143
acpConnection.bind(client);
143144
const workspaceManager = core.accessor.get(IWorkspaceInstanceManager);
144-
const acpRuntimeProvider = new AcpRuntimeProviderFactory(acpConnection, core.accessor.get(IHostEnvironment));
145+
const acpRuntimeProvider = new AcpRuntimeProviderFactory(
146+
acpConnection,
147+
core.accessor.get(IHostEnvironment),
148+
core.accessor.get(IHostProcessService),
149+
);
145150
const acpProviderRegistration = await workspaceManager.addProvider(acpRuntimeProvider);
146151
const sessionWorkspaces = new Map<string, string>();
147152
server = new AcpServer(client, klient, acpConnection, {

packages/acp-server/test/acp-terminal.test.ts

Lines changed: 107 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,54 @@
11
import { describe, expect, it } from 'vitest';
22

33
import type {
4+
HostProcessOptions,
45
IHostEnvironment,
6+
IHostProcess,
7+
IHostProcessService,
58
Runtime,
69
RuntimeProviderHost,
710
} from '@pymodel/agent-core-v2';
811

9-
import type { IAcpConnection } from '../src/acp-fs/acpConnection';
12+
import type { IAcpConnection, IAcpTerminalHandle } from '../src/acp-fs/acpConnection';
1013
import { AcpHostFileSystem } from '../src/acp-fs/acpFsService';
1114
import { AcpRuntimeProviderFactory } from '../src/acp-terminal/acpTerminalRunner';
1215

13-
function makeConnection(): IAcpConnection {
16+
function makeConnection(
17+
options: { terminalEnabled?: boolean; createTerminal?: () => IAcpTerminalHandle } = {},
18+
): IAcpConnection {
1419
return {
1520
_serviceBrand: undefined,
1621
bound: true,
1722
fsReadTextFile: true,
1823
fsWriteTextFile: true,
19-
terminalEnabled: true,
24+
terminalEnabled: options.terminalEnabled ?? true,
2025
bind: () => {},
21-
get: () => ({}) as never,
26+
get: () => ({ createTerminal: async () => options.createTerminal?.() }) as never,
2227
bindFsCapabilities: () => {},
2328
bindTerminalCapability: () => {},
2429
notifyTerminalCreated: () => {},
2530
onTerminalCreated: () => () => {},
2631
};
2732
}
2833

34+
interface LocalSpawnCall {
35+
readonly command: string;
36+
readonly args: readonly string[];
37+
readonly options: HostProcessOptions | undefined;
38+
}
39+
40+
function makeLocalProcessService(): { local: IHostProcessService; calls: LocalSpawnCall[] } {
41+
const calls: LocalSpawnCall[] = [];
42+
const local: IHostProcessService = {
43+
_serviceBrand: undefined,
44+
spawn: async (command, args = [], options) => {
45+
calls.push({ command, args, options });
46+
return {} as IHostProcess;
47+
},
48+
};
49+
return { local, calls };
50+
}
51+
2952
function makeEnvironment(overrides: Partial<IHostEnvironment> = {}): IHostEnvironment {
3053
return {
3154
_serviceBrand: undefined,
@@ -41,15 +64,22 @@ function makeEnvironment(overrides: Partial<IHostEnvironment> = {}): IHostEnviro
4164
} as IHostEnvironment;
4265
}
4366

44-
async function bindRuntime(environment: IHostEnvironment): Promise<Runtime> {
67+
async function bindRuntime(
68+
environment: IHostEnvironment,
69+
options: { connection?: IAcpConnection; local?: IHostProcessService } = {},
70+
): Promise<Runtime> {
4571
const runtimes: Runtime[] = [];
4672
const host = {
4773
registerRuntime: (runtime: Runtime) => {
4874
runtimes.push(runtime);
4975
return { remove: async () => {} };
5076
},
5177
} as unknown as RuntimeProviderHost;
52-
const factory = new AcpRuntimeProviderFactory(makeConnection(), environment);
78+
const factory = new AcpRuntimeProviderFactory(
79+
options.connection ?? makeConnection(),
80+
environment,
81+
options.local ?? makeLocalProcessService().local,
82+
);
5383
await factory.attach({ id: 'w1' } as never, host);
5484
factory.bindSession('w1', 's1', '/repo');
5585
const runtime = runtimes[0];
@@ -61,7 +91,7 @@ describe('AcpSessionRuntime', () => {
6191
it('mirrors the probed host environment and exposes fs + process capabilities', async () => {
6292
const runtime = await bindRuntime(makeEnvironment());
6393

64-
expect([...runtime.capabilities].sort()).toEqual(['fs', 'process']);
94+
expect([...runtime.capabilities].toSorted()).toEqual(['fs', 'process']);
6595
expect(runtime.environment).toMatchObject({
6696
osKind: 'macOS',
6797
osArch: 'arm64',
@@ -98,3 +128,73 @@ describe('AcpSessionRuntime', () => {
98128
expect(runtime.path.resolve('C:\\repo', 'src')).toBe('C:\\repo\\src');
99129
});
100130
});
131+
132+
describe('AcpProcessService local fallback', () => {
133+
const bashEnv = { NO_COLOR: '1', TERM: 'dumb' };
134+
135+
function makeTerminalHandle(): IAcpTerminalHandle {
136+
return {
137+
id: 'term-1',
138+
currentOutput: async () => ({ output: '', truncated: false }),
139+
waitForExit: async () => ({ exitCode: 0 }),
140+
kill: async () => ({}),
141+
release: async () => ({}),
142+
};
143+
}
144+
145+
it('runs Bash-shaped spawns in the client terminal when the capability is advertised', async () => {
146+
let created = 0;
147+
const connection = makeConnection({
148+
terminalEnabled: true,
149+
createTerminal: () => {
150+
created += 1;
151+
return makeTerminalHandle();
152+
},
153+
});
154+
const { local, calls } = makeLocalProcessService();
155+
const runtime = await bindRuntime(makeEnvironment(), { connection, local });
156+
157+
await runtime.process!.spawn('/bin/bash', ['-c', 'echo hi'], { env: { ...bashEnv } });
158+
159+
expect(created).toBe(1);
160+
expect(calls).toHaveLength(0);
161+
});
162+
163+
it('falls back to local execution for Bash-shaped spawns without the terminal capability', async () => {
164+
const connection = makeConnection({ terminalEnabled: false });
165+
const { local, calls } = makeLocalProcessService();
166+
const runtime = await bindRuntime(makeEnvironment(), { connection, local });
167+
168+
await runtime.process!.spawn('/bin/bash', ['-c', 'echo hi'], { env: { ...bashEnv } });
169+
170+
expect(calls).toHaveLength(1);
171+
expect(calls[0]).toMatchObject({
172+
command: '/bin/bash',
173+
args: ['-c', 'echo hi'],
174+
options: { env: bashEnv, cwd: '/repo' },
175+
});
176+
});
177+
178+
it('falls back to local execution for non-Bash spawns even with the terminal capability', async () => {
179+
let created = 0;
180+
const connection = makeConnection({
181+
terminalEnabled: true,
182+
createTerminal: () => {
183+
created += 1;
184+
return makeTerminalHandle();
185+
},
186+
});
187+
const { local, calls } = makeLocalProcessService();
188+
const runtime = await bindRuntime(makeEnvironment(), { connection, local });
189+
190+
await runtime.process!.spawn('rg', ['--files', '--hidden']);
191+
192+
expect(created).toBe(0);
193+
expect(calls).toHaveLength(1);
194+
expect(calls[0]).toMatchObject({
195+
command: 'rg',
196+
args: ['--files', '--hidden'],
197+
options: { cwd: '/repo' },
198+
});
199+
});
200+
});

packages/acp-server/test/convert.test.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describe('acpMcpServersToConfigRecord', () => {
1919
expect(acpMcpServersToConfigRecord([])).toBeUndefined();
2020
});
2121

22-
it('rejects stdio servers that cannot declare a runtime identity', () => {
22+
it('maps stdio servers (no type field) to local stdio configs', () => {
2323
const servers: McpServer[] = [
2424
{
2525
name: 'fs',
@@ -31,9 +31,15 @@ describe('acpMcpServersToConfigRecord', () => {
3131
],
3232
},
3333
];
34-
expect(() => acpMcpServersToConfigRecord(servers)).toThrow(
35-
'ACP stdio MCP server fs does not declare a runtime identity',
36-
);
34+
expect(acpMcpServersToConfigRecord(servers)).toEqual({
35+
fs: {
36+
transport: 'stdio',
37+
command: '/usr/local/bin/mcp-fs',
38+
args: ['--root', '/tmp'],
39+
env: { API_KEY: 'secret', DEBUG: '1' },
40+
runtime_id: 'local',
41+
},
42+
});
3743
});
3844

3945
it('maps http and sse servers with header pairs as a record', () => {

packages/acp-server/test/e2e-turn.test.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -991,15 +991,15 @@ describe('acp-server terminal reverse-RPC (clientCapabilities.terminal)', () =>
991991
expect(JSON.stringify(secondCall)).toContain('hello_from_terminal');
992992
}, 30_000);
993993

994-
it('rejects Bash without falling back when the client does not advertise terminal capability', async () => {
994+
it('falls back to local execution when the client does not advertise the capability', async () => {
995995
const c = await boot({});
996996
const terminals = fakeTerminalClient(c, 'should_not_be_used\n');
997997
scriptBashTurn('echo hello_from_bash');
998998

999999
const { stopReason } = await runPrompt(c);
10001000
expect(stopReason).toBe('end_turn');
10011001

1002-
// No terminal reverse-RPC at all — behavior identical to today.
1002+
// No terminal reverse-RPC at all — the command ran locally.
10031003
expect(terminals).toHaveLength(0);
10041004
const terminalRpcs = c.received.filter(
10051005
(m) => typeof m.method === 'string' && m.method.startsWith('terminal/'),
@@ -1009,7 +1009,6 @@ describe('acp-server terminal reverse-RPC (clientCapabilities.terminal)', () =>
10091009
// The tool card carries the textual output, exactly as before.
10101010
const completed = toolCallUpdates(c).find((u) => u.status === 'completed');
10111011
const text = completed?.content?.map((entry) => entry.content?.text ?? '').join('\n') ?? '';
1012-
expect(text).not.toContain('hello_from_bash');
1013-
expect(JSON.stringify(scripted!.callHistory()[1])).toContain('ACP terminal capability is unavailable');
1012+
expect(text).toContain('hello_from_bash');
10141013
}, 30_000);
10151014
});

0 commit comments

Comments
 (0)