Skip to content

Commit 33dd8c1

Browse files
committed
fix: review improvements for CLI bug fixes (CLI-FR, CLI-TM, CLI-RN)
- api: narrow control-char regex to line breaks only + add log.warn() (keeps rejectControlChars meaningful for NUL bytes) - auth: add defaultCommand "status" (replaces help.ts suggestion hack) - auth: revert help.ts enrichment and /info suggestion entry - dashboard: fix missed x->col rename in edit.ts re-validation - dashboard: add missing test/types/dashboard.test.ts updates - docs: add "Intent-First Correction" UX philosophy to AGENTS.md
1 parent 39b8c14 commit 33dd8c1

10 files changed

Lines changed: 63 additions & 55 deletions

File tree

AGENTS.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -550,6 +550,18 @@ const { sql, values } = upsert(
550550
);
551551
```
552552

553+
### UX Philosophy: Intent-First Correction
554+
555+
When the user's intent is unambiguous, **do what they meant** instead of rejecting with an error. Show a `log.warn()` notice explaining what was corrected and how to do it properly next time. Only reject when intent is ambiguous or the input is genuinely dangerous (e.g., path traversal).
556+
557+
This applies across the CLI:
558+
- **Input cleanup** — strip copy-paste artifacts (line breaks, indentation) and warn, rather than throwing `ValidationError`
559+
- **Entity type recovery** — resolve wrong-type identifiers to the correct type (see "Auto-Recovery" below)
560+
- **Argument order swapping** — fix swapped positional args with a stderr warning
561+
- **Slug/org matching** — redirect bare slugs to the most likely intent
562+
563+
When recovery is **ambiguous or impossible**, keep the error but add entity-aware suggestions and fuzzy matches.
564+
553565
### Error Handling
554566

555567
All CLI errors extend the `CliError` base class from `src/lib/errors.ts`:
@@ -1070,4 +1082,5 @@ mock.module("./some-module", () => ({
10701082
10711083
<!-- lore:019cc43d-e651-7154-a88e-1309c4a2a2b6 -->
10721084
* **Testing Stricli command func() bodies via spyOn mocking**: To unit-test a Stricli command's \`func()\` body: (1) \`const func = await cmd.loader()\`, (2) \`func.call(mockContext, flags, ...args)\` with mock \`stdout\`, \`stderr\`, \`cwd\`, \`setContext\`. (3) \`spyOn\` namespace imports to mock dependencies (e.g., \`spyOn(apiClient, 'getLogs')\`). The \`loader()\` return type union causes \`.call()\` LSP errors — these are false positives that pass \`tsc --noEmit\`. When API functions are renamed (e.g., \`getLog\`\`getLogs\`), update both spy target name AND mock return shape (single → array). Slug normalization (\`normalizeSlug\`) replaces underscores with dashes but does NOT lowercase — test assertions must match original casing (e.g., \`'CAM-82X'\` not \`'cam-82x'\`).
1085+
10731086
<!-- End lore-managed section -->

src/commands/api.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,13 +85,17 @@ export function parseMethod(value: string): HttpMethod {
8585
* @internal Exported for testing
8686
*/
8787
export function normalizeEndpoint(endpoint: string): string {
88-
// Strip ASCII control characters and any adjacent whitespace before
89-
// validation. Users often copy-paste multi-line URLs from docs or
90-
// scripts, producing newlines and indentation (CLI-FR, 215 events).
91-
// biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally stripping control chars from user input
92-
const cleaned = endpoint.replace(/\s*[\x00-\x1f]+\s*/g, "").trim();
88+
// Strip line breaks and surrounding indentation from copy-pasted
89+
// endpoints. Users often paste multi-line URLs from docs or scripts,
90+
// producing newlines and indentation (CLI-FR, 215 events).
91+
// Other control characters (NUL, etc.) are left for validateEndpoint
92+
// to reject — those indicate corruption, not copy-paste.
93+
const cleaned = endpoint.replace(/[ \t]*[\r\n]+[ \t]*/g, "").trim();
94+
if (cleaned !== endpoint) {
95+
log.warn("Stripped line breaks from endpoint (copy-paste artifact)");
96+
}
9397

94-
// Reject path traversal after cleaning
98+
// Reject path traversal and remaining control characters after cleaning
9599
validateEndpoint(cleaned);
96100

97101
// Remove leading slash if present (rawApiRequest handles the base URL)

src/commands/auth/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ export const authRoute = buildRouteMap({
1515
token: tokenCommand,
1616
whoami: whoamiCommand,
1717
},
18+
defaultCommand: "status",
1819
docs: {
1920
brief: "Authenticate with Sentry",
2021
fullDescription:

src/commands/dashboard/widget/edit.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ export const editCommand = buildCommand({
364364
// because the fallback dimensions weren't known yet.
365365
if (replacement.layout && !existing.layout) {
366366
validateWidgetLayout(
367-
{ x: replacement.layout.x, width: replacement.layout.w },
367+
{ col: replacement.layout.x, width: replacement.layout.w },
368368
replacement.layout
369369
);
370370
}

src/commands/help.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
import type { SentryContext } from "../context.js";
1212
import { buildCommand } from "../lib/command.js";
13-
import { getCommandSuggestion } from "../lib/command-suggestions.js";
1413
import { OutputError } from "../lib/errors.js";
1514
import { CommandOutput } from "../lib/formatters/output.js";
1615
import {
@@ -59,17 +58,6 @@ export const helpCommand = buildCommand({
5958
// This ensures --json mode always gets structured output.
6059
const result = introspectCommand(commandPath);
6160
if ("error" in result) {
62-
// Enrich the error with a command suggestion when the unknown token
63-
// matches a known synonym (e.g., "info" → "sentry auth status").
64-
// Without this, agents and scripts that run `sentry info` get a bare
65-
// "command not found" dump instead of an actionable hint (CLI-TM).
66-
const suggestion = getCommandSuggestion("", commandPath[0] ?? "");
67-
if (suggestion) {
68-
const hint = suggestion.explanation
69-
? `${suggestion.explanation} ${suggestion.command}`
70-
: `Try: ${suggestion.command}`;
71-
result.suggestions = [hint, ...(result.suggestions ?? [])];
72-
}
7361
// OutputError renders through the output system but exits non-zero
7462
throw new OutputError(result);
7563
}

src/lib/command-suggestions.ts

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,6 @@ const SUGGESTIONS: ReadonlyMap<string, CommandSuggestion> = new Map([
9797
},
9898
],
9999

100-
// --- top-level synonyms (from CLI-TM telemetry, 54 events) ---
101-
[
102-
"/info",
103-
{
104-
command: "sentry auth status",
105-
explanation: "For account info, use",
106-
},
107-
],
108-
109100
// --- old sentry-cli commands (~5 events) ---
110101
["cli/info", { command: "sentry auth status" }],
111102
[

test/commands/api.test.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,9 @@ describe("normalizeEndpoint: path traversal hardening (#350)", () => {
127127
);
128128
});
129129

130-
test("strips control characters from endpoint", () => {
131-
expect(normalizeEndpoint("organizations/\x00admin/")).toBe(
132-
"organizations/admin/"
130+
test("rejects control characters in endpoint", () => {
131+
expect(() => normalizeEndpoint("organizations/\x00admin/")).toThrow(
132+
/Invalid/
133133
);
134134
});
135135

@@ -141,11 +141,18 @@ describe("normalizeEndpoint: path traversal hardening (#350)", () => {
141141
).toBe("organizations/my-org/issues/?environment=Production&project=123");
142142
});
143143

144-
test("strips tabs and carriage returns from endpoint", () => {
145-
expect(normalizeEndpoint("organizations/\tmy-org/\r\nissues/")).toBe(
144+
test("strips carriage returns and surrounding indentation", () => {
145+
expect(normalizeEndpoint("organizations/my-org/\r\n issues/")).toBe(
146146
"organizations/my-org/issues/"
147147
);
148148
});
149+
150+
test("preserves tabs within segments (not line-break related)", () => {
151+
// Tabs without adjacent line breaks are control chars — rejected
152+
expect(() => normalizeEndpoint("organizations/\tmy-org/")).toThrow(
153+
/Invalid/
154+
);
155+
});
149156
});
150157

151158
describe("parseFieldKey error cases", () => {

test/lib/command-suggestions.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,11 @@ describe("routes with defaultCommand", () => {
144144
}
145145
});
146146

147-
test("route groups without view do not have defaultCommand", () => {
148-
expect(routesWithDefault.has("auth")).toBe(false);
147+
test("auth route group has defaultCommand (status)", () => {
148+
expect(routesWithDefault.has("auth")).toBe(true);
149+
});
150+
151+
test("route groups without defaultCommand", () => {
149152
expect(routesWithDefault.has("cli")).toBe(false);
150153
expect(routesWithDefault.has("sourcemap")).toBe(false);
151154
expect(routesWithDefault.has("repo")).toBe(false);

test/lib/completions.property.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ describe("proposeCompletions: Stricli integration", () => {
182182
// potential positional arg for the default command and proposes flags
183183
// instead. These groups are tested separately below.
184184
const groupsWithDefaultCommand = new Set([
185+
"auth",
185186
"issue",
186187
"event",
187188
"org",

test/types/dashboard.test.ts

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -974,15 +974,15 @@ describe("validateWidgetLayout", () => {
974974

975975
test("accepts valid layout flags", () => {
976976
expect(() =>
977-
validateWidgetLayout({ x: 0, y: 0, width: 3, height: 2 })
977+
validateWidgetLayout({ col: 0, row: 0, width: 3, height: 2 })
978978
).not.toThrow();
979979
expect(() =>
980-
validateWidgetLayout({ x: 5, y: 10, width: 1, height: 1 })
980+
validateWidgetLayout({ col: 5, row: 10, width: 1, height: 1 })
981981
).not.toThrow();
982982
});
983983

984984
test("accepts partial layout flags", () => {
985-
expect(() => validateWidgetLayout({ x: 3 })).not.toThrow();
985+
expect(() => validateWidgetLayout({ col: 3 })).not.toThrow();
986986
expect(() => validateWidgetLayout({ width: 6 })).not.toThrow();
987987
expect(() => validateWidgetLayout({ height: 4 })).not.toThrow();
988988
});
@@ -991,17 +991,17 @@ describe("validateWidgetLayout", () => {
991991
expect(() => validateWidgetLayout({})).not.toThrow();
992992
});
993993

994-
test("rejects x >= GRID_COLUMNS", () => {
995-
expect(() => validateWidgetLayout({ x: 6 })).toThrow(ValidationError);
996-
expect(() => validateWidgetLayout({ x: 100 })).toThrow(ValidationError);
994+
test("rejects col >= GRID_COLUMNS", () => {
995+
expect(() => validateWidgetLayout({ col: 6 })).toThrow(ValidationError);
996+
expect(() => validateWidgetLayout({ col: 100 })).toThrow(ValidationError);
997997
});
998998

999-
test("rejects negative x", () => {
1000-
expect(() => validateWidgetLayout({ x: -1 })).toThrow(ValidationError);
999+
test("rejects negative col", () => {
1000+
expect(() => validateWidgetLayout({ col: -1 })).toThrow(ValidationError);
10011001
});
10021002

1003-
test("rejects negative y", () => {
1004-
expect(() => validateWidgetLayout({ y: -1 })).toThrow(ValidationError);
1003+
test("rejects negative row", () => {
1004+
expect(() => validateWidgetLayout({ row: -1 })).toThrow(ValidationError);
10051005
});
10061006

10071007
test("rejects width < 1", () => {
@@ -1018,32 +1018,32 @@ describe("validateWidgetLayout", () => {
10181018
expect(() => validateWidgetLayout({ height: -1 })).toThrow(ValidationError);
10191019
});
10201020

1021-
test("rejects x + width > GRID_COLUMNS", () => {
1022-
expect(() => validateWidgetLayout({ x: 4, width: 4 })).toThrow(
1021+
test("rejects col + width > GRID_COLUMNS", () => {
1022+
expect(() => validateWidgetLayout({ col: 4, width: 4 })).toThrow(
10231023
ValidationError
10241024
);
1025-
expect(() => validateWidgetLayout({ x: 5, width: 2 })).toThrow(
1025+
expect(() => validateWidgetLayout({ col: 5, width: 2 })).toThrow(
10261026
ValidationError
10271027
);
10281028
});
10291029

1030-
test("allows x + width = GRID_COLUMNS (exactly fills)", () => {
1031-
expect(() => validateWidgetLayout({ x: 3, width: 3 })).not.toThrow();
1032-
expect(() => validateWidgetLayout({ x: 0, width: 6 })).not.toThrow();
1030+
test("allows col + width = GRID_COLUMNS (exactly fills)", () => {
1031+
expect(() => validateWidgetLayout({ col: 3, width: 3 })).not.toThrow();
1032+
expect(() => validateWidgetLayout({ col: 0, width: 6 })).not.toThrow();
10331033
});
10341034

10351035
test("cross-validates with existing layout", () => {
10361036
const existing = { x: 4, y: 0, w: 2, h: 1 };
1037-
// Changing only x=5 with existing w=2 → 5+2=7 > 6
1038-
expect(() => validateWidgetLayout({ x: 5 }, existing)).toThrow(
1037+
// Changing only col=5 with existing w=2 → 5+2=7 > 6
1038+
expect(() => validateWidgetLayout({ col: 5 }, existing)).toThrow(
10391039
ValidationError
10401040
);
10411041
// Changing only width=3 with existing x=4 → 4+3=7 > 6
10421042
expect(() => validateWidgetLayout({ width: 3 }, existing)).toThrow(
10431043
ValidationError
10441044
);
1045-
// Valid: x=4 with existing w=2 → 4+2=6 ≤ 6
1046-
expect(() => validateWidgetLayout({ x: 4 }, existing)).not.toThrow();
1045+
// Valid: col=4 with existing w=2 → 4+2=6 ≤ 6
1046+
expect(() => validateWidgetLayout({ col: 4 }, existing)).not.toThrow();
10471047
});
10481048
});
10491049

0 commit comments

Comments
 (0)