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.

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.

Thanks — deferring. Unit tests for the normalizer event types will be added in a follow-up PR.

[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

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.

[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

[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

[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

[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';
}

2. 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

[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

[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

[Nit] command.trim() removes intentional leading whitespace

The server route also applies command.trim() (server.ts line 1888), which strips leading/trailing whitespace from the user's input. For most commands this is fine, but ! echo hello becomes echo hello, and ! echo ' spaces ' would work correctly since only the outer trim applies.

This is consistent with how the CLI handles shell commands, so it may be intentional. Just noting it as a potential edge case if users expect to paste commands with intentional leading whitespace.

This review was generated by QoderWork AI

[Nit] Extra blank lines after removing formatShellCommandPrompt

After removing the formatShellCommandPrompt function, two blank lines remain (lines 1085-1086). Minor cleanup — one trailing blank line before the file end is the common convention.

This review was generated by QoderWork AI

[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