Skip to content

Commit cbd4e2a

Browse files
committed
fix: address round-2 feedback — remove api/0? handling, check getSystemErrorMap upfront
- Remove api/0? query-string edge case (reviewer: clearly an error) - Replace try-catch patch with upfront hasGetSystemErrorMap check at module load — conditionally excludes NodeSystemError integration when util.getSystemErrorMap is missing (Bun)
1 parent 0bd2090 commit cbd4e2a

3 files changed

Lines changed: 23 additions & 53 deletions

File tree

src/commands/api.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,8 @@ export function normalizeEndpoint(endpoint: string): string {
9292
// Strip api/0/ prefix if user accidentally included it — the base URL
9393
// already includes /api/0/, so keeping it would produce a doubled path
9494
// like /api/0/api/0/... (see CLI-K1).
95-
// Handles: "api/0/path", "api/0" (bare), and "api/0?query" (query without slash).
96-
if (
97-
trimmed.startsWith("api/0/") ||
98-
trimmed.startsWith("api/0?") ||
99-
trimmed === "api/0"
100-
) {
95+
// Also strip bare "api/0" to maintain idempotency.
96+
if (trimmed.startsWith("api/0/") || trimmed === "api/0") {
10197
log.warn(
10298
"Endpoint includes the /api/0/ prefix which is added automatically — stripping it to avoid a doubled path"
10399
);

src/lib/telemetry.ts

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import { getRealUsername } from "./utils.js";
2525

2626
export type { Span } from "@sentry/core";
2727

28-
import type { Integration } from "@sentry/core";
29-
3028
/** Re-imported locally because Span is exported via re-export */
3129
type Span = Sentry.Span;
3230

@@ -265,37 +263,22 @@ const EXCLUDED_INTEGRATIONS = new Set([
265263
]);
266264

267265
/**
268-
* Patch the NodeSystemError integration's processEvent to catch missing
269-
* `util.getSystemErrorMap()` on Bun. Without this, the SDK crashes during
270-
* event processing instead of sending the error report (CLI-K1).
266+
* Check whether `util.getSystemErrorMap` exists at setup time.
267+
* Bun does not implement this Node.js API, which the SDK's NodeSystemError
268+
* integration uses in its `processEvent` hook. When missing, the hook crashes
269+
* during event processing instead of sending the error report (CLI-K1).
271270
*
272-
* We wrap rather than exclude the integration so that on runtimes (or future
273-
* Bun versions) that implement `getSystemErrorMap`, system error context is
274-
* still attached.
271+
* Checked once at module load so the integration filter is a simple boolean.
275272
*/
276-
function patchNodeSystemErrorIntegration(
277-
integration: Integration
278-
): Integration {
279-
if (integration.name !== "NodeSystemError") {
280-
return integration;
281-
}
282-
283-
const original = integration.processEvent?.bind(integration);
284-
if (!original) {
285-
return integration;
273+
const hasGetSystemErrorMap = (() => {
274+
try {
275+
// Dynamic require to avoid bundler issues — the check only matters at runtime
276+
const util = require("node:util") as Record<string, unknown>;
277+
return typeof util.getSystemErrorMap === "function";
278+
} catch {
279+
return false;
286280
}
287-
288-
integration.processEvent = (...args) => {
289-
try {
290-
return original(...args);
291-
} catch {
292-
// getSystemErrorMap is not available — skip enrichment, keep the event
293-
return args[0];
294-
}
295-
};
296-
297-
return integration;
298-
}
281+
})();
299282

300283
/** Current beforeExit handler, tracked so it can be replaced on re-init */
301284
let currentBeforeExitHandler: (() => void) | null = null;
@@ -347,12 +330,15 @@ export function initSentry(
347330
const client = Sentry.init({
348331
dsn: SENTRY_CLI_DSN,
349332
enabled,
350-
// Keep default integrations but filter out ones that add overhead without benefit
351-
// Important: Don't use defaultIntegrations: false as it may break debug ID support
333+
// Keep default integrations but filter out ones that add overhead without benefit.
334+
// Important: Don't use defaultIntegrations: false as it may break debug ID support.
335+
// NodeSystemError is excluded on runtimes missing util.getSystemErrorMap (Bun) — CLI-K1.
352336
integrations: (defaults) =>
353-
defaults
354-
.filter((integration) => !EXCLUDED_INTEGRATIONS.has(integration.name))
355-
.map(patchNodeSystemErrorIntegration),
337+
defaults.filter(
338+
(integration) =>
339+
!EXCLUDED_INTEGRATIONS.has(integration.name) &&
340+
(integration.name !== "NodeSystemError" || hasGetSystemErrorMap)
341+
),
356342
environment,
357343
// Enable Sentry structured logs for non-exception telemetry (e.g., unexpected input warnings)
358344
enableLogs: true,

test/commands/api.test.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,18 +87,6 @@ describe("normalizeEndpoint: api/0/ prefix stripping (CLI-K1)", () => {
8787
expect(normalizeEndpoint("api/0")).toBe("/");
8888
});
8989

90-
test("strips api/0 followed directly by query string", () => {
91-
// After stripping "api/0", the remaining "?query=test" gets a trailing
92-
// slash on the (empty) path portion → "/?query=test"
93-
expect(normalizeEndpoint("api/0?query=test")).toBe("/?query=test");
94-
});
95-
96-
test("strips /api/0 followed directly by query string", () => {
97-
expect(normalizeEndpoint("/api/0?query=test&page=2")).toBe(
98-
"/?query=test&page=2"
99-
);
100-
});
101-
10290
test("does not strip partial api/ prefix", () => {
10391
expect(normalizeEndpoint("api/1/organizations/")).toBe(
10492
"api/1/organizations/"

0 commit comments

Comments
 (0)