Merge remote-tracking branch 'origin/main' into codex/pr-3355-review-2979

electron/bridges/sftpBridge.cjs

@@ -55,6 +55,7 @@ let sftpClients = null;
 let electronModule = null;
 let sessions = null;
 let reportOpenedSessionActivity = null;
+let reportSuppressedError = null;
 const rendererSftpSourceSessions = new Map();
 const REMOTE_DELETE_EXEC_OPEN_TIMEOUT_MS = 15_000;
 const REMOTE_DELETE_EXEC_RUN_TIMEOUT_MS = 10 * 60_000;
@@ -734,6 +735,9 @@ function init(deps) {
   reportOpenedSessionActivity = typeof deps.reportOpenedSessionActivity === "function"
     ? deps.reportOpenedSessionActivity
     : null;
+  reportSuppressedError = typeof deps.reportSuppressedError === "function"
+    ? deps.reportSuppressedError
+    : null;
   rendererSftpSourceSessions.clear();
 }
 
@@ -2015,10 +2019,14 @@ async function openSftpForSession(_event, payload) {
     }
 
     try {
-      await requireSftpChannel(client, {
+      // This is a fresh client: retain the initial channel-open error instead
+      // of passing through recovery, which replaces it with a generic error.
+      const channel = await tryOpenSftpChannel(client, {
         signal: payload?.abortSignal,
         timeoutMs: payload?.timeoutMs,
       });
+      if (!hasSftpChannelApi(channel)) throw new Error("SFTP channel unavailable");
+      client.sftp = channel;
       client.__netcattyFileProtocol = "sftp";
     } catch (sftpErr) {
       if (fileProtocol === "sftp") throw sftpErr;
@@ -2027,6 +2035,23 @@ async function openSftpForSession(_event, payload) {
         `[SFTP] openSftpForSession SFTP channel failed for ${sessionId}; falling back to SCP mode:`,
         sftpErr?.message || String(sftpErr),
       );
+      // The terminal worker has no Electron app: forward to main's logger.
+      try {
+        const source = "sftpBridge.openSftpForSession";
+        const message = `SFTP channel failed for ${sourceSessionId}; falling back to SCP mode (${fileProtocol})`;
+        if (reportSuppressedError) {
+          reportSuppressedError(source, sftpErr, message);
+        } else {
+          require("./crashLogBridge.cjs").captureDiagnostic(source, message, {
+            sessionId: sourceSessionId,
+            fileProtocol,
+            reason: sftpErr?.message || String(sftpErr),
+            reasonCode: sftpErr?.code ?? sftpErr?.level ?? undefined,
+          });
+        }
+      } catch {
+        // Diagnostic failures must not prevent the fallback.
+      }
       client.__netcattyFileProtocol = "scp";
       client.sftp = null;
       try {

electron/bridges/sftpBridge.fallback.test.cjs

@@ -0,0 +1,47 @@
+"use strict";
+
+const assert = require("node:assert/strict");
+const { EventEmitter } = require("node:events");
+const test = require("node:test");
+const bridge = require("./sftpBridge.cjs");
+
+for (const loggerThrows of [false, true]) {
+  test(`Auto fallback reports the channel failure; loggerThrows=${loggerThrows}`, async () => {
+    const failure = Object.assign(new Error("subsystem request rejected"), { code: "CHANNEL_OPEN_FAILURE" });
+    const reports = [];
+    const conn = {
+      sftp(callback) { callback(failure); },
+      exec(command, callback) {
+        const stream = new EventEmitter();
+        stream.stderr = new EventEmitter();
+        stream.close = () => {};
+        callback(null, stream);
+        queueMicrotask(() => {
+          if (!command.includes("command -v scp")) {
+            stream.emit("data", Buffer.from("RAW:/home/test\n"));
+          }
+          stream.emit("close", 0);
+        });
+      },
+    };
+    bridge.init({
+      electronModule: { webContents: { fromId: () => null } },
+      sessions: new Map([["fallback-session", { conn }]]),
+      sftpClients: new Map(),
+      reportSuppressedError(...args) {
+        reports.push(args);
+        if (loggerThrows) throw new Error("logger unavailable");
+      },
+    });
+    const opened = await bridge.openSftpForSession(null, { sessionId: "fallback-session" });
+    try {
+      assert.equal(opened.fileProtocol, "scp");
+      assert.equal(reports.length, 1);
+      assert.equal(reports[0][0], "sftpBridge.openSftpForSession");
+      assert.equal(reports[0][1], failure);
+      assert.match(reports[0][2], /fallback-session.*SCP mode \(auto\)/);
+    } finally {
+      await bridge.closeSftp(null, { sftpId: opened.sftpId });
+    }
+  });
+}

electron/bridges/sftpBridge/scpBackend.cjs

@@ -190,8 +190,16 @@ function createScpBackend(deps = {}) {
   async function stat(remotePath, options = {}) {
     const signal = options.signal || null;
     const encoding = options.encoding || "utf-8";
-    const result = await runOrThrow(buildStatCommand(remotePath, encoding), { signal });
-    return parseStatRecord(result.stdout);
+    // allowNonZero: parseStatRecord needs the exit code + stderr to tell a real
+    // ENOENT (exit 2) apart from an empty exec-channel response.
+    const result = await runOrThrow(buildStatCommand(remotePath, encoding), {
+      allowNonZero: true,
+      signal,
+    });
+    return parseStatRecord(result.stdout, {
+      stderr: result.stderr,
+      exitCode: result.code,
+    });
   }
 
   async function mkdir(remotePath, options = {}) {

electron/bridges/sftpBridge/scpBackend.test.cjs

@@ -175,6 +175,137 @@ describe("scpBackend browse/manage with fake exec", () => {
     assert.match(statCommand, /\[ ! -L "\$p" \]/, "broken symlinks must not be reported as missing");
   });
 
+  it("reports ENOENT when the stat command exits 2 with an ENOENT marker", async () => {
+    const missingBackend = createScpBackend({
+      exec: async () => ({ stdout: "", stderr: "ENOENT\n", code: 2 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => missingBackend.stat("/home/test/gone.txt"),
+      (err) => err.code === "ENOENT" && err.message === "No such file",
+    );
+  });
+
+  it("reports ENOENT when a shell banner precedes the marker on stderr", async () => {
+    // Codex: non-interactive shells may print warnings before the command, so
+    // the ENOENT marker can appear on a later stderr line.
+    const bannerBackend = createScpBackend({
+      exec: async () => ({ stdout: "", stderr: "warning: env cleared\nENOENT\n", code: 2 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => bannerBackend.stat("/home/test/gone.txt"),
+      (err) => err.code === "ENOENT" && err.message === "No such file",
+    );
+  });
+
+  it("reports ENOENT when stdout contains a login banner", async () => {
+    const bannerBackend = createScpBackend({
+      exec: async () => ({ stdout: "Welcome\n", stderr: "ENOENT\n", code: 2 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => bannerBackend.stat("/home/test/gone.txt"),
+      (err) => err.code === "ENOENT",
+    );
+  });
+
+  it("does not mask an empty exec response as ENOENT in stat", async () => {
+    const emptyBackend = createScpBackend({
+      // Simulates the reporter's case: the exec channel returns no output at
+      // all (e.g. channel negotiation failure) — not a missing file.
+      exec: async () => ({ stdout: "", stderr: "", code: 0 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => emptyBackend.stat("/home/test/readme.txt"),
+      (err) => {
+        assert.equal(err.code, "EMPTY_RESPONSE");
+        assert.match(err.message, /empty response/);
+        assert.ok(!(err instanceof Error && err.message === "No such file"));
+        return true;
+      },
+    );
+  });
+
+  it("surfaces stderr and exit code when the stat command fails without output", async () => {
+    const failingBackend = createScpBackend({
+      exec: async () => ({ stdout: "", stderr: "sh: syntax error\n", code: 1 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => failingBackend.stat("/home/test/readme.txt"),
+      (err) => {
+        assert.equal(err.code, "EMPTY_RESPONSE");
+        assert.match(err.message, /exit 1/);
+        assert.match(err.message, /syntax error/);
+        assert.equal(err.exitCode, 1);
+        assert.equal(err.stderr, "sh: syntax error");
+        return true;
+      },
+    );
+  });
+
+  it("does not treat exit 2 without an ENOENT marker as a missing file", async () => {
+    // Codex: a remote shell failure (e.g. syntax/usage error) can also exit 2;
+    // without the ENOENT marker it must surface the real stderr, not ENOENT.
+    const shellFailBackend = createScpBackend({
+      exec: async () => ({ stdout: "", stderr: "sh: syntax error\n", code: 2 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => shellFailBackend.stat("/home/test/readme.txt"),
+      (err) => {
+        assert.equal(err.code, "EMPTY_RESPONSE");
+        assert.match(err.message, /exit 2/);
+        assert.match(err.message, /syntax error/);
+        assert.equal(err.exitCode, 2);
+        assert.equal(err.stderr, "sh: syntax error");
+        return true;
+      },
+    );
+  });
+
+  it("does not treat a stdout ENOENT marker with a nonzero, non-2 exit as missing", async () => {
+    // Codex: a forced-command wrapper can print "ENOENT" on stdout before
+    // failing with another status; it must not be reported as ENOENT.
+    const wrapperBackend = createScpBackend({
+      exec: async () => ({ stdout: "ENOENT\n", stderr: "denied\n", code: 126 }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => wrapperBackend.stat("/home/test/readme.txt"),
+      (err) => {
+        assert.notEqual(err.code, "ENOENT");
+        assert.equal(err.exitCode, 126);
+        return true;
+      },
+    );
+  });
+
+  it("rejects stat metadata forwarded by a wrapper that exits nonzero", async () => {
+    // Codex: a forced-command wrapper can forward the stat record but exit
+    // nonzero; the metadata is untrustworthy and must not be returned.
+    const wrapperBackend = createScpBackend({
+      exec: async () => ({
+        stdout: "f|-rw-r--r--|5|1700000000|/home/test/readme.txt|12345\n",
+        stderr: "forced command failed\n",
+        code: 1,
+      }),
+      execStream: async () => createMockStream(),
+    });
+    await assert.rejects(
+      () => wrapperBackend.stat("/home/test/readme.txt"),
+      (err) => {
+        assert.match(err.message, /exited 1/);
+        assert.match(err.message, /forced command failed/);
+        assert.equal(err.exitCode, 1);
+        assert.equal(err.stderr, "forced command failed");
+        return true;
+      },
+    );
+  });
+
   it("resolves home directory", async () => {
     const home = await backend.homeDir();
     assert.equal(home, "/home/test");

electron/bridges/sftpBridge/scpShell.cjs

@@ -327,16 +327,57 @@ function parseLsLaOutput(stdout, { basePath = "" } = {}) {
   return results;
 }
 
-function parseStatRecord(stdout) {
+function parseStatRecord(stdout, { stderr = "", exitCode = null } = {}) {
   const line = String(stdout || "").trim().split(/\r?\n/)[0] || "";
+  const stderrText = String(stderr || "").trim();
+  // A missing path emits an ENOENT marker and exits 2. Shell startup
+  // messages may precede it on either stream; exit status alone is not enough.
+  const hasMissingMarker = line === "ENOENT"
+    || stderrText.split(/\r?\n/).some((entry) => entry.trim() === "ENOENT");
+  if (hasMissingMarker && (exitCode == null || exitCode === 2)) {
+    throw new ScpShellError("No such file", "ENOENT");
+  }
   if (!line || line === "ENOENT") {
-    const err = new ScpShellError("No such file", "ENOENT");
-    err.code = "ENOENT";
+    const detail = [
+      "exec channel returned empty response",
+      exitCode != null ? `exit ${exitCode}` : null,
+      stderrText ? `stderr: ${stderrText.slice(0, 200)}` : null,
+    ].filter(Boolean).join("; ");
+    const err = new ScpShellError(detail, "EMPTY_RESPONSE");
+    err.code = "EMPTY_RESPONSE";
+    err.exitCode = exitCode;
+    err.stderr = stderrText;
+    throw err;
+  }
+  // Any other nonzero status is a remote execution failure — even when stdout
+  // looks like a valid record (e.g. a forced-command wrapper that forwards the
+  // stat record before failing). Such metadata is untrustworthy, so reject it
+  // instead of letting the caller proceed on it.
+  if (exitCode != null && exitCode !== 0) {
+    const detail = [
+      `stat command exited ${exitCode}`,
+      stderrText ? `stderr: ${stderrText.slice(0, 200)}` : null,
+    ].filter(Boolean).join("; ");
+    const err = new ScpShellError(detail);
+    err.exitCode = exitCode;
+    err.stderr = stderrText;
     throw err;
   }
   const parts = line.split("|");
   if (parts.length < 5) {
-    throw new ScpShellError(`Malformed stat record: ${line.slice(0, 80)}`);
+    // Non-empty stdout that isn't a stat record: the remote command may have
+    // failed (nonzero exit, e.g. a restricted shell printing a banner) while
+    // still writing to stdout. Preserve exit code and stderr so failures with
+    // output aren't reduced to a bare "malformed" message.
+    const detail = [
+      `Malformed stat record: ${line.slice(0, 80)}`,
+      exitCode != null ? `exit ${exitCode}` : null,
+      stderrText ? `stderr: ${stderrText.slice(0, 200)}` : null,
+    ].filter(Boolean).join("; ");
+    const err = new ScpShellError(detail);
+    err.exitCode = exitCode;
+    err.stderr = stderrText;
+    throw err;
   }
   const [t, modeStr, sizeStr, mtimeStr, abs, inoStr] = parts;
   const ino = inoStr && /^\d+$/.test(String(inoStr).trim())

electron/terminalWorker/process.cjs

@@ -301,6 +301,7 @@ function main() {
     selectZmodemUploadFiles,
     selectZmodemDownloadDirectory,
     terminalDataPipeline,
+    reportSuppressedError: reportWorkerError,
   };
 
   runtime = createTerminalWorkerRuntime({