diff --git a/src/commands.ts b/src/commands.ts index 2add1997..1f48d36a 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -930,6 +930,12 @@ export class Commands { const configDir = this.pathResolver.getGlobalConfigDir(safeHost); const configs = vscode.workspace.getConfiguration(); const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); + // Same threat model as the connection-time write in remote.ts: token + // goes to the file (or keyring on supported systems), never env, since + // child env is sibling-readable on most platforms. + await this.cliManager.configure(baseUrl, client.getSessionToken() ?? "", { + silent: true, + }); return { binary, configs, auth, featureSet }; } diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index acf2d207..6eaef21a 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -56,16 +56,13 @@ export class CliCredentialManager { * setting is enabled and the CLI supports it; otherwise writes plaintext * files under --global-config. * - * Keyring and files are mutually exclusive — never both. - * - * When `keyringOnly` is set, silently returns if the keyring is unavailable - * instead of falling back to file storage. + * Keyring and files are mutually exclusive, never both. */ public async storeToken( url: string, token: string, configs: Pick, - options?: { signal?: AbortSignal; keyringOnly?: boolean }, + options?: { signal?: AbortSignal }, ): Promise { const binPath = await this.resolveKeyringBinary( url, @@ -73,9 +70,6 @@ export class CliCredentialManager { "keyringAuth", ); if (!binPath) { - if (options?.keyringOnly) { - return; - } await this.writeCredentialFiles(url, token); return; } diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index dcdd0807..7f92d86e 100644 --- a/src/core/cliExec.ts +++ b/src/core/cliExec.ts @@ -2,7 +2,7 @@ import { type ExecFileException, execFile, spawn } from "node:child_process"; import { promisify } from "node:util"; import * as vscode from "vscode"; -import { toError } from "../error/errorUtils"; +import { isAbortError, toError } from "../error/errorUtils"; import { type CliAuth, getGlobalFlags, @@ -17,23 +17,6 @@ export interface CliEnv { configs: Pick; } -const execFileAsync = promisify(execFile); - -function isExecFileException(error: unknown): error is ExecFileException { - return error instanceof Error && "code" in error; -} - -/** Prefer stderr over the default message which includes the full command line. */ -function cliError(error: unknown): Error { - if (isExecFileException(error)) { - const stderr = error.stderr?.trim(); - if (stderr) { - return new Error(stderr, { cause: error }); - } - } - return toError(error); -} - /** * Return the version from the binary. Throw if unable to execute the binary or * find the version for any reason. @@ -135,6 +118,48 @@ export function ping(env: CliEnv, workspaceName: string): vscode.Terminal { }); } +/** + * SSH into a workspace and run a command via `terminal.sendText`. + */ +export async function openAppStatusTerminal( + env: CliEnv, + app: { + name?: string; + command?: string; + workspace_name: string; + }, +): Promise { + const globalFlags = getGlobalShellFlags(env.configs, env.auth); + const terminal = vscode.window.createTerminal({ name: app.name }); + terminal.sendText( + `${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${escapeCommandArg(app.workspace_name)}`, + ); + await new Promise((resolve) => setTimeout(resolve, 5000)); + terminal.sendText(app.command ?? ""); + terminal.show(false); +} + +const execFileAsync = promisify(execFile); + +/** Prefer stderr over the default message which includes the full command line. */ +function cliError(error: unknown): Error { + // Pass aborts through; wrapping erases the AbortError name and would surface stale CLI warnings as the failure. + if (isAbortError(error)) { + return error; + } + if (isExecFileException(error)) { + const stderr = error.stderr?.trim(); + if (stderr) { + return new Error(stderr, { cause: error }); + } + } + return toError(error); +} + +function isExecFileException(error: unknown): error is ExecFileException { + return error instanceof Error && "code" in error; +} + /** * Spawn a CLI command in a PTY terminal with process lifecycle management. */ @@ -142,7 +167,7 @@ function spawnCliInTerminal(options: { name: string; binary: string; args: string[]; - banner?: string[]; + banner: string[]; }): vscode.Terminal { const writeEmitter = new vscode.EventEmitter(); const closeEmitter = new vscode.EventEmitter(); @@ -186,10 +211,8 @@ function spawnCliInTerminal(options: { onDidWrite: writeEmitter.event, onDidClose: closeEmitter.event, open: () => { - if (options.banner) { - for (const line of options.banner) { - writeEmitter.fire(line + "\r\n"); - } + for (const line of options.banner) { + writeEmitter.fire(line + "\r\n"); } }, close: () => { @@ -262,24 +285,3 @@ function spawnCliInTerminal(options: { terminal.show(false); return terminal; } - -/** - * SSH into a workspace and run a command via `terminal.sendText`. - */ -export async function openAppStatusTerminal( - env: CliEnv, - app: { - name?: string; - command?: string; - workspace_name: string; - }, -): Promise { - const globalFlags = getGlobalShellFlags(env.configs, env.auth); - const terminal = vscode.window.createTerminal(app.name); - terminal.sendText( - `${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${escapeCommandArg(app.workspace_name)}`, - ); - await new Promise((resolve) => setTimeout(resolve, 5000)); - terminal.sendText(app.command ?? ""); - terminal.show(false); -} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 9be0e1f5..022d29a7 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -817,6 +817,9 @@ export class CliManager { * * Both URL and token are required. Empty tokens are allowed (e.g. mTLS * authentication) but the URL must be a non-empty string. + * + * @param options.silent Suppress the progress notification only; failures + * still surface via {@link handleStoreError} (logged + error toast). */ public async configure( url: string, diff --git a/src/error/errorUtils.ts b/src/error/errorUtils.ts index 18283b90..8f16cf44 100644 --- a/src/error/errorUtils.ts +++ b/src/error/errorUtils.ts @@ -4,7 +4,7 @@ import util from "node:util"; import { toError as baseToError } from "@repo/shared"; /** Check whether an unknown thrown value is an AbortError (signal cancellation). */ -export function isAbortError(error: unknown): boolean { +export function isAbortError(error: unknown): error is Error { return error instanceof Error && error.name === "AbortError"; } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index d894c383..018d9558 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -153,17 +153,11 @@ export class LoginCoordinator implements vscode.Disposable { }); await this.mementoManager.addToUrlHistory(url); - // Fire-and-forget: sync token to OS keyring for the CLI. if (result.token) { this.cliCredentialManager - .storeToken(url, result.token, vscode.workspace.getConfiguration(), { - keyringOnly: true, - }) + .storeToken(url, result.token, vscode.workspace.getConfiguration()) .catch((error) => { - this.logger.warn( - "Failed to store token in keyring at login:", - error, - ); + this.logger.warn("Failed to store token at login:", error); }); } } diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 4130b150..1167654b 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -269,42 +269,6 @@ describe("CliCredentialManager", () => { }), ).rejects.toThrow("The operation was aborted"); }); - - it.each([ - { scenario: "keyring disabled", keyringEnabled: false }, - { scenario: "CLI version too old", keyringEnabled: true }, - ])( - "never writes files when keyringOnly and $scenario", - async ({ keyringEnabled }) => { - vi.mocked(isKeyringEnabled).mockReturnValue(keyringEnabled); - if (keyringEnabled) { - vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); - } - const { manager } = setup(); - - await manager.storeToken(TEST_URL, "token", configs, { - keyringOnly: true, - }); - - expect(execFile).not.toHaveBeenCalled(); - expect(memfs.existsSync(URL_FILE)).toBe(false); - expect(memfs.existsSync(SESSION_FILE)).toBe(false); - }, - ); - - it("uses keyring without writing files when keyringOnly and keyring available", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - const { manager } = setup(); - - await manager.storeToken(TEST_URL, "my-token", configs, { - keyringOnly: true, - }); - - expect(execFile).toHaveBeenCalled(); - expect(memfs.existsSync(URL_FILE)).toBe(false); - expect(memfs.existsSync(SESSION_FILE)).toBe(false); - }); }); describe("readToken", () => { diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index 995e9e6e..7d1fcbd8 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -4,7 +4,13 @@ import path from "path"; import { beforeAll, describe, expect, it, vi } from "vitest"; import { MockConfigurationProvider } from "../../mocks/testHelpers"; -import { isWindows, quoteCommand, writeExecutable } from "../../utils/platform"; +import { + isWindows, + quoteCommand, + writeExecutable, + writeStderrJs, + writeStdoutJs, +} from "../../utils/platform"; import type { CliEnv } from "@/core/cliExec"; @@ -36,7 +42,7 @@ describe("cliExec", () => { /** JS code for a fake CLI that writes a fixed string to stdout. */ function echoBin(output: string): string { - return `process.stdout.write(${JSON.stringify(output)});`; + return writeStdoutJs(output); } /** @@ -46,10 +52,11 @@ describe("cliExec", () => { function oldCliBin(stderr: string, stdout: string): string { return [ `if (process.argv.includes("--output")) {`, - ` process.stderr.write(${JSON.stringify(stderr)});`, - ` process.exitCode = 1;`, + ` ${writeStderrJs(stderr)}`, + ` process.exit(1);`, `} else {`, - ` process.stdout.write(${JSON.stringify(stdout)});`, + ` ${writeStdoutJs(stdout)}`, + ` process.exit(0);`, `}`, ].join("\n"); } @@ -159,8 +166,8 @@ describe("cliExec", () => { it("surfaces stderr instead of full command line on failure", async () => { const code = [ - `process.stderr.write("invalid argument for -t flag\\n");`, - `process.exitCode = 1;`, + writeStderrJs("invalid argument for -t flag\n"), + `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "speedtest-err", code); const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); @@ -168,6 +175,18 @@ describe("cliExec", () => { cliExec.speedtest(env, "owner/workspace", "bad"), ).rejects.toThrow("invalid argument for -t flag"); }); + + it("preserves AbortError name when cancelled via signal", async () => { + // Hangs forever so the only way out is the abort signal. + const code = `setInterval(() => {}, 1000);`; + const bin = await writeExecutable(tmp, "speedtest-hang", code); + const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); + const ac = new AbortController(); + ac.abort(); + await expect( + cliExec.speedtest(env, "owner/workspace", undefined, ac.signal), + ).rejects.toMatchObject({ name: "AbortError" }); + }); }); describe("supportBundle", () => { @@ -204,8 +223,8 @@ describe("cliExec", () => { it("surfaces stderr on failure", async () => { const code = [ - `process.stderr.write("workspace not found\\n");`, - `process.exitCode = 1;`, + writeStderrJs("workspace not found\n"), + `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "sb-err", code); const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); diff --git a/test/unit/error/errorUtils.test.ts b/test/unit/error/errorUtils.test.ts index b201954a..1aa14617 100644 --- a/test/unit/error/errorUtils.test.ts +++ b/test/unit/error/errorUtils.test.ts @@ -1,6 +1,47 @@ import { describe, it, expect } from "vitest"; -import { getErrorDetail, toError } from "@/error/errorUtils"; +import { getErrorDetail, isAbortError, toError } from "@/error/errorUtils"; + +describe("isAbortError", () => { + it("returns true for an Error named AbortError", () => { + const err = new Error("aborted"); + err.name = "AbortError"; + expect(isAbortError(err)).toBe(true); + }); + + it("returns true for DOMException-style abort thrown by AbortController", () => { + const ac = new AbortController(); + ac.abort(); + // `signal.reason` is a DOMException with name "AbortError" in modern Node. + const reason = ac.signal.reason; + expect(isAbortError(reason)).toBe(true); + }); + + it("returns false for a plain Error", () => { + expect(isAbortError(new Error("nope"))).toBe(false); + }); + + it.each<[string, unknown]>([ + ["null", null], + ["undefined", undefined], + ["string", "AbortError"], + ["object with name only", { name: "AbortError" }], + ])("returns false for %s", (_name, input) => { + expect(isAbortError(input)).toBe(false); + }); + + it("narrows the type to Error", () => { + const err: unknown = Object.assign(new Error("aborted"), { + name: "AbortError", + }); + if (isAbortError(err)) { + // Type-only assertion: this line must compile without a cast. + expect(err.message).toBe("aborted"); + } else { + throw new Error("expected isAbortError to narrow"); + } + }); +}); describe("getErrorDetail", () => { it("returns detail from API error", () => { diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index fa6a2aed..cf376a97 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -500,7 +500,7 @@ describe("LoginCoordinator", () => { return { ...ctx, user, login }; } - it("calls storeToken with keyringOnly after successful login", async () => { + it("calls storeToken after successful login", async () => { const { mockCredentialManager, login } = await loginWithStoredToken(); await login(); @@ -509,7 +509,6 @@ describe("LoginCoordinator", () => { TEST_URL, "stored-token", expect.anything(), - { keyringOnly: true }, ); }); diff --git a/test/utils/platform.test.ts b/test/utils/platform.test.ts index 71e09d08..35bbc866 100644 --- a/test/utils/platform.test.ts +++ b/test/utils/platform.test.ts @@ -13,6 +13,7 @@ import { printEnvCommand, shimExecFile, writeExecutable, + writeStdoutJs, } from "./platform"; describe("platform utils", () => { @@ -123,11 +124,7 @@ describe("platform utils", () => { }); it("runs .js files through node", async () => { - const script = await writeExecutable( - tmp, - "echo", - 'process.stdout.write("ok");', - ); + const script = await writeExecutable(tmp, "echo", writeStdoutJs("ok")); const { stdout } = await execFileAsync(script); expect(stdout).toBe("ok"); }); @@ -136,7 +133,7 @@ describe("platform utils", () => { const script = await writeExecutable( tmp, "echo-args", - "process.stdout.write(process.argv.slice(2).join(','));", + `require("fs").writeSync(1, process.argv.slice(2).join(","));`, ); const { stdout } = await execFileAsync(script, ["a", "b", "c"]); expect(stdout).toBe("a,b,c"); @@ -149,11 +146,7 @@ describe("platform utils", () => { }); it("preserves the callback form", async () => { - const script = await writeExecutable( - tmp, - "cb-echo", - 'process.stdout.write("cb");', - ); + const script = await writeExecutable(tmp, "cb-echo", writeStdoutJs("cb")); const stdout = await new Promise((resolve, reject) => { mod.execFile(script, (err, out) => err ? reject(new Error(err.message)) : resolve(out), diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 76b24a80..5edb8587 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -40,6 +40,20 @@ export function printEnvCommand(key: string, varName: string): string { return `node -e "process.stdout.write('${key}=' + process.env.${varName})"`; } +/** + * Generates a JS snippet that synchronously writes `data` to stdout. Use in + * fake CLI scripts so output isn't lost on exit (process.stdout.write is async + * on POSIX pipes; see https://nodejs.org/api/process.html#a-note-on-process-io). + */ +export function writeStdoutJs(data: string): string { + return `require("fs").writeSync(1, ${JSON.stringify(data)});`; +} + +/** Same as {@link writeStdoutJs} but writes to stderr (fd 2). */ +export function writeStderrJs(data: string): string { + return `require("fs").writeSync(2, ${JSON.stringify(data)});`; +} + /** * Write a JS file that can be executed cross-platform. * Tests that use `execFile` on the returned path should apply