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
5 changes: 5 additions & 0 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ export async function runCli(cliArgs: string[]): Promise<void> {
const { startCleanupOldBinary } = await import("./lib/upgrade.js");
const {
abortPendingVersionCheck,
getErrorUpdateNotification,
getUpdateNotification,
maybeCheckForUpdateInBackground,
shouldSuppressNotification,
Expand Down Expand Up @@ -439,6 +440,10 @@ export async function runCli(cliArgs: string[]): Promise<void> {
}
process.stderr.write(`${error("Error:")} ${formatError(err)}\n`);
process.exitCode = getExitCode(err);
const notification = getErrorUpdateNotification(err, hoistedArgs);
if (notification) {
process.stderr.write(notification);
}
return;
} finally {
// Abort any pending version check to allow clean exit
Expand Down
45 changes: 45 additions & 0 deletions src/lib/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,51 @@ export function getExitCode(error: unknown): number {
return 1;
}

/**
* Classify errors caused by user input, configuration, auth state, or account
* settings. These errors already tell the user what to fix, so upgrade nudges
* should use the neutral update banner instead of implying a CLI bug fix.
*
* Generic {@link CliError} instances are treated as user-facing by default.
* Explicit non-user subclasses must be checked before that fallback.
*/
export function isUserError(error: unknown): boolean {
if (error instanceof ApiError) {
// Status 0 = network-level failure (DNS, ECONNREFUSED) — user environment,
// not a CLI bug. 400 usually means the CLI constructed a bad request.
// Other 4xx statuses are user/account/API-state problems.
if (error.status === 0) {
return true;
}
return error.status > 400 && error.status < 500;
Comment thread
sentry[bot] marked this conversation as resolved.
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Network errors get misleading upgrade nudge message

Low Severity

ApiError instances with status 0 (network errors — DNS failures, connection refused, etc.) are classified as non-user errors by isUserError, causing the contextual "Upgrading may resolve this" nudge to appear. In infrastructure.ts, throwApiError creates ApiError with status 0 when no HTTP response is received. Since 0 > 400 is false, these fall through to the non-user-error path. A user seeing "Unable to reach Sentry API. Check your internet connection" followed by "Upgrading may resolve this" gets contradictory guidance. The ApiError check only considers HTTP status ranges (401–499 as user errors) but doesn't account for the synthetic status 0 convention used for connectivity failures.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3702653. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch — throwApiError uses status 0 for network-level failures (DNS, ECONNREFUSED), and those were falling through to the contextual nudge path. added an early return for status === 0 since network errors are user-environment issues, not CLI bugs. fixed in ef582a5.


if (
error instanceof AbortError ||
error instanceof TimeoutError ||
error instanceof UpgradeError
) {
return false;
}

if (
error instanceof AuthError ||
error instanceof HostScopeError ||
error instanceof ConfigError ||
error instanceof ContextError ||
error instanceof ResolutionError ||
error instanceof ValidationError ||
error instanceof DeviceFlowError ||
error instanceof SeerError ||
error instanceof OutputError ||
error instanceof WizardError
) {
return true;
}

return error instanceof CliError;
}

/** Result when the guarded operation succeeded */
export type AuthGuardSuccess<T> = { ok: true; value: T };

Expand Down
61 changes: 54 additions & 7 deletions src/lib/version-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
prefetchStablePatches,
} from "./delta-upgrade.js";
import { getEnv } from "./env.js";
import { isUserError } from "./errors.js";
import { cyan, muted } from "./formatters/colors.js";
import { cleanupPatchCache } from "./patch-cache.js";
import { fetchLatestFromGitHub, fetchLatestNightlyVersion } from "./upgrade.js";
Expand Down Expand Up @@ -240,7 +241,7 @@ function canNotifyAgain(lastNotified: number | null): boolean {
}

/**
* Get the update notification message if a new version is available.
* Build an update notification when a newer version is available.
* Returns null if up-to-date, no cached version info, rate-limited, on a
* non-TTY stderr, or on error. Never throws — errors are caught and
* reported to Sentry.
Expand All @@ -249,7 +250,9 @@ function canNotifyAgain(lastNotified: number | null): boolean {
* persists `last_notified = now` via {@link markUpdateNotified} so
* subsequent invocations within the rate-limit window return null.
*/
function getUpdateNotificationImpl(): string | null {
function getUpdateNotificationWithCopy(
formatNotification: (latestVersion: string) => string
): string | null {
// Gate 1: non-TTY stderr (scripts, CI, pipes).
if (!isStderrTTY()) {
return null;
Expand Down Expand Up @@ -278,9 +281,7 @@ function getUpdateNotificationImpl(): string | null {
return null;
}

const channel = getReleaseChannel();
const label =
channel === "nightly" ? "New nightly available:" : "Update available:";
const notification = formatNotification(latestVersion);

// Record that we're about to print the banner so repeat invocations
// within the rate-limit window stay silent. Failures here are
Expand All @@ -293,14 +294,30 @@ function getUpdateNotificationImpl(): string | null {
}
notifiedThisProcess = true;

return `\n${muted(label)} ${cyan(CLI_VERSION)} -> ${cyan(latestVersion)} Run ${cyan('"sentry cli upgrade"')} to update.\n`;
return notification;
} catch (error) {
// DB access failed - report to Sentry but don't crash CLI
Sentry.captureException(error);
return null;
}
}

function formatStandardUpdateNotification(latestVersion: string): string {
const channel = getReleaseChannel();
const label =
channel === "nightly" ? "New nightly available:" : "Update available:";

return `\n${muted(label)} ${cyan(CLI_VERSION)} -> ${cyan(latestVersion)} Run ${cyan('"sentry cli upgrade"')} to update.\n`;
}

function formatContextualUpdateNotification(latestVersion: string): string {
return (
`\n${muted("A new version of sentry-cli is available")} (${cyan(latestVersion)})${muted(".")} ` +
`${muted("Upgrading may resolve this — we fix a lot of bugs in every release.")} ` +
`${muted("Run")} ${cyan('"sentry cli upgrade"')} ${muted("to update.")}\n`
);
}

/**
* Reset the in-process "already notified" latch.
*
Expand Down Expand Up @@ -341,5 +358,35 @@ export function getUpdateNotification(): string | null {
if (isUpdateCheckDisabled()) {
return null;
}
return getUpdateNotificationImpl();
return getUpdateNotificationWithCopy(formatStandardUpdateNotification);
}

/**
* Get an update notification for the error path.
*
* User errors get the standard neutral banner. Non-user errors get a
* contextual nudge suggesting an upgrade may resolve the failure.
*
* Shares the same gates as {@link getUpdateNotification} (TTY, rate-limit,
* once-per-process) so the two never double-emit.
*
* Returns null when no notification should be shown (suppressed command,
* disabled, up-to-date, non-TTY, rate-limited, or on error).
*/
export function getErrorUpdateNotification(
error: unknown,
args: string[]
): string | null {
if (shouldSuppressNotification(args)) {
return null;
}

if (isUserError(error)) {
return getUpdateNotification();
}

if (isUpdateCheckDisabled()) {
return null;
}
return getUpdateNotificationWithCopy(formatContextualUpdateNotification);
}
36 changes: 36 additions & 0 deletions test/lib/errors.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, expect, test } from "bun:test";
import {
AbortError,
ApiError,
AuthError,
CliError,
Expand All @@ -10,6 +11,7 @@ import {
formatError,
getExitCode,
HostScopeError,
isUserError,
OutputError,
ResolutionError,
SeerError,
Expand Down Expand Up @@ -478,6 +480,40 @@ describe("getExitCode", () => {
});
});

describe("isUserError", () => {
test.each([
["generic CliError", new CliError("message"), true],
["HostScopeError", new HostScopeError("blocked"), true],
["AuthError", new AuthError("invalid"), true],
["ConfigError", new ConfigError("bad config"), true],
["OutputError", new OutputError({ error: "not found" }), true],
["ContextError", new ContextError("Organization", "sentry org list"), true],
[
"ResolutionError",
new ResolutionError("Project 'x'", "not found", "sentry project list"),
true,
],
["ValidationError", new ValidationError("bad input"), true],
["DeviceFlowError", new DeviceFlowError("slow_down"), true],
["SeerError", new SeerError("not_enabled"), true],
["WizardError", new WizardError("wizard failed"), true],
["ApiError 0 (network)", new ApiError("network error", 0), true],
["ApiError 400", new ApiError("bad request", 400), false],
["ApiError 401", new ApiError("unauthorized", 401), true],
["ApiError 418", new ApiError("teapot", 418), true],
["ApiError 499", new ApiError("client closed", 499), true],
["ApiError 500", new ApiError("server error", 500), false],
["TimeoutError", new TimeoutError("timed out"), false],
["UpgradeError", new UpgradeError("network_error"), false],
["AbortError", new AbortError(), false],
["standard Error", new Error("boom"), false],
["string throw", "boom", false],
["null", null, false],
])("classifies %s", (_label, errorValue, expected) => {
expect(isUserError(errorValue)).toBe(expected);
});
});

describe("exit codes", () => {
test("CliError base defaults to EXIT.GENERAL (1)", () => {
expect(new CliError("err").exitCode).toBe(EXIT.GENERAL);
Expand Down
147 changes: 147 additions & 0 deletions test/lib/version-check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,14 @@ import {
getVersionCheckInfo,
setVersionCheckInfo,
} from "../../src/lib/db/version-check.js";
import {
ApiError,
ContextError,
ValidationError,
} from "../../src/lib/errors.js";
import {
abortPendingVersionCheck,
getErrorUpdateNotification,
getUpdateNotification,
maybeCheckForUpdateInBackground,
resetUpdateNotificationState,
Expand Down Expand Up @@ -210,6 +216,147 @@ describe("getUpdateNotification", () => {
});
});

describe("getErrorUpdateNotification", () => {
useTestConfigDir("test-version-error-notif-");
const defaultArgs = ["issue", "list"];
let savedNoUpdateCheck: string | undefined;
let restoreTTY: (() => void) | undefined;

beforeEach(() => {
savedNoUpdateCheck = process.env.SENTRY_CLI_NO_UPDATE_CHECK;
delete process.env.SENTRY_CLI_NO_UPDATE_CHECK;
resetUpdateNotificationState();
restoreTTY = withStderrTTY(true);
});

afterEach(() => {
if (savedNoUpdateCheck !== undefined) {
process.env.SENTRY_CLI_NO_UPDATE_CHECK = savedNoUpdateCheck;
}
restoreTTY?.();
restoreTTY = undefined;
});

test("returns null when no version info is cached", () => {
expect(
getErrorUpdateNotification(new Error("boom"), defaultArgs)
).toBeNull();
});

test("returns null when cached version is same as current", () => {
setVersionCheckInfo("0.0.0-dev");
expect(
getErrorUpdateNotification(new Error("boom"), defaultArgs)
).toBeNull();
});

test("returns contextual message for unexpected errors when newer version is available", () => {
setVersionCheckInfo("99.0.0");
const notification = getErrorUpdateNotification(
new Error("boom"),
defaultArgs
);

expect(notification).not.toBeNull();
expect(notification).toContain("99.0.0");
expect(notification).toContain("Upgrading may resolve this");
expect(notification).toContain("sentry cli upgrade");
});

test("does not contain standard 'Update available:' label", () => {
setVersionCheckInfo("99.0.0");
const notification = getErrorUpdateNotification(
new Error("boom"),
defaultArgs
);

expect(notification).not.toBeNull();
expect(notification).not.toContain("Update available:");
});

test.each([
["ContextError", new ContextError("Organization", "sentry org list")],
["ValidationError", new ValidationError("bad input")],
["ApiError 404", new ApiError("not found", 404)],
])("returns standard update copy for user error %s", (_label, errorValue) => {
setVersionCheckInfo("99.0.0");
const notification = getErrorUpdateNotification(errorValue, defaultArgs);

expect(notification).not.toBeNull();
expect(notification).toContain("Update available:");
expect(notification).not.toContain("Upgrading may resolve this");
});

test.each([
["ApiError 400", new ApiError("bad request", 400)],
["ApiError 500", new ApiError("server error", 500)],
["generic Error", new Error("boom")],
])("returns contextual update copy for non-user error %s", (_label, errorValue) => {
setVersionCheckInfo("99.0.0");
const notification = getErrorUpdateNotification(errorValue, defaultArgs);

expect(notification).not.toBeNull();
expect(notification).toContain("Upgrading may resolve this");
expect(notification).not.toContain("Update available:");
});

test.each([
["json flag", ["issue", "list", "--json"]],
["init", ["init"]],
["upgrade", ["upgrade"]],
["cli setup", ["cli", "setup"]],
])("returns null for suppressed args: %s", (_label, args) => {
setVersionCheckInfo("99.0.0");
expect(getErrorUpdateNotification(new Error("boom"), args)).toBeNull();
});

test("returns null when stderr is not a TTY", () => {
restoreTTY?.();
restoreTTY = withStderrTTY(false);

setVersionCheckInfo("99.0.0");
expect(
getErrorUpdateNotification(new Error("boom"), defaultArgs)
).toBeNull();
});

test("shares once-per-process latch with getUpdateNotification", () => {
setVersionCheckInfo("99.0.0");

// Error notification fires first
const first = getErrorUpdateNotification(new Error("boom"), defaultArgs);
expect(first).not.toBeNull();

// Standard notification is suppressed (same latch)
const second = getUpdateNotification();
expect(second).toBeNull();
});

test("standard notification suppresses error notification too", () => {
setVersionCheckInfo("99.0.0");

const first = getUpdateNotification();
expect(first).not.toBeNull();

const second = getErrorUpdateNotification(new Error("boom"), defaultArgs);
expect(second).toBeNull();
});

test("rate-limits across CLI invocations", () => {
setVersionCheckInfo("99.0.0");

const first = getErrorUpdateNotification(new Error("boom"), defaultArgs);
expect(first).not.toBeNull();

// Simulate fresh invocation
resetUpdateNotificationState();

// Still within 24h window
const second = getErrorUpdateNotification(new Error("boom"), defaultArgs);
expect(second).toBeNull();
});
});

describe("abortPendingVersionCheck", () => {
test("does not throw when no pending check", () => {
// Should be safe to call even when nothing is pending
Expand Down
Loading