Skip to content

Commit 707a57c

Browse files
committed
fix: improve error messages — fix ContextError/ResolutionError misuse
Fix the 'Try: ... Or: ...' error message pattern that produced confusing output for hundreds of users (CLI-7V: 127 events, CLI-4A: 21 events, etc). Problems fixed: - ContextError misused for resolution failures, producing messages like 'Project is required. Specify it using: No project found...' - Grammar: 'Trace ID and span ID is required' → 'are required' - Irrelevant DSN/project alternatives shown for missing positional IDs - Ad-hoc 'Try:' strings in raw CliError bypassing structured errors - Pagination cursor using ContextError instead of ValidationError - event view throwing confusing error for issue URLs (now fetches latest event) Prevention measures: - Runtime assertion in ContextError constructor: command must be single-line - CI check script (check:errors) scanning for multiline ContextError commands and ad-hoc 'Try:' patterns in CliError - Improved JSDoc explaining when to use ContextError vs ResolutionError - AGENTS.md updated with error class selection guidance Fixes CLI-7V, CLI-4A, CLI-EZ, CLI-CC, CLI-DQ
1 parent 701aab8 commit 707a57c

File tree

22 files changed

+533
-144
lines changed

22 files changed

+533
-144
lines changed

.github/workflows/ci.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ jobs:
149149
- run: bun run lint
150150
- run: bun run typecheck
151151
- run: bun run check:deps
152+
- run: bun run check:errors
152153

153154
test-unit:
154155
name: Unit Tests

AGENTS.md

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -399,19 +399,37 @@ CliError (base)
399399
├── AuthError (authentication - reason: 'not_authenticated' | 'expired' | 'invalid')
400400
├── ConfigError (configuration - suggestion?)
401401
├── ContextError (missing context - resource, command, alternatives)
402+
├── ResolutionError (value provided but not found - resource, headline, hint, suggestions)
402403
├── ValidationError (input validation - field?)
403404
├── DeviceFlowError (OAuth flow - code)
404405
├── SeerError (Seer AI - reason: 'not_enabled' | 'no_budget' | 'ai_disabled')
405406
└── UpgradeError (upgrade - reason: 'unknown_method' | 'network_error' | 'execution_failed' | 'version_not_found')
407+
```
408+
409+
**Choosing between ContextError, ResolutionError, and ValidationError:**
410+
411+
| Scenario | Error Class | Example |
412+
|----------|-------------|---------|
413+
| User **omitted** a required value | `ContextError` | No org/project provided |
414+
| User **provided** a value that wasn't found | `ResolutionError` | Project 'cli' not found |
415+
| User input is **malformed** | `ValidationError` | Invalid hex ID format |
406416
407-
// Usage: throw specific error types
408-
import { ApiError, AuthError, SeerError } from "../lib/errors.js";
409-
throw new AuthError("not_authenticated");
410-
throw new ApiError("Request failed", 404, "Not found");
411-
throw new SeerError("not_enabled", orgSlug); // Includes actionable URL
417+
**ContextError rules:**
418+
- `command` must be a **single-line** CLI usage example (e.g., `"sentry org view <slug>"`)
419+
- Constructor throws if `command` contains `\n` (catches misuse in tests)
420+
- Pass `alternatives: []` when defaults are irrelevant (e.g., for missing Trace ID, Event ID)
421+
- Use `" and "` in `resource` for plural grammar: `"Trace ID and span ID"` → "are required"
412422
413-
// In commands: let errors propagate to central handler
414-
// The bin.ts entry point catches and formats all errors consistently
423+
**CI enforcement:** `bun run check:errors` scans for `ContextError` with multiline commands and `CliError` with ad-hoc "Try:" strings.
424+
425+
```typescript
426+
// Usage examples
427+
throw new ContextError("Organization", "sentry org view <org-slug>");
428+
throw new ContextError("Trace ID", "sentry trace view <trace-id>", []); // no alternatives
429+
throw new ResolutionError("Project 'cli'", "not found", "sentry issue list <org>/cli", [
430+
"No project with this slug found in any accessible organization",
431+
]);
432+
throw new ValidationError("Invalid trace ID format", "traceId");
415433
```
416434
417435
### Async Config Functions

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@
7474
"generate:skill": "bun run script/generate-skill.ts",
7575
"generate:schema": "bun run script/generate-api-schema.ts",
7676
"check:skill": "bun run script/check-skill.ts",
77-
"check:deps": "bun run script/check-no-deps.ts"
77+
"check:deps": "bun run script/check-no-deps.ts",
78+
"check:errors": "bun run script/check-error-patterns.ts"
7879
},
7980
"type": "module"
8081
}

script/check-error-patterns.ts

Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
#!/usr/bin/env bun
2+
/**
3+
* Check for Error Class Misuse Patterns
4+
*
5+
* Scans source files for common anti-patterns in error class usage:
6+
*
7+
* 1. `new ContextError(resource, command)` where command contains `\n`
8+
* → Should use ResolutionError for resolution failures
9+
*
10+
* 2. `new CliError(... "Try:" ...)` — ad-hoc "Try:" strings
11+
* → Should use ResolutionError with structured hint/suggestions
12+
*
13+
* Usage:
14+
* bun run script/check-error-patterns.ts
15+
*
16+
* Exit codes:
17+
* 0 - No anti-patterns found
18+
* 1 - Anti-patterns detected
19+
*/
20+
21+
export {};
22+
23+
type Violation = { file: string; line: number; message: string };
24+
25+
const CONTEXT_ERROR_RE = /new ContextError\(/g;
26+
const TRY_PATTERN_RE = /["'`]Try:/;
27+
28+
const glob = new Bun.Glob("src/**/*.ts");
29+
const violations: Violation[] = [];
30+
31+
/** Characters that open a nesting level in JavaScript source. */
32+
function isOpener(ch: string): boolean {
33+
return ch === "(" || ch === "[" || ch === "{";
34+
}
35+
36+
/** Characters that close a nesting level in JavaScript source. */
37+
function isCloser(ch: string): boolean {
38+
return ch === ")" || ch === "]" || ch === "}";
39+
}
40+
41+
/** Characters that start a string literal in JavaScript source. */
42+
function isQuote(ch: string): boolean {
43+
return ch === '"' || ch === "'" || ch === "`";
44+
}
45+
46+
/**
47+
* Skip past a `${...}` expression inside a template literal.
48+
* @param content - Full source text
49+
* @param start - Index right after the `{` in `${`
50+
* @returns Index right after the closing `}`
51+
*/
52+
function skipTemplateExpression(content: string, start: number): number {
53+
let braceDepth = 1;
54+
let i = start;
55+
while (i < content.length && braceDepth > 0) {
56+
const ec = content[i];
57+
if (ec === "\\") {
58+
i += 2;
59+
} else if (ec === "`") {
60+
i = skipTemplateLiteral(content, i + 1);
61+
} else if (ec === "{") {
62+
braceDepth += 1;
63+
i += 1;
64+
} else if (ec === "}") {
65+
braceDepth -= 1;
66+
i += 1;
67+
} else {
68+
i += 1;
69+
}
70+
}
71+
return i;
72+
}
73+
74+
/**
75+
* Skip past a template literal, handling nested `${...}` expressions.
76+
* @param content - Full source text
77+
* @param start - Index right after the opening backtick
78+
* @returns Index right after the closing backtick
79+
*/
80+
function skipTemplateLiteral(content: string, start: number): number {
81+
let i = start;
82+
while (i < content.length) {
83+
const ch = content[i];
84+
if (ch === "\\") {
85+
i += 2;
86+
} else if (ch === "`") {
87+
return i + 1;
88+
} else if (ch === "$" && content[i + 1] === "{") {
89+
i = skipTemplateExpression(content, i + 2);
90+
} else {
91+
i += 1;
92+
}
93+
}
94+
return i;
95+
}
96+
97+
/**
98+
* Advance past a string literal (single-quoted, double-quoted, or template).
99+
* @param content - Full source text
100+
* @param start - Index of the opening quote character
101+
* @returns Index right after the closing quote
102+
*/
103+
function skipString(content: string, start: number): number {
104+
const quote = content[start];
105+
if (quote === "`") {
106+
return skipTemplateLiteral(content, start + 1);
107+
}
108+
let i = start + 1;
109+
while (i < content.length) {
110+
const ch = content[i];
111+
if (ch === "\\") {
112+
i += 2;
113+
} else if (ch === quote) {
114+
return i + 1;
115+
} else {
116+
i += 1;
117+
}
118+
}
119+
return i;
120+
}
121+
122+
/**
123+
* Advance one token in JS source, skipping strings as atomic units.
124+
* @returns The next index and the character at position `i` (or the string span's first char).
125+
*/
126+
function advanceToken(
127+
content: string,
128+
i: number
129+
): { next: number; ch: string } {
130+
const ch = content[i] ?? "";
131+
if (isQuote(ch)) {
132+
return { next: skipString(content, i), ch };
133+
}
134+
return { next: i + 1, ch };
135+
}
136+
137+
/**
138+
* Walk from `startIdx` (just inside the opening `(`) to find the matching `)`,
139+
* tracking commas at depth 1.
140+
* @returns The index of the first comma (between arg1 and arg2) and the closing paren index.
141+
*/
142+
function findCallBounds(
143+
content: string,
144+
startIdx: number
145+
): { commaIdx: number; closingIdx: number } | null {
146+
let depth = 1;
147+
let commaCount = 0;
148+
let commaIdx = -1;
149+
let i = startIdx;
150+
151+
while (i < content.length && depth > 0) {
152+
const { next, ch } = advanceToken(content, i);
153+
if (isOpener(ch)) {
154+
depth += 1;
155+
} else if (isCloser(ch)) {
156+
depth -= 1;
157+
} else if (ch === "," && depth === 1) {
158+
commaCount += 1;
159+
if (commaCount === 1) {
160+
commaIdx = i;
161+
}
162+
}
163+
i = next;
164+
}
165+
166+
if (commaIdx === -1) {
167+
return null;
168+
}
169+
return { commaIdx, closingIdx: i - 1 };
170+
}
171+
172+
/**
173+
* Extract the second argument of a `new ContextError(...)` call from source text.
174+
* Properly handles template literals so backticks don't break depth tracking.
175+
* @returns The raw source text of the second argument, or null if not found.
176+
*/
177+
function extractSecondArg(content: string, startIdx: number): string | null {
178+
const bounds = findCallBounds(content, startIdx);
179+
if (!bounds) {
180+
return null;
181+
}
182+
183+
const { commaIdx, closingIdx } = bounds;
184+
185+
// Find end of second arg: next comma at depth 1 or closing paren
186+
let endIdx = closingIdx;
187+
let d = 1;
188+
for (let j = commaIdx + 1; j < closingIdx; j += 1) {
189+
const { next, ch } = advanceToken(content, j);
190+
if (isOpener(ch)) {
191+
d += 1;
192+
} else if (isCloser(ch)) {
193+
d -= 1;
194+
} else if (ch === "," && d === 1) {
195+
endIdx = j;
196+
break;
197+
}
198+
// advanceToken may skip multiple chars (strings), adjust loop var
199+
j = next - 1; // -1 because for-loop increments
200+
}
201+
202+
return content.slice(commaIdx + 1, endIdx).trim();
203+
}
204+
205+
/**
206+
* Detect `new ContextError(` where the second argument contains `\n`.
207+
* This catches resolution-failure prose stuffed into the command parameter.
208+
*/
209+
function checkContextErrorNewlines(content: string, filePath: string): void {
210+
let match = CONTEXT_ERROR_RE.exec(content);
211+
while (match !== null) {
212+
const startIdx = match.index + match[0].length;
213+
const secondArg = extractSecondArg(content, startIdx);
214+
215+
if (secondArg?.includes("\\n")) {
216+
const line = content.slice(0, match.index).split("\n").length;
217+
violations.push({
218+
file: filePath,
219+
line,
220+
message:
221+
"ContextError command contains '\\n'. Use ResolutionError for multi-line resolution failures.",
222+
});
223+
}
224+
match = CONTEXT_ERROR_RE.exec(content);
225+
}
226+
}
227+
228+
/**
229+
* Detect `new CliError(... "Try:" ...)` — ad-hoc "Try:" strings that bypass
230+
* the structured ResolutionError pattern.
231+
*/
232+
function checkAdHocTryPatterns(content: string, filePath: string): void {
233+
const lines = content.split("\n");
234+
let inCliError = false;
235+
236+
for (let i = 0; i < lines.length; i += 1) {
237+
const line = lines[i] ?? "";
238+
if (line.includes("new CliError(")) {
239+
inCliError = true;
240+
}
241+
if (inCliError && TRY_PATTERN_RE.test(line)) {
242+
violations.push({
243+
file: filePath,
244+
line: i + 1,
245+
message:
246+
'CliError contains "Try:" — use ResolutionError with structured hint/suggestions instead.',
247+
});
248+
inCliError = false;
249+
}
250+
// Reset after a reasonable window (closing paren)
251+
if (inCliError && line.includes(");")) {
252+
inCliError = false;
253+
}
254+
}
255+
}
256+
257+
for await (const filePath of glob.scan(".")) {
258+
const content = await Bun.file(filePath).text();
259+
checkContextErrorNewlines(content, filePath);
260+
checkAdHocTryPatterns(content, filePath);
261+
}
262+
263+
if (violations.length === 0) {
264+
console.log("✓ No error class anti-patterns found");
265+
process.exit(0);
266+
}
267+
268+
console.error(`✗ Found ${violations.length} error class anti-pattern(s):\n`);
269+
for (const v of violations) {
270+
console.error(` ${v.file}:${v.line}`);
271+
console.error(` ${v.message}\n`);
272+
}
273+
console.error(
274+
"Fix: Use ResolutionError for resolution failures, ValidationError for input errors."
275+
);
276+
console.error(
277+
"See ContextError JSDoc in src/lib/errors.ts for usage guidance."
278+
);
279+
280+
process.exit(1);

0 commit comments

Comments
 (0)