Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}

Expand Down
10 changes: 2 additions & 8 deletions src/core/cliCredentialManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,26 +56,20 @@ 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<WorkspaceConfiguration, "get">,
options?: { signal?: AbortSignal; keyringOnly?: boolean },
options?: { signal?: AbortSignal },
): Promise<void> {
const binPath = await this.resolveKeyringBinary(
url,
configs,
"keyringAuth",
);
if (!binPath) {
if (options?.keyringOnly) {
return;
}
await this.writeCredentialFiles(url, token);
return;
}
Expand Down
90 changes: 46 additions & 44 deletions src/core/cliExec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -17,23 +17,6 @@ export interface CliEnv {
configs: Pick<vscode.WorkspaceConfiguration, "get">;
}

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.
Expand Down Expand Up @@ -135,14 +118,56 @@ 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<void> {
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)) {
Comment thread
EhabY marked this conversation as resolved.
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.
*/
function spawnCliInTerminal(options: {
name: string;
binary: string;
args: string[];
banner?: string[];
banner: string[];
}): vscode.Terminal {
const writeEmitter = new vscode.EventEmitter<string>();
const closeEmitter = new vscode.EventEmitter<number | void>();
Expand Down Expand Up @@ -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: () => {
Expand Down Expand Up @@ -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<void> {
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);
}
3 changes: 3 additions & 0 deletions src/core/cliManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/error/errorUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment thread
EhabY marked this conversation as resolved.
return error instanceof Error && error.name === "AbortError";
}

Expand Down
10 changes: 2 additions & 8 deletions src/login/loginCoordinator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
}
Expand Down
36 changes: 0 additions & 36 deletions test/unit/core/cliCredentialManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
37 changes: 28 additions & 9 deletions test/unit/core/cliExec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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");
}
Expand Down Expand Up @@ -159,15 +166,27 @@ 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);
await expect(
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", () => {
Expand Down Expand Up @@ -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);
Expand Down
43 changes: 42 additions & 1 deletion test/unit/error/errorUtils.test.ts
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand Down
3 changes: 1 addition & 2 deletions test/unit/login/loginCoordinator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -509,7 +509,6 @@ describe("LoginCoordinator", () => {
TEST_URL,
"stored-token",
expect.anything(),
{ keyringOnly: true },
);
});

Expand Down
Loading