Skip to content

Commit 760faa5

Browse files
authored
fix: harden background questions, session prompts, and ACP execution (#170)
## Related Issue No issue — internal bug fixes found while auditing the agent engine and the ACP bridge. ## Problem Four defects, all reachable from normal use: 1. **The system prompt changed underneath a running session.** `ProfileService` rebuilt it on every `AGENTS.md` change, so editing instructions mid-session silently swapped the prompt the model had already been reasoning against, and invalidated the prompt cache. 2. **`AskUserQuestion` offered background tasks that could not exist.** The tool advertised `background: true` in its schema and description regardless of whether `TaskList`, `TaskOutput`, and `TaskStop` were active. Choosing it produced a task nothing could list, read, or stop. 3. **ACP sessions died on clients without a terminal.** `AcpTerminalRunner` threw when the client declared no terminal capability, so a whole class of ACP clients could not run a single shell command. 4. **ACP rejected typeless stdio MCP servers, and a reloaded ACP session could not re-bind its runtime.** A failed teardown left the staged entry in the runtime list forever, so the retry hit a stale slot. ## What changed 1. Dropped the `instructions.onDidChange` re-render. `refreshSystemPrompt` still reads instructions, so an explicitly requested refresh works — only the implicit rebuild is gone. 2. `AskUserQuestion` now derives its schema *and* its description from the live tool policy, in both engines. The v1 path passes the **predicate**, not a resolved boolean, so a policy change later in the session is picked up rather than frozen at construction. 3. `AcpTerminalRunner` falls back to the local process service instead of throwing. `IHostProcessService` is threaded through the session runtime, the workspace attachment, the provider factory, and `start.ts`. 4. A typeless ACP MCP server maps to a stdio server. `runtimeUnitHost` prunes the staged entry in a `finally`, so a throwing teardown cannot strand it. ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/PyModel/pythinker-code/blob/main/CONTRIBUTING.md) document. - [ ] I have linked a related issue (external PRs: the issue must have a maintainer's `/approve`). - [x] I have added tests that prove my feature works. - [x] Ran `gen-changesets` skill, or this PR needs no changeset. - [x] Ran `gen-docs` skill, or this PR needs no doc update. ## Verification `pnpm run typecheck` 0 · `pnpm run lint` 0 errors · agent-core 54/54 · agent-core-v2 and acp suites green. The `AskUserQuestion` fix is mutation-proven: replacing the predicate with a resolved boolean turns `rechecks AskUserQuestion background mode after the task policy changes` red (1 failed / 20 passed), and restoring it returns 21/21. That distinction is the entire point of the change, so it is the one guarded by a test that provably fails without it. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - ACP sessions can run supported shell commands through terminal access or locally when terminal access is unavailable. - Stdio-based MCP servers are supported in new and reloaded sessions. - Runtime connections recover correctly after session reloads. - **Bug Fixes** - Background questions are offered only when task controls are available. - Session system prompts remain stable after `AGENTS.md` changes. - Runtime cleanup supports reliable re-registration after teardown failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent f62da4c commit 760faa5

20 files changed

Lines changed: 705 additions & 81 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 a command locally when an ACP client provides no terminal or the command is not a shell, 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: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,25 @@ const OUTPUT_BYTE_LIMIT = 4 * 1024 * 1024;
2424
const OUTPUT_POLL_MS = 250;
2525
let nextGeneration = 1;
2626

27-
function isBashToolInvocation(args: readonly string[], options?: HostProcessOptions): boolean {
27+
const SHELL_EXECUTABLES = new Set(['sh', 'bash', 'zsh', 'dash', 'ksh', 'fish']);
28+
29+
/**
30+
* The Bash tool always spawns the configured shell. Classifying the executable
31+
* keeps another caller's `-c` invocation — `python -c ...` carrying the same
32+
* non-interactive env — on the local path, where the client cannot refuse it.
33+
*/
34+
function isShellExecutable(command: string): boolean {
35+
const base = (command.split(/[\\/]/).pop() ?? command).toLowerCase();
36+
return SHELL_EXECUTABLES.has(base.endsWith('.exe') ? base.slice(0, -4) : base);
37+
}
38+
39+
function isBashToolInvocation(
40+
command: string,
41+
args: readonly string[],
42+
options?: HostProcessOptions,
43+
): boolean {
2844
return (
45+
isShellExecutable(command) &&
2946
args.length === 2 &&
3047
args[0] === '-c' &&
3148
options?.env?.['NO_COLOR'] === '1' &&
@@ -47,18 +64,16 @@ class AcpProcessService implements IHostProcessService {
4764
private readonly sessionId: string,
4865
private readonly cwd: string,
4966
private readonly connection: IAcpConnection,
67+
private readonly local: IHostProcessService,
5068
) {}
5169

5270
async spawn(
5371
command: string,
5472
args: readonly string[] = [],
5573
options?: HostProcessOptions,
5674
): 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');
75+
if (!this.connection.terminalEnabled || !isBashToolInvocation(command, args, options)) {
76+
return this.local.spawn(command, args, { ...options, cwd: options?.cwd ?? this.cwd });
6277
}
6378

6479
const handle = await this.connection.get().createTerminal({
@@ -178,6 +193,7 @@ class AcpSessionRuntime implements Runtime {
178193
cwd: string,
179194
connection: IAcpConnection,
180195
environment: IHostEnvironment,
196+
local: IHostProcessService,
181197
) {
182198
this.identity = {
183199
workspaceId,
@@ -205,7 +221,7 @@ class AcpSessionRuntime implements Runtime {
205221
dirname: (p: string) => path.dirname(p),
206222
};
207223
this.fs = new AcpHostFileSystem({ sessionId } as unknown as ISessionContext, connection);
208-
this.process = new AcpProcessService(sessionId, cwd, connection);
224+
this.process = new AcpProcessService(sessionId, cwd, connection, local);
209225
}
210226

211227
dispose(): void {}
@@ -219,13 +235,21 @@ class AcpWorkspaceRuntimeAttachment implements RuntimeProviderAttachment {
219235
private readonly host: RuntimeProviderHost,
220236
private readonly connection: IAcpConnection,
221237
private readonly environment: IHostEnvironment,
238+
private readonly local: IHostProcessService,
222239
) {}
223240

224241
bindSession(sessionId: string, cwd: string): string {
225242
const runtimeId = AcpRuntimeProviderFactory.runtimeId(sessionId);
226243
if (this.sessions.has(sessionId)) return runtimeId;
227244
const registration = this.host.registerRuntime(
228-
new AcpSessionRuntime(this.workspace.id, sessionId, cwd, this.connection, this.environment),
245+
new AcpSessionRuntime(
246+
this.workspace.id,
247+
sessionId,
248+
cwd,
249+
this.connection,
250+
this.environment,
251+
this.local,
252+
),
229253
);
230254
this.sessions.set(sessionId, registration);
231255
return runtimeId;
@@ -241,7 +265,7 @@ class AcpWorkspaceRuntimeAttachment implements RuntimeProviderAttachment {
241265
async dispose(): Promise<void> {
242266
const registrations = [...this.sessions.values()];
243267
this.sessions.clear();
244-
for (const registration of registrations.reverse()) await registration.remove();
268+
for (const registration of registrations.toReversed()) await registration.remove();
245269
}
246270
}
247271

@@ -253,14 +277,21 @@ export class AcpRuntimeProviderFactory implements RuntimeProviderFactory {
253277
constructor(
254278
private readonly connection: IAcpConnection,
255279
private readonly environment: IHostEnvironment,
280+
private readonly local: IHostProcessService,
256281
) {}
257282

258283
static runtimeId(sessionId: string): string {
259284
return `acp:${sessionId}`;
260285
}
261286

262287
async attach(workspace: RuntimeProviderContext, host: RuntimeProviderHost): Promise<RuntimeProviderAttachment> {
263-
const attachment = new AcpWorkspaceRuntimeAttachment(workspace, host, this.connection, this.environment);
288+
const attachment = new AcpWorkspaceRuntimeAttachment(
289+
workspace,
290+
host,
291+
this.connection,
292+
this.environment,
293+
this.local,
294+
);
264295
this.attachments.set(workspace.id, attachment);
265296
return {
266297
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: 146 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,112 @@ 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 a non-shell -c command carrying the Bash env', 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('python', ['-c', 'print(1)'], { env: { ...bashEnv } });
191+
192+
expect(created).toBe(0);
193+
expect(calls).toHaveLength(1);
194+
expect(calls[0]).toMatchObject({ command: 'python', args: ['-c', 'print(1)'] });
195+
});
196+
197+
it('routes a shell spawn to the terminal regardless of the shell binary or its path', async () => {
198+
for (const shell of ['/bin/zsh', '/usr/local/bin/fish', 'C:\\Program Files\\Git\\bin\\bash.exe']) {
199+
let created = 0;
200+
const connection = makeConnection({
201+
terminalEnabled: true,
202+
createTerminal: () => {
203+
created += 1;
204+
return makeTerminalHandle();
205+
},
206+
});
207+
const { local, calls } = makeLocalProcessService();
208+
const runtime = await bindRuntime(makeEnvironment(), { connection, local });
209+
210+
await runtime.process!.spawn(shell, ['-c', 'echo hi'], { env: { ...bashEnv } });
211+
212+
expect(created, shell).toBe(1);
213+
expect(calls, shell).toHaveLength(0);
214+
}
215+
});
216+
217+
it('falls back to local execution for non-Bash spawns even with the terminal capability', async () => {
218+
let created = 0;
219+
const connection = makeConnection({
220+
terminalEnabled: true,
221+
createTerminal: () => {
222+
created += 1;
223+
return makeTerminalHandle();
224+
},
225+
});
226+
const { local, calls } = makeLocalProcessService();
227+
const runtime = await bindRuntime(makeEnvironment(), { connection, local });
228+
229+
await runtime.process!.spawn('rg', ['--files', '--hidden']);
230+
231+
expect(created).toBe(0);
232+
expect(calls).toHaveLength(1);
233+
expect(calls[0]).toMatchObject({
234+
command: 'rg',
235+
args: ['--files', '--hidden'],
236+
options: { cwd: '/repo' },
237+
});
238+
});
239+
});

0 commit comments

Comments
 (0)