From 71171c0713ddabe148c05ef32a7ad74f1ddb5de5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 28 Apr 2026 00:48:06 +0000 Subject: [PATCH 1/2] feat: add hidden --org/--project global flags for LLM compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LLMs trained on the older sentry-cli generate commands with --org and --project flags. Instead of showing a confusing 'unknown flag' error, silently accept them and map to SENTRY_ORG/SENTRY_PROJECT env vars so the existing resolution chain handles them at priority #2. The flags are hidden (not shown in --help), have no short aliases (-p conflicts with release create), and are stripped before the command's func() receives its flags object. Commands that define their own --org or --project flag (e.g. release create --project) keep their own semantics — the compat shim skips injection for those. Also refactors flag merging into a dedicated mergeGlobalFlags() helper to keep buildCommand under Biome's cognitive complexity limit. --- AGENTS.md | 109 +++++------ src/lib/command.ts | 176 +++++++++++++++--- src/lib/global-flags.ts | 6 + test/lib/argv-hoist.property.test.ts | 11 +- test/lib/argv-hoist.test.ts | 52 ++++++ test/lib/command.test.ts | 268 +++++++++++++++++++++++++++ 6 files changed, 539 insertions(+), 83 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 57a946b78..27eed8233 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -991,86 +991,89 @@ mock.module("./some-module", () => ({ ### Architecture - -* **@sentry/api SDK integration: type wrapping pattern and pagination helpers**: @sentry/api SDK integration: wrap SDK types at \`src/lib/api/\*.ts\` boundaries with \`as unknown as SentryX\` casts; never leak SDK types to commands. Wrappers in \`src/types/sentry.ts\` use \`Partial\ & RequiredCore\`. \`src/lib/region.ts\` imports \`retrieveAnOrganization\` directly to avoid circular dep with api-client. \`unwrapResult\`/\`unwrapPaginatedResult\` MUST stay CLI-owned — SDK versions throw plain \`Error\`, breaking the 'all errors are CliError subclasses' invariant (see also 365e4299). Body-shape casts use \`Parameters\\[0]\["body"]\`. + +* **api-client.ts split into domain modules under src/lib/api/**: Monolithic \`src/lib/api-client.ts\` was split into domain modules under \`src/lib/api/\`: infrastructure.ts, organizations.ts, projects.ts, teams.ts, repositories.ts, issues.ts, events.ts, traces.ts, logs.ts, seer.ts, trials.ts, users.ts. Original file is now a ~100-line barrel re-export (already in \`biome.jsonc\` \`noBarrelFile\` override). Add new API functions to the appropriate domain module, not the barrel file. - -* **apiRequestToRegion/rawApiRequest options shape — no timeout/signal/headers on the typed API**: \`ApiRequestOptions\\` in \`src/lib/api/infrastructure.ts\` has only \`{ method, body, params, schema }\`. \`rawApiRequest\` adds \`headers?\`; neither exposes \`timeout\`/\`signal\`. Call sites pass \`(url, init: RequestInit)\` to authenticated fetch — never a \`Request\` (only @sentry/api SDK does). \`apiRequestToRegion\` auto-sets JSON Content-Type and \`JSON.stringify\`s body; \`rawApiRequest\` preserves string bodies, only sets JSON Content-Type when body is object and caller didn't provide one. 204/205 throw \`ApiError\` rather than crashing on \`response.json()\` — bulk-mutate callers must catch. + +* **Issue resolve --in grammar: release + @next + @commit sentinels**: \`sentry issue resolve --in\` grammar: (a) omitted→immediate resolve, (b) \`\\`→\`inRelease\` (monorepo \`spotlight@1.2.3\` pass-through), (c) \`@next\`→\`inNextRelease\`, (d) \`@commit\`→auto-detect git HEAD + match Sentry repos, (e) \`@commit:\@\\`→explicit. Sentinel matching case-insensitive; unknown \`@\`-prefixed tokens throw \`ValidationError\`. \`parseResolveSpec\` splits on LAST \`@\` to handle scoped names like \`@acme/web\`. \`resolveCommitSpec\` uses \`getHeadCommit\`/\`getRepositoryName\` from \`src/lib/git.ts\`, matching Sentry repo \`externalSlug\` or \`name\` via \`listRepositoriesCached\`. API requires \`statusDetails.inCommit: {commit, repository}\` — not bare SHA. - -* **Completion fast-path skips Sentry SDK via SENTRY\_CLI\_NO\_TELEMETRY and SQLite telemetry queue**: Shell completions (\`\_\_complete\`) set \`SENTRY\_CLI\_NO\_TELEMETRY=1\` in \`bin.ts\` before any imports, skipping \`createTracedDatabase\` and avoiding \`@sentry/node-core/light\` load (~85ms). Completion timing queued to \`completion\_telemetry\_queue\` SQLite table (~1ms); normal runs drain via \`DELETE FROM ... RETURNING\` and emit as \`Sentry.metrics.distribution\`. Achieves ~60ms dev / ~140ms CI within 200ms e2e budget. + +* **npm bundle requires Node.js >= 22 due to node:sqlite polyfill**: The npm package (dist/bin.cjs) requires Node.js >= 22 because the bun:sqlite polyfill uses \`node:sqlite\`. A runtime version guard in the esbuild banner catches this early. When writing esbuild banner strings in TS template literals, double-escape: \`\\\\\\\n\` in TS → \`\\\n\` in output → newline at runtime. Single \`\\\n\` produces a literal newline inside a JS string, causing SyntaxError. - -* **Fuzzy recovery auto-resolves dash/underscore slug mismatches without original-slug fallback**: Display-name project input (contains spaces) skips slug lookup, goes to name-based fuzzy search across four resolution sites: \`resolveProjectBySlug\`, \`resolveOrgProjectTarget\` (project-search), \`org-list.ts#handleProjectSearch\`, \`project/list.ts#handleProjectNotFound\`. \`parseOrgProjectArg\` detects spaces via \`looksLikeDisplayName()\` and sets \`originalSlug\` on \`project-search\`; sites check \`isDisplayName = originalSlug !== undefined\` and skip \`findProjectsBySlug\` (404s on URL-encoded spaces), going directly to \`triageProjectNotFound\` → \`findSimilarProjectsAcrossOrgs\`. \*\*Critical\*\*: recursive fuzzy recovery calls must NOT pass \`originalSlug\` — otherwise the recovered slug also skips lookup, causing infinite skip→empty→not-found loop. + +* **repo\_cache SQLite table for offline Sentry repo lookups**: Schema v14 adds \`repo\_cache\` table in \`src/lib/db/schema.ts\` + helpers in \`src/lib/db/repo-cache.ts\` (7-day TTL). \`listAllRepositories(org)\` in \`src/lib/api/repositories.ts\` paginates through \`listRepositoriesPaginated\` using \`API\_MAX\_PER\_PAGE\` and \`MAX\_PAGINATION\_PAGES\` — never use the unpaginated \`listRepositories\` for cache-backed lookups (silently caps at ~25). \`listRepositoriesCached(org)\` wraps it with cache-first lookup and a try/catch around \`setCachedRepos\` so read-only databases (macOS \`sudo brew install\`) don't crash commands whose API fetch already succeeded. Used by \`@commit\` resolver to match git origin \`owner/repo\` against Sentry repo \`externalSlug\` or \`name\`. - -* **Project cache is org-scoped with three key formats and three population paths**: \`project\_cache\` SQLite table uses three key shapes: \`{orgId}:{projectId}\` (DSN resolution), \`dsn:{publicKey}\` (DSN without orgId), \`list:{orgSlug}/{projectSlug}\` (batch from API). Helpers: \`getCachedProject\`, \`getCachedProjectByDsnKey\`, \`getCachedProjectsForOrg\` (completions), \`getCachedProjectBySlug\` (queries all three shapes for hot-path slug lookups; used by \`fetchProjectId\` to skip \`GET /projects/{org}/{project}/\`). Population paths: DSN resolution in resolve-target.ts, \`listProjects()\` batch via \`cacheProjectsForOrg\`, \`fetchProjectId\` seeds on API success. Resolution errors use live API via \`findSimilarProjectsAcrossOrgs\` — no cross-org cache search. + +* **Seer trial prompt uses middleware layering in bin.ts error handling chain**: Seer trial prompt via error middleware layering: \`bin.ts\` chain is \`main() → executeWithAutoAuth() → executeWithSeerTrialPrompt() → runCommand()\`. Seer trial prompts (\`no\_budget\`/\`not\_enabled\`) caught by inner wrapper; auth errors bubble to outer. Trial API: \`GET /api/0/customers/{org}/\` → \`productTrials\[]\` (prefer \`seerUsers\`, fallback \`seerAutofix\`). Start: \`PUT /api/0/customers/{org}/product-trial/\`. SaaS-only; self-hosted 404s gracefully. \`ai\_disabled\` excluded. \`startSeerTrial\` accepts \`category\` from trial object — don't hardcode. - -* **Sentry API: events require org+project, issues have legacy global endpoint**: Sentry API scoping/auth quirks: (1) Events require org+project (\`/projects/{org}/{project}/events/{id}/\`); issues use legacy global \`/api/0/issues/{id}/\`; traces need only org. Cross-project search via Discover \`/organizations/{org}/events/?query=id:{eventId}\`. (2) \`/users/me/\` returns 403 for OAuth tokens — use \`/auth/\` instead (all token types, control silo). \`getControlSiloUrl()\` routes; \`SentryUserSchema\` uses \`.passthrough()\` since \`/auth/\` only requires \`id\`. (3) Chunk upload endpoint returns camelCase (\`chunkSize\`, \`chunksPerRequest\`, \`maxRequestSize\`, \`hashAlgorithm\`); \`AssembleResponse\` also camelCase — exception to snake\_case convention. +### Decision - -* **Sentry CLI authenticated fetch architecture with response caching**: Authenticated fetch (\`createAuthenticatedFetch\` in \`src/lib/sentry-client.ts\`): auth headers, 30s \`REQUEST\_TIMEOUT\_MS\`, retry max 2, 401 refresh, span tracing. Dual input: SDK \`Request\` vs \`(url, init)\`. Body-reuse: \`buildAttemptFactory\` yields fresh \`(input, init)\` per attempt; \`Request\` clones per attempt; \`FormData\`/\`Blob\`/\`URLSearchParams\` pass through (fetch re-derives multipart boundary). Only bare \`ReadableStream\` needs materialization. Do NOT materialize FormData — strips auto-negotiated \`Content-Type: multipart/form-data; boundary=...\`. Timeouts: internal aborts tagged \`INTERNAL\_TIMEOUT\_MARKER\` Symbol; last attempt throws \`TimeoutError\`. Per-endpoint \`ENDPOINT\_TIMEOUT\_OVERRIDES\` (e.g. \`/autofix/\` 120s). Response cache: \`http-cache-semantics\` RFC 7234 at \`~/.sentry/cache/responses/\`; GET 2xx only. On 4xx/5xx, \`apiRequestToRegion\` attaches allow-listed response headers to Sentry scope as \`api\_response\_headers\` context for empty-detail triage. + +* **Raw markdown output for non-interactive terminals, rendered for TTY**: Markdown-first output pipeline: custom renderer in \`src/lib/formatters/markdown.ts\` walks \`marked\` tokens to produce ANSI-styled output. Commands build CommonMark using helpers (\`mdKvTable()\`, \`mdRow()\`, \`colorTag()\`, \`escapeMarkdownCell()\`, \`safeCodeSpan()\`) and pass through \`renderMarkdown()\`. \`isPlainOutput()\` precedence: \`SENTRY\_PLAIN\_OUTPUT\` > \`NO\_COLOR\` > \`FORCE\_COLOR\` > \`!isTTY\`. \`--json\` always outputs JSON. Colors defined in \`COLORS\` object in \`colors.ts\`. Tests run non-TTY so assertions match raw CommonMark; use \`stripAnsi()\` helper for rendered-mode assertions. - -* **Sentry CLI resolve-target cascade has 5 priority levels with env var support**: resolve-target.ts cascade has 5 priority levels: (1) Explicit CLI flags, (2) SENTRY\_ORG/SENTRY\_PROJECT env vars, (3) SQLite config defaults, (4) DSN auto-detection, (5) Directory name inference. SENTRY\_PROJECT supports combo notation \`org/project\` — when used, SENTRY\_ORG is ignored. If combo parse fails (e.g. \`org/\`), the entire value is discarded. \`resolveFromEnvVars()\` helper is injected into all four resolution functions. +### Gotcha -### Decision + +* **--json schema stability: collapse=organization drops nested org fields**: --json schema + response cache gotchas: (1) \`?collapse=organization\` shrinks \`organization\` to \`{id, slug}\` — silent --json regression. \`jsonTransform\` re-hydrates \`organization.name\` via \`resolveOrgDisplayName\` against \`org\_regions\` cache. (2) \`buildCacheKey()\` normalizes URL with sorted query params, so \`invalidateCachedResponse(baseUrl)\` misses entries with query suffixes. Use \`invalidateCachedResponsesMatching(prefix)\` (raw \`startsWith()\`); \`buildApiUrl()\` always emits trailing slash → safe prefix. (3) When \`jsonTransform\` is set, \`jsonExclude\` and \`filterFields\` are NOT applied — transform must call \`filterFields(result, fields)\` and omit excluded keys itself. - -* **Issue list global limit with fair per-project distribution and representation guarantees**: \`issue list --limit\` is a global total across all detected projects. \`fetchWithBudget\` Phase 1 divides evenly, Phase 2 redistributes surplus via cursor resume. \`trimWithProjectGuarantee\` ensures ≥1 issue per project before filling remaining slots. JSON output wraps in \`{ data, hasMore }\` with optional \`errors\` array. Compound cursor (pipe-separated) enables \`-c last\` for multi-target pagination, keyed by sorted target fingerprint. + +* **@sentry/api SDK passes Request object to custom fetch — headers lost on Node.js**: @sentry/api SDK calls \`\_fetch(request)\` with no init object. In \`authenticatedFetch\`, \`init\` is undefined so \`prepareHeaders\` creates empty headers — on Node.js this strips Content-Type (HTTP 415). Fix: fall back to \`input.headers\` when \`init\` is undefined. Use \`unwrapPaginatedResult\` (not \`unwrapResult\`) to access Link header for pagination. \`per\_page\` not in SDK types; cast query to pass at runtime. Also: SDK returns \`data = {}\` (not \`\[]\`) when response body is empty/204/missing Content-Type — \`unwrapResult\` returns as-is, so always guard array results with \`Array.isArray(data)\` before \`.map()\`. Self-hosted instances behind reverse proxies commonly trigger this. - -* **Prefer dedicated SQLite tables + migrations over metadata KV for non-trivial caches**: Prefer dedicated SQLite tables + migrations over \`metadata\` KV for non-trivial caches. Schema migrations are cheap — don't shoehorn structured caches into \`metadata\` with dotted-prefix keys. Dedicated tables give clearer schema, proper indexes, simpler bulk-clear, no prefix collisions. \`metadata\` KV is fine for small scalars (defaults.\*, install.\*). Example: \`issue\_org\_cache\` (schema v15) replaced \`metadata\` keys \`issue\_org.{numericId}\`. Migration pattern: bump \`CURRENT\_SCHEMA\_VERSION\`, add \`EXPECTED\_TABLES.foo\`, add \`if (currentVersion < N) db.exec(EXPECTED\_TABLES.foo)\`. HTTP response cache (URL+headers, short TTLs) can't answer structural questions like 'which org owns issue 123?' — use dedicated tables for structural/mapping questions, HTTP cache for content. + +* **Biome noUselessUndefined also rejects () => {} empty arrow callbacks**: Biome lint traps: (1) \`noUselessUndefined\` rejects \`() => undefined\` AND \`noEmptyBlockStatements\` rejects \`() => {}\` — use top-level \`function noop(): void {}\`. (2) \`noExcessiveCognitiveComplexity\` caps at 15. (3) \`expect(() => fn()).toThrow(X)\` must be one line. (4) Plugin forbids raw \`metadata\` table queries — use \`getMetadata\`/\`setMetadata\`/\`clearMetadata\`. (5) Also enforced: \`useBlockStatements\`, \`noNestedTernary\`, \`useAtIndex\`, \`noStaticOnlyClass\`, \`useSimplifiedLogicExpression\`, \`noShadow\`. Namespace imports forbidden. (6) \`useYield\` fires on \`async \*func()\` with statements but not empty bodies — only add \`biome-ignore\` to generators with statements (empty-body suppressions trigger \`suppressions/unused\`). \`lint:fix\` differs from CI \`lint\`: auto-fix hides \`noPrecisionLoss\` on >2^53 literals, \`noIncrementDecrement\`, and import ordering. biome-ignore suppressions only valid when rule actually fires. Always \`bun run lint\` before pushing. -### Gotcha + +* **Bun --isolate coverage inflates LF count for files with verbose comments/JSDoc**: Under \`bun test --isolate --parallel\` (used in CI's \`test:unit\`), Bun's coverage instrumentation reports a higher \`LF\` (lines found) count for files than non-isolate runs — comments, blank lines, type annotations, and closing braces inside function bodies get counted as "executable". Locally \`LF=165\` for \`zstd-transport.ts\` becomes \`LF=210\` under \`--isolate\`, dropping coverage from 99% to 78%. Codecov sees the inflated number. Workaround: trim verbose inline comments inside function bodies (move rationale to JSDoc above the function or extract to module-level doc). Doesn't affect correctness, just codecov's patch coverage gate. Statement coverage is still 100% — the "missing" lines are non-executable. - -* **@sentry/api SDK can return non-array data for empty/edge responses**: \`@sentry/api\` SDK (in \`node\_modules/@sentry/api/dist/index.js\`) returns \`data = {}\` (not \`\[]\`) when response body is empty, has \`Content-Length: 0\`, or status 204; and returns a \`ReadableStream\` when \`Content-Type\` is missing. \`unwrapResult\` from \`src/lib/api/infrastructure.ts\` returns \`data\` as-is, and \`as unknown as SentryX\[]\` casts silently lie. Always guard array-typed SDK results with \`Array.isArray(data)\` before \`.map()\` — applied in \`listOrganizationsInRegion\` (CLI-1CQ). Self-hosted instances behind reverse proxies (nginx, Cloudflare, WAFs) commonly trigger this by stripping bodies or wrapping responses. Throw a descriptive \`ApiError\` on mismatch rather than letting \`TypeError: x.map is not a function\` bubble up minified. + +* **Bun 1.3.11 tty.ReadStream leaks libuv handle — process.stdin.unref is undefined**: Bun 1.3.11 macOS TTY bug: \`process.stdin\` via kqueue \`EVFILT\_READ\` on reopened non-session-leader TTY fd fails to deliver keystrokes when fd 0 inherited via \`exec bin \ -* **Bun bytecode: true crashes esbuild→compile ESM bundles (Bun 1.3.11)**: Bun build flags for compiled CLI (\`script/build.ts\`): (1) Do NOT enable \`bytecode: true\` with esbuild→\`Bun.build({ compile })\` pipeline. Still broken on Bun 1.3.13 — crashes \`TypeError: Expected CommonJS module to have a function wrapper\` at entry.instantiate (esbuild emits ESM; bytecode loader mis-caches as CJS). Exit 0, no output. Upstream: oven-sh/bun#21097, #23490. (2) Pass \`autoloadDotenv: false\` and \`autoloadBunfig: false\` — otherwise user's \`.env\`/\`bunfig.toml\` silently injects into \`process.env\` (e.g. Next.js \`.env.local\` could override stored OAuth token). Shell env vars still work; suggest direnv for dir-scoped vars. + +* **MastraClient has no dispose API — use AbortController for cleanup**: MastraClient has no \`close()\`/\`dispose()\` API — cleanup via \`ClientOptions.abortSignal\` (constructor) or per-prompt \`signal\`. Without explicit abort, Bun's fetch dispatcher keep-alive sockets hold the event loop alive past natural exit. Pattern in \`src/lib/init/wizard-runner.ts\`: create \`AbortController\` per \`runWizard\`, pass \`abortSignal: controller.signal\` to \`new MastraClient(...)\`, abort via \`using \_ = { \[Symbol.dispose]: () => controller.abort() }\`. Custom \`fetch\` wrapper must preserve \`init.signal\` via spread. Tests capture \`ClientOptions\` via \`spyOn(MastraClient.prototype, 'getWorkflow').mockImplementation(function() { capturedOpts.push(this.options); ... })\`. - -* **dist/bin.cjs runtime Node version check must match engines.node**: Node 20 dropped; \`engines.node >=22.12\` matches Astro 6 floor. CI \`Build npm Package\` matrix \`\["22","24"]\`. Docs build jobs pin \`actions/setup-node@v6\` with \`node-version: "24"\` after \`setup-bun\` for astro's node shebang. The npm package's \`dist/bin.cjs\` (from \`script/bundle.ts\`) contains an inline Node guard that MUST match \`engines.node\`. Simple \`parseInt(process.versions.node) < 22\` misses 22.0.0–22.11.x — use explicit major+minor: \`let v=process.versions.node.split('.').map(Number);if(v\[0]<22||(v\[0]===22&\&v\[1]<12)){...}\`. When bumping, update BIN\_WRAPPER string AND error message in lockstep. Without \`engine-strict=true\`, npm only warns — the runtime guard is real enforcement. + +* **Multi-region fan-out: distinguish all-403 from empty orgs with hasSuccessfulRegion flag**: In \`listOrganizationsUncached\` (\`src/lib/api/organizations.ts\`), \`Promise.allSettled\` collects multi-region results. Don't use \`flatResults.length === 0\` to detect all-regions-failed — a region returning 200 OK with zero orgs pushes nothing into \`flatResults\`. Track a \`hasSuccessfulRegion\` boolean on any \`"fulfilled"\` settlement. Only re-throw 403 \`ApiError\` when \`!hasSuccessfulRegion && lastScopeError\`. - -* **Making clearAuth() async breaks model-based tests — use non-async Promise\ return instead**: Making \`clearAuth()\` \`async\` breaks fast-check model-based tests — real async yields during \`asyncModelRun\` cause \`createIsolatedDbContext\` cleanup to interleave. Keep non-async; return \`clearResponseCache().catch(...)\` directly. Model-based tests also need explicit timeouts (e.g., \`30\_000\`) — Bun's default 5s causes false failures during shrinking. +### Pattern - -* **script/generate-api-schema.ts regex is brittle against SDK bundler output changes**: \`script/generate-api-schema.ts\` parses \`node\_modules/@sentry/api/dist/index.js\` with a regex (\`/var (\w+) = \\(options\S\*\\) => \\(options\S\*client \\?\\? client\\)\\.(\w+)\\(/g\`) to map SDK function names to URL+method pairs, producing \`src/generated/api-schema.json\`. If the SDK changes its generator's bundle format (e.g., switches to \`const\`, arrow vs function, different client-selection pattern), schema generation silently produces empty \`fn\` fields. When bumping \`@sentry/api\`, verify \`sentry schema\` output still populates function names. \`src/generated/api-schema.json\` is gitignored — regenerates on every dev/build/typecheck via \`bun run generate:schema\`. + +* **CLI-1D3 Windows download visibility race: poll statSync with exponential backoff**: Windows upgrade download visibility race (CLI-1D3): \`waitForBinaryVisible\` in \`src/lib/upgrade.ts\` polls via \`statSync\` with exponential backoff. Budget: 6 attempts, 5 sleeps (100+200+400+800+1600 = 3.1s). Loop breaks BEFORE final sleep, so \`VERIFY\_MAX\_ATTEMPTS=N\` yields N-1 sleeps — off-by-one trap. Covers Windows + Bun 1.3.9 race where \`Bun.file().writer().end()\` returns before OS surfaces file by path, causing opaque \`Executable not found in $PATH\` from \`Bun.spawn\`. Safety net \`isEnoentSpawnError()\` in \`src/commands/cli/upgrade.ts\` detects both \`code === 'ENOENT'\` and Bun's path-string error, translates to \`UpgradeError('execution\_failed')\`. - -* **Sentry /auth/ endpoint returns 400 (not 401) for Bearer tokens pre-fix**: Sentry \`/auth/\` endpoint historically returned \*\*400 with empty body\*\* for valid Bearer tokens — \`AuthIndexEndpoint\` excluded \`UserAuthTokenAuthentication\`. Fixed server-side by getsentry/sentry#112853 (release 26.4.1, Apr 22 2026), now fully rolled out to SaaS. Fix adds \`UserAuthTokenAuthentication\` only — \`OrgAuthTokenAuthentication\` was NOT added, and \`UserAuthTokenAuthentication.accepts\_auth\` rejects \`sntrys\_\` prefix. So \`sntrys\_\` org-auth-tokens always fail on \`/auth/\` (and whoami is semantically meaningless for org tokens — no user). CLI short-circuits \`sntrys\_\` via \`classifySentryToken\` before the call, throwing \`ResolutionError\` with \`sentry auth status\` hint. The 400-translation path in \`translateWhoamiApiError\` was dropped post-rollout (PR #841) as vestigial — anomalous 400s now bubble as \`ApiError\` and are diagnosable via the \`api\_response\_headers\` Sentry context. \`sentry auth status\` verifies via \`listOrganizationsUncached()\` so it works with org tokens. + +* **Cross-compile sentry-cli with patched Bun: drop compile.target to use selfExePath**: Cross-compile sentry-cli with patched Bun: \`Bun.build({compile})\` downloads stock Bun from npm when \`compile.target\` is set. Workaround in \`script/build.ts\`: omit \`target\` entirely so Bun hits \`isDefault()\` branch → uses \`selfExePath()\` = the running Bun as embed runtime. Only works when host OS/arch matches desired output. Escape hatch: place file at \`$CWD/bun-\-\-v\\` (e.g. \`bun-darwin-arm64-v1.3.13\`) picked up via \`bun.FD.cwd().existsAt(version\_str)\` in \`src/compile\_target.zig:exePath\`. Build also requires \`SENTRY\_CLIENT\_ID\` env var. - -* **Source Map v3 spec allows null entries in sources array**: The Source Map v3 spec allows \`null\` entries in the \`sources\` array, and bundlers like esbuild actually produce them. Any code iterating over \`sources\` and calling string methods (e.g., \`.replaceAll()\`) must guard against null: \`map.sources.map((s) => typeof s === "string" ? s.replaceAll("\\\\", "/") : s)\`. Without the guard, \`null.replaceAll()\` throws \`TypeError\`. This applies to \`src/lib/sourcemap/debug-id.ts\` and any future sourcemap manipulation code. + +* **Dedupe resolved entity IDs in batch operations before API call**: Batch issue merge (src/commands/issue/merge.ts): (1) Dedupe by resolved numeric ID after \`Promise.all(args.map(resolveIssue))\`, not raw input (users pass same entity as \`CLI-K9\`, \`my-org/CLI-K9\`, \`123\`). Throw ValidationError if \`new Set(ids).size < 2\`. (2) Reject undefined orgs in cross-org check — bare numeric IDs without DSN/config resolve with \`org: undefined\`; filtering them out lets mixed-org merges slip through. (3) Pass \`--into\` through \`resolveIssue()\` for alias/org-qualified parity; compare by numeric \`id\`, not \`shortId\`. (4) Sentry bulk merge API picks canonical parent by event count — \`--into\` is preference only; warn when API's \`parent\` differs. Empty results return 204. - -* **Starlight 0.33+ route data moved from Astro.props to Astro.locals.starlightRoute**: Starlight 0.33+ / Astro 6 docs migration: (1) Route data moved from \`Astro.props\` to \`Astro.locals.starlightRoute\` — old \`Astro.props.sidebar\` is \`undefined\`. Field rename: \`slug\` → \`id\`. Import types via \`@astrojs/starlight/route-data\` (typed on \`Astro.locals\` via \`locals.d.ts\`). Built-in children (SiteTitle, Search, SocialIcons) take no props. \`starlight.social\` is array-form. Content collections must migrate to Content Layer API: rename \`src/content/config.ts\` → \`src/content.config.ts\`, use \`docsLoader()\` + \`docsSchema()\`. Landing-page detection: \`id === ""\` (Starlight's \`normalizeIndexSlug\` maps \`"index"\` → \`""\`). + +* **findProjectsByPattern as fuzzy fallback for exact slug misses**: When \`findProjectsBySlug\` returns empty (no exact match), use \`findProjectsByPattern\` as a fallback to suggest similar projects. \`findProjectsByPattern\` does bidirectional word-boundary matching (\`matchesWordBoundary\`) against all projects in all orgs — the same logic used for directory name inference. In the \`project-search\` handler, call it after the exact miss, format matches as \`\/\\` suggestions in the \`ResolutionError\`. This avoids a dead-end error for typos like 'patagonai' when 'patagon-ai' exists. Note: \`findProjectsByPattern\` makes additional API calls (lists all projects per org), so only call it on the failure path. -### Pattern + +* **Grouped widget --limit auto-default via applyGroupLimitAutoDefault helper**: Dashboard widget flag normalization: (1) Dataset aliases (errors→error-events) normalize ONCE at top of \`func()\` via \`normalizeDataset()\` in \`src/commands/dashboard/resolve.ts\`. In \`edit.ts\`, pass \`normalizedFlags\` to \`buildReplacement\` — \`validateAggregateNames\` reads \`flags.dataset\` and rejects valid aggregates like \`failure\_rate\` if it sees raw alias. (2) Grouped widgets need \`limit\` (API rejects). \`applyGroupLimitAutoDefault\` defaults to \`DEFAULT\_GROUP\_BY\_LIMIT=5\` only when user passed \`--group-by\` without \`--limit\`; skip for auto-defaulted columns like \`\["issue"]\`. (3) Tests asserting \`--limit\` >10 survives into PUT body must use \`display: "line"\` — \`prepareWidgetQueries\` clamps bar/table to max=10. + + +* **Hidden --org/--project compat flags via mergeGlobalFlags**: Hidden global \`--org\`/\`--project\` flags accept old \`sentry-cli\` syntax. Defined in \`GLOBAL\_FLAGS\` (global-flags.ts) so argv-hoist relocates them. \`mergeGlobalFlags()\` in command.ts injects hidden flag shapes (skip if command owns the flag — e.g. \`release create --project -p\`) and returns \`stripKeys\` set used by \`cleanRawFlags\`. \`applyOrgProjectFlags()\` writes values to \`SENTRY\_ORG\`/\`SENTRY\_PROJECT\` via \`getEnv()\` before auth guard, overwriting existing env vars (explicit CLI > env var). Resolution chain in resolve-target.ts picks them up at priority #2. No short aliases (\`-p\` conflicts). The helper extraction was needed to keep \`buildCommand\` under Biome's cognitive complexity limit of 15. - -* **Bun global installs use .bun path segment for detection**: Bun global installs place scripts under \`~/.bun/install/global/node\_modules/\`. In \`detectPackageManagerFromPath()\`, check \`segments.includes('.bun')\` before npm fallback. Order: \`.pnpm\` → pnpm, \`.bun\` → bun, other \`node\_modules\` → npm. Yarn classic shares npm layout so falls through — acceptable because path detection is \*\*fallback\*\* after subprocess calls (which identify yarn correctly). Path detection must NOT override stored DB info, only serve as fallback when subprocess fails (e.g., Windows \`.cmd\` ENOENT). + +* **Preserve ApiError type so classifySilenced can silence 4xx errors**: Preserve ApiError type for classifySilenced: \`classifySilenced\` (src/lib/error-reporting.ts) only silences \`ApiError\` with status 401-499 — wrapping in generic \`CliError\` loses \`status\` and causes 403s to be captured. Re-throw via \`new ApiError(msg, error.status, error.detail, error.endpoint)\` with terse message (\`ApiError.format()\` appends detail/endpoint). \`ValidationError\` without \`field\` collapses unfielded errors into one fingerprint; always pass \`field\`. Fingerprint rule changes don't retroactively re-fingerprint — manually merge new groups into canonical old parents. \`ApiError\` rule keys by \`api\_status + command\`. - -* **Evict-then-read pattern: return cacheEvicted flag from helpers that clear cache on 404**: When a helper function transparently evicts a stale cache entry on 404 and falls back to an unscoped call, callers holding the now-stale cached value will let it win \`??\` chains. Fix: helper must return \`{ result, cacheEvicted }\` so callers compute \`effectiveCachedValue = cacheEvicted ? null : cachedValue\` before the \`??\` fallback, and re-cache the freshly-derived value. Applied in \`fetchIssueByNumericId\` in \`src/commands/issue/utils.ts\` — both \`resolveNumericIssue\` and \`resolveShareIssue\` consume the flag. A local cached variable outliving its DB entry is the common shape of this bug; always audit post-eviction read paths. + +* **Race-free delayed-write test: poll for precondition before mutating**: PTY/stdin test gotchas: (1) Race-free delayed-write test — when testing retry/recovery that mutates a file mid-loop, don't use \`Bun.sleep(N)\` + \`Bun.write(good)\`; slow CI can let good write land BEFORE production's bad write. Pattern: delayed writer polls until bad state exists, THEN overwrites. Add \`expect(onDisk).toEqual(good)\` after. Example: \`test/lib/upgrade.test.ts\`. (2) PTY reproduction of \`curl | bash\` requires Python \`pty.fork()\` + \`os.dup2\` to set fd 0 to a pipe before \`execvp('bash', '-c', 'exec bin \ -* **Non-essential DB cache writes should be guarded with try-catch**: Non-essential DB cache writes (e.g., \`setUserInfo()\`, \`setInstallInfo()\`) must be wrapped in try-catch so a broken/read-only DB doesn't crash a command whose primary operation succeeded. Pattern: \`try { setInstallInfo(...) } catch { log.debug(...) }\`. In login.ts, \`getCurrentUser()\` failure after token save must not block auth — log warning, continue. In upgrade.ts, \`setInstallInfo\` after legacy detection is guarded same way. Exception: \`getUserRegions()\` failure should \`clearAuth()\` and fail hard (indicates invalid token). This is enforced by BugBot reviews — any \`setInstallInfo\`/\`setUserInfo\` call outside setup.ts's \`bestEffort()\` wrapper needs its own try-catch. + +* **Sentry SDK tree-shaking patches must be regenerated via bun patch workflow**: Sentry SDK tree-shaking via bun patch: \`patchedDependencies\` in \`package.json\` strips unused exports from \`@sentry/core\` and \`@sentry/node-core\`. Non-light root of \`@sentry/node-core\` pulls uninstalled \`@opentelemetry/instrumentation\` — \*\*always import from \`@sentry/node-core/light\`\*\* (subpaths: \`.\`, \`./light\`, \`./light/otlp\`, \`./init\`, \`./loader\`, \`./import\`). No supported import for \`HttpsProxyAgent\`. Bumping SDK: remove old patches, \`rm -rf ~/.bun/install/cache/@sentry\`, \`bun install\`, \`bun patch @sentry/core\`, edit, \`bun patch --commit\`; repeat for node-core. Preserved: \`\_INTERNAL\_safeUnref\`, \`\_INTERNAL\_safeDateNow\`, \`nodeRuntimeMetricsIntegration\`. Before stripping any core export, grep \`node-core/build/{cjs,esm}/light/sdk.js\` for runtime usage (e.g. \`spanStreamingIntegration\` when \`traceLifecycle === 'stream'\`). Remove \`.bun-tag-\*\` hunks from generated patches. Manual \`git diff\` patches fail. - -* **Sentry CLI command docs are auto-generated from Stricli route tree with CI freshness check**: Sentry CLI command docs are auto-generated from Stricli route tree: Docs in \`docs/src/content/docs/commands/\*.md\` and skill files in \`plugins/sentry-cli/skills/sentry-cli/references/\*.md\` are generated via \`bun run generate:docs\`. Content between \`\\` markers is regenerated; hand-written examples go in \`docs/src/fragments/commands/\`. CI checks \`check:command-docs\` and \`check:skill\` fail if stale. Run generators after changing command parameters/flags/docs. + +* **Shared pagination infrastructure: buildPaginationContextKey and parseCursorFlag**: Bidirectional pagination via cursor stack in \`src/lib/db/pagination.ts\`. \`resolveCursor(flag, key, contextKey)\` maps keywords (next/prev/first/last) to \`{cursor, direction}\`. \`advancePaginationState\` manages stack — back-then-forward truncates stale entries. \`hasPreviousPage\` checks \`page\_index > 0\`. \`paginationHint()\` builds nav strings. All list commands use this. Critical: \`resolveCursor()\` must be called inside \`org-all\` override closures, not before \`dispatchOrgScopedList\`. - -* **Stricli buildCommand output config injects json flag into func params**: Stricli command gotchas: (1) In \`func()\` handlers use \`this.stdout\`/\`this.stderr\` directly — NOT \`this.process.stdout\`. \`SentryContext\` has \`process\` and \`stdout\`/\`stderr\` as separate top-level properties; test mocks omit full \`process\` so \`this.process.stdout\` throws \`TypeError\` at runtime (TS doesn't catch). (2) \`output: { json: true, human: formatFn }\` auto-injects \`--json\`/\`--fields\` flags — type flags explicitly (\`flags: { json?: boolean }\`). Commands with interactive side effects (prompts, QR codes) should check \`flags.json\` and skip. (3) Route maps with \`defaultCommand\` blend the default command's flags into subcommand completions — completion tests must track \`hasDefaultCommand\` and skip strict subcommand-matching. + +* **Telemetry instrumentation pattern: withTracingSpan + captureException for handled errors**: For graceful-fallback operations, use \`withTracingSpan\` from \`src/lib/telemetry.ts\` for child spans and \`captureException\` from \`@sentry/bun\` (named import — Biome forbids namespace imports) with \`level: 'warning'\` for non-fatal errors. \`withTracingSpan\` uses \`onlyIfParent: true\` — no-op without active transaction. User-visible fallbacks use \`log.warn()\` not \`log.debug()\`. Several commands bypass telemetry by importing \`buildCommand\` from \`@stricli/core\` directly instead of \`../../lib/command.js\` (trace/list, trace/view, log/view, api.ts, help.ts). - -* **Token-type classification via literal prefix match (classifySentryToken)**: \`src/lib/token-type.ts\` \`classifySentryToken(token)\` returns \`'org-auth-token'\` (\`sntrys\_\` prefix), \`'user-auth-token'\` (\`sntryu\_\` prefix), or \`'oauth-or-legacy'\` (OAuth, legacy PATs, JWT-shaped). Case-sensitive literal \`startsWith\` — matches Sentry backend's \`SENTRY\_ORG\_AUTH\_TOKEN\_PREFIX\`. Use to short-circuit commands where a token type is semantically invalid (e.g. \`whoami\` with org token) before a confusing API failure. \`getAuthToken()\` from \`db/auth\` returns the effective token (env + DB fallback). + +* **Testing Stricli command func() bodies via spyOn mocking**: Testing Stricli command func() bodies: (1) \`const func = await cmd.loader(); func.call(mockContext, flags, ...args)\` with mock \`stdout\`, \`stderr\`, \`cwd\`, \`setContext\`. \`loader()\` return type union causes \`.call()\` LSP false-positives that pass \`tsc --noEmit\`. (2) When API functions are renamed, update both spy target AND mock return shape. (3) \`normalizeSlug\` replaces \`\_\`→\`-\` but does NOT lowercase. (4) Bun \`mockFetch()\` replaces \`globalThis.fetch\` — use one unified mock dispatching by URL. (5) \`mock.module()\` pollutes module registry for ALL subsequent files — put in \`test/isolated/\` and run via \`test:isolated\`. (6) For \`Bun.spawn\`, use direct property assignment in \`beforeEach\`/\`afterEach\`. ### Preference - -* **PR workflow: address review comments, resolve threads, wait for CI**: PR workflow: (1) wait for CI; (2) check unresolved comments via \`gh api repos/.../pulls/N/comments\`; (3) fix in follow-up commits (NEVER amend a pushed commit without explicit user request + force push); (4) reply explaining fix; (5) resolve thread via \`gh api graphql resolveReviewThread\`; (6) push + re-check CI. BugBot/Seer/Warden/Cursor post new comments per-commit and often catch bugs in fix commits — re-check after each push. Dispatch a subagent review before declaring merge-ready; has caught real backwards-compat, error-path, and patch-hygiene bugs. Branches: \`fix/\*\` or \`feat/\*\`. Style: \`Array.from(set)\` over spreads; 'allowlist' not 'whitelist'; \`arr.at(-1)\` over index math. Reviewer questions may be inquiries — confirm intent before changing. + +* **Bot review triage: distinguish real bugs from SDK-mirroring false positives**: When Sentry Seer or Cursor Bugbot flags 'unusual' code that intentionally mirrors upstream SDK behavior (e.g., \`http\_proxy\` as last-resort fallback for HTTPS URLs — deliberate in \`@sentry/node-core\` \`applyNoProxyOption\`), decline with a written rationale referencing the SDK source rather than silently changing behavior. Removing the mirror creates a divergence where users get different proxy semantics from our transport vs. the SDK default. BYK's pattern: verify against \`node\_modules/@sentry/node-core/build/esm/transports/http.js\`, post a reply explaining the precedent, and resolve the thread. Real bugs (uppercase env var support, whitespace trimming, wildcard handling) get fixed; SDK-mirroring 'bugs' get explained and dismissed. diff --git a/src/lib/command.ts b/src/lib/command.ts index bc68e6e73..f9ef6e55f 100644 --- a/src/lib/command.ts +++ b/src/lib/command.ts @@ -38,6 +38,7 @@ import { } from "@stricli/core"; import type { Writer } from "../types/index.js"; import { getAuthConfig } from "./db/auth.js"; +import { getEnv } from "./env.js"; import { AuthError, CliError, OutputError } from "./errors.js"; import { warning } from "./formatters/colors.js"; import { parseFieldsList } from "./formatters/json.js"; @@ -58,6 +59,7 @@ import { GLOBAL_FLAGS } from "./global-flags.js"; import { LOG_LEVEL_NAMES, type LogLevelName, + logger, parseLogLevel, setLogLevel, } from "./logger.js"; @@ -230,6 +232,40 @@ export const FIELDS_FLAG = { optional: true as const, } as const; +// --------------------------------------------------------------------------- +// Hidden org/project compat flags (LLM error recovery) +// --------------------------------------------------------------------------- + +/** + * Hidden `--org` flag injected into every command by {@link buildCommand}. + * + * Backward-compatibility shim for the older `sentry-cli` that accepted + * `--org` on every command. LLMs trained on the old CLI generate this flag. + * The value is written to `SENTRY_ORG` so the existing resolution chain + * in `resolve-target.ts` picks it up at priority #2 (env vars). + */ +const ORG_FLAG = { + kind: "parsed" as const, + parse: String, + brief: "Organization slug", + optional: true as const, + hidden: true as const, +} as const; + +/** + * Hidden `--project` flag injected into every command by {@link buildCommand}. + * + * Same backward-compatibility shim as `--org`. Written to `SENTRY_PROJECT` + * before the command's `func` runs. + */ +const PROJECT_FLAG = { + kind: "parsed" as const, + parse: String, + brief: "Project slug", + optional: true as const, + hidden: true as const, +} as const; + /** The flag key for the injected --log-level flag (always stripped) */ const LOG_LEVEL_KEY = "log-level"; @@ -253,6 +289,37 @@ export function applyLoggingFlags( } } +const orgProjectLog = logger.withTag("compat-flags"); + +/** + * Write `--org` / `--project` flag values to environment variables. + * + * Called by the {@link buildCommand} wrapper before the command's `func()` + * runs. The values are written to `SENTRY_ORG` and `SENTRY_PROJECT` so + * the existing resolution chain in `resolve-target.ts` picks them up at + * priority #2 (env vars). Overwrites existing env vars because explicit + * CLI flags are highest-priority user intent. + * + * Skipped when the command defines its own `--org` / `--project` flag + * (e.g., `release create --project`) — those are passed through to `func()`. + */ +export function applyOrgProjectFlags( + org: string | undefined, + project: string | undefined, + commandOwnsOrg: boolean, + commandOwnsProject: boolean +): void { + const env = getEnv(); + if (org && !commandOwnsOrg) { + orgProjectLog.debug(`--org flag → SENTRY_ORG=${org}`); + env.SENTRY_ORG = org; + } + if (project && !commandOwnsProject) { + orgProjectLog.debug(`--project flag → SENTRY_PROJECT=${project}`); + env.SENTRY_PROJECT = project; + } +} + // --------------------------------------------------------------------------- // buildCommand — the single entry point for all Sentry CLI commands // --------------------------------------------------------------------------- @@ -334,6 +401,70 @@ function enrichDocsWithSchema( }; } +/** + * Global flag defaults keyed by flag name. + * Used by {@link mergeGlobalFlags} to inject flags when the command + * doesn't define its own. + */ +const GLOBAL_FLAG_DEFAULTS: Record = { + [LOG_LEVEL_KEY]: LOG_LEVEL_FLAG, + verbose: VERBOSE_FLAG, + org: ORG_FLAG, + project: PROJECT_FLAG, +}; + +/** Flags that are always stripped (command never sees them). */ +const ALWAYS_STRIP = new Set([LOG_LEVEL_KEY]); + +/** + * Merge global flags into a command's existing flags. + * + * For each global flag, if the command doesn't already define it, the + * default shape is injected and its key is added to the returned + * `stripKeys` set (so the wrapper strips it before calling `func`). + * + * @returns The merged flags, ownership booleans, and keys to strip. + */ +function mergeGlobalFlags( + existingFlags: Record, + // biome-ignore lint/suspicious/noExplicitAny: OutputConfig type is erased at the builder level + outputConfig?: OutputConfig +): { + mergedFlags: Record; + commandOwnsOrg: boolean; + commandOwnsProject: boolean; + stripKeys: Set; +} { + const commandOwnsJson = "json" in existingFlags; + const commandOwnsOrg = "org" in existingFlags; + const commandOwnsProject = "project" in existingFlags; + + const mergedFlags: Record = { ...existingFlags }; + const stripKeys = new Set(ALWAYS_STRIP); + + for (const [name, shape] of Object.entries(GLOBAL_FLAG_DEFAULTS)) { + if (!(name in existingFlags)) { + mergedFlags[name] = shape; + stripKeys.add(name); + } + } + + // Inject --json and --fields when output config is set + if (outputConfig) { + if (!commandOwnsJson) { + mergedFlags.json = JSON_FLAG; + } + mergedFlags.fields = buildFieldsFlag(outputConfig); + } + + return { + mergedFlags, + commandOwnsOrg, + commandOwnsProject, + stripKeys, + }; +} + export function buildCommand< const FLAGS extends BaseFlags = NonNullable, const ARGS extends BaseArgs = [], @@ -345,34 +476,15 @@ export function buildCommand< const outputConfig = builderArgs.output; const requiresAuth = builderArgs.auth !== false; - // Merge logging flags into the command's flag definitions. - // Quoted keys produce kebab-case CLI flags: "log-level" → --log-level + // Merge global flags into the command's flag definitions. const existingParams = (builderArgs.parameters ?? {}) as Record< string, unknown >; const existingFlags = (existingParams.flags ?? {}) as Record; - // If the command already defines --verbose (e.g. api command), don't override it. - const commandOwnsVerbose = "verbose" in existingFlags; - // If the command already defines --json (e.g. custom brief), don't override it. - const commandOwnsJson = "json" in existingFlags; - - const mergedFlags: Record = { - ...existingFlags, - [LOG_LEVEL_KEY]: LOG_LEVEL_FLAG, - }; - if (!commandOwnsVerbose) { - mergedFlags.verbose = VERBOSE_FLAG; - } - - // Inject --json and --fields when output config is set - if (outputConfig) { - if (!commandOwnsJson) { - mergedFlags.json = JSON_FLAG; - } - mergedFlags.fields = buildFieldsFlag(outputConfig); - } + const { mergedFlags, commandOwnsOrg, commandOwnsProject, stripKeys } = + mergeGlobalFlags(existingFlags, outputConfig); // Enrich fullDescription with JSON fields when schema is registered. // This makes field info visible in Stricli's --help output. @@ -434,19 +546,16 @@ export function buildCommand< /** * Strip injected flags from the raw Stricli-parsed flags object. - * --log-level is always stripped. --verbose is stripped only when we - * injected it (not when the command defines its own). --fields is - * pre-parsed from comma-string to string[] when output: { human: ... }. + * Global flags we injected (tracked in `stripKeys` from + * {@link mergeGlobalFlags}) are removed. --fields is pre-parsed from + * comma-string to string[] when output: { human: ... }. */ function cleanRawFlags( raw: Record ): Record { const clean: Record = {}; for (const [key, value] of Object.entries(raw)) { - if (key === LOG_LEVEL_KEY) { - continue; - } - if (key === "verbose" && !commandOwnsVerbose) { + if (stripKeys.has(key)) { continue; } clean[key] = value; @@ -549,6 +658,15 @@ export function buildCommand< flags.verbose as boolean ); + // Map --org / --project compat flags to env vars before anything + // else reads them (auth guard, org/project resolution, etc.) + applyOrgProjectFlags( + flags.org as string | undefined, + flags.project as string | undefined, + commandOwnsOrg, + commandOwnsProject + ); + const cleanFlags = cleanRawFlags(flags as Record); setFlagContext(cleanFlags); if (args.length > 0) { diff --git a/src/lib/global-flags.ts b/src/lib/global-flags.ts index 179c5af19..4fed56899 100644 --- a/src/lib/global-flags.ts +++ b/src/lib/global-flags.ts @@ -40,4 +40,10 @@ export const GLOBAL_FLAGS: readonly GlobalFlagDef[] = [ { name: "log-level", short: null, kind: "value" }, { name: "json", short: null, kind: "boolean" }, { name: "fields", short: null, kind: "value" }, + // Hidden compat shims: LLMs trained on the older sentry-cli generate + // `--org` and `--project` flags. We silently accept them and map to + // SENTRY_ORG / SENTRY_PROJECT env vars so the resolution chain handles them. + // No short aliases: -p conflicts with release create's -p (--project). + { name: "org", short: null, kind: "value" }, + { name: "project", short: null, kind: "value" }, ]; diff --git a/test/lib/argv-hoist.property.test.ts b/test/lib/argv-hoist.property.test.ts index 8f3dac59c..0513bd5f6 100644 --- a/test/lib/argv-hoist.property.test.ts +++ b/test/lib/argv-hoist.property.test.ts @@ -21,6 +21,8 @@ const GLOBAL_FLAG_TOKENS = [ "-v", "--log-level", "--fields", + "--org", + "--project", ] as const; /** Tokens that should never be hoisted */ @@ -62,6 +64,8 @@ const HOISTABLE_SET = new Set([ "-v", "--log-level", "--fields", + "--org", + "--project", ]); function isHoistableToken(token: string): boolean { @@ -120,7 +124,12 @@ describe("property: hoistGlobalFlags", () => { test("hoisted tokens appear after all non-hoisted tokens", () => { /** Value-taking flags whose next token also gets hoisted */ - const VALUE_TAKING = new Set(["--log-level", "--fields"]); + const VALUE_TAKING = new Set([ + "--log-level", + "--fields", + "--org", + "--project", + ]); fcAssert( property(argvArb, (argv) => { diff --git a/test/lib/argv-hoist.test.ts b/test/lib/argv-hoist.test.ts index 46b0194c8..abe7f11b3 100644 --- a/test/lib/argv-hoist.test.ts +++ b/test/lib/argv-hoist.test.ts @@ -176,6 +176,58 @@ describe("hoistGlobalFlags", () => { ]); }); + // ------------------------------------------------------------------------- + // Value flag: --org (compat) + // ------------------------------------------------------------------------- + + test("hoists --org with separate value", () => { + expect(hoistGlobalFlags(["--org", "sentry", "issue", "list"])).toEqual([ + "issue", + "list", + "--org", + "sentry", + ]); + }); + + test("hoists --org=sentry as single token", () => { + expect(hoistGlobalFlags(["--org=sentry", "issue", "list"])).toEqual([ + "issue", + "list", + "--org=sentry", + ]); + }); + + // ------------------------------------------------------------------------- + // Value flag: --project (compat) + // ------------------------------------------------------------------------- + + test("hoists --project with separate value", () => { + expect(hoistGlobalFlags(["--project", "cli", "issue", "list"])).toEqual([ + "issue", + "list", + "--project", + "cli", + ]); + }); + + test("hoists --project=cli as single token", () => { + expect(hoistGlobalFlags(["--project=cli", "issue", "list"])).toEqual([ + "issue", + "list", + "--project=cli", + ]); + }); + + // ------------------------------------------------------------------------- + // Combined: --org + --project (compat) + // ------------------------------------------------------------------------- + + test("hoists --org and --project together", () => { + expect( + hoistGlobalFlags(["--org", "sentry", "--project", "cli", "issue", "list"]) + ).toEqual(["issue", "list", "--org", "sentry", "--project", "cli"]); + }); + // ------------------------------------------------------------------------- // Multiple flags // ------------------------------------------------------------------------- diff --git a/test/lib/command.test.ts b/test/lib/command.test.ts index 75833a800..41d663524 100644 --- a/test/lib/command.test.ts +++ b/test/lib/command.test.ts @@ -17,6 +17,7 @@ import { } from "@stricli/core"; import { applyLoggingFlags, + applyOrgProjectFlags, buildCommand, FIELDS_FLAG, JSON_FLAG, @@ -1440,3 +1441,270 @@ describe("buildCommand return-based output", () => { expect(jsonOutput).toEqual({ error: "not found" }); }); }); + +// --------------------------------------------------------------------------- +// applyOrgProjectFlags +// --------------------------------------------------------------------------- + +describe("applyOrgProjectFlags", () => { + let savedOrg: string | undefined; + let savedProject: string | undefined; + + beforeEach(() => { + savedOrg = process.env.SENTRY_ORG; + savedProject = process.env.SENTRY_PROJECT; + delete process.env.SENTRY_ORG; + delete process.env.SENTRY_PROJECT; + }); + + afterEach(() => { + if (savedOrg !== undefined) { + process.env.SENTRY_ORG = savedOrg; + } else { + delete process.env.SENTRY_ORG; + } + if (savedProject !== undefined) { + process.env.SENTRY_PROJECT = savedProject; + } else { + delete process.env.SENTRY_PROJECT; + } + }); + + test("sets both env vars when flags provided", () => { + applyOrgProjectFlags("my-org", "my-proj", false, false); + expect(process.env.SENTRY_ORG).toBe("my-org"); + expect(process.env.SENTRY_PROJECT).toBe("my-proj"); + }); + + test("skips when command owns the flag", () => { + applyOrgProjectFlags("my-org", "my-proj", true, true); + expect(process.env.SENTRY_ORG).toBeUndefined(); + expect(process.env.SENTRY_PROJECT).toBeUndefined(); + }); + + test("sets only org when project is undefined", () => { + applyOrgProjectFlags("my-org", undefined, false, false); + expect(process.env.SENTRY_ORG).toBe("my-org"); + expect(process.env.SENTRY_PROJECT).toBeUndefined(); + }); + + test("sets only project when org is undefined", () => { + applyOrgProjectFlags(undefined, "my-proj", false, false); + expect(process.env.SENTRY_ORG).toBeUndefined(); + expect(process.env.SENTRY_PROJECT).toBe("my-proj"); + }); + + test("overwrites existing env var", () => { + process.env.SENTRY_ORG = "old"; + applyOrgProjectFlags("new", undefined, false, false); + expect(process.env.SENTRY_ORG).toBe("new"); + }); + + test("does nothing when both flags are undefined", () => { + applyOrgProjectFlags(undefined, undefined, false, false); + expect(process.env.SENTRY_ORG).toBeUndefined(); + expect(process.env.SENTRY_PROJECT).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// buildCommand --org/--project compat flags (end-to-end) +// --------------------------------------------------------------------------- + +describe("buildCommand --org/--project compat flags", () => { + let savedOrg: string | undefined; + let savedProject: string | undefined; + + beforeEach(() => { + savedOrg = process.env.SENTRY_ORG; + savedProject = process.env.SENTRY_PROJECT; + delete process.env.SENTRY_ORG; + delete process.env.SENTRY_PROJECT; + }); + + afterEach(() => { + if (savedOrg !== undefined) { + process.env.SENTRY_ORG = savedOrg; + } else { + delete process.env.SENTRY_ORG; + } + if (savedProject !== undefined) { + process.env.SENTRY_PROJECT = savedProject; + } else { + delete process.env.SENTRY_PROJECT; + } + }); + + test("--org sets SENTRY_ORG env var", async () => { + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + async *func() { + // no-op + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--org", "my-org"], ctx as TestContext); + + expect(process.env.SENTRY_ORG).toBe("my-org"); + }); + + test("--project sets SENTRY_PROJECT env var", async () => { + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + async *func() { + // no-op + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--project", "my-project"], ctx as TestContext); + + expect(process.env.SENTRY_PROJECT).toBe("my-project"); + }); + + test("--org overwrites existing SENTRY_ORG", async () => { + process.env.SENTRY_ORG = "original-org"; + + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + async *func() { + // no-op + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--org", "new-org"], ctx as TestContext); + + expect(process.env.SENTRY_ORG).toBe("new-org"); + }); + + test("--org and --project are stripped from func flags", async () => { + let receivedFlags: Record | null = null; + + const command = buildCommand<{ limit: number }, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: { + flags: { + limit: { + kind: "parsed", + parse: numberParser, + brief: "Limit", + default: "10", + }, + }, + }, + // biome-ignore lint/correctness/useYield: test command + async *func(this: TestContext, flags: { limit: number }) { + receivedFlags = flags as unknown as Record; + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run( + app, + ["test", "--org", "sentry", "--project", "cli", "--limit", "50"], + ctx as TestContext + ); + + expect(receivedFlags).toBeDefined(); + expect(receivedFlags!.limit).toBe(50); + expect(receivedFlags!.org).toBeUndefined(); + expect(receivedFlags!.project).toBeUndefined(); + // Both env vars set + expect(process.env.SENTRY_ORG).toBe("sentry"); + expect(process.env.SENTRY_PROJECT).toBe("cli"); + }); + + test("command-owned --project is preserved and NOT written to env", async () => { + let receivedFlags: Record | null = null; + + // Simulates release create which has its own --project flag + const command = buildCommand<{ project?: string }, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: { + flags: { + project: { + kind: "parsed", + parse: String, + brief: "Project slug", + optional: true, + }, + }, + }, + // biome-ignore lint/correctness/useYield: test command + async *func(this: TestContext, flags: { project?: string }) { + receivedFlags = flags as unknown as Record; + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--project", "my-project"], ctx as TestContext); + + // Command's own --project is passed through (not stripped) + expect(receivedFlags).toBeDefined(); + expect(receivedFlags!.project).toBe("my-project"); + // Should NOT be written to env (command owns the flag) + expect(process.env.SENTRY_PROJECT).toBeUndefined(); + }); + + test("--org=value equals form works", async () => { + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + async *func() { + // no-op + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--org=my-org"], ctx as TestContext); + + expect(process.env.SENTRY_ORG).toBe("my-org"); + }); +}); From 8a0d250f8e95297a8a6bc7dfd39154d965f73347 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 28 Apr 2026 01:11:08 +0000 Subject: [PATCH 2/2] review: trim whitespace in compat flags + add resolution-chain integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from self-review pass: 1. Trim whitespace in applyOrgProjectFlags before deciding whether to set the env var. Previously `--org " "` would overwrite a real pre-existing SENTRY_ORG with whitespace, which resolveFromEnvVars then discarded via .trim() — silent data loss for LLM-generated commands containing stray whitespace. 2. Treat empty/whitespace-only values as 'not provided' so they fall through to the rest of the resolution chain rather than clobbering real env vars. 3. Add end-to-end integration test that verifies the full --org/--project → env var → resolveOrgAndProject pipeline. This is the contract the PR exists to deliver and was not previously tested. 4. Add symmetric --project overwrite test (PR description claimed both --org and --project overwrite env vars; only --org was tested). 5. Add explicit tests for whitespace-trimming and empty-string handling. --- src/lib/command.ts | 19 ++++++--- test/lib/command.test.ts | 84 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/src/lib/command.ts b/src/lib/command.ts index f9ef6e55f..fb4328972 100644 --- a/src/lib/command.ts +++ b/src/lib/command.ts @@ -300,6 +300,11 @@ const orgProjectLog = logger.withTag("compat-flags"); * priority #2 (env vars). Overwrites existing env vars because explicit * CLI flags are highest-priority user intent. * + * Empty and whitespace-only values are treated as "not provided" — they + * fall through to the rest of the resolution chain rather than overwriting + * a real pre-existing env var with garbage. This matters for LLM-generated + * commands that occasionally emit `--org ""` or stray whitespace. + * * Skipped when the command defines its own `--org` / `--project` flag * (e.g., `release create --project`) — those are passed through to `func()`. */ @@ -310,13 +315,15 @@ export function applyOrgProjectFlags( commandOwnsProject: boolean ): void { const env = getEnv(); - if (org && !commandOwnsOrg) { - orgProjectLog.debug(`--org flag → SENTRY_ORG=${org}`); - env.SENTRY_ORG = org; + const orgTrimmed = org?.trim(); + const projectTrimmed = project?.trim(); + if (orgTrimmed && !commandOwnsOrg) { + orgProjectLog.debug(`--org flag → SENTRY_ORG=${orgTrimmed}`); + env.SENTRY_ORG = orgTrimmed; } - if (project && !commandOwnsProject) { - orgProjectLog.debug(`--project flag → SENTRY_PROJECT=${project}`); - env.SENTRY_PROJECT = project; + if (projectTrimmed && !commandOwnsProject) { + orgProjectLog.debug(`--project flag → SENTRY_PROJECT=${projectTrimmed}`); + env.SENTRY_PROJECT = projectTrimmed; } } diff --git a/test/lib/command.test.ts b/test/lib/command.test.ts index 41d663524..4341fd34a 100644 --- a/test/lib/command.test.ts +++ b/test/lib/command.test.ts @@ -28,6 +28,7 @@ import { import { OutputError } from "../../src/lib/errors.js"; import { CommandOutput } from "../../src/lib/formatters/output.js"; import { LOG_LEVEL_NAMES, logger, setLogLevel } from "../../src/lib/logger.js"; +import { resolveOrgAndProject } from "../../src/lib/resolve-target.js"; import { buildRouteMap } from "../../src/lib/route-map.js"; /** Minimal context for test commands */ @@ -1505,6 +1506,26 @@ describe("applyOrgProjectFlags", () => { expect(process.env.SENTRY_ORG).toBeUndefined(); expect(process.env.SENTRY_PROJECT).toBeUndefined(); }); + + test("trims whitespace from flag values before writing", () => { + applyOrgProjectFlags(" my-org ", " my-proj ", false, false); + expect(process.env.SENTRY_ORG).toBe("my-org"); + expect(process.env.SENTRY_PROJECT).toBe("my-proj"); + }); + + test("treats empty string as not provided — preserves existing env var", () => { + process.env.SENTRY_ORG = "preserved-org"; + applyOrgProjectFlags("", undefined, false, false); + // Empty string must NOT clobber a real pre-existing env var + expect(process.env.SENTRY_ORG).toBe("preserved-org"); + }); + + test("treats whitespace-only as not provided — preserves existing env var", () => { + process.env.SENTRY_PROJECT = "preserved-proj"; + applyOrgProjectFlags(undefined, " ", false, false); + // Whitespace-only must NOT clobber a real pre-existing env var + expect(process.env.SENTRY_PROJECT).toBe("preserved-proj"); + }); }); // --------------------------------------------------------------------------- @@ -1707,4 +1728,67 @@ describe("buildCommand --org/--project compat flags", () => { expect(process.env.SENTRY_ORG).toBe("my-org"); }); + + test("--project overwrites existing SENTRY_PROJECT", async () => { + process.env.SENTRY_PROJECT = "original-proj"; + + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + async *func() { + // no-op + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run(app, ["test", "--project", "new-proj"], ctx as TestContext); + + expect(process.env.SENTRY_PROJECT).toBe("new-proj"); + }); + + test("--org/--project values flow into resolveOrgAndProject (end-to-end)", async () => { + // Integration test: verifies the full plumbing from CLI flag → env var + // → resolveFromEnvVars (priority #2 in the resolution chain). This is the + // contract this PR exists to deliver: LLM-generated `--org foo --project + // bar` should make org/project resolution succeed without an explicit + // positional arg. + let resolved: Awaited> = null; + + const command = buildCommand, [], TestContext>({ + auth: false, + docs: { brief: "Test" }, + parameters: {}, + // biome-ignore lint/correctness/useYield: test command — no output to yield + async *func(this: TestContext) { + resolved = await resolveOrgAndProject({ cwd: "/tmp" }); + }, + }); + + const routeMap = buildRouteMap({ + routes: { test: command }, + docs: { brief: "Test app" }, + }); + const app = buildApplication(routeMap, { name: "test" }); + const ctx = createTestContext(); + + await run( + app, + ["test", "--org", "flag-org", "--project", "flag-proj"], + ctx as TestContext + ); + + expect(resolved).not.toBeNull(); + expect(resolved?.org).toBe("flag-org"); + expect(resolved?.project).toBe("flag-proj"); + // detectedFrom proves the resolution went through the env-var path, + // not some other branch. + expect(resolved?.detectedFrom).toContain("env var"); + }); });