diff --git a/electron/bridges/ai/ptyExec.test.cjs b/electron/bridges/ai/ptyExec.test.cjs index aeed2e845..b4adaaf72 100644 --- a/electron/bridges/ai/ptyExec.test.cjs +++ b/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 = { diff --git a/electron/bridges/ai/ptyExecHelpers.cjs b/electron/bridges/ai/ptyExecHelpers.cjs index b5ace564f..f788ad910 100644 --- a/electron/bridges/ai/ptyExecHelpers.cjs +++ b/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` ); } } diff --git a/electron/bridges/ai/shellUtils.cjs b/electron/bridges/ai/shellUtils.cjs index b740d0be5..4e5d4f010 100644 --- a/electron/bridges/ai/shellUtils.cjs +++ b/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, diff --git a/electron/bridges/ai/shellUtils.test.cjs b/electron/bridges/ai/shellUtils.test.cjs index 951cd6ec0..6bf461a83 100644 --- a/electron/bridges/ai/shellUtils.test.cjs +++ b/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>"), diff --git a/electron/bridges/aiBridge/cattyExecHandlers.cjs b/electron/bridges/aiBridge/cattyExecHandlers.cjs index a36680e48..d10abd9c6 100644 --- a/electron/bridges/aiBridge/cattyExecHandlers.cjs +++ b/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, }); }, diff --git a/electron/bridges/mcpServerBridge.cjs b/electron/bridges/mcpServerBridge.cjs index fb4eb3198..21457ea7d 100644 --- a/electron/bridges/mcpServerBridge.cjs +++ b/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, }); } diff --git a/electron/capabilities/schemas/toolInputs.cjs b/electron/capabilities/schemas/toolInputs.cjs index 5a776b082..659bafb2c 100644 --- a/electron/capabilities/schemas/toolInputs.cjs +++ b/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": diff --git a/electron/terminalWorker/aiExec.cjs b/electron/terminalWorker/aiExec.cjs index 11adc4b82..d7fd716aa 100644 --- a/electron/terminalWorker/aiExec.cjs +++ b/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, }); }, diff --git a/infrastructure/ai/harness/generated/cattyToolSpecs.json b/infrastructure/ai/harness/generated/cattyToolSpecs.json index 0585783af..d64d87ed6 100644 --- a/infrastructure/ai/harness/generated/cattyToolSpecs.json +++ b/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", diff --git a/infrastructure/ai/harness/generated/globalAgentToolSpecs.json b/infrastructure/ai/harness/generated/globalAgentToolSpecs.json index 2040c5aab..6d259a418 100644 --- a/infrastructure/ai/harness/generated/globalAgentToolSpecs.json +++ b/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",