diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index ed8e99eba..724f67544 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -526,7 +526,7 @@ CliError (base, exitCode=1) - Pass `alternatives: []` when defaults are irrelevant (e.g., for missing Trace ID, Event ID) - Use `" and "` in `resource` for plural grammar: `"Trace ID and span ID"` → "are required" -**CI enforcement:** `pnpm run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory). +**CI enforcement:** `pnpm run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (ratchet baseline — new ones fail CI). ```typescript // Usage examples @@ -572,9 +572,12 @@ Use `logger.withTag("command-name")` for tagged logging in command files. **CI enforcement:** `pnpm run check:errors` includes a silent-catch scan that flags `catch` blocks which are empty, comment-only, or return-only without surfacing the -error. It is currently **advisory** (warns, does not fail CI) because of a pre-existing -backlog; run with `SENTRY_STRICT_SILENT_CATCH=1` to enforce. Do not add new silent -catches — they will appear in the scan output during review. +error. It is enforced with a **ratchet baseline** (`script/silent-catch-baseline.json`) +recording the per-file count of the pre-existing backlog: a *new* silent catch (a file +exceeding its baseline, or one not in the baseline) fails CI, and removing silent +catches without lowering the baseline also fails — so the backlog can only shrink. +When you fix or intentionally add a silent catch, refresh the baseline with +`pnpm run check:errors -- --update` and commit it. ### Auto-Recovery for Wrong Entity Types diff --git a/packages/cli/script/check-error-patterns.ts b/packages/cli/script/check-error-patterns.ts index 20a7dafea..b11ad2263 100644 --- a/packages/cli/script/check-error-patterns.ts +++ b/packages/cli/script/check-error-patterns.ts @@ -16,36 +16,41 @@ * `noEmptyBlockStatements` only catches syntactically empty `catch {}`; * this catches comment-only and return-only blocks too. * + * Silent catches are enforced with a **ratchet baseline** + * (`silent-catch-baseline.json`): the repo has a pre-existing backlog of + * best-effort catches (UI teardown, cleanup paths, etc.). The baseline records + * the known per-file count so that: + * - a *new* silent catch (a file exceeding its baseline, or a file absent from + * the baseline) fails CI, and + * - removing silent catches without lowering the baseline also fails CI, so + * the backlog can only shrink. + * Run with `--update` to regenerate the baseline after intentionally changing + * the set of silent catches. + * * Usage: - * tsx script/check-error-patterns.ts + * tsx script/check-error-patterns.ts # check (fails CI on drift) + * tsx script/check-error-patterns.ts --update # rewrite the baseline * * Exit codes: - * 0 - No anti-patterns found - * 1 - Anti-patterns detected + * 0 - No anti-patterns found and silent-catch baseline is in sync + * 1 - Anti-patterns detected or silent-catch baseline drifted */ -import { readFile } from "node:fs/promises"; +import { readFile, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; import { glob } from "tinyglobby"; -type Violation = { file: string; line: number; message: string }; +export type Violation = { file: string; line: number; message: string }; + +/** Per-file count of grandfathered silent catch blocks. */ +export type SilentCatchBaseline = Record; const CONTEXT_ERROR_RE = /new ContextError\(/g; const TRY_PATTERN_RE = /["'`]Try:/; -const files = await glob("src/**/*.ts"); - -/** Hard violations — these fail CI. */ -const violations: Violation[] = []; - -/** - * Advisory silent-catch findings. Reported as warnings but do NOT fail CI yet: - * the repo has a pre-existing backlog of intentional best-effort catches (e.g. - * UI teardown, cleanup paths). The check surfaces them for incremental cleanup - * and so new ones are visible in review. Set SENTRY_STRICT_SILENT_CATCH=1 to - * promote them to hard failures once the backlog is cleared. - */ -const silentCatchWarnings: Violation[] = []; -const STRICT_SILENT_CATCH = process.env.SENTRY_STRICT_SILENT_CATCH === "1"; +const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)); +export const BASELINE_PATH = join(SCRIPT_DIR, "silent-catch-baseline.json"); /** Characters that open a nesting level in JavaScript source. */ function isOpener(ch: string): boolean { @@ -225,7 +230,11 @@ function extractSecondArg(content: string, startIdx: number): string | null { * Detect `new ContextError(` where the second argument contains `\n`. * This catches resolution-failure prose stuffed into the command parameter. */ -function checkContextErrorNewlines(content: string, filePath: string): void { +export function findContextErrorNewlines( + content: string, + filePath: string +): Violation[] { + const found: Violation[] = []; let match = CONTEXT_ERROR_RE.exec(content); while (match !== null) { const startIdx = match.index + match[0].length; @@ -233,7 +242,7 @@ function checkContextErrorNewlines(content: string, filePath: string): void { if (secondArg?.includes("\\n")) { const line = content.slice(0, match.index).split("\n").length; - violations.push({ + found.push({ file: filePath, line, message: @@ -242,13 +251,18 @@ function checkContextErrorNewlines(content: string, filePath: string): void { } match = CONTEXT_ERROR_RE.exec(content); } + return found; } /** * Detect `new CliError(... "Try:" ...)` — ad-hoc "Try:" strings that bypass * the structured ResolutionError pattern. */ -function checkAdHocTryPatterns(content: string, filePath: string): void { +export function findAdHocTryPatterns( + content: string, + filePath: string +): Violation[] { + const found: Violation[] = []; const lines = content.split("\n"); let inCliError = false; @@ -258,7 +272,7 @@ function checkAdHocTryPatterns(content: string, filePath: string): void { inCliError = true; } if (inCliError && TRY_PATTERN_RE.test(line)) { - violations.push({ + found.push({ file: filePath, line: i + 1, message: @@ -271,6 +285,7 @@ function checkAdHocTryPatterns(content: string, filePath: string): void { inCliError = false; } } + return found; } /** Matches the start of a catch block in both statement and promise form. */ @@ -319,7 +334,11 @@ function stripComments(snippet: string): string { * empty or contain only a bare `return;`/`return ;` with no logging or * re-throw. These hide errors and violate the AGENTS.md no-silent-catch rule. */ -function checkSilentCatch(content: string, filePath: string): void { +export function findSilentCatches( + content: string, + filePath: string +): Violation[] { + const found: Violation[] = []; let match = CATCH_RE.exec(content); while (match !== null) { const openBraceIdx = match.index + match[0].length - 1; @@ -337,8 +356,7 @@ function checkSilentCatch(content: string, filePath: string): void { (code.length === 0 || returnOnly); if (silent) { const line = content.slice(0, match.index).split("\n").length; - const target = STRICT_SILENT_CATCH ? violations : silentCatchWarnings; - target.push({ + found.push({ file: filePath, line, message: @@ -347,43 +365,176 @@ function checkSilentCatch(content: string, filePath: string): void { } match = CATCH_RE.exec(content); } + return found; } -for (const filePath of files) { - const content = await readFile(filePath, "utf-8"); - checkContextErrorNewlines(content, filePath); - checkAdHocTryPatterns(content, filePath); - checkSilentCatch(content, filePath); +export type ScanResult = { + /** Hard violations — always fail CI. */ + violations: Violation[]; + /** Every silent catch found, across all scanned files. */ + silentCatches: Violation[]; +}; + +/** Scan the given files and collect violations and silent catches. */ +export async function scanFiles(files: string[]): Promise { + const violations: Violation[] = []; + const silentCatches: Violation[] = []; + for (const filePath of files) { + const content = await readFile(filePath, "utf-8"); + violations.push(...findContextErrorNewlines(content, filePath)); + violations.push(...findAdHocTryPatterns(content, filePath)); + silentCatches.push(...findSilentCatches(content, filePath)); + } + return { violations, silentCatches }; } -if (silentCatchWarnings.length > 0) { - console.warn( - `⚠ ${silentCatchWarnings.length} silent catch block(s) found (advisory; not failing CI).` - ); - console.warn( - " Add log.debug()/log.warn() or re-throw. Run with SENTRY_STRICT_SILENT_CATCH=1 to enforce.\n" - ); - for (const v of silentCatchWarnings) { - console.warn(` ${v.file}:${v.line}`); +/** Group silent catches into a per-file count map. */ +export function countByFile(silentCatches: Violation[]): SilentCatchBaseline { + const counts: SilentCatchBaseline = {}; + for (const v of silentCatches) { + counts[v.file] = (counts[v.file] ?? 0) + 1; + } + return counts; +} + +export type BaselineDrift = { + /** Files with more silent catches than the baseline allows (or new files). */ + regressions: { file: string; baseline: number; actual: number }[]; + /** Files with fewer silent catches than the baseline records. */ + improvements: { file: string; baseline: number; actual: number }[]; +}; + +/** + * Compare the current per-file silent-catch counts against the committed + * baseline. A regression (new silent catch) always fails CI. An improvement + * (silent catch removed without updating the baseline) also fails so the + * baseline stays honest and can only ratchet down. + */ +export function compareToBaseline( + actual: SilentCatchBaseline, + baseline: SilentCatchBaseline +): BaselineDrift { + const regressions: BaselineDrift["regressions"] = []; + const improvements: BaselineDrift["improvements"] = []; + const files = new Set([...Object.keys(actual), ...Object.keys(baseline)]); + for (const file of files) { + const a = actual[file] ?? 0; + const b = baseline[file] ?? 0; + if (a > b) { + regressions.push({ file, baseline: b, actual: a }); + } else if (a < b) { + improvements.push({ file, baseline: b, actual: a }); + } + } + regressions.sort((x, y) => x.file.localeCompare(y.file)); + improvements.sort((x, y) => x.file.localeCompare(y.file)); + return { regressions, improvements }; +} + +/** Load the committed baseline, treating a missing file as an empty baseline. */ +async function loadBaseline(): Promise { + try { + return JSON.parse(await readFile(BASELINE_PATH, "utf-8")); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return {}; + } + throw error; + } +} + +/** Serialize the baseline with stable key ordering and a trailing newline. */ +function serializeBaseline(counts: SilentCatchBaseline): string { + const sorted: SilentCatchBaseline = {}; + for (const key of Object.keys(counts).sort()) { + sorted[key] = counts[key] as number; } - console.warn(""); + return `${JSON.stringify(sorted, null, 2)}\n`; } -if (violations.length === 0) { - console.log("✓ No error class anti-patterns found"); +async function main(): Promise { + const update = process.argv.includes("--update"); + const files = await glob("src/**/*.ts"); + const { violations, silentCatches } = await scanFiles(files); + const actual = countByFile(silentCatches); + + if (update) { + await writeFile(BASELINE_PATH, serializeBaseline(actual)); + const total = silentCatches.length; + console.log( + `✓ Wrote silent-catch baseline: ${total} catch(es) across ${Object.keys(actual).length} file(s).` + ); + } + + const baseline = update ? actual : await loadBaseline(); + const { regressions, improvements } = compareToBaseline(actual, baseline); + + let failed = false; + + if (violations.length > 0) { + failed = true; + console.error( + `✗ Found ${violations.length} error class anti-pattern(s):\n` + ); + for (const v of violations) { + console.error(` ${v.file}:${v.line}`); + console.error(` ${v.message}\n`); + } + console.error( + "Fix: Use ResolutionError for resolution failures, ValidationError for input errors." + ); + console.error( + "See ContextError JSDoc in src/lib/errors.ts for usage guidance.\n" + ); + } + + if (regressions.length > 0) { + failed = true; + const added = regressions.reduce((n, r) => n + (r.actual - r.baseline), 0); + console.error( + `✗ ${added} new silent catch block(s) beyond the baseline:\n` + ); + for (const r of regressions) { + console.error(` ${r.file}: ${r.baseline} → ${r.actual}`); + } + console.error( + "\nEvery catch must re-throw, log.debug()/log.warn(), or return a fallback " + + "with a log.debug() explaining the suppression (AGENTS.md)." + ); + console.error( + "If a silent catch is truly intentional, run `pnpm run check:errors -- --update`.\n" + ); + } + + if (improvements.length > 0) { + failed = true; + const removed = improvements.reduce( + (n, r) => n + (r.baseline - r.actual), + 0 + ); + console.error( + `✗ ${removed} silent catch block(s) removed but the baseline is stale:\n` + ); + for (const r of improvements) { + console.error(` ${r.file}: ${r.baseline} → ${r.actual}`); + } + console.error( + "\nNice — the backlog shrank. Lock it in with `pnpm run check:errors -- --update`.\n" + ); + } + + if (failed) { + process.exit(1); + } + + const total = silentCatches.length; + console.log( + `✓ No error class anti-patterns found (silent-catch baseline: ${total} grandfathered).` + ); process.exit(0); } -console.error(`✗ Found ${violations.length} error class anti-pattern(s):\n`); -for (const v of violations) { - console.error(` ${v.file}:${v.line}`); - console.error(` ${v.message}\n`); +// Only run when invoked directly, not when imported by tests. +if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) { + await main(); } -console.error( - "Fix: Use ResolutionError for resolution failures, ValidationError for input errors." -); -console.error( - "See ContextError JSDoc in src/lib/errors.ts for usage guidance." -); - -process.exit(1); diff --git a/packages/cli/script/silent-catch-baseline.json b/packages/cli/script/silent-catch-baseline.json new file mode 100644 index 000000000..501d60e85 --- /dev/null +++ b/packages/cli/script/silent-catch-baseline.json @@ -0,0 +1,86 @@ +{ + "src/cli.ts": 2, + "src/commands/api.ts": 4, + "src/commands/auth/login.ts": 2, + "src/commands/auth/whoami.ts": 1, + "src/commands/cli/fix.ts": 1, + "src/commands/cli/setup.ts": 1, + "src/commands/dashboard/create.ts": 1, + "src/commands/issue/resolve-commit-spec.ts": 1, + "src/commands/org/list.ts": 1, + "src/commands/project/delete.ts": 1, + "src/commands/release/set-commits.ts": 2, + "src/commands/snapshots/diff.ts": 1, + "src/lib/api/projects.ts": 4, + "src/lib/binary.ts": 2, + "src/lib/browser.ts": 1, + "src/lib/cache-keys.ts": 1, + "src/lib/clipboard.ts": 1, + "src/lib/constants.ts": 1, + "src/lib/custom-ca.ts": 1, + "src/lib/db/auth.ts": 5, + "src/lib/db/dsn-cache.ts": 3, + "src/lib/db/index.ts": 2, + "src/lib/db/migration.ts": 1, + "src/lib/db/project-root-cache.ts": 1, + "src/lib/db/regions.ts": 1, + "src/lib/db/schema.ts": 2, + "src/lib/db/sqlite.ts": 1, + "src/lib/delta-upgrade.ts": 1, + "src/lib/detect-agent.ts": 2, + "src/lib/dev-script.ts": 2, + "src/lib/dif/find.ts": 3, + "src/lib/docs-context.ts": 4, + "src/lib/docs-service.ts": 1, + "src/lib/dsn/detector.ts": 1, + "src/lib/dsn/errors.ts": 1, + "src/lib/dsn/parser.ts": 2, + "src/lib/dsn/project-root.ts": 3, + "src/lib/dsn/resolver.ts": 1, + "src/lib/error-reporting.ts": 2, + "src/lib/errors.ts": 1, + "src/lib/formatters/conversation.ts": 1, + "src/lib/formatters/markdown.ts": 1, + "src/lib/formatters/sql.ts": 2, + "src/lib/git.ts": 10, + "src/lib/hex-id-recovery.ts": 1, + "src/lib/init/preflight.ts": 3, + "src/lib/init/stdin-reopen.ts": 3, + "src/lib/init/tools/file-changes/prepare.ts": 4, + "src/lib/init/tools/file-exists-batch.ts": 1, + "src/lib/init/tools/list-dir.ts": 3, + "src/lib/init/tools/read-files.ts": 1, + "src/lib/init/tools/shared.ts": 2, + "src/lib/init/ui/ink-ui.ts": 8, + "src/lib/init/verify-setup.ts": 1, + "src/lib/init/wizard-runner.ts": 1, + "src/lib/init/workflow-inputs.ts": 1, + "src/lib/logger.ts": 1, + "src/lib/oauth.ts": 1, + "src/lib/progress.ts": 1, + "src/lib/react-native/wrap-call.ts": 2, + "src/lib/region.ts": 2, + "src/lib/resolve-target.ts": 5, + "src/lib/resolve-team.ts": 1, + "src/lib/response-cache.ts": 7, + "src/lib/scan/worker-pool.ts": 1, + "src/lib/scope-recovery.ts": 1, + "src/lib/sdk-invoke.ts": 2, + "src/lib/search-query.ts": 1, + "src/lib/sentry-client.ts": 1, + "src/lib/sentry-url-parser.ts": 1, + "src/lib/sentry-urls.ts": 4, + "src/lib/shell.ts": 2, + "src/lib/sixel.ts": 3, + "src/lib/sourcemap/debug-id.ts": 1, + "src/lib/telemetry.ts": 6, + "src/lib/telemetry/zstd-transport.ts": 1, + "src/lib/timezone.ts": 4, + "src/lib/token-claims.ts": 1, + "src/lib/trace-target.ts": 1, + "src/lib/upgrade.ts": 3, + "src/lib/utils.ts": 1, + "src/lib/version-check.ts": 1, + "src/lib/which.ts": 1, + "src/lib/wrangler.ts": 1 +} diff --git a/packages/cli/test/script/check-error-patterns.test.ts b/packages/cli/test/script/check-error-patterns.test.ts new file mode 100644 index 000000000..a0e144aba --- /dev/null +++ b/packages/cli/test/script/check-error-patterns.test.ts @@ -0,0 +1,141 @@ +/** + * Tests for the error-pattern checker (script/check-error-patterns.ts). + * + * The script both runs standalone (globbing src/, printing, process.exit) and + * exports its detection + baseline logic so it can be unit-tested against string + * fixtures. We exercise the pure functions directly and run the whole check as a + * subprocess against the real source tree to guard the committed baseline. + */ + +import { spawnSync } from "node:child_process"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, test } from "vitest"; +import { + compareToBaseline, + countByFile, + findAdHocTryPatterns, + findContextErrorNewlines, + findSilentCatches, +} from "../../script/check-error-patterns.ts"; + +const pkgRoot = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); + +function runCheck(args: string[] = []) { + return spawnSync("pnpm", ["tsx", "script/check-error-patterns.ts", ...args], { + cwd: pkgRoot, + encoding: "utf-8", + }); +} + +describe("findSilentCatches", () => { + test("flags an empty catch", () => { + const src = "try { f(); } catch {}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(1); + }); + + test("flags a comment-only catch", () => { + const src = "try { f(); } catch (e) {\n // ignore\n}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(1); + }); + + test("flags a return-only catch", () => { + const src = "try { f(); } catch {\n return null;\n}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(1); + }); + + test("flags a return-only .catch() handler", () => { + const src = "p.catch((e) => {\n return null;\n});"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(1); + }); + + test("allows a catch that logs", () => { + const src = "try { f(); } catch (e) {\n log.debug('x', e);\n}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(0); + }); + + test("allows a catch that re-throws", () => { + const src = "try { f(); } catch (e) {\n throw e;\n}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(0); + }); + + test("allows a catch that forwards the error", () => { + const src = + "try { f(); } catch (error) {\n return handleFetchError(error);\n}"; + expect(findSilentCatches(src, "a.ts")).toHaveLength(0); + }); +}); + +describe("findContextErrorNewlines", () => { + test("flags a multi-line command argument", () => { + const src = 'throw new ContextError("issue", "run this\\nthen that");'; + expect(findContextErrorNewlines(src, "a.ts")).toHaveLength(1); + }); + + test("allows a single-line command argument", () => { + const src = 'throw new ContextError("issue", "sentry issue list");'; + expect(findContextErrorNewlines(src, "a.ts")).toHaveLength(0); + }); +}); + +describe("findAdHocTryPatterns", () => { + test('flags a CliError with an ad-hoc "Try:" string', () => { + const src = 'throw new CliError(\n "nope",\n "Try: sentry login",\n);'; + expect(findAdHocTryPatterns(src, "a.ts")).toHaveLength(1); + }); + + test("allows a CliError without a Try string", () => { + const src = 'throw new CliError("something went wrong");'; + expect(findAdHocTryPatterns(src, "a.ts")).toHaveLength(0); + }); +}); + +describe("countByFile", () => { + test("groups violations into per-file counts", () => { + const counts = countByFile([ + { file: "a.ts", line: 1, message: "" }, + { file: "a.ts", line: 9, message: "" }, + { file: "b.ts", line: 3, message: "" }, + ]); + expect(counts).toEqual({ "a.ts": 2, "b.ts": 1 }); + }); +}); + +describe("compareToBaseline", () => { + test("reports a new silent catch as a regression", () => { + const drift = compareToBaseline({ "a.ts": 2 }, { "a.ts": 1 }); + expect(drift.regressions).toEqual([ + { file: "a.ts", baseline: 1, actual: 2 }, + ]); + expect(drift.improvements).toEqual([]); + }); + + test("reports a file absent from the baseline as a regression", () => { + const drift = compareToBaseline({ "new.ts": 1 }, {}); + expect(drift.regressions).toEqual([ + { file: "new.ts", baseline: 0, actual: 1 }, + ]); + }); + + test("reports a removed silent catch as an improvement (stale baseline)", () => { + const drift = compareToBaseline({ "a.ts": 1 }, { "a.ts": 3 }); + expect(drift.improvements).toEqual([ + { file: "a.ts", baseline: 3, actual: 1 }, + ]); + expect(drift.regressions).toEqual([]); + }); + + test("reports no drift when counts match", () => { + const drift = compareToBaseline({ "a.ts": 2 }, { "a.ts": 2 }); + expect(drift.regressions).toEqual([]); + expect(drift.improvements).toEqual([]); + }); +}); + +describe("check-error-patterns (subprocess)", () => { + test("passes against the current source tree and committed baseline", () => { + const result = runCheck(); + expect(result.status, result.stdout + result.stderr).toBe(0); + expect(result.stdout).toContain("No error class anti-patterns found"); + }); +});