Skip to content

Commit fa4d785

Browse files
committed
fix: address review feedback — remove dead swap detection, fix AGENTS.md
- Remove unreachable detectSwappedViewArgs call from log/view.ts: validateHexId runs before swap detection, and valid hex IDs never contain '/', so the swap check could never trigger. Keep the looksLikeIssueShortId suggestion which still works (first arg is the target, not validated as hex). - Remove stray '=======' merge conflict marker from AGENTS.md line 693 - Remove duplicate Zod lore entry (019cc303) in AGENTS.md Addresses Seer and Cursor BugBot review comments.
1 parent c3a741f commit fa4d785

File tree

3 files changed

+13
-26
lines changed

3 files changed

+13
-26
lines changed

AGENTS.md

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -680,17 +680,14 @@ mock.module("./some-module", () => ({
680680
<!-- lore:019cc303-e397-75b9-9762-6f6ad108f50a -->
681681
* **Zod z.coerce.number() converts null to 0 silently**: Zod gotchas in this codebase: (1) \`z.coerce.number()\` passes input through \`Number()\`, so \`null\` silently becomes \`0\`. Be aware if \`null\` vs \`0\` distinction matters. (2) Zod v4 \`.default({})\` short-circuits — it returns the default value without parsing through inner schema defaults. So \`.object({ enabled: z.boolean().default(true) }).default({})\` returns \`{}\`, not \`{ enabled: true }\`. Fix: provide fully-populated default objects. This affected nested config sections in src/config.ts during the v3→v4 upgrade.
682682
683-
<!-- lore:019cc303-e397-75b9-9762-6f6ad108f50a -->
684-
* **Zod z.coerce.number() converts null to 0 silently**: Zod gotchas in this codebase: (1) \`z.coerce.number()\` passes input through \`Number()\`, so \`null\` silently becomes \`0\`. Be aware if \`null\` vs \`0\` distinction matters. (2) Zod v4 \`.default({})\` short-circuits — it returns the default value without parsing through inner schema defaults. So \`.object({ enabled: z.boolean().default(true) }).default({})\` returns \`{}\`, not \`{ enabled: true }\`. Fix: provide fully-populated default objects. This affected nested config sections in src/config.ts during the v3→v4 upgrade.
685-
686683
### Pattern
687684
688685
<!-- lore:dbd63348-2049-42b3-bb99-d6a3d64369c7 -->
689686
* **Branch naming and commit message conventions for Sentry CLI**: Branch naming: \`feat/\<short-description>\` or \`fix/\<issue-number>-\<short-description>\` (e.g., \`feat/ghcr-nightly-distribution\`, \`fix/268-limit-auto-pagination\`). Commit message format: \`type(scope): description (#issue)\` (e.g., \`fix(issue-list): auto-paginate --limit beyond 100 (#268)\`, \`feat(nightly): distribute via GHCR instead of GitHub Releases\`). Types seen: fix, refactor, meta, release, feat. PRs are created as drafts via \`gh pr create --draft\`. Implementation plans are attached to commits via \`git notes add\` rather than in PR body or commit message.
690687
691688
<!-- lore:5ac4e219-ea1f-41cb-8e97-7e946f5848c0 -->
692689
* **PR workflow: wait for Seer and Cursor BugBot before resolving**: After pushing a PR in the getsentry/cli repo, the CI pipeline includes Seer Code Review and Cursor Bugbot as advisory checks. Both typically take 2-3 minutes but may not trigger on draft PRs — only ready-for-review PRs reliably get bot reviews. The workflow is: push → wait for all CI (including npm build jobs which test the actual bundle) → check for inline review comments from Seer/BugBot → fix if needed → repeat. Use \`gh pr checks \<PR> --watch\` to monitor. Review comments are fetched via \`gh api repos/OWNER/REPO/pulls/NUM/comments\` and \`gh api repos/OWNER/REPO/pulls/NUM/reviews\`.
693-
=======
690+
694691
<!-- lore:019cc3e6-0cf5-720d-beb7-97c9c9901295 -->
695692
* **Codecov patch coverage only counts test:unit and test:isolated, not E2E**: CI coverage merges \`test:unit\` (\`test/lib test/commands test/types --coverage\`) and \`test:isolated\` (\`test/isolated --coverage\`) into \`coverage/merged.lcov\`. E2E tests (\`test/e2e\`) are NOT included in coverage reports. So func tests that spy on exports (e.g., \`spyOn(apiClient, 'getLogs')\`) give zero coverage to the mocked function's body. To cover \`api-client.ts\` function bodies in unit tests, mock \`globalThis.fetch\` + \`setOrgRegion()\` + \`setAuthToken()\` and call the real function.
696693

src/commands/log/view.ts

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import { isatty } from "node:tty";
99
import type { SentryContext } from "../../context.js";
1010
import { getLogs } from "../../lib/api-client.js";
1111
import {
12-
detectSwappedViewArgs,
1312
looksLikeIssueShortId,
1413
parseOrgProjectArg,
1514
parseSlashSeparatedArg,
@@ -75,8 +74,6 @@ function splitLogIds(arg: string): string[] {
7574
export function parsePositionalArgs(args: string[]): {
7675
logIds: string[];
7776
targetArg: string | undefined;
78-
/** Warning message if arguments appear to be in the wrong order */
79-
warning?: string;
8077
/** Suggestion when first arg looks like an issue short ID */
8178
suggestion?: string;
8279
} {
@@ -111,17 +108,15 @@ export function parsePositionalArgs(args: string[]): {
111108
if (logIds.length === 0) {
112109
throw new ContextError("Log ID", USAGE_HINT);
113110
}
114-
// With exactly 2 args, detect swapped args or issue short IDs.
115-
const warning =
116-
args.length === 2 && args[1]
117-
? (detectSwappedViewArgs(first, args[1]) ?? undefined)
118-
: undefined;
119-
const suggestion =
120-
!warning && looksLikeIssueShortId(first)
121-
? `Did you mean: sentry issue view ${first}`
122-
: undefined;
123-
124-
return { logIds, targetArg: first, warning, suggestion };
111+
// Swap detection is not useful here: validateHexId above rejects non-hex
112+
// log IDs (which include any containing "/"), so detectSwappedViewArgs
113+
// (which checks for "/" in the second arg) can never trigger.
114+
// We still check for issue short IDs in the first (target) position.
115+
const suggestion = looksLikeIssueShortId(first)
116+
? `Did you mean: sentry issue view ${first}`
117+
: undefined;
118+
119+
return { logIds, targetArg: first, suggestion };
125120
}
126121

127122
/**
@@ -350,11 +345,7 @@ export const viewCommand = buildCommand({
350345
const cmdLog = logger.withTag("log.view");
351346

352347
// Parse positional args
353-
const { logIds, targetArg, warning, suggestion } =
354-
parsePositionalArgs(args);
355-
if (warning) {
356-
cmdLog.warn(warning);
357-
}
348+
const { logIds, targetArg, suggestion } = parsePositionalArgs(args);
358349
if (suggestion) {
359350
cmdLog.warn(suggestion);
360351
}

test/commands/log/view.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,9 @@ describe("parsePositionalArgs", () => {
193193
});
194194
});
195195

196-
describe("swap detection and suggestions", () => {
196+
describe("suggestions and error handling", () => {
197197
test("swapped args (hex ID first, org/project second) throws ValidationError", () => {
198-
// With hex validation, the second arg ("my-org/my-project") fails
199-
// validateHexId before swap detection can trigger.
198+
// Non-hex second arg fails validateHexId — user gets a clear error.
200199
expect(() =>
201200
parsePositionalArgs([
202201
"968c763c740cfda8b6728f27fb9e9b01",

0 commit comments

Comments
 (0)