Skip to content

feat(daemon): server-side shell command execution for ! (bang) prefix #4576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
doudouOUC merged 3 commits into from
May 28, 2026
Merged

feat(daemon): server-side shell command execution for ! (bang) prefix #4576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da581fc
feat(daemon): server-side shell command execution for ! (bang) prefix
doudouOUC May 27, 2026
e3504e8
fix: use typeof guard for shellCommand capability check
doudouOUC May 27, 2026
0e83458
fix: address wenshao review — 7 fixes
doudouOUC May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions packages/acp-bridge/src/bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import type { ApprovalMode } from '@qwen-code/qwen-code-core';
import {
TrustGateError,
getCurrentGeminiMdFilename,
ShellExecutionService,
type ShellOutputEvent,
} from '@qwen-code/qwen-code-core';
import type { ShellCommandResult } from './bridgeTypes.js';
import type { AcpChannel } from './channel.js';
import { EventBus, DEFAULT_RING_SIZE, type BridgeEvent } from './eventBus.js';
import {
Expand Down Expand Up @@ -444,6 +447,8 @@ const MCP_RESTART_TIMEOUT_MS = 300_000;
* disconnect cancellation in v1 (see server.ts route comment).
*/
const SESSION_RECAP_TIMEOUT_MS = 60_000;
const SHELL_COMMAND_TIMEOUT_MS = 120_000;
const MAX_SHELL_OUTPUT_FOR_HISTORY = 10_000;
const DEFAULT_MAX_SESSIONS = 20;
/**
* Soft upper bound on `BridgeOptions.eventRingSize` to catch operator
Expand Down Expand Up @@ -3155,6 +3160,133 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
};
},

async executeShellCommand(
sessionId,
command,
signal,
context,
): Promise<ShellCommandResult> {
const entry = byId.get(sessionId);
if (!entry) throw new SessionNotFoundError(sessionId);
const originatorClientId = resolveTrustedClientId(
entry,
context?.clientId,
);

if (signal?.aborted) {
return { exitCode: null, output: '', aborted: true };
}

const cwd = entry.workspaceCwd;

entry.events.publish({
type: 'user_shell_command',
data: { sessionId, command, cwd },
...(originatorClientId ? { originatorClientId } : {}),
});

const outputChunks: string[] = [];
const abort = new AbortController();
const onSignalAbort = () => abort.abort();
signal?.addEventListener('abort', onSignalAbort, { once: true });

Comment on lines +3188 to +3192

@doudouOUC doudouOUC May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — won't take this one. The 120s SHELL_COMMAND_TIMEOUT_MS provides the bound — same pattern as the CLI's ShellExecutionService usage which also accumulates unboundedly until timeout. Capping mid-stream would silently truncate output the user explicitly asked to see.

try {
Comment thread
doudouOUC marked this conversation as resolved.
const handle = await ShellExecutionService.execute(
command,
cwd,
(event: ShellOutputEvent) => {
if (event.type === 'data') {
const chunk =
typeof event.chunk === 'string'
? event.chunk
: event.chunk
.map((line: Array<{ text: string }>) =>
line.map((t) => t.text).join(''),
)
.join('\n');
outputChunks.push(chunk);

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Unbounded output buffer — potential OOM for long-running commands

outputChunks collects the full command output in memory without any size limit. While historyOutput is correctly capped at 10KB for LLM history injection, the raw output returned in ShellCommandResult has no bound. A command like cat on a multi-GB file or yes running for the full 120s timeout would consume unbounded memory before being returned.

Consider capping the buffer (e.g. keep first/last 500KB) or switching to a ring buffer, similar to how the CLI already handles shell output limits. The 10KB cap for history is good, but the full output buffer should also have a ceiling.

This review was generated by QoderWork AI

entry.events.publish({
type: 'session_update',
data: {
sessionId,
update: {
sessionUpdate: 'shell_output',
output: chunk,
_meta: { serverTimestamp: Date.now(), source: 'user-shell' },
},
},
...(originatorClientId ? { originatorClientId } : {}),
});
}
},
abort.signal,
false,
{ terminalWidth: 120, terminalHeight: 40 },

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Timeout starts after await ShellExecutionService.execute() resolves

The timeout setTimeout is set after the await ShellExecutionService.execute() returns the handle. If the execute call itself takes significant time (e.g. process spawning overhead), the actual command runtime exceeds the intended 120s limit by that overhead. While usually negligible, consider starting the timer before the await for more accurate timeout semantics:

const timeoutId = setTimeout(() => abort.abort(), SHELL_COMMAND_TIMEOUT_MS);
timeoutId.unref();
try {
  const handle = await ShellExecutionService.execute(...);
  const result = await handle.result;
} finally {
  clearTimeout(timeoutId);
}

This review was generated by QoderWork AI

{ streamStdout: true },
);

const timeoutId = setTimeout(
() => abort.abort(),
SHELL_COMMAND_TIMEOUT_MS,
);
timeoutId.unref();

const result = await handle.result;
clearTimeout(timeoutId);

const exitCode = result.exitCode;
const aborted = result.aborted;
const output = outputChunks.join('') || result.output;

entry.events.publish({
type: 'user_shell_result',
data: {
sessionId,
exitCode,
signal: result.signal,
aborted,
_meta: { serverTimestamp: Date.now() },
},
...(originatorClientId ? { originatorClientId } : {}),
});

const historyOutput =
output.length > MAX_SHELL_OUTPUT_FOR_HISTORY
? output.substring(0, MAX_SHELL_OUTPUT_FOR_HISTORY) +
'\n... (truncated)'
: output;

try {

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] role: 'user' for shell history injection may mislead the LLM

The shell command result is injected as a user message in the LLM's chat history. This makes it appear as if the user typed the command and output themselves, which could confuse the model about the user's intent in subsequent turns (e.g. the model might think the user is showing it a command rather than reporting an execution result).

If the Gemini API supports system or model roles for addHistory, consider using those. Alternatively, prefix with a clearer attribution like [System: Shell command executed by user via daemon] to disambiguate. Check what the CLI's addShellCommandToGeminiHistory uses for consistency.

This review was generated by QoderWork AI

await entry.connection.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionShellHistory,
{ sessionId, command, output: historyOutput, exitCode },
);

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] History injection template vulnerable to backtick escaping

outputText is placed inside a triple-backtick code fence, but if the command output itself contains ```, it will break the code fence and corrupt the LLM's chat history structure. This could cause confusing LLM behavior in subsequent turns.

Apply the same dynamic backtick fencing that ChannelBase.ts already implements:

const longestRun = Math.max(0, ...Array.from(outputText.matchAll(/`+/g), m => m[0].length));
const fence = '`'.repeat(Math.max(3, longestRun + 1));
geminiClient.addHistory({
  role: 'user',
  parts: [{ text: `I ran the following shell command:\n${fence}sh\n${command}\n${fence}\n\nThis produced the following result:\n${fence}\n${outputText}\n${fence}` }],
});

This review was generated by QoderWork AI

} catch (err) {
writeServeDebugLine(
`shell history injection failed for session ${sessionId}: ${err instanceof Error ? err.message : String(err)}`,
);
}

return { exitCode, output, aborted };
} catch (err) {
entry.events.publish({
type: 'user_shell_result',
data: {
sessionId,
exitCode: null,
signal: null,
aborted: false,
error: err instanceof Error ? err.message : String(err),
_meta: { serverTimestamp: Date.now() },
},
...(originatorClientId ? { originatorClientId } : {}),
});
throw err;
} finally {
signal?.removeEventListener('abort', onSignalAbort);
}
},

async setWorkspaceToolEnabled(toolName, enabled, originatorClientId) {
// #4175 Wave 4 PR 17. Pure file IO + event fan-out — no ACP
// roundtrip. The settings file is the source of truth; live
Expand Down
19 changes: 19 additions & 0 deletions packages/acp-bridge/src/bridgeTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,19 @@ export interface HttpAcpBridge {
context?: BridgeClientRequestContext,
): Promise<{ sessionId: string; recap: string | null }>;

/**
* Execute a shell command directly on the daemon (no LLM involvement).
* Streams output through the session's SSE bus and injects the
* command+result into the LLM's chat history via extMethod.
* Throws `SessionNotFoundError` for unknown ids.
*/
executeShellCommand(
sessionId: string,
command: string,
signal?: AbortSignal,
context?: BridgeClientRequestContext,
): Promise<ShellCommandResult>;

/**
* Add or remove a tool name from the workspace's `tools.disabled`
* settings list and fan-out a `tool_toggled` event to every live
Expand Down Expand Up @@ -469,3 +482,9 @@ export interface HttpAcpBridge {
/** Close all live child processes; called on daemon shutdown. */
shutdown(): Promise<void>;
}

export interface ShellCommandResult {
exitCode: number | null;
output: string;
aborted: boolean;
}
1 change: 1 addition & 0 deletions packages/acp-bridge/src/status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export const SERVE_CONTROL_EXT_METHODS = {
sessionClose: 'qwen/control/session/close',
sessionApprovalMode: 'qwen/control/session/approval_mode',
sessionRecap: 'qwen/control/session/recap',
sessionShellHistory: 'qwen/control/session/shell_history',
workspaceMcpRestart: 'qwen/control/workspace/mcp/restart',
} as const;

Expand Down
40 changes: 40 additions & 0 deletions packages/channels/base/src/ChannelBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,46 @@ export abstract class ChannelBase {
this.config.cwd,
);

// 3.5. Bang (!) shell command — direct execution, no LLM
if (envelope.text.startsWith('!')) {
const cmd = envelope.text.slice(1).trim();
const bridgeShellCommand = (this.bridge as unknown as Record<string, unknown>)['shellCommand'];

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Unsafe Record<string, unknown> type cast — use typed interface or type guard

This cast to Record<string, unknown> followed by bracket access and a typeof check is a type-safety anti-pattern. It bypasses TypeScript's type checking entirely and is fragile to refactoring — renaming shellCommand on the bridge won't produce a compile error here.

Consider either:

  1. Adding a proper capability check to the bridge interface:
interface ShellCapableBridge {
  shellCommand(sessionId: string, command: string): Promise<...>;
}
function hasShellCommand(bridge: unknown): bridge is ShellCapableBridge {
  return typeof (bridge as ShellCapableBridge)?.shellCommand === 'function';
}
  1. Or declaring shellCommand as an optional method on the base bridge type and using this.bridge.shellCommand?.(...).

Also note: when the bridge doesn't support shellCommand, the ! command silently falls through to LLM processing (see related Minor finding on ChannelBase.ts line 278).

This review was generated by QoderWork AI

if (cmd && typeof bridgeShellCommand === 'function') {
try {

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Silent fall-through when ! command is not supported

When ! is detected but either cmd is empty or the bridge doesn't support shellCommand, the code falls through to normal LLM processing. This means:

  • ! alone (no command) gets sent to the LLM as a bare exclamation mark
  • !ls on a bridge without shellCommand capability gets sent as raw text to the LLM

The user typed ! expecting shell execution. They should get feedback, not a confusing LLM response. Add an else branch:

if (cmd && typeof bridgeShellCommand === 'function') {
  // ... existing logic
} else if (envelope.text.startsWith('!')) {
  await this.sendMessage(
    envelope.chatId,
    cmd ? 'Shell command execution is not supported in this mode' : 'Usage: ! <command>',
  );
  return;
}

This review was generated by QoderWork AI

const result = (await bridgeShellCommand(sessionId, cmd)) as {
exitCode: number | null;
output: string;
aborted: boolean;
};
const longestRun = Math.max(
0,
...Array.from(
(result.output || '').matchAll(/`+/g),
(m) => m[0].length,
),
);
const fence = '`'.repeat(Math.max(3, longestRun + 1));
const output = result.output
? `${fence}\n${result.output}\n${fence}`
: '(no output)';
const exitLine =
result.exitCode !== null && result.exitCode !== 0
? `\nExit code: ${result.exitCode}`
: '';
await this.sendMessage(
envelope.chatId,
`$ ${cmd}\n${output}${exitLine}`,
);
} catch (error) {
await this.sendMessage(
envelope.chatId,
`Shell command failed: ${error instanceof Error ? error.message : String(error)}`,
);
}
return;
}
}

// Prepend referenced (quoted) message text for reply context
let promptText = envelope.text;
if (envelope.referencedText) {
Expand Down
16 changes: 16 additions & 0 deletions packages/channels/base/src/DaemonChannelBridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export interface DaemonChannelSessionClient {
requestId: string,
response: RequestPermissionResponse,
): Promise<boolean>;
shellCommand?(
command: string,
signal?: AbortSignal,
): Promise<{ exitCode: number | null; output: string; aborted: boolean }>;
}

export interface DaemonChannelSessionFactoryRequest {
Expand Down Expand Up @@ -313,6 +317,18 @@ export class DaemonChannelBridge extends EventEmitter {
}
}

async shellCommand(
sessionId: string,
command: string,
signal?: AbortSignal,
): Promise<{ exitCode: number | null; output: string; aborted: boolean }> {
const session = this.ensureSession(sessionId);
if (!session.shellCommand) {
throw new Error('Shell command not supported by this session client');
}
return session.shellCommand(command, signal);
}

async cancelSession(sessionId: string): Promise<void> {
const session = this.ensureSession(sessionId);
await session.cancel();
Expand Down
30 changes: 30 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2441,6 +2441,36 @@ class QwenAgent implements Agent {
);
return { sessionId, recap };
}
case SERVE_CONTROL_EXT_METHODS.sessionShellHistory: {
const sessionId = params['sessionId'];
if (typeof sessionId !== 'string' || sessionId.length === 0) {
throw RequestError.invalidParams(
undefined,
'Invalid or missing sessionId',
);
}
const command = params['command'];
if (typeof command !== 'string') {
throw RequestError.invalidParams(
undefined,
'Invalid or missing command',
);
}
const session = this.sessionOrThrow(sessionId);
const config = session.getConfig();
const geminiClient = config.getGeminiClient()!;
const outputText =
typeof params['output'] === 'string' ? params['output'] : '';
geminiClient.addHistory({
role: 'user',
parts: [
{
text: `I ran the following shell command:\n\`\`\`sh\n${command}\n\`\`\`\n\nThis produced the following result:\n\`\`\`\n${outputText}\n\`\`\``,

@chiga0 chiga0 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] History injection output not fenced against backtick content

Same concern as bridge.ts — the template literal wraps outputText in triple backticks. If outputText itself contains ``` (common in markdown-aware tools or when cat-ing README files), the code fence breaks and the LLM history gets corrupted.

Use dynamic backtick fencing here too (calculate the longest backtick run in outputText and use longestRun + 1 backticks for the fence). See ChannelBase.ts lines 287-293 for the existing implementation.

This review was generated by QoderWork AI

},
],
});
return { sessionId, injected: true };
}
case 'deleteSession': {
const sessionId = params['sessionId'] as string;
if (!sessionId || !SESSION_ID_RE.test(sessionId)) {
Expand Down
45 changes: 45 additions & 0 deletions packages/cli/src/serve/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,6 +1873,51 @@ export function createServeApp(
}
});

app.post('/session/:id/shell', mutate(), async (req, res) => {

@doudouOUC doudouOUC May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — won't take this one. The shell route deliberately uses non-strict mutate() to match POST /session/:id/prompt, which can also execute arbitrary commands via the LLM's Bash tool. Both routes share the same security posture by design; the prompt route is already the precedent.

@wenshao wenshao May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The PR adds ~327 lines of new code across 16 source files with 0 test files. Critical untested paths include:

  • This POST /session/:id/shell route (validation, abort-on-disconnect, error handling)
  • bridge.executeShellCommand (timeout, output truncation, event publishing, abort propagation)
  • Channel ! handler in ChannelBase.ts (fallback when bridge lacks shellCommand, output formatting)
  • sessionShellHistory ext method in acpAgent.ts (history injection)
  • SDK DaemonClient.shellCommand (URL construction, error handling)

Every comparable route/method in the codebase has dedicated test coverage. Consider adding tests for at least the server route, bridge method, and channel handler.

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC May 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the test coverage gap is real. Deferring to a follow-up PR — this PR is already 16 files and adding tests for bridge/server/normalizer would double the scope.

const sessionId = req.params['id'];
const body = safeBody(req);
const command = body['command'];
if (typeof command !== 'string' || command.trim().length === 0) {
res.status(400).json({
error: '`command` is required and must be a non-empty string',
});
return;
}
const abort = new AbortController();
const onResClose = () => {
if (!res.writableEnded) abort.abort();
};
res.once('close', onResClose);
const clientId = parseClientIdHeader(req, res);
if (clientId === null) {
res.off('close', onResClose);
return;
}
try {
const result = await bridge.executeShellCommand(
sessionId,
command.trim(),
abort.signal,
clientId !== undefined ? { clientId } : undefined,
);
res.status(200).json(result);
} catch (err) {
if (
err instanceof DOMException &&
err.name === 'AbortError' &&
abort.signal.aborted
) {
return;
}
sendBridgeError(res, err, {
route: 'POST /session/:id/shell',
sessionId,
});
} finally {
res.off('close', onResClose);
}
});

app.post(
'/session/:id/approval-mode',
mutate({ strict: true }),
Expand Down
22 changes: 22 additions & 0 deletions packages/sdk-typescript/src/daemon/DaemonClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import type {
DaemonInitWorkspaceResult,
DaemonMcpRestartResult,
DaemonSessionRecapResult,
DaemonShellCommandResult,
DaemonToolToggleResult,
} from './types.js';

Expand Down Expand Up @@ -1066,6 +1067,27 @@ export class DaemonClient {
return (await res.json()) as DaemonSessionRecapResult;
}

async shellCommand(
sessionId: string,
command: string,
opts?: { signal?: AbortSignal; clientId?: string },
): Promise<DaemonShellCommandResult> {
const res = await this._fetch(
`${this.baseUrl}/session/${encodeURIComponent(sessionId)}/shell`,
{
method: 'POST',
headers: this.headers(
{ 'Content-Type': 'application/json' },
opts?.clientId,
),
body: JSON.stringify({ command }),
signal: opts?.signal,
},
);
if (!res.ok) throw await this.failOnError(res, 'POST /session/:id/shell');
return (await res.json()) as DaemonShellCommandResult;
}

/**
* #4175 Wave 4 PR 17. Toggle a tool name in the workspace's
* `tools.disabled` settings list. Strict-gated mutation route — the
Expand Down
Loading