Skip to content

Commit d326956

Browse files
committed
chore: address adversarial review findings for monorepo pre-shape
Follow-ups from the PR #1254 review (no behavior change to shipped CLI): - check-patches.ts: harden resolvePackageFile() to derive the package root from the FIRST `<pkgName>/` after the last `node_modules/` boundary, instead of `lastIndexOf('/<pkgName>/')`. A package that vendors a nested copy of itself would otherwise truncate at the wrong inner segment. Also refresh the stale `node-linker=hoisted` comment. - biome.jsonc / packages/cli/.gitignore: reconcile the contradictory comments about which .gitignore Biome reads. Biome honors the workspace-root .gitignore via `vcs.root: "../.."`; the package-level .gitignore is for plain git only. Drop the dead `!docs` ignore (no packages/cli/docs after the move). - build.ts / bundle.ts: fix comments that still claimed deps are "hoisted to the workspace-root node_modules" — under node-linker=isolated they live in the isolated store; the code (module resolution / `pnpm exec`) is unchanged and correct. - packages/cli/AGENTS.md: replace stale Bun-era guidance with the current pnpm/Node/Vitest reality — command reference, "Use Node.js APIs" section, test-runner isolation notes (VITEST_POOL_ID), vitest imports and `vi.mock`, moved docs path (apps/cli-docs), and removed the dangling `.cursor/rules/bun-cli.mdc` reference. Verified: typecheck, lint, all check:* scripts, build (+ binary smoke).
1 parent a1cac94 commit d326956

7 files changed

Lines changed: 95 additions & 76 deletions

File tree

AGENTS.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<!-- This section is maintained by the coding agent via lore (https://github.com/BYK/loreai) -->
2+
## Long-term Knowledge
3+
4+
For long-term knowledge entries managed by [lore](https://github.com/BYK/loreai) (gotchas, patterns, decisions, architecture), see [`.lore.md`](.lore.md) in the project root.
5+
<!-- End lore-managed section -->

packages/cli/.gitignore

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
# Package-level ignores (see the workspace-root .gitignore for shared rules).
2-
# Duplicated here because Biome resolves useIgnoreFile relative to biome.jsonc.
1+
# Package-level ignores for git. The workspace-root .gitignore mirrors these
2+
# with `packages/cli/`-prefixed patterns (that root file is what Biome reads,
3+
# since biome.jsonc sets `vcs.root: "../.."`). This file exists so plain `git`
4+
# ignores build artifacts within the package regardless of the root file.
35

46
# dependencies
57
node_modules

packages/cli/AGENTS.md

Lines changed: 55 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,15 @@ Guidelines for AI agents working in this codebase.
44

55
## Project Overview
66

7-
**Sentry CLI** is a command-line interface for [Sentry](https://sentry.io), built with [Bun](https://bun.sh) and [Stricli](https://bloomberg.github.io/stricli/).
7+
**Sentry CLI** is a command-line interface for [Sentry](https://sentry.io), built with Node.js, [pnpm](https://pnpm.io), and [Stricli](https://bloomberg.github.io/stricli/). It lives in `packages/cli` of a pnpm workspace; the docs site lives in `apps/cli-docs`.
88

99
### Goals
1010

1111
- **Zero-config experience** - Auto-detect project context from DSNs in source code and env files
1212
- **AI-powered debugging** - Integrate Seer AI for root cause analysis and fix plans
1313
- **Developer-friendly** - Follow `gh` CLI conventions for intuitive UX
1414
- **Agent-friendly** - JSON output and predictable behavior for AI coding agents
15-
- **Fast** - Native binaries via Bun, SQLite caching for API responses
15+
- **Fast** - Native binaries via Node SEA (fossilize), SQLite caching for API responses
1616

1717
### Key Features
1818

@@ -28,86 +28,83 @@ Guidelines for AI agents working in this codebase.
2828

2929
Before working on this codebase, read the Cursor rules:
3030

31-
- **`.cursor/rules/bun-cli.mdc`** - Bun API usage, file I/O, process spawning, testing
3231
- **`.cursor/rules/ultracite.mdc`** - Code style, formatting, linting rules
3332

3433
## Quick Reference: Commands
3534

36-
> **Note**: Always check `package.json` for the latest scripts.
35+
> **Note**: Always check `package.json` for the latest scripts. All commands run
36+
> from `packages/cli` (or via `pnpm --filter sentry run <script>` from the repo root).
3737
3838
```bash
3939
# Development
40-
bun install # Install dependencies
41-
bun run dev # Run CLI in dev mode
42-
bun run --env-file=.env.local src/bin.ts # Dev with env vars
40+
pnpm install # Install dependencies (from repo root)
41+
pnpm run dev # Run CLI in dev mode
42+
pnpm run cli -- <args> # Run the CLI with arguments
4343

4444
# Build
45-
bun run build # Build for current platform
46-
bun run build:all # Build for all platforms
45+
pnpm run build # Build for current platform
46+
pnpm run build:all # Build for all platforms
4747

4848
# Type Checking
49-
bun run typecheck # Check types
49+
pnpm run typecheck # Check types (tsc)
5050

5151
# Linting & Formatting
52-
bun run lint # Check for issues
53-
bun run lint:fix # Auto-fix issues (run before committing)
54-
55-
# Testing
56-
bun test # Run all tests
57-
bun test path/to/file.test.ts # Run single test file
58-
bun test --watch # Watch mode
59-
bun test --filter "test name" # Run tests matching pattern
60-
bun run test:unit # Run unit tests only
61-
bun run test:e2e # Run e2e tests only
52+
pnpm run lint # Check for issues (Biome)
53+
pnpm run lint:fix # Auto-fix issues (run before committing)
54+
55+
# Testing (Vitest)
56+
pnpm run test:unit # Run unit tests
57+
pnpm run test:e2e # Run e2e tests
58+
pnpm exec vitest run test/lib/foo.test.ts # Run a single test file (skips generate steps)
59+
pnpm exec vitest # Watch mode
6260
```
6361

6462
## Rules: No Runtime Dependencies
6563

66-
**CRITICAL**: All packages must be in `devDependencies`, never `dependencies`. Everything is bundled at build time via esbuild. CI enforces this with `bun run check:deps`.
64+
**CRITICAL**: All packages must be in `devDependencies`, never `dependencies`. Everything is bundled at build time via esbuild. CI enforces this with `pnpm run check:deps`.
6765

68-
When adding a package, always use `bun add -d <package>` (the `-d` flag).
66+
When adding a package, always use `pnpm add -D <package>` (the `-D` flag).
6967

7068
When the `@sentry/api` SDK provides types for an API response, import them directly from `@sentry/api` instead of creating redundant Zod schemas in `src/types/sentry.ts`.
7169

72-
## Rules: Use Bun APIs
70+
## Rules: Use Node.js APIs
7371

74-
**CRITICAL**: This project uses Bun as runtime. Always prefer Bun-native APIs over Node.js equivalents.
72+
**CRITICAL**: This project runs on Node.js. Use Node.js standard-library
73+
(`node:*`) APIs; there is no Bun runtime. (This project was migrated from Bun to
74+
Node — do not reintroduce `Bun.*` globals, `bun:test`, or `bun` CLI commands.)
7575

76-
Read the full guidelines in `.cursor/rules/bun-cli.mdc`.
76+
### Quick API Reference
7777

78-
**Bun Documentation**: https://bun.sh/docs - Consult these docs when unsure about Bun APIs.
78+
| Task | Use This |
79+
|------|----------|
80+
| Read file | `await readFile(path, "utf-8")` (`node:fs/promises`) |
81+
| Write file | `await writeFile(path, content)` (`node:fs/promises`) |
82+
| Check file exists | `existsSync(path)` (`node:fs`) |
83+
| Spawn process | `spawn()` / `execFile()` (`node:child_process`) |
84+
| Shell commands | `execSync()` (`node:child_process`) |
85+
| Find executable | `which` helper in `src/lib/which.ts` |
86+
| Glob patterns | the scanner in `src/lib/scan/` |
87+
| Sleep | `await setTimeout(ms)` (`node:timers/promises`) |
88+
| Parse JSON file | `JSON.parse(await readFile(path, "utf-8"))` |
7989

80-
### Quick Bun API Reference
81-
82-
| Task | Use This | NOT This |
83-
|------|----------|----------|
84-
| Read file | `await Bun.file(path).text()` | `fs.readFileSync()` |
85-
| Write file | `await Bun.write(path, content)` | `fs.writeFileSync()` |
86-
| Check file exists | `await Bun.file(path).exists()` | `fs.existsSync()` |
87-
| Spawn process | `Bun.spawn()` | `child_process.spawn()` |
88-
| Shell commands | `Bun.$\`command\`` ⚠️ | `child_process.exec()` |
89-
| Find executable | `Bun.which("git")` | `which` package |
90-
| Glob patterns | `new Bun.Glob()` | `glob` / `fast-glob` packages |
91-
| Sleep | `await Bun.sleep(ms)` | `setTimeout` with Promise |
92-
| Parse JSON file | `await Bun.file(path).json()` | Read + JSON.parse |
93-
94-
**Exception**: Use `node:fs` for directory creation with permissions:
90+
Use `node:fs` for directory creation with permissions:
9591
```typescript
9692
import { mkdirSync } from "node:fs";
9793
mkdirSync(dir, { recursive: true, mode: 0o700 });
9894
```
9995

100-
**Exception**: `Bun.$` (shell tagged template) has no shim in `script/node-polyfills.ts` and will crash on the npm/node distribution. Until a shim is added, use `execSync` from `node:child_process` for shell commands that must work in both runtimes:
96+
For shell commands, prefer `execFileSync` with array args over `execSync` with a
97+
string when passing user-controlled values, to avoid shell injection:
10198
```typescript
102-
import { execSync } from "node:child_process";
103-
const result = execSync("id -u username", { encoding: "utf-8", stdio: ["pipe", "pipe", "ignore"] });
99+
import { execFileSync } from "node:child_process";
100+
const result = execFileSync("id", ["-u", "username"], { encoding: "utf-8", stdio: ["pipe", "pipe", "ignore"] });
104101
```
105102

106103
## Architecture
107104

108105
The full project-structure tree — including the live command/subcommand list and the
109106
domain API modules — is generated from the route tree and lives in
110-
[`docs/src/content/docs/contributing.md`](docs/src/content/docs/contributing.md)
107+
[`apps/cli-docs/src/content/docs/contributing.md`](apps/cli-docs/src/content/docs/contributing.md)
111108
(the `project-structure` block produced by `script/generate-docs-sections.ts`). It is
112109
kept in sync automatically, so it is **not** duplicated here to avoid drift. For the
113110
current command list run `ls src/commands/` or `sentry --help`.
@@ -127,8 +124,9 @@ Top-level layout:
127124
- **`src/types/`** — TypeScript types and Zod schemas.
128125
- **`test/`** — tests mirroring `src/` (unit, `*.property.test.ts`,
129126
`*.model-based.test.ts`, `e2e/`, `fixtures/`, `mocks/`).
130-
- **`docs/`** — documentation site (Astro + Starlight); **`script/`** — build/utility
131-
scripts; **`.cursor/rules/`** — Cursor AI rules; **`biome.jsonc`** — lint config.
127+
- **`../../apps/cli-docs/`** — documentation site (Astro + Starlight);
128+
**`script/`** — build/utility scripts; **`.cursor/rules/`** — Cursor AI rules;
129+
**`biome.jsonc`** — lint config.
132130

133131
## Key Patterns
134132

@@ -409,12 +407,12 @@ Use `"date"` for timestamp-based sort (not `"time"`). Export sort types from the
409407

410408
### Generated Docs & Skills
411409

412-
All command docs and skill files are generated via `bun run generate:docs` (which runs `generate:command-docs` then `generate:skill`). This runs automatically as part of `dev`, `build`, `typecheck`, and `test` scripts.
410+
All command docs and skill files are generated via `pnpm run generate:docs` (which runs `generate:command-docs` then `generate:skill`). This runs automatically as part of `dev`, `build`, `typecheck`, and `test` scripts.
413411

414412
- **Command docs** (`docs/src/content/docs/commands/*.md`) are **gitignored** and generated from CLI metadata + hand-written fragments in `docs/src/fragments/commands/`.
415413
- **Skill files** (`plugins/sentry-cli/skills/sentry-cli/`) are **committed** (consumed by external plugin systems) and auto-committed by CI when stale.
416414
- Edit fragments in `docs/src/fragments/commands/` for custom examples and guides.
417-
- `bun run check:fragments` validates fragment ↔ route consistency.
415+
- `pnpm run check:fragments` validates fragment ↔ route consistency.
418416
- Positional `placeholder` values must be descriptive: `"org/project/trace-id"` not `"args"`.
419417

420418
### Zod Schemas for Validation
@@ -510,7 +508,7 @@ CliError (base, exitCode=1)
510508
- Pass `alternatives: []` when defaults are irrelevant (e.g., for missing Trace ID, Event ID)
511509
- Use `" and "` in `resource` for plural grammar: `"Trace ID and span ID"` → "are required"
512510

513-
**CI enforcement:** `bun run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory).
511+
**CI enforcement:** `pnpm run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory).
514512

515513
```typescript
516514
// Usage examples
@@ -554,7 +552,7 @@ catch (error) {
554552

555553
Use `logger.withTag("command-name")` for tagged logging in command files.
556554

557-
**CI enforcement:** `bun run check:errors` includes a silent-catch scan that flags
555+
**CI enforcement:** `pnpm run check:errors` includes a silent-catch scan that flags
558556
`catch` blocks which are empty, comment-only, or return-only without surfacing the
559557
error. It is currently **advisory** (warns, does not fail CI) because of a pre-existing
560558
backlog; run with `SENTRY_STRICT_SILENT_CATCH=1` to enforce. Do not add new silent
@@ -700,7 +698,7 @@ await deleteUserData(userId)
700698
### Goal
701699
Minimal comments, maximum clarity. Comments explain **intent and reasoning**, not syntax.
702700

703-
## Testing (bun:test + fast-check)
701+
## Testing (Vitest + fast-check)
704702

705703
**Prefer property-based and model-based testing** over traditional unit tests. These approaches find edge cases automatically and provide better coverage with less code.
706704

@@ -734,7 +732,7 @@ Tests that need a database or config directory **must** use `useTestConfigDir()`
734732
- `const baseDir = process.env[CONFIG_DIR_ENV_VAR]!` at module scope — This captures a value that may be stale
735733
- Manual `beforeEach`/`afterEach` that sets/deletes `SENTRY_CONFIG_DIR`
736734

737-
**Why**: Bun's test runner uses `--isolate --parallel` (see `test:unit` in `package.json`), so each test file runs in a fresh global environment within a worker process. That bounds most cross-file leaks to a single worker, but `process.env` is still shared within a file's lifecycle — if your `afterEach` deletes the env var, the next describe/test's module-level code (or a beforeEach that re-reads env) gets `undefined`, causing `TypeError: The "paths[0]" property must be of type string`. Also, `TEST_TMP_DIR` is namespaced by `BUN_TEST_WORKER_ID` in `test/constants.ts` so parallel workers don't wipe each other's temp state during preload.
735+
**Why**: Vitest runs each test file in a worker pool (see `test:unit` in `package.json`), so each file runs in its own worker process. That bounds most cross-file leaks to a single worker, but `process.env` is still shared within a file's lifecycle — if your `afterEach` deletes the env var, the next describe/test's module-level code (or a beforeEach that re-reads env) gets `undefined`, causing `TypeError: The "paths[0]" property must be of type string`. Also, `TEST_TMP_DIR` is namespaced by `VITEST_POOL_ID` in `test/constants.ts` so parallel workers don't wipe each other's temp state during preload.
738736

739737
```typescript
740738
// CORRECT: Use the helper
@@ -757,7 +755,7 @@ afterEach(() => { delete process.env.SENTRY_CONFIG_DIR; }); // BUG!
757755
Use property-based tests when verifying invariants that should hold for **any valid input**.
758756

759757
```typescript
760-
import { describe, expect, test } from "bun:test";
758+
import { describe, expect, test } from "vitest";
761759
import { constantFrom, assert as fcAssert, property, tuple } from "fast-check";
762760
import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js";
763761

@@ -805,7 +803,7 @@ describe("property: myFunction", () => {
805803
Use model-based tests for **stateful systems** where sequences of operations should maintain invariants.
806804

807805
```typescript
808-
import { describe, expect, test } from "bun:test";
806+
import { describe, expect, test } from "vitest";
809807
import {
810808
type AsyncCommand,
811809
asyncModelRun,
@@ -933,7 +931,7 @@ When adding property tests for a function that already has unit tests, **remove
933931
```
934932

935933
```typescript
936-
import { describe, expect, test, mock } from "bun:test";
934+
import { describe, expect, test, vi } from "vitest";
937935

938936
describe("feature", () => {
939937
test("should return specific value", async () => {
@@ -942,7 +940,7 @@ describe("feature", () => {
942940
});
943941

944942
// Mock modules when needed
945-
mock.module("./some-module", () => ({
943+
vi.mock("./some-module", () => ({
946944
default: () => "mocked",
947945
}));
948946
```

packages/cli/biome.jsonc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
{
22
"$schema": "./node_modules/@biomejs/biome/configuration_schema.json",
33
"extends": ["ultracite/core"],
4-
// The workspace .gitignore lives at the monorepo root (two levels up from this
5-
// package). Point Biome's VCS integration there so `useIgnoreFile` resolves it.
4+
// The .gitignore Biome honors lives at the monorepo root (two levels up).
5+
// Point Biome's VCS integration there so `useIgnoreFile` reads the root file,
6+
// whose patterns are `packages/cli/`-prefixed. (The package-level .gitignore
7+
// is for plain git only; Biome does not read it.)
68
"vcs": {
79
"enabled": true,
810
"clientKind": "git",
@@ -24,7 +26,6 @@
2426
// on the node:tls module graph — an internal Biome bug that surfaces
2527
// non-deterministically as error vs warning. See biome issue tracker.
2628
"includes": [
27-
"!docs",
2829
"!test/init-eval/templates",
2930
"!dist-build",
3031
// The codemod package is a self-contained Codemod (jssg) project with

packages/cli/script/build.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -322,9 +322,9 @@ async function compileAllTargets(
322322
// key MUST equal this path string (see src/lib/dif/index.ts).
323323
//
324324
// Resolve via Node module resolution rather than a hardcoded
325-
// node_modules path: in the pnpm workspace, @sentry/symbolic is hoisted to
326-
// the workspace-root node_modules, not this package's, so a package-relative
327-
// literal path would not find it.
325+
// node_modules path: in the pnpm workspace @sentry/symbolic may live in the
326+
// isolated store (linked under this package's node_modules) rather than at a
327+
// fixed literal path, so module resolution is the reliable way to find it.
328328
let DIF_WASM_SRC: string;
329329
try {
330330
DIF_WASM_SRC = createRequire(import.meta.url).resolve(
@@ -349,8 +349,8 @@ async function compileAllTargets(
349349
);
350350

351351
// Invoke fossilize via `pnpm exec` so it resolves from the workspace bin
352-
// regardless of whether it's hoisted to the workspace-root node_modules (the
353-
// pnpm workspace hoists it there, not into this package's node_modules).
352+
// regardless of where pnpm places it (isolated store vs. root), rather than
353+
// assuming a fixed node_modules/.bin location.
354354
const fossilizeBin = "pnpm exec fossilize";
355355

356356
try {

packages/cli/script/bundle.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,8 +420,8 @@ console.log(" -> dist/index.d.cts + dist/index.d.mts (type declarations)");
420420
// which resolves relative to dist/index.cjs (see src/lib/dif/index.ts).
421421
await mkdir("./dist/vendor", { recursive: true });
422422
await copyFile(
423-
// Resolve via module resolution: in the pnpm workspace @sentry/symbolic is
424-
// hoisted to the workspace-root node_modules, not this package's.
423+
// Resolve via module resolution rather than a fixed node_modules path:
424+
// in the pnpm workspace @sentry/symbolic may live in the isolated store.
425425
createRequire(import.meta.url).resolve("@sentry/symbolic/symbolic_bg.wasm"),
426426
"./dist/vendor/symbolic_bg.wasm"
427427
);

packages/cli/script/check-patches.ts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,24 @@ const ROOT_PKG_PATH = "../../package.json";
5757
/**
5858
* Resolve an installed package file to the copy THIS package actually uses.
5959
*
60-
* With `node-linker=hoisted` in a pnpm workspace, a transitive (unpatched)
61-
* copy of a dependency can win the hoist to the workspace-root `node_modules`,
62-
* so a naive `node_modules/<pkg>` path may point at the wrong version. Using
63-
* `require.resolve` rooted at this package respects pnpm's per-package
64-
* resolution and finds the patched copy the `sentry` package links to.
60+
* In a pnpm workspace, a transitive (unpatched) copy of a dependency can
61+
* coexist with the patched direct copy (e.g. an unpatched `@sentry/core`
62+
* pulled in via another dependency). A naive `node_modules/<pkg>` path may
63+
* point at the wrong copy. Using `require.resolve` rooted at this package
64+
* respects pnpm's per-package resolution and finds the patched copy the
65+
* `sentry` package links to.
6566
*
6667
* We resolve the package's MAIN entry (not its `package.json`, which many
6768
* packages exclude from their `exports` map) and then derive the package root
6869
* by truncating the resolved path at the package-name segment.
6970
*
71+
* The package root is the FIRST `<pkgName>/` segment that follows the last
72+
* `node_modules/` boundary in the resolved path. Anchoring on `node_modules/`
73+
* (rather than `lastIndexOf('/<pkgName>/')`) is important: a package that
74+
* vendors a nested copy of itself (e.g.
75+
* `.../node_modules/@scope/pkg/vendor/@scope/pkg/index.js`) would otherwise
76+
* truncate at the wrong, inner segment.
77+
*
7078
* @param subpath - Package subpath, e.g. `@sentry/core/build/cjs/index.js`.
7179
* @returns Absolute path to the resolved file, or null if unresolvable.
7280
*/
@@ -80,9 +88,14 @@ function resolvePackageFile(subpath: string): string | null {
8088
try {
8189
// Resolve the package's main entry to locate the exact installed copy.
8290
const mainEntry = require_.resolve(pkgName);
83-
// Derive the package root: everything up to and including "<pkgName>".
91+
// The install dir is "<...>/node_modules/<pkgName>". Anchor on the last
92+
// "node_modules/" boundary, then take the first "<pkgName>/" after it so a
93+
// self-vendored nested copy can't shift the truncation point.
94+
const nmMarker = "/node_modules/";
95+
const nmIdx = mainEntry.lastIndexOf(nmMarker);
8496
const marker = `/${pkgName}/`;
85-
const idx = mainEntry.lastIndexOf(marker);
97+
const searchFrom = nmIdx === -1 ? 0 : nmIdx + nmMarker.length - 1;
98+
const idx = mainEntry.indexOf(marker, searchFrom);
8699
if (idx === -1) {
87100
return null;
88101
}

0 commit comments

Comments
 (0)