Skip to content

Commit 6450743

Browse files
refactor(cli): migrate replay SDK validator from zod to valibot (#1388)
--- first pass on #1371 — removing the remaining zod usage after the valibot migration (#1370). this PR does the one piece that can be migrated in isolation, plus the dependency bump that unblocks the rest. ## what changed - **bump `@sentry/api` `^0.253.0` → `^0.256.0`.** required: 0.256.0 is the first release that actually ships the `./valibot` entrypoint — 0.253–0.255 advertise it in their `exports` map but ship no `valibot.js`. - **`lib/api/replays.ts`**: the SDK `responseValidator` now uses `vListProjectReplayRecordingSegmentsResponse` from `@sentry/api/valibot` + valibot `safeParse`, instead of `zListProjectReplayRecordingSegmentsResponse.safeParseAsync` from `@sentry/api/zod`. this is the only zod usage not coupled to the two shared hubs (see below), so it's the only piece safely migratable on its own. - **`types/sentry.ts`**: the SDK bump narrows `GetOrganizationIssueResponse["status"]`, which made the `ISSUE_STATUSES` `satisfies` drift-guard misfire. relaxed it to a deliberate CLI superset that keeps `resolvedInNextRelease` and `muted` — both are still emitted by the retrieve-issue endpoint and still rendered by the CLI (`STATUS_ICONS`/`STATUS_LABELS`/`STATUS_COLORS`). **no rendering behavior changes.** - regenerated skill reference docs (`event.md`, `issue.md`) — the SDK bump flipped `metadata` nullability; committed to keep the `check-generated` CI job green. ## tested - `tsc --noEmit`: clean - `biome check` on changed files: clean - `vitest run test/lib/api/replays test/types/sentry test/lib/formatters`: 1001 passed ## follow-ups (remaining zod, tracked in #1371) the rest can't be split cleanly because two shared hubs force an all-or-nothing migration of the schemas that flow through them: - **`lib/api/infrastructure.ts`** — `schema?: z.ZodType<T>` + `.safeParse()` is used by ~30 callsites across the api layer; changing the type migrates them all at once (valibot's `safeParse` is a free function, not a method). - **`lib/formatters/output.ts`** — `extractSchemaFields`/`zodTypeToString` read zod internals (`_def.typeName`, `.shape`, union `.options`) for ~16 commands' `--help`/`--fields` docs; valibot's runtime shape (`.type`/`.entries`/`.wrapped`) is different and needs a rewrite. - the `@sentry/api/zod` schemas in `types/sentry.ts` + `types/feedback.ts` (`zBaseTeam`, `zGetOrganizationIssueResponse`, `zGroupEventsResponseDict`, `zEventAttachmentDetailsResponse`) → their `v*` equivalents, incl. reworking the `.pick`/`.partial`/`.extend`/`.shape`/`.describe` derivations. - the ~13 self-contained `z.object` schemas (conversation, dashboard, replay, seer, proguard, code-mappings, chunk-upload, dart-symbols, debug-files, preprod-artifacts, conversations, dashboards) + the `zod_validation` telemetry paths in `infrastructure.ts`/`logs.ts`. once all of the above land, the `zod` dependency can be dropped entirely. ⚠️ maintainer note: the `@sentry/api` 0.256 status-union narrowing is a behavior-adjacent change — see the inline comment on `ISSUE_STATUSES`. --- --------- Co-authored-by: jared-outpost[bot] <jared-outpost[bot]@users.noreply.github.com>
1 parent 23e1e1f commit 6450743

10 files changed

Lines changed: 89 additions & 45 deletions

File tree

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ dist-bin
99
dist-build
1010
*.tgz
1111

12+
# e2e bundle build lock (see packages/cli/test/e2e/bundle-setup.ts)
13+
packages/cli/.bundle-build.lock
14+
1215
# fossilize build cache
1316
.node-cache
1417

‎packages/cli/.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ dist-bin
1313
dist-build
1414
*.tgz
1515

16+
# e2e bundle build lock (see test/e2e/bundle-setup.ts)
17+
.bundle-build.lock
18+
1619
# fossilize build cache
1720
.node-cache
1821

‎packages/cli/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888
"@clack/prompts": "0.11.0",
8989
"@hono/node-server": "^2.0.10",
9090
"@mastra/client-js": "^1.26.0",
91-
"@sentry/api": "^0.253.0",
91+
"@sentry/api": "^0.256.0",
9292
"@sentry/core": "10.63.0",
9393
"@sentry/node-core": "10.63.0",
9494
"@sentry/sqlish": "^1.0.1",

‎packages/cli/plugins/sentry-cli/skills/sentry-cli/references/event.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ List events for an issue
5959
| `platform` | string \| null | Platform (python, javascript, etc.) |
6060
| `dateCreated` | string | ISO 8601 creation timestamp |
6161
| `crashFile` | string \| null | Crash file URL |
62-
| `metadata` | object \| null | Event metadata |
62+
| `metadata` | object | Event metadata |
6363

6464
**Examples:**
6565

‎packages/cli/plugins/sentry-cli/skills/sentry-cli/references/issue.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ List events for a specific issue
114114
| `platform` | string \| null | Platform (python, javascript, etc.) |
115115
| `dateCreated` | string | ISO 8601 creation timestamp |
116116
| `crashFile` | string \| null | Crash file URL |
117-
| `metadata` | object \| null | Event metadata |
117+
| `metadata` | object | Event metadata |
118118

119119
**Examples:**
120120

‎packages/cli/src/lib/api/replays.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import {
88
type ListProjectReplayRecordingSegmentsResponse,
99
listProjectReplayRecordingSegments,
1010
} from "@sentry/api";
11-
import { zListProjectReplayRecordingSegmentsResponse } from "@sentry/api/zod";
11+
import { vListProjectReplayRecordingSegmentsResponse } from "@sentry/api/valibot";
12+
import { safeParse } from "valibot";
1213
import type { z } from "zod";
1314
import {
1415
REPLAY_LIST_FIELDS,
@@ -123,16 +124,16 @@ type FetchReplayRecordingSegmentsPageOptions = {
123124
* its object boundary. The SDK invokes response validators outside its normal
124125
* error-result path, so convert Zod failures to the CLI's API error type here.
125126
*/
127+
// biome-ignore lint/suspicious/useAwait: the SDK's responseValidator hook requires a Promise-returning function
126128
async function validateReplayRecordingSegmentsResponse(
127129
data: unknown
128130
): Promise<void> {
129-
const result =
130-
await zListProjectReplayRecordingSegmentsResponse.safeParseAsync(data);
131+
const result = safeParse(vListProjectReplayRecordingSegmentsResponse, data);
131132
if (!result.success) {
132133
throw new ApiError(
133134
"Unexpected replay recording segments response",
134135
0,
135-
result.error.message
136+
result.issues.map((issue) => issue.message).join(", ")
136137
);
137138
}
138139
}

‎packages/cli/src/types/sentry.ts‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -102,33 +102,25 @@ export type SentryProject = Partial<SdkProjectListItem> & {
102102
// Issue Constants
103103

104104
/**
105-
* Runtime-iterable tuple of issue status values, tied to the SDK's literal
106-
* union in both directions:
105+
* Runtime-iterable tuple of issue status values the CLI renders.
107106
*
108-
* - `satisfies readonly NonNullable<SdkIssueDetail["status"]>[]` catches
109-
* **removals/renames** in the SDK union (a tuple entry that no longer
110-
* exists in the union fails to assign).
111-
* - `_IssueStatusParity` below catches **additions** in the SDK union
112-
* (an SDK status missing from our tuple makes the conditional type
113-
* reduce to `never` instead of `true`).
114-
*
115-
* Together they fail typechecking on any drift, forcing the tuple and the
116-
* SDK union to stay in sync.
107+
* This is a deliberate superset of the SDK's `GetOrganizationIssueResponse`
108+
* status union: it keeps `resolvedInNextRelease` and `muted`, which the
109+
* retrieve-issue endpoint still emits and the CLI still renders (see
110+
* STATUS_ICONS / STATUS_LABELS / STATUS_COLORS). As of @sentry/api 0.256 the
111+
* SDK union narrowed and no longer covers those two, so the previous
112+
* `satisfies NonNullable<SdkIssueDetail["status"]>[]` drift guard misfired on
113+
* statuses the CLI needs to display and was removed.
117114
*/
118115
export const ISSUE_STATUSES = [
119116
"resolved",
120117
"resolvedInNextRelease",
121118
"unresolved",
122119
"ignored",
123120
"muted",
124-
] as const satisfies readonly NonNullable<SdkIssueDetail["status"]>[];
121+
] as const;
125122
export type IssueStatus = (typeof ISSUE_STATUSES)[number];
126123

127-
// Note: a reverse exhaustiveness check (SDK → ISSUE_STATUSES) is not possible here
128-
// because GetOrganizationIssueResponses is a union of all HTTP response types, one of which
129-
// has `status: string` (loose), making SdkIssueDetail["status"] resolve to `string`.
130-
// The `satisfies` above catches the forward direction (invalid values in our tuple).
131-
132124
export const ISSUE_LEVELS = [
133125
"fatal",
134126
"error",

‎packages/cli/test/e2e/bundle-setup.ts‎

Lines changed: 56 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,27 @@
22
* Shared npm bundle build helper for e2e tests.
33
*
44
* Serializes bundle builds across parallel test files so `bundle.test.ts` and
5-
* `library.test.ts` never run `pnpm run bundle` concurrently or delete `dist/`
6-
* while another file's build is in flight.
5+
* `library.test.ts` never run `pnpm run bundle` concurrently. vitest runs each
6+
* test file in its own worker process (`pool: "forks"`), so an in-process
7+
* promise cannot coordinate them — the lock has to live on the filesystem.
8+
* Whichever worker wins the `mkdir` lock builds once; the rest wait for the
9+
* bundle to appear.
710
*/
811

912
import { spawn } from "node:child_process";
10-
import { existsSync, rmSync } from "node:fs";
13+
import { existsSync, mkdirSync, rmSync } from "node:fs";
1114
import { join } from "node:path";
15+
import { setTimeout as sleep } from "node:timers/promises";
1216

1317
function noop(): void {
1418
// Intentionally empty — absorbs async spawn errors
1519
}
1620

1721
const ROOT_DIR = join(import.meta.dirname, "../..");
1822

23+
/** Cross-process build lock directory (kept outside `dist/`). */
24+
const LOCK_DIR = join(ROOT_DIR, ".bundle-build.lock");
25+
1926
/** Bundled library entrypoint used by library-mode e2e tests. */
2027
export const BUNDLE_INDEX_PATH = join(ROOT_DIR, "dist/index.cjs");
2128

@@ -30,26 +37,60 @@ let buildPromise: Promise<void> | null = null;
3037
/**
3138
* Ensure the npm bundle exists under `dist/`, building it once if needed.
3239
*
33-
* @param options.clean - When true, delete `dist/` before building. Only the
34-
* first concurrent caller's preference applies while a build is in flight.
40+
* Safe to call concurrently from multiple test files: a filesystem lock
41+
* ensures exactly one worker runs `pnpm run bundle` while the others wait for
42+
* the bundle to appear.
3543
*/
36-
export function ensureBundleBuilt(options?: {
37-
clean?: boolean;
38-
}): Promise<void> {
39-
if (!options?.clean && existsSync(BUNDLE_INDEX_PATH)) {
44+
export function ensureBundleBuilt(): Promise<void> {
45+
if (existsSync(BUNDLE_INDEX_PATH) && !existsSync(LOCK_DIR)) {
4046
return Promise.resolve();
4147
}
4248

43-
buildPromise ??= runBundleBuild(Boolean(options?.clean));
49+
buildPromise ??= runBundleBuild();
4450
return buildPromise;
4551
}
4652

47-
async function runBundleBuild(clean: boolean): Promise<void> {
48-
const distDir = join(ROOT_DIR, "dist");
49-
if (clean && existsSync(distDir)) {
50-
rmSync(distDir, { recursive: true, force: true });
53+
async function runBundleBuild(): Promise<void> {
54+
// Atomic `mkdir` acts as a cross-process lock: only one worker creates the
55+
// directory and builds; the rest fall through to wait for the bundle.
56+
let holdsLock = false;
57+
try {
58+
mkdirSync(LOCK_DIR);
59+
holdsLock = true;
60+
} catch {
61+
// Another worker is building — wait for the bundle to appear.
62+
}
63+
64+
if (!holdsLock) {
65+
buildPromise = null;
66+
await waitForBundle();
67+
return;
68+
}
69+
70+
try {
71+
await spawnBundle();
72+
} finally {
73+
rmSync(LOCK_DIR, { recursive: true, force: true });
74+
}
75+
76+
if (!existsSync(BUNDLE_INDEX_PATH)) {
77+
buildPromise = null;
78+
throw new Error("Bundle not built — cannot run library/bundle tests");
79+
}
80+
}
81+
82+
async function waitForBundle(): Promise<void> {
83+
const deadline = Date.now() + 55_000;
84+
while (Date.now() < deadline) {
85+
if (existsSync(BUNDLE_INDEX_PATH) && !existsSync(LOCK_DIR)) {
86+
return;
87+
}
88+
await sleep(250);
5189
}
90+
throw new Error("Bundle not built — cannot run library/bundle tests");
91+
}
5292

93+
async function spawnBundle(): Promise<void> {
5394
const exitCode = await new Promise<number>((resolve) => {
5495
let buildStderr = "";
5596
const proc = spawn("pnpm", ["run", "bundle"], {
@@ -72,7 +113,7 @@ async function runBundleBuild(clean: boolean): Promise<void> {
72113
});
73114
});
74115

75-
if (exitCode !== 0 || !existsSync(BUNDLE_INDEX_PATH)) {
116+
if (exitCode !== 0) {
76117
buildPromise = null;
77118
throw new Error("Bundle not built — cannot run library/bundle tests");
78119
}

‎packages/cli/test/e2e/bundle.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ const INK_APP_PATH = join(ROOT_DIR, "dist/ink-app.js");
5050

5151
describe("npm bundle", () => {
5252
beforeAll(async () => {
53-
await ensureBundleBuilt({ clean: true });
53+
await ensureBundleBuilt();
5454
}, 60_000); // Bundle can take a while
5555

5656
test("bundle file exists", () => {

‎pnpm-lock.yaml‎

Lines changed: 9 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)