Run agent commands in a subshell so failures cannot close the session (#1945)

* Run agent commands in a subshell so failures cannot close the session

Agent-generated commands were eval'd directly in the user's active login
shell, so a generated script containing `set -e` (or `exit`) terminated
the whole SSH session on the first failure. Wrap the eval in a subshell
unconditionally instead of trying to detect dangerous commands — the
detection approaches from PRs #1852/#1882 grew into shell parsing and
were abandoned. Output stays live in the user's terminal; the trade-off
is that cd/export no longer persist between calls, which the tool
descriptions now tell the model about.

Also fix multi-line command display: synthetic echo now normalizes line
breaks to CRLF (no more staircase indentation in xterm), and the POSIX
wrapper rebuilds newlines via printf so the typed command stays on one
physical line and PS2 continuation echoes ("> ...") no longer leak past
the marker filter.

Fixes #1850

Co-authored-by: Cursor <cursoragent@cursor.com>

* Move shellUtils require to module scope in cattyExecHandlers

The inline require("./ai/shellUtils.cjs") inside the with(ctx) block did
resolve correctly at runtime (bare `require` hits ctx.require, based in
electron/bridges/), but the path only works through that indirection and
reads as wrong relative to this file. Require at module scope with the
unambiguous ../ai path instead.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>

electron/bridges/ai/ptyExec.test.cjs

@@ -1,5 +1,9 @@
 const test = require("node:test");
 const assert = require("node:assert/strict");
+const { spawnSync } = require("node:child_process");
+const { mkdtempSync, rmSync, realpathSync } = require("node:fs");
+const { tmpdir } = require("node:os");
+const { join } = require("node:path");
 
 const {
   resolveEffectiveShellKind,
@@ -98,7 +102,7 @@ test("treats a CR-redrawn last line as the effective prompt, not the doubled str
   );
 });
 
-test("rejects spoofed `PS >` (literal space then `>`) — default PowerShell never emits this", () => {
+test("rejects spoofed `PS >` (literal space then `>`) � default PowerShell never emits this", () => {
   assert.equal(resolveEffectiveShellKind(undefined, "PS >"), "posix");
 });
 
@@ -118,6 +122,83 @@ test("cmd wrapper uses interactive cmd variable expansion", () => {
   assert.doesNotMatch(wrapped, /"%%__NCMCP_TEST___CMD%%"/);
 });
 
+// Issue #1850: agent-generated commands run inside a subshell so that
+// shell-terminating constructs (set -e + failure, exit, ...) end only the
+// subshell, never the user's active login shell / SSH session.
+test("posix wrapper isolates set -e failures from the active shell", () => {
+  const marker = "__NCMCP_TEST__";
+  const wrapped = buildWrappedCommand(
+    "set -e\ncd /nonexistent-dir-1850\necho SHOULD_NOT_PRINT",
+    "posix",
+    marker,
+  );
+  const result = spawnSync("sh", ["-c", `${wrapped}printf 'PARENT_STILL_ALIVE\\n'`], {
+    encoding: "utf8",
+  });
+
+  assert.equal(result.error, undefined);
+  assert.equal(result.status, 0);
+  assert.match(result.stdout, new RegExp(`${marker}_S`));
+  assert.match(result.stdout, new RegExp(`${marker}_E:[1-9]`));
+  assert.match(result.stdout, /PARENT_STILL_ALIVE/);
+  assert.doesNotMatch(result.stdout, /SHOULD_NOT_PRINT/);
+});
+
+test("posix wrapper types multi-line commands as one physical line (no PS2 leak) and preserves semantics", () => {
+  const marker = "__NCMCP_TEST__";
+  const wrapped = buildWrappedCommand(
+    "echo first\necho \"it's quoted\"\n\necho last",
+    "posix",
+    marker,
+  );
+  // A single physical line: the interactive shell must never show PS2
+  // ("> ") continuation echoes, which would leak past the preload filter.
+  assert.equal(wrapped.indexOf("\n"), wrapped.length - 1);
+
+  const result = spawnSync("sh", ["-c", wrapped], { encoding: "utf8" });
+  assert.equal(result.error, undefined);
+  assert.match(result.stdout, /first\n/);
+  assert.match(result.stdout, /it's quoted\n/);
+  assert.match(result.stdout, /last\n/);
+  assert.match(result.stdout, new RegExp(`${marker}_E:0`));
+});
+
+test("posix wrapper isolates explicit exit from the active shell and reports its code", () => {
+  const marker = "__NCMCP_TEST__";
+  const wrapped = buildWrappedCommand("exit 7", "posix", marker);
+  const result = spawnSync("sh", ["-c", `${wrapped}printf 'PARENT_STILL_ALIVE\\n'`], {
+    encoding: "utf8",
+  });
+
+  assert.equal(result.error, undefined);
+  assert.equal(result.status, 0);
+  assert.match(result.stdout, new RegExp(`${marker}_E:7`));
+  assert.match(result.stdout, /PARENT_STILL_ALIVE/);
+});
+
+test("posix wrapper keeps cd contained in the subshell (documented trade-off)", () => {
+  const marker = "__NCMCP_TEST__";
+  const cwd = realpathSync(mkdtempSync(join(tmpdir(), "netcatty-pty-cd-")));
+  try {
+    const wrapped = buildWrappedCommand("cd / && pwd", "posix", marker);
+    const result = spawnSync("sh", ["-c", `${wrapped}pwd`], {
+      encoding: "utf8",
+      cwd,
+    });
+
+    assert.equal(result.error, undefined);
+    assert.equal(result.status, 0);
+    assert.match(result.stdout, new RegExp(`${marker}_E:0`));
+    const lines = result.stdout.trim().split("\n");
+    // The command itself sees the cd take effect (pwd inside prints /)...
+    assert.ok(lines.includes("/"), `expected command pwd "/" in: ${result.stdout}`);
+    // ...but the active shell's cwd is untouched (trailing pwd prints cwd).
+    assert.equal(lines[lines.length - 1], cwd);
+  } finally {
+    rmSync(cwd, { recursive: true, force: true });
+  }
+});
+
 test("execViaChannel registers a pending-cancel marker before the SSH channel opens", () => {
   // Regression for the IPC-transit race surfaced by codex on #1101
   // problem 3: if `cancelPtyExecsForSession` runs while we're still
@@ -128,7 +209,7 @@ test("execViaChannel registers a pending-cancel marker before the SSH channel op
   let execCallback;
   const fakeClient = {
     exec(_command, callback) {
-      // Capture but do not invoke yet — simulates the channel-open
+      // Capture but do not invoke yet � simulates the channel-open
       // delay where the race window lives.
       execCallback = callback;
     },
@@ -182,7 +263,7 @@ test("execViaChannel short-circuits when cancel fires before the SSH channel ope
     if (entry.chatSessionId === "chat-2") entry.cancel();
   }
 
-  // Now the channel "opens" — even though `sshClient.exec` would
+  // Now the channel "opens" � even though `sshClient.exec` would
   // hand us a working stream, we must short-circuit because the user
   // already cancelled.
   const fakeExecStream = {

electron/bridges/ai/ptyExecHelpers.cjs

@@ -177,8 +177,31 @@ function buildWrappedCommand(command, shellKind, marker) {
       //    cannot cause bash to flush the end marker from the input buffer.
       //    trap ':' INT lets child processes receive SIGINT normally while
       //    preventing the shell from aborting the compound command.
+      //
+      // 4) The eval runs inside a subshell ( ... ) so shell-terminating
+      //    constructs in the generated command — set -e / set -o errexit
+      //    followed by a failure, exit, shell option changes, traps,
+      //    function/alias definitions — end or mutate only the subshell,
+      //    never the user's active login shell (issue #1850). set -e still
+      //    behaves normally *inside* the command, and the subshell shares
+      //    the PTY so the user sees all output live. The intentional
+      //    trade-off is that cd/export no longer persist into the user's
+      //    shell or across agent commands; the terminal.execute tool
+      //    description tells the model to combine cd with its command.
+      //    Earlier attempts (PRs #1852/#1882) that instead tried to detect
+      //    dangerous commands grew into shell parsing and were abandoned —
+      //    do not reintroduce detection here.
       const noPager = "PAGER=cat SYSTEMD_PAGER= GIT_PAGER=cat LESS= ";
-      const escaped = escapePosixSingleQuoted(command);
+      // Multi-line commands must not embed raw newlines in the typed
+      // wrapper: the interactive shell would echo PS2 continuation lines
+      // ("> cd ...") that don't contain the marker and leak past the
+      // preload filter. Rebuild the newlines inside the shell instead via
+      // printf so the wrapper stays one physical line. Single-line
+      // commands keep the plain assignment.
+      const commandLines = String(command || "").replace(/\r\n?/g, "\n").split("\n");
+      const cmdAssign = commandLines.length > 1
+        ? `${marker}_cmd=$(printf '%s\\n' ${commandLines.map((line) => `'${escapePosixSingleQuoted(line)}'`).join(" ")})`
+        : `${marker}_cmd='${escapePosixSingleQuoted(command)}'`;
       // Leading single space: lets bash/zsh skip recording this command
       // in history when the user already has HISTCONTROL=ignorespace
       // (bash) or HIST_IGNORE_SPACE (zsh) configured — Debian/Ubuntu and
@@ -187,7 +210,7 @@ function buildWrappedCommand(command, shellKind, marker) {
       // Without that config the prefix is harmless; it just doesn't
       // suppress history recording.
       return (
-        ` ${marker}=0; ${marker}_cmd='${escaped}'; { printf '%s\\n' '${marker}_S'; trap ':' INT; ${noPager}eval "$${marker}_cmd"; __NCMCP_rc=$?; trap - INT; printf '%s\\n' '${marker}_E:'\"$__NCMCP_rc\"; (exit $__NCMCP_rc); }\n`
+        ` ${marker}=0; ${cmdAssign}; { printf '%s\\n' '${marker}_S'; trap ':' INT; ( ${noPager}eval "$${marker}_cmd" ); __NCMCP_rc=$?; trap - INT; printf '%s\\n' '${marker}_E:'\"$__NCMCP_rc\"; (exit $__NCMCP_rc); }\n`
       );
     }
   }

electron/bridges/ai/shellUtils.cjs

@@ -38,6 +38,17 @@ function stripAnsi(input) {
   return String(input || "").replace(ANSI_OSC_REGEX, "").replace(ANSI_ESCAPE_REGEX, "");
 }
 
+// ── Synthetic command echo ──
+//
+// The agent's typed command is not echoed by the PTY as-is (the wrapper
+// line is filtered out in preload), so exec bridges emit a synthetic echo
+// for the user to see. xterm.js treats a bare \n as "move down, keep
+// column", which renders multi-line commands as a staircase. Normalize
+// every line break to \r\n so each line starts at column 0.
+function formatSyntheticEcho(command) {
+  return `${String(command ?? "").replace(/\r?\n/g, "\r\n")}\r\n`;
+}
+
 // Default PowerShell prompt (e.g. `PS C:\Users\alice>`, `PS>`,
 // `PS /home/alice>`). Anchored so command output that merely starts with
 // `PS` (e.g. `PSO>`) doesn't match. The `\S` after `\s+` rejects literal
@@ -838,6 +849,7 @@ function invalidateShellEnvCache() {
 
 module.exports = {
   stripAnsi,
+  formatSyntheticEcho,
   extractTrailingIdlePrompt,
   getFreshIdlePrompt,
   isDefaultPowerShellPromptLine,

electron/bridges/ai/shellUtils.test.cjs

@@ -5,6 +5,7 @@ const {
   addCodexExecutableEnvForSdk,
   buildWindowsShellCommandLine,
   extractTrailingIdlePrompt,
+  formatSyntheticEcho,
   getFreshIdlePrompt,
   isDefaultPowerShellPromptLine,
   isPlausibleCliVersionOutput,
@@ -24,6 +25,17 @@ const fs = require("node:fs");
 const os = require("node:os");
 const path = require("node:path");
 
+test("formatSyntheticEcho normalizes multi-line commands to CRLF so xterm doesn't staircase", () => {
+  assert.equal(
+    formatSyntheticEcho("set -e\ncd /tmp\necho done"),
+    "set -e\r\ncd /tmp\r\necho done\r\n",
+  );
+  // Already-CRLF input is not doubled.
+  assert.equal(formatSyntheticEcho("a\r\nb"), "a\r\nb\r\n");
+  // Single-line commands keep the original shape.
+  assert.equal(formatSyntheticEcho("npm test"), "npm test\r\n");
+});
+
 test("extracts a trailing PowerShell idle prompt", () => {
   assert.equal(
     extractTrailingIdlePrompt("Microsoft Windows...\r\nPS C:\\Users\\alice>"),

electron/bridges/aiBridge/cattyExecHandlers.cjs

@@ -1,4 +1,9 @@
 /* eslint-disable no-undef */
+// Module-level require on purpose: code inside registerCattyExecHandlers
+// runs under `with (ctx)` where bare `require` resolves to ctx.require
+// (based in electron/bridges/). Requiring here keeps the path unambiguous.
+const { formatSyntheticEcho } = require("../ai/shellUtils.cjs");
+
 function getWorkerExecutionMeta(mcpServerBridge, sessionId, chatSessionId) {
   return mcpServerBridge.getSessionMeta?.(sessionId, chatSessionId) || {};
 }
@@ -164,7 +169,7 @@ function registerCattyExecHandlers(ctx) {
             const contents = electronModule?.webContents?.fromId?.(session.webContentsId);
             safeSend(contents, "netcatty:data", {
               sessionId,
-              data: `${rawCommand}\r\n`,
+              data: formatSyntheticEcho(rawCommand),
               syntheticEcho: true,
             });
           },

electron/bridges/mcpServerBridge.cjs

@@ -12,7 +12,7 @@ const fs = require("node:fs");
 const path = require("node:path");
 const { existsSync } = require("node:fs");
 
-const { toUnpackedAsarPath, getFreshIdlePrompt } = require("./ai/shellUtils.cjs");
+const { toUnpackedAsarPath, getFreshIdlePrompt, formatSyntheticEcho } = require("./ai/shellUtils.cjs");
 const { appendVaultAgentGuidance } = require("../shared/vaultAgentGuidance.cjs");
 const { execViaPty, startPtyJob, execViaChannel, execViaRawPty } = require("./ai/ptyExec.cjs");
 const { safeSend } = require("./ipcUtils.cjs");
@@ -318,7 +318,7 @@ function echoCommandToSession(session, sessionId, command) {
   const contents = electronModule.webContents?.fromId?.(session.webContentsId);
   safeSend(contents, "netcatty:data", {
     sessionId,
-    data: `${command}\r\n`,
+    data: formatSyntheticEcho(command),
     syntheticEcho: true,
   });
 }

electron/capabilities/schemas/toolInputs.cjs

@@ -285,9 +285,9 @@ const TOOL_INPUT_FIELDS = Object.freeze({
 /** Long-form model guidance appended to terminal tool descriptions from catalog. */
 const MODEL_DESCRIPTION_HINTS = Object.freeze({
   "terminal.execute":
-    "Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll.",
+    "Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll. Commands run in an isolated subshell of the visible terminal: the user sees the output live, but shell state such as cd, export, or set does not persist between calls — use absolute paths or combine cd with the command (cd /path && cmd).",
   "terminal.start":
-    "Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes.",
+    "Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes. Shell state such as cd or export does not persist between calls — combine cd with the command.",
   "terminal.poll":
     "Wait at least about 30 seconds between polls unless output justifies checking sooner.",
   "vault.host.notes.get":

electron/terminalWorker/aiExec.cjs

@@ -7,7 +7,7 @@ const {
   execViaChannel,
   execViaRawPty,
 } = require("../bridges/ai/ptyExec.cjs");
-const { getFreshIdlePrompt } = require("../bridges/ai/shellUtils.cjs");
+const { getFreshIdlePrompt, formatSyntheticEcho } = require("../bridges/ai/shellUtils.cjs");
 
 const DEFAULT_BACKGROUND_JOB_TIMEOUT_MS = 60 * 60 * 1000;
 const DEFAULT_BACKGROUND_JOB_POLL_INTERVAL_MS = 30 * 1000;
@@ -270,7 +270,7 @@ function createWorkerAiExecHandler({
         echoCommand: (rawCommand) => {
           event?.sender?.send?.("netcatty:data", {
             sessionId,
-            data: `${rawCommand}\r\n`,
+            data: formatSyntheticEcho(rawCommand),
             syntheticEcho: true,
           });
         },
@@ -375,7 +375,7 @@ function createWorkerAiJobStartHandler({
         echoCommand: (rawCommand) => {
           event?.sender?.send?.("netcatty:data", {
             sessionId,
-            data: `${rawCommand}\r\n`,
+            data: formatSyntheticEcho(rawCommand),
             syntheticEcho: true,
           });
         },

infrastructure/ai/harness/generated/cattyToolSpecs.json

@@ -66,7 +66,7 @@
     "toolName": "terminal_execute",
     "rpcMethod": "netcatty/exec",
     "localExecution": false,
-    "description": "Execute a short command in a terminal session and wait for completion. Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll.",
+    "description": "Execute a short command in a terminal session and wait for completion. Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll. Commands run in an isolated subshell of the visible terminal: the user sees the output live, but shell state such as cd, export, or set does not persist between calls — use absolute paths or combine cd with the command (cd /path && cmd).",
     "inputShape": {
       "sessionId": {
         "type": "string",
@@ -94,7 +94,7 @@
     "toolName": "terminal_start",
     "rpcMethod": "netcatty/jobStart",
     "localExecution": false,
-    "description": "Start a long-running command in a terminal session. Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes.",
+    "description": "Start a long-running command in a terminal session. Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes. Shell state such as cd or export does not persist between calls — combine cd with the command.",
     "inputShape": {
       "sessionId": {
         "type": "string",

infrastructure/ai/harness/generated/globalAgentToolSpecs.json

@@ -69,7 +69,7 @@
     "toolName": "terminal_execute",
     "rpcMethod": "netcatty/exec",
     "localExecution": false,
-    "description": "Execute a short command in a terminal session and wait for completion. Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll.",
+    "description": "Execute a short command in a terminal session and wait for completion. Use only for commands expected to finish within about 60 seconds. For long-running commands use terminal_start and terminal_poll. Commands run in an isolated subshell of the visible terminal: the user sees the output live, but shell state such as cd, export, or set does not persist between calls — use absolute paths or combine cd with the command (cd /path && cmd).",
     "inputShape": {
       "sessionId": {
         "type": "string",
@@ -98,7 +98,7 @@
     "toolName": "terminal_start",
     "rpcMethod": "netcatty/jobStart",
     "localExecution": false,
-    "description": "Start a long-running command in a terminal session. Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes.",
+    "description": "Start a long-running command in a terminal session. Prefer for builds, scans, log-following, or anything likely to exceed about 2 minutes. Shell state such as cd or export does not persist between calls — combine cd with the command.",
     "inputShape": {
       "sessionId": {
         "type": "string",