diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1a27dcf46..14486246b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -328,7 +328,7 @@ jobs: echo "${{ secrets.GITHUB_TOKEN }}" | oras login ghcr.io -u ${{ github.actor }} --password-stdin # Find previous nightly tag - TAGS=$(oras repo tags "${REPO}" 2>/dev/null | grep '^nightly-' | sort -V || echo "") + TAGS=$(oras repo tags "${REPO}" 2>/dev/null | grep '^nightly-[0-9]' | sort -V || echo "") PREV_TAG="" for tag in $TAGS; do # Current tag may not exist yet (publish-nightly hasn't run), @@ -408,6 +408,56 @@ jobs: echo "Generated ${GENERATED} patches" + - name: Validate patch sizes + if: env.HAS_PREV == 'true' + env: + GH_TOKEN: ${{ github.token }} + # Client rejects chains exceeding 60% of gzipped binary size + # (SIZE_THRESHOLD_RATIO in src/lib/delta-upgrade.ts). Use 50% here + # so single-step patches are caught with margin to spare. + MAX_RATIO: 50 + run: | + shopt -s nullglob + OVERSIZED="" + + for patch_file in patches/*.patch; do + name=$(basename "$patch_file" .patch) + gz_binary="new-binaries/${name}.gz" + [ -f "$gz_binary" ] || continue + + patch_size=$(stat --printf='%s' "$patch_file") + gz_size=$(stat --printf='%s' "$gz_binary") + ratio=$(awk "BEGIN { printf \"%.0f\", ($patch_size / $gz_size) * 100 }") + + if [ "$ratio" -gt "$MAX_RATIO" ]; then + echo "::error::Patch ${name}.patch is ${ratio}% of gzipped binary (limit: ${MAX_RATIO}%)" + OVERSIZED="$(printf '%s\n- `%s.patch`: %s%% of gzipped binary (%s / %s bytes)' "$OVERSIZED" "$name" "$ratio" "$patch_size" "$gz_size")" + rm "$patch_file" + fi + done + + if [ -n "$OVERSIZED" ]; then + TITLE="Delta patch generation produced oversized patches" + BODY="$(cat </dev/null | grep '^nightly-' | sort -V || echo "") + TAGS=$(oras repo tags "${REPO}" 2>/dev/null | grep '^nightly-[0-9]' | sort -V || echo "") PREV_TAG="" for tag in $TAGS; do if [ "$tag" = "nightly-${VERSION}" ]; then break; fi diff --git a/.github/workflows/cleanup-nightlies.yml b/.github/workflows/cleanup-nightlies.yml index 5691a5293..013bda68c 100644 --- a/.github/workflows/cleanup-nightlies.yml +++ b/.github/workflows/cleanup-nightlies.yml @@ -34,7 +34,7 @@ jobs: REPO="ghcr.io/getsentry/cli" # List all nightly-* tags sorted by version, oldest first - NIGHTLY_TAGS=$(oras repo tags "${REPO}" 2>/dev/null | grep '^nightly-' | sort -V || echo "") + NIGHTLY_TAGS=$(oras repo tags "${REPO}" 2>/dev/null | grep '^nightly-[0-9]' | sort -V || echo "") if [ -z "$NIGHTLY_TAGS" ]; then NIGHTLY_COUNT=0 else diff --git a/AGENTS.md b/AGENTS.md index 141c3abc3..4d64f1eac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -984,95 +984,93 @@ mock.module("./some-module", () => ({ ### Architecture - -* **API client wraps all errors as CliError subclasses — no raw exceptions escape**: The API client (src/lib/api-client.ts) wraps ALL errors as CliError subclasses (ApiError or AuthError) — no raw exceptions escape. Commands don't need try-catch for error display; the central handler in app.ts formats CliError cleanly. Only add try-catch when a command needs to handle errors specially (e.g., login continuing despite user-info fetch failure). + +* **api-client.ts split into domain modules under src/lib/api/**: The original monolithic \`src/lib/api-client.ts\` (1,977 lines) was split into 12 focused domain modules under \`src/lib/api/\`: infrastructure.ts (shared helpers, types, raw requests), organizations.ts, projects.ts, teams.ts, repositories.ts, issues.ts, events.ts, traces.ts, logs.ts, seer.ts, trials.ts, users.ts. The original \`api-client.ts\` was converted to a ~100-line barrel re-export file preserving all existing import paths. The \`biome.jsonc\` override for \`noBarrelFile\` already includes \`api-client.ts\`. When adding new API functions, place them in the appropriate domain module under \`src/lib/api/\`, not in the barrel file. - -* **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\` wrapper 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\`. Fast-path achieves ~60ms dev / ~140ms CI, 200ms e2e budget. + +* **Bun compiled binary sourcemap options and size impact**: Binary build (\`script/build.ts\`) is a two-step process: (1) esbuild bundles TS → single minified JS + external \`.map\` (switched from \`Bun.build()\` due to identifier collision bug oven-sh/bun#14585). esbuild config: \`platform: "node"\`, \`format: "esm"\`, \`external: \["bun:\*"]\`, \`target: "esnext"\`. \`mkdirSync("dist-bin")\` required since esbuild doesn't auto-create output dirs. (2) \`injectDebugId()\` injects debug IDs, \`uploadSourcemaps()\` uploads to Sentry with URL prefix \`~/$bunfs/root/\`. (3) \`Bun.build()\` with \`compile: true\` produces native binaries — \`minify: false\` to avoid double-minification. Sourcemap never embedded; uploaded then deleted. Bun binaries use \`/$bunfs/root/bin.js\` as virtual path. \`binpunch\` hole-punches unused ICU data. esbuild produces 27k+ names in sourcemaps vs Bun's empty names array — critical for Sentry stack trace resolution. - -* **Craft artifact discovery uses commit SHA and workflow name, not tags**: Craft artifact discovery and release pipeline: Craft (\`getsentry/craft@v2\`) finds artifacts by commit SHA + workflow \*\*name\*\* (not tags). Jobs in \`ci.yml\` (named \`Build\`) auto-discovered; artifacts matching \`/^sentry-.\*$/\` picked up without \`.craft.yml\` changes. GitHub Releases are immutable post-publish — all assets (including delta patches) must be workflow artifacts before craft runs. Delta patches use TRDIFF10 (zig-bsdiff + zstd). \`generate-patches\` job runs on \`main\`/\`release/\*\*\` with \`continue-on-error\`. Nightly: old binary from GHCR via ORAS. Stable: \`gh release download\`. Binary download steps use \`sentry-\*-\*\` pattern to avoid matching \`sentry-patches\`. Bash glob loops need \`shopt -s nullglob\`. + +* **CLI telemetry DSN is public write-only — safe to embed in install script**: The CLI's Sentry DSN (\`SENTRY\_CLI\_DSN\` in \`src/lib/constants.ts\`) is a public write-only ingest key already baked into every binary. Safe to hardcode in install scripts. Opt-out: \`SENTRY\_CLI\_NO\_TELEMETRY=1\`. - -* **Sentry API: events require org+project, issues have legacy global endpoint**: Sentry API scoping: Events require org+project in URL path (\`/projects/{org}/{project}/events/{id}/\`). Issues use legacy global endpoint (\`/api/0/issues/{id}/\`) without org context. Traces need only org (\`/organizations/{org}/trace/{traceId}/\`). Two-step lookup for events: fetch issue → extract org/project from response → fetch event. Cross-project event search possible via Discover endpoint \`/organizations/{org}/events/\` with \`query=id:{eventId}\`. + +* **cli.sentry.dev is served from gh-pages branch via GitHub Pages**: \`cli.sentry.dev\` is served from gh-pages branch via GitHub Pages. Craft's gh-pages target runs \`git rm -r -f .\` before extracting docs — persist extra files via \`postReleaseCommand\` in \`.craft.yml\`. Install script supports \`--channel nightly\`, downloading from the \`nightly\` release tag directly. version.json is only used by upgrade/version-check flow. - -* **Sentry CLI authenticated fetch architecture with response caching**: \`createAuthenticatedFetch()\` wraps fetch with auth headers, 30s timeout, retry (max 2), 401 token refresh, and span tracing. Response caching via \`http-cache-semantics\` (RFC 7234) stored at \`~/.sentry/cache/responses/\`. Fallback TTL tiers: immutable (24hr), stable (5min), volatile (60s), no-cache (0). Only GET 2xx cached. \`--fresh\` and \`SENTRY\_NO\_CACHE=1\` bypass cache. Cache cleared on login/logout. + +* **Nightly delta upgrade buildNightlyPatchGraph fetches ALL patch tags — O(N) HTTP calls**: Delta upgrade in \`src/lib/delta-upgrade.ts\` supports stable (GitHub Releases) and nightly (GHCR) channels. \`filterAndSortChainTags\` filters \`patch-\*\` tags by version range using \`Bun.semver.order()\`. GHCR uses \`fetchWithRetry\` (10s timeout + 1 retry; blobs 30s) with optional \`signal?: AbortSignal\` combined via \`AbortSignal.any()\`. \`isExternalAbort(error, signal)\` skips retries for external aborts — critical for background prefetch. Patches cached to \`~/.sentry/patch-cache/\` (file-based, 7-day TTL). \`validateChainStep\` returns a discriminated union \`ChainStepResult\` with typed failure reasons (\`version-mismatch\`, \`missing-layer\`, \`size-exceeded\`) for debug logging. CI tag filtering uses \`grep '^nightly-\[0-9]'\` to exclude non-versioned tags. - -* **Sentry CLI has two distribution channels with different runtimes**: Sentry CLI ships two ways: (1) Standalone binary via \`Bun.build()\` with \`compile: true\`. (2) npm package via esbuild producing CJS \`dist/bin.cjs\` for Node 22+, with Bun API polyfills from \`script/node-polyfills.ts\`. \`Bun.$\` has NO polyfill — use \`execSync\` instead. SDK is \`@sentry/node-core/light\` (not \`@sentry/bun\`). + +* **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. - -* **Sentry CLI resolve-target cascade has 5 priority levels with env var support**: Resolve-target cascade (src/lib/resolve-target.ts) 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. The \`resolveFromEnvVars()\` helper is injected into all four resolution functions. + +* **Numeric issue ID resolution returns org:undefined despite API success**: Numeric issue ID resolution in \`resolveNumericIssue()\`: (1) try DSN/env/config for org, (2) if found use \`getIssueInOrg(org, id)\` with region routing, (3) else fall back to unscoped \`getIssue(id)\`, (4) extract org from \`issue.permalink\` via \`parseSentryUrl\` as final fallback. \`parseSentryUrl\` handles path-based (\`/organizations/{org}/...\`) and subdomain-style URLs. \`matchSubdomainOrg()\` filters region subdomains by requiring slug length > 2. Self-hosted uses path-based only. -### Decision + +* **Seer trial prompt uses middleware layering in bin.ts error handling chain**: Error recovery middlewares in \`bin.ts\` are layered: \`main() → executeWithAutoAuth() → executeWithSeerTrialPrompt() → runCommand()\`. Seer trial prompts (for \`no\_budget\`/\`not\_enabled\`) caught by inner wrapper; auth errors bubble to outer. Auth retry goes through full chain. 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. - -* **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 at least 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. + +* **SQLite DB functions are synchronous — async signatures are historical artifacts**: All \`src/lib/db/\` functions do synchronous SQLite operations (both \`bun:sqlite\` and the \`node:sqlite\` polyfill's \`DatabaseSync\` are sync). Many functions still have \`async\` signatures — this is a historical artifact from PR #89 which migrated config storage from JSON files (using async \`Bun.file().text()\` / \`Bun.write()\`) to SQLite. The function signatures were preserved to minimize diff size and never cleaned up. These can safely be converted to synchronous. Exceptions that ARE legitimately async: \`clearAuth()\` (cache dir cleanup), \`getCachedDetection()\`/\`getCachedProjectRoot()\`/\`setCachedProjectRoot()\` (stat for mtime), \`refreshToken()\`/\`performTokenRefresh()\` (HTTP calls). - -* **Sentry CLI config dir should stay at ~/.sentry/, not move to XDG**: Config dir stays at \`~/.sentry/\` (not XDG). The readonly DB errors on macOS are from \`sudo brew install\` creating root-owned files. Fixes: (1) bestEffort() makes setup steps non-fatal, (2) tryRepairReadonly() detects root-owned files and prints \`sudo chown\` instructions, (3) \`sentry cli fix\` handles ownership repair. Ownership must be checked BEFORE permissions — root-owned files cause chmod to EPERM. +### Decision -### Gotcha + +* **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. - -* **Biome noExcessiveCognitiveComplexity max 15 requires extracting helpers from command handlers**: Biome lint rules that frequently trip developers in this codebase: (1) \`noExcessiveCognitiveComplexity\` max 15 — extract helpers from Stricli \`func()\` handlers to stay under limit; improves testability too. (2) \`useBlockStatements\` — always use braces, no braceless \`if/return/break\`. Nested ternaries banned (\`noNestedTernary\`) — use if/else or sequential assignments. (3) \`useAtIndex\` — use \`arr.at(-1)\` not \`arr\[arr.length - 1]\`. (4) \`noStaticOnlyClass\` — use branded instances instead of static-only classes. + +* **whoami should be separate from auth status command**: The \`sentry auth whoami\` command should be a dedicated command separate from \`sentry auth status\`. They serve different purposes: \`status\` shows everything about auth state (token, expiry, defaults, org verification), while \`whoami\` just shows user identity (name, email, username, ID) by fetching live from \`/auth/\` endpoint. \`sentry whoami\` should be a top-level alias (like \`sentry issues\` → \`sentry issue list\`). \`whoami\` should support \`--json\` for machine consumption and be lightweight — no credential verification, no defaults listing. - -* **brew is not in VALID\_METHODS but Homebrew formula passes --method brew**: Homebrew install: \`isHomebrewInstall()\` detects via Cellar realpath (checked before stored install info). Upgrade command tells users \`brew upgrade getsentry/tools/sentry\`. Formula runs \`sentry cli setup --method brew --no-modify-path\` as post\_install. Version pinning throws 'unsupported\_operation'. Uses .gz artifacts. Tap at getsentry/tools. +### Gotcha - -* **Bun mock.module() leaks globally across test files in same process**: Bun's mock.module() replaces modules globally and leaks across test files in the same process. Solution: tests using mock.module() must run in a separate \`bun test\` invocation. In package.json, use \`bun run test:unit && bun run test:isolated\` instead of \`bun test\`. The \`test/isolated/\` directory exists for these tests. This was the root cause of ~100 test failures (getsentry/cli#258). + +* **@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 the Response's Link header for pagination. \`per\_page\` is not in SDK types; cast query to pass it at runtime. - -* **CI paths-filter for skill regeneration misses package.json version changes**: The \`check-generated\` job in \`.github/workflows/ci.yml\` gates on \`dorny/paths-filter\` with a \`skill\` filter matching \`src/\*\*\`, \`docs/\*\*\`, and generator scripts. But \`generate-skill.ts\` reads the version from \`package.json\` and embeds it in skill file frontmatter. Version bump commits (from Craft) only touch \`package.json\` and \`plugin.json\` — neither matches the filter, so skill regeneration is skipped and files go stale. Fix: add \`package.json\` to the \`skill\` paths-filter. The \`release/\*\*\` branch fallback doesn't help because the version bump lands on \`main\`. GitHub App token (sentry-release-bot) correctly triggers CI — the skip is purely from paths-filter mismatch. + +* **Bun binary build requires SENTRY\_CLIENT\_ID env var**: The build script (\`script/bundle.ts\`) requires \`SENTRY\_CLIENT\_ID\` environment variable and exits with code 1 if missing. When building locally, use \`bun run --env-file=.env.local build\` or set the env var explicitly. The binary build (\`bun run build\`) also needs it. Without it you get: \`Error: SENTRY\_CLIENT\_ID environment variable is required.\` - -* **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. Fix: keep non-async, return \`clearResponseCache().catch(...)\` directly. Also: model-based tests need explicit timeouts (e.g., \`30\_000\`) — Bun's default 5s causes false failures during shrinking. + +* **Bun.build() minifier has identifier collision bug — use esbuild for bundling step**: Bun's bundler has an unfixed identifier collision bug (oven-sh/bun#14585) where minified output produces name collisions across module scopes, causing runtime \`X is not a function\` errors. PR #17930 merged a workaround but the Bun team acknowledged the real bug in the renamer/hoisting code remains. Additionally, Bun's sourcemaps produce an empty \`names\` array, making Sentry stack traces useless. Fix: use esbuild for Step 1 (TS→JS bundling) with \`platform: "node"\`, \`format: "esm"\`, \`external: \["bun:\*"]\`, then feed the output to \`Bun.build({ compile: true })\` for Step 2. esbuild produces correct minification and rich sourcemaps (27k+ names). Size difference is negligible (~1.7% smaller bundle). - -* **Multiregion mock must include all control silo API routes**: When changing which Sentry API endpoint a function uses, mock routes must be updated in BOTH \`test/mocks/routes.ts\` (single-region) AND \`test/mocks/multiregion.ts\` \`createControlSiloRoutes()\`. Missing the multiregion mock causes 404s in multi-region test scenarios. + +* **Dashboard tracemetrics dataset uses comma-separated aggregate format**: SDK v10+ custom metrics (, , ) emit envelope items. Dashboard widgets for these MUST use with aggregate format — e.g., . The parameter must match the SDK emission exactly: if no unit specified, for memory metrics, for uptime. only supports , , , , display types — no or . Widgets with always require . Sort expressions must reference aggregates present in . - -* **Sentry /users/me/ endpoint returns 403 for OAuth tokens — use /auth/ instead**: The Sentry \`/users/me/\` endpoint returns 403 for OAuth tokens. Use \`/auth/\` instead — it works with ALL token types and lives on the control silo. In the CLI, \`getControlSiloUrl()\` handles routing correctly. \`SentryUserSchema\` (with \`.passthrough()\`) handles the \`/auth/\` response since it only requires \`id\`. + +* **GitHub immutable releases prevent rolling nightly tag pattern**: getsentry/cli has immutable GitHub releases — assets can't be modified and tags can NEVER be reused. Nightly builds publish to GHCR with versioned tags like \`nightly-0.14.0-dev.1772661724\`, not GitHub Releases or npm. \`fetchManifest()\` throws \`UpgradeError("network\_error")\` for both network failures and non-200 — callers must check message for HTTP 404/403. Craft with no \`preReleaseCommand\` silently skips \`bump-version.sh\` if only target is \`github\`. - -* **Sentry chunk upload API returns camelCase not snake\_case**: The Sentry chunk upload options endpoint (\`/api/0/organizations/{org}/chunk-upload/\`) returns camelCase keys (\`chunkSize\`, \`chunksPerRequest\`, \`maxRequestSize\`, \`hashAlgorithm\`), NOT snake\_case. Zod schemas for these responses should use camelCase field names. This is an exception to the typical Sentry API convention of snake\_case. Verified by direct API query. The \`AssembleResponse\` also uses camelCase (\`missingChunks\`). + +* **Install script: BSD sed and awk JSON parsing breaks OCI digest extraction**: The install script parses OCI manifests with awk (no jq). Key trap: BSD sed \`\n\` is literal, not newline. Fix: single awk pass tracking last-seen \`"digest"\`, printing when \`"org.opencontainers.image.title"\` matches target. The config digest (\`sha256:44136fa...\`) is a 2-byte \`{}\` blob — downloading it instead of the real binary causes \`gunzip: unexpected end of file\`. - -* **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. + +* **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\`. - -* **Stricli command context uses this.stdout not this.process.stdout**: In Stricli command \`func()\` handlers, use \`this.stdout\` and \`this.stderr\` directly — NOT \`this.process.stdout\`. The \`SentryContext\` interface has both \`process\` and \`stdout\`/\`stderr\` as separate top-level properties. Test mock contexts typically provide \`stdout\` but not a full \`process\` object, so \`this.process.stdout\` causes \`TypeError: undefined is not an object\` at runtime in tests even though TypeScript doesn't flag it. + +* **Multiple mockFetch calls replace each other — use unified mocks for multi-endpoint tests**: Bun test mocking gotchas: (1) \`mockFetch()\` replaces \`globalThis.fetch\` — calling it twice replaces the first mock. Use a single unified fetch mock dispatching by URL pattern. (2) \`mock.module()\` pollutes the module registry for ALL subsequent test files. Tests using it must live in \`test/isolated/\` and run via \`test:isolated\`. This also causes \`delta-upgrade.test.ts\` to fail when run alongside \`test/isolated/delta-upgrade.test.ts\` — the isolated test's \`mock.module()\` replaces \`CLI\_VERSION\` for all subsequent files. (3) For \`Bun.spawn\`, use direct property assignment in \`beforeEach\`/\`afterEach\`. - -* **Stricli defaultCommand blends default command flags into route completions**: When a Stricli route map has \`defaultCommand\` set, requesting completions for that route (e.g. \`\["issues", ""]\`) returns both the subcommand names AND the default command's flags/positional completions. This means completion tests that compare against \`extractCommandTree()\` subcommand lists will fail for groups with defaultCommand, since the actual completions include extra entries like \`--limit\`, \`--query\`, etc. Solution: track \`hasDefaultCommand\` in the command tree and skip strict subcommand-matching assertions for those groups. + +* **Stale nightly-test GHCR tag breaks delta patch generation via sort -V**: A stale \`nightly-test\` tag in GHCR poisoned ALL nightly delta patches. \`sort -V\` places \`nightly-test\` after all \`nightly-0.x.x\` tags. The \`generate-patches\` CI job runs BEFORE \`publish-nightly\`, so the current version's tag doesn't exist yet — the loop never breaks, and \`PREV\_TAG\` becomes \`nightly-test\` (which contains 0-byte binaries). Patches are generated from empty files, producing full-size patches that exceed the client's 60% budget. Fix: changed \`grep '^nightly-'\` to \`grep '^nightly-\[0-9]'\` in all 3 locations (ci.yml generate-patches, ci.yml publish-nightly, cleanup-nightlies.yml). Also need to manually delete the tag (GHCR package version ID 706511735). The cleanup workflow never removed it because it always sorted into the "keep latest 30" set. - -* **Upgrade command tests are flaky when run in full suite due to test ordering**: The \`upgradeCommand.func\` tests in \`test/commands/cli/upgrade.test.ts\` sometimes fail when run as part of the full \`bun run test:unit\` suite but pass consistently in isolation (\`bun test test/commands/cli/upgrade.test.ts\`). This is a test-ordering/state-leak issue, not a code bug. Don't chase these failures when your changes are unrelated to the upgrade command. + +* **useTestConfigDir without isolateProjectRoot causes DSN scanning of repo tree**: \`useTestConfigDir()\` creates temp dirs under \`.test-tmp/\` in the repo tree. Without \`{ isolateProjectRoot: true }\`, \`findProjectRoot\` walks up and finds the repo's \`.git\`, causing DSN detection to scan real source code and trigger network calls against test mocks (timeouts). Always pass \`isolateProjectRoot: true\` when tests exercise \`resolveOrg\`, \`detectDsn\`, or \`findProjectRoot\`. ### Pattern - -* **Bun global installs use .bun path segment for detection**: Bun global package installs place scripts under \`~/.bun/install/global/node\_modules/\`. The \`.bun\` directory in the path distinguishes bun from npm/yarn installs. In \`detectPackageManagerFromPath()\`, check \`segments.includes('.bun')\` before the npm fallback. Detection order: \`.pnpm\` → pnpm, \`.bun\` → bun, other \`node\_modules\` → npm. Yarn classic uses same flat layout as npm so falls through to npm detection — this is acceptable because path-based detection is a \*\*fallback\*\* after subprocess calls (which correctly identify yarn). Path detection must NOT be authoritative/override stored DB info, only serve as fallback when subprocess detection fails (e.g., Windows where .cmd files cause ENOENT). - - -* **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. + +* **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. - -* **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. + +* **Org-scoped SDK calls follow getOrgSdkConfig + unwrapResult pattern**: All org-scoped API calls in src/lib/api-client.ts: (1) call \`getOrgSdkConfig(orgSlug)\` for regional URL + SDK config, (2) spread into SDK function: \`{ ...config, path: { organization\_id\_or\_slug: orgSlug, ... } }\`, (3) pass to \`unwrapResult(result, errorContext)\`. Shared helpers \`resolveAllTargets\`/\`resolveOrgAndProject\` must NOT call \`fetchProjectId\` — commands that need it enrich targets themselves. - -* **Stricli buildCommand output config injects json flag into func params**: When a Stricli command uses \`output: { json: true, human: formatFn }\`, \`--json\` and \`--fields\` flags are auto-injected. The \`func\` handler receives these in its first parameter — type explicitly (e.g., \`flags: { json?: boolean }\`) to access them. Commands with interactive side effects (browser prompts, QR codes) should check \`flags.json\` and skip them when true. + +* **PR workflow: wait for Seer and Cursor BugBot before resolving**: CI includes Seer Code Review and Cursor Bugbot as advisory checks (~2-3 min, only on ready-for-review PRs). Workflow: push → wait for all CI (including npm build) → check inline review comments from Seer/BugBot → fix valid findings → repeat. Bugbot sometimes catches real logic bugs, not just style — always review before merging. Use \`gh pr checks \ --watch\` to monitor. Fetch comments via \`gh api repos/OWNER/REPO/pulls/NUM/comments\`. - -* **ZipWriter and file handle cleanup: always use try/finally with close()**: ZipWriter in \`src/lib/sourcemap/zip.ts\` has \`close()\` for error cleanup; \`finalize()\` uses try/finally for \`fh.close()\`. Callers must wrap usage in try/catch and call \`zip.close()\` in catch. Pattern: create zip → try { add entries + finalize } catch { zip.close(); throw }. Prevents file handle leaks on partial failures. + +* **Sentry SDK tree-shaking patches must be regenerated via bun patch workflow**: The CLI uses \`patchedDependencies\` in \`package.json\` to tree-shake unused exports from \`@sentry/core\` and \`@sentry/node-core\` (AI integrations, feature flags, profiler, etc.). When bumping SDK versions: (1) remove old patches and \`patchedDependencies\` entries, (2) \`rm -rf ~/.bun/install/cache/@sentry\` to clear bun's cache (edits persist in cache otherwise), (3) \`bun install\` fresh, (4) \`bun patch @sentry/core\` then edit files and \`bun patch --commit\`, repeat for node-core. Key preserved exports: \`\_INTERNAL\_safeUnref\`, \`\_INTERNAL\_safeDateNow\` (core), \`nodeRuntimeMetricsIntegration\` (node-core). Manually generating patch files with \`git diff\` may fail — bun expects specific hash formats. Always use \`bun patch --commit\` to generate patches. -### Preference + +* **Shared pagination infrastructure: buildPaginationContextKey and parseCursorFlag**: Schema v12 replaced \`pagination\_cursors.cursor TEXT\` with \`cursor\_stack TEXT\` (JSON array) + \`page\_index INTEGER\`. Stack-based API in \`src/lib/db/pagination.ts\`: \`resolveCursor(flag, key, contextKey)\` maps keywords (next/prev/previous/first/last) to \`{cursor, direction}\`. \`advancePaginationState(key, contextKey, direction, nextCursor)\` pushes/pops the stack — back-then-forward truncates stale entries. \`hasPreviousPage(key, contextKey)\` checks \`page\_index > 0\`. \`clearPaginationState(key)\` removes state. \`parseCursorFlag\` in \`list-command.ts\` accepts next/prev/previous/first/last keywords. \`paginationHint()\` in \`org-list.ts\` builds bidirectional hints (\`-c prev | -c next\`). JSON envelope includes \`hasPrev\` boolean. All 7 list commands (trace, span, issue, project, team, repo, dashboard) use this stack API. \`resolveCursor()\` must be called inside \`org-all\` override closures. - -* **Code style: Array.from() over spread for iterators, allowlist not whitelist**: User prefers \`Array.from(map.keys())\` over \`\[...map.keys()]\` for converting iterators to arrays (avoids intermediate spread). Use "allowlist" terminology instead of "whitelist" in comments and variable names. When a reviewer asks "Why not .filter() here?" — it may be a question, not a change request; the \`for..of\` loop may be intentionally more efficient. Confirm intent before changing. + +* **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). - -* **PR workflow: address review comments, resolve threads, wait for CI**: PR workflow: (1) Wait for CI, (2) Check unresolved review comments via \`gh api\`, (3) Fix in follow-up commits (not amends), (4) Reply explaining fix, (5) Resolve thread via \`gh api graphql\` \`resolveReviewThread\` mutation, (6) Push and re-check CI, (7) Final sweep. Use \`git notes add\` for implementation plans. Branch naming: \`fix/descriptive-slug\` or \`feat/descriptive-slug\`. + +* **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'\`). diff --git a/src/lib/delta-upgrade.ts b/src/lib/delta-upgrade.ts index 5ef561554..866148156 100644 --- a/src/lib/delta-upgrade.ts +++ b/src/lib/delta-upgrade.ts @@ -495,29 +495,79 @@ type ValidateChainOpts = { fullGzSize: number; }; +/** Reason a chain step failed validation */ +type ChainStepFailure = + | { reason: "version-mismatch"; expected: string; actual: string | null } + | { reason: "missing-layer"; layerName: string } + | { reason: "size-exceeded"; layerSize: number; budget: number }; + +/** Result of validating a single chain step */ +type ChainStepResult = + | { ok: true; digest: string; size: number } + | { ok: false; failure: ChainStepFailure }; + /** * Validate a single step in the nightly chain. * - * @returns Layer digest and size if valid, or null if the step is invalid + * Checks three conditions in order: + * 1. Manifest's `from-version` annotation matches the expected previous version + * 2. A layer with the expected platform title exists in the manifest + * 3. The layer's size fits within the remaining download budget + * + * @returns Layer digest and size on success, or a typed failure reason */ -function validateChainStep( +export function validateChainStep( manifest: OciManifest, opts: { expectedFrom: string; patchLayerName: string; sizeLimit: number } -): { digest: string; size: number } | null { +): ChainStepResult { const fromVersion = getPatchFromVersion(manifest); if (fromVersion !== opts.expectedFrom) { - return null; + return { + ok: false, + failure: { + reason: "version-mismatch", + expected: opts.expectedFrom, + actual: fromVersion, + }, + }; } const layer = manifest.layers.find((l) => { const title = l.annotations?.["org.opencontainers.image.title"]; return title === opts.patchLayerName; }); - if (!layer || layer.size > opts.sizeLimit) { - return null; + if (!layer) { + return { + ok: false, + failure: { reason: "missing-layer", layerName: opts.patchLayerName }, + }; + } + if (layer.size > opts.sizeLimit) { + return { + ok: false, + failure: { + reason: "size-exceeded", + layerSize: layer.size, + budget: opts.sizeLimit, + }, + }; } - return { digest: layer.digest, size: layer.size }; + return { ok: true, digest: layer.digest, size: layer.size }; +} + +/** Format a chain step failure reason for debug logging */ +function formatStepFailure(failure: ChainStepFailure): string { + switch (failure.reason) { + case "version-mismatch": + return `version mismatch (expected=${failure.expected}, actual=${failure.actual ?? "missing"})`; + case "missing-layer": + return `platform layer not found (expected=${failure.layerName})`; + case "size-exceeded": + return `patch too large (size=${failure.layerSize}, budget=${failure.budget})`; + default: + return "unknown failure"; + } } /** @@ -553,20 +603,20 @@ function validateNightlyChain( } const remainingBudget = fullGzSize * SIZE_THRESHOLD_RATIO - totalSize; - const step = validateChainStep(manifest, { + const result = validateChainStep(manifest, { expectedFrom: prevVersion, patchLayerName, sizeLimit: remainingBudget, }); - if (!step) { + if (!result.ok) { log.debug( - `Nightly chain step ${i + 1} failed validation (from=${prevVersion}, budget=${remainingBudget})` + `Nightly chain step ${i + 1} failed: ${formatStepFailure(result.failure)}` ); return null; } - digests.push(step.digest); - totalSize += step.size; + digests.push(result.digest); + totalSize += result.size; prevVersion = tag.slice(PATCH_TAG_PREFIX.length); if (i === manifests.length - 1) { diff --git a/test/lib/delta-upgrade.test.ts b/test/lib/delta-upgrade.test.ts index a7dec035b..748fff335 100644 --- a/test/lib/delta-upgrade.test.ts +++ b/test/lib/delta-upgrade.test.ts @@ -33,6 +33,7 @@ import { resolveNightlyDelta, resolveStableChain, resolveStableDelta, + validateChainStep, } from "../../src/lib/delta-upgrade.js"; import type { OciManifest } from "../../src/lib/ghcr.js"; @@ -702,6 +703,93 @@ describe("filterAndSortChainTags", () => { }); }); +// validateChainStep + +describe("validateChainStep", () => { + const PATCH_LAYER_NAME = `${getPlatformBinaryName()}.patch`; + + function makeLayer( + title: string, + size: number + ): OciManifest["layers"][number] { + return { + digest: `sha256:${title.replace(/\W/g, "")}`, + mediaType: "application/octet-stream", + size, + annotations: { "org.opencontainers.image.title": title }, + }; + } + + test("returns version-mismatch when from-version differs", () => { + const manifest = makePatchManifest("0.1.0", {}, [ + makeLayer(PATCH_LAYER_NAME, 500), + ]); + const result = validateChainStep(manifest, { + expectedFrom: "0.0.9", + patchLayerName: PATCH_LAYER_NAME, + sizeLimit: 100_000, + }); + expect(result).toEqual({ + ok: false, + failure: { + reason: "version-mismatch", + expected: "0.0.9", + actual: "0.1.0", + }, + }); + }); + + test("returns missing-layer when platform layer is absent", () => { + const manifest = makePatchManifest("0.0.9", {}, [ + makeLayer("sentry-other-platform.patch", 500), + ]); + const result = validateChainStep(manifest, { + expectedFrom: "0.0.9", + patchLayerName: PATCH_LAYER_NAME, + sizeLimit: 100_000, + }); + expect(result).toEqual({ + ok: false, + failure: { reason: "missing-layer", layerName: PATCH_LAYER_NAME }, + }); + }); + + test("returns size-exceeded when layer exceeds budget", () => { + const manifest = makePatchManifest("0.0.9", {}, [ + makeLayer(PATCH_LAYER_NAME, 200_000), + ]); + const result = validateChainStep(manifest, { + expectedFrom: "0.0.9", + patchLayerName: PATCH_LAYER_NAME, + sizeLimit: 100_000, + }); + expect(result).toEqual({ + ok: false, + failure: { + reason: "size-exceeded", + layerSize: 200_000, + budget: 100_000, + }, + }); + }); + + test("returns ok with digest and size on success", () => { + const manifest = makePatchManifest("0.0.9", {}, [ + makeLayer(PATCH_LAYER_NAME, 500), + ]); + const result = validateChainStep(manifest, { + expectedFrom: "0.0.9", + patchLayerName: PATCH_LAYER_NAME, + sizeLimit: 100_000, + }); + expect(result).toEqual({ + ok: true, + digest: `sha256:${PATCH_LAYER_NAME.replace(/\W/g, "")}`, + size: 500, + }); + }); +}); + // =================================================================== // Async functions (fetch-mocked) // ===================================================================