Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .lore.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@

### Preference

<!-- lore:019ead3f-be6c-7168-9b32-e27cc888a801 -->
* **Always batch multiple fixes into a single implementation pass with tests verified after**: When addressing code review findings, the user groups all fixes into a single batch (e.g., 'Fix 1 through Fix 6') and applies them together in one commit/pass, then runs the full test suite once at the end to confirm all tests pass. The user does not apply fixes incrementally with separate test runs between each fix. Fixes are prioritized by severity (Critical → Medium → Low → Nit) but all delivered together. The user expects the assistant to implement all requested fixes in one coordinated effort and report a single consolidated test result.

<!-- lore:019ea9dc-bdfe-7a4f-a5f9-f5c4d4acc42e -->
* **Always fix lint errors immediately after they are flagged, even if it overrides prior directives**: When Biome (or any linter) flags an error in modified files, the user expects it to be fixed right away — even if a prior directive said otherwise (e.g., 'keep handleScriptOutput async' was overridden when the linter flagged useAwait). The user does not defer lint fixes or leave them as known issues. After applying a fix, always re-run the linter to confirm a clean result before considering the task done. Pre-existing lint errors in unmodified files are noted but not fixed unless they are in files the current work touches.

Expand All @@ -89,3 +86,6 @@

<!-- lore:019e517a-f897-7dbd-8c96-a96ee5b9abaa -->
* **Always migrate Bun-specific APIs and tooling to Node.js equivalents**: Bun→Node.js migration complete. Replace Bun APIs: \`Bun.spawn\`→\`node:child\_process\`, \`Bun.sleep\`→\`node:timers/promises\`, \`bun:sqlite\`→\`node:sqlite\`, \`bun run\`→\`pnpm run\`/\`tsx\`, \`Bun.file().text()\`→\`readFile(path,'utf-8')\`, \`Bun.write()\`→\`writeFile()\`, \`Bun.which()\`→Node-compatible pkg, \`Bun.Glob\`→\`tinyglobby\`/\`picomatch\`, \`Bun.randomUUIDv7()\`→\`uuidv7\`, \`Bun.semver.order()\`→\`semver.compare()\`, \`Bun.zstdCompressSync()\`→zlib/\`zstd-napi\`. Exception: \`script/build.ts\` uses fossilize (not \`Bun.build\`) and stays on Bun for build-binary CI job. \`script/bundle.ts\` uses esbuild via tsx. \`packageManager\`: \`pnpm@10.11.0\`. bun.lock deleted, vitest.config.ts added. \`.npmrc\`: \`node-linker=hoisted\`. \`patchedDependencies\` moved to \`pnpm\` config block. \`NODE\_VERSION='lts'\`. \`new Worker(new URL(...))\` HANGS in SEA — use Blob+URL.createObjectURL. \`textImportPlugin\` handles \`with { type: 'text' }\` (inline as string) and \`with { type: 'file' }\` (pre-bundle sidecar) for esbuild. Prefer \`import { setTimeout } from 'node:timers/promises'\` over \`new Promise((r) => setTimeout(r, ms))\` — the latter is used in chunk-upload.ts/proguard.ts but is inconsistent with broader project direction.

<!-- lore:019ead59-db61-71ae-9e62-24a3b5fd994f -->
* **Always verify fixes by running tests and confirming all pass**: After applying any code fix or change, the user consistently runs the relevant test suite and expects confirmation that all tests pass (with exact counts and duration). The user also expects new tests to be added for each non-trivial fix to verify the specific behavior changed. Test results should be reported with file names, test counts, and timing. If tests fail, the session is not considered complete. This pattern applies to bug fixes, refactors, and feature additions alike.
88 changes: 7 additions & 81 deletions src/commands/code-mappings/upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* 5. Report created/updated/errors counts
*/

import { execSync } from "node:child_process";
import { readFile } from "node:fs/promises";

import type { SentryContext } from "../../context.js";
Expand All @@ -26,12 +25,13 @@ import { buildCommand } from "../../lib/command.js";
import { ContextError, ValidationError } from "../../lib/errors.js";
import { mdKvTable, renderMarkdown } from "../../lib/formatters/markdown.js";
import { CommandOutput } from "../../lib/formatters/output.js";
import { inferDefaultBranch, inferRepositoryName } from "../../lib/git.js";
import { logger } from "../../lib/logger.js";
import { resolveOrgAndProject } from "../../lib/resolve-target.js";

const log = logger.withTag("code-mappings.upload");

// ── Types ───────────────────────────────────────────────────────────
// Types

/** Structured result for the upload command. */
type CodeMappingsUploadResult = {
Expand All @@ -50,7 +50,7 @@ type CodeMappingsUploadResult = {
}>;
};

// ── Formatter ───────────────────────────────────────────────────────
// Formatter

const USAGE_HINT = "sentry code-mappings upload <path>";

Expand Down Expand Up @@ -82,81 +82,7 @@ function formatUploadResult(data: CodeMappingsUploadResult): string {
return output;
}

/** SSH remote URL pattern: git@host:path.git — captures the full path after `:` */
const SSH_REMOTE_RE = /:(.+?)(?:\.git)?$/;
/** HTTPS remote URL pattern: https://host/path.git — captures the path after the host */
const HTTPS_REMOTE_RE = /^https?:\/\/[^/]+\/(.+?)(?:\.git)?$/;

// ── Helpers ─────────────────────────────────────────────────────────

/**
* Infer the repository name and the remote used from local git remotes.
*
* Tries remotes in order: upstream → origin. Extracts `owner/repo` from
* the remote URL. Falls back to the next remote if the URL can't be parsed.
*
* Returns both the repo name and which remote was used (for branch inference).
*/
function inferRepo(): { name: string; remote: string } | null {
for (const remote of ["upstream", "origin"]) {
try {
const remoteUrl = execSync(`git remote get-url ${remote}`, {
encoding: "utf-8",
stdio: ["pipe", "pipe", "ignore"],
}).trim();
const name = extractRepoName(remoteUrl);
if (name) {
return { name, remote };
}
log.debug(`Could not parse repo name from '${remote}' URL: ${remoteUrl}`);
} catch {
log.debug(`No '${remote}' remote found`);
}
}
return null;
}

/**
* Extract the repository path from a git remote URL.
*
* Handles HTTPS, SSH, and git:// URLs. Supports nested paths for
* GitLab subgroups (e.g., `group/subgroup/project`).
*/
function extractRepoName(url: string): string | null {
// SSH: git@github.com:owner/repo.git or git@gitlab.com:group/sub/project.git
const sshMatch = url.match(SSH_REMOTE_RE);
if (sshMatch?.[1]) {
return sshMatch[1];
}
// HTTPS: https://github.com/owner/repo.git or https://gitlab.com/group/sub/project.git
const httpsMatch = url.match(HTTPS_REMOTE_RE);
if (httpsMatch?.[1]) {
return httpsMatch[1];
}
return null;
}

/**
* Infer the default branch from a git remote's HEAD ref.
*
* @param remote - The remote name to check (e.g., "origin", "upstream")
*/
function inferDefaultBranch(remote: string): string {
try {
const output = execSync(`git symbolic-ref refs/remotes/${remote}/HEAD`, {
encoding: "utf-8",
stdio: ["pipe", "pipe", "ignore"],
}).trim();
// refs/remotes/origin/main → main
const parts = output.split("/");
return parts.at(-1) ?? "main";
} catch {
log.debug(
`Could not infer default branch from '${remote}' remote HEAD, using 'main'`
);
return "main";
}
}
// Helpers

/**
* Read and validate the code mappings JSON file.
Expand Down Expand Up @@ -231,7 +157,7 @@ async function readAndValidateMappings(
return mappings;
}

// ── Command ─────────────────────────────────────────────────────────
// Command

export const uploadCommand = buildCommand({
auth: true,
Expand Down Expand Up @@ -304,7 +230,7 @@ export const uploadCommand = buildCommand({
const { org, project } = resolved;

// 3. Resolve repository name and the remote it came from
const repoInfo = flags.repo ? null : inferRepo();
const repoInfo = flags.repo ? undefined : inferRepositoryName(this.cwd);
const repository = flags.repo ?? repoInfo?.name ?? null;
if (!repository) {
throw new ContextError(
Expand All @@ -320,7 +246,7 @@ export const uploadCommand = buildCommand({
// 4. Resolve default branch (from the same remote that provided the repo name)
const defaultBranch =
flags["default-branch"] ??
inferDefaultBranch(repoInfo?.remote ?? "origin");
inferDefaultBranch(repoInfo?.remote ?? "origin", this.cwd);

log.info(
`Uploading ${mappings.length} code mapping(s) for ${org}/${project} → ${repository}`
Expand Down
8 changes: 4 additions & 4 deletions src/commands/dart-symbol-map/upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { resolveOrgAndProject } from "../../lib/resolve-target.js";

const log = logger.withTag("dart-symbol-map.upload");

// ── Types ───────────────────────────────────────────────────────────
// Types

/** Structured result for the upload command. */
type DartSymbolMapUploadResult = {
Expand All @@ -39,7 +39,7 @@ type DartSymbolMapUploadResult = {
uploaded: boolean;
};

// ── Formatter ───────────────────────────────────────────────────────
// Formatter

const USAGE_HINT = "sentry dart-symbol-map upload --debug-id <uuid> <path>";

Expand All @@ -58,7 +58,7 @@ function formatUploadResult(data: DartSymbolMapUploadResult): string {
return renderMarkdown(mdKvTable(rows));
}

// ── Helpers ─────────────────────────────────────────────────────────
// Helpers

/** UUID format: 8-4-4-4-12 hex with hyphens. */
const UUID_RE =
Expand Down Expand Up @@ -152,7 +152,7 @@ async function readMappingFile(path: string): Promise<Buffer> {
}
}

// ── Command ─────────────────────────────────────────────────────────
// Command

export const uploadCommand = buildCommand({
// Auth is not required for --no-upload (dry-run mode).
Expand Down
8 changes: 4 additions & 4 deletions src/lib/api/code-mappings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { apiRequestToRegion } from "./infrastructure.js";

const log = logger.withTag("api.code-mappings");

// ── Schemas ─────────────────────────────────────────────────────────
// Schemas

/** A single code mapping entry. */
export const CodeMappingSchema = z.object({
Expand Down Expand Up @@ -49,12 +49,12 @@ export type BulkCodeMappingsResponse = z.infer<

export type CodeMappingResult = z.infer<typeof CodeMappingResultSchema>;

// ── Constants ───────────────────────────────────────────────────────
// Constants

/** Maximum mappings per API request. */
const BATCH_SIZE = 300;

// ── Types ───────────────────────────────────────────────────────────
// Types

/** Options for {@link uploadCodeMappings}. */
export type CodeMappingsUploadOptions = {
Expand All @@ -73,7 +73,7 @@ export type MergedCodeMappingsResponse = {
mappings: CodeMappingResult[];
};

// ── API Function ────────────────────────────────────────────────────
// API Function

/**
* Upload code mappings to Sentry via the bulk endpoint.
Expand Down
8 changes: 4 additions & 4 deletions src/lib/api/dart-symbols.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { apiRequestToRegion } from "./infrastructure.js";

const log = logger.withTag("api.dart-symbols");

// ── Types ───────────────────────────────────────────────────────────
// Types

/** A single dart symbol map file to upload. */
export type DartSymbolMap = {
Expand All @@ -52,7 +52,7 @@ export type DartSymbolMapUploadOptions = {
mapping: DartSymbolMap;
};

// ── Schemas ─────────────────────────────────────────────────────────
// Schemas

/**
* DIF assemble response — keyed by overall checksum, each value has
Expand All @@ -62,7 +62,7 @@ const DifAssembleResponseSchema = z.record(z.string(), AssembleResponseSchema);

type DifAssembleResponse = z.infer<typeof DifAssembleResponseSchema>;

// ── Helpers ─────────────────────────────────────────────────────────
// Helpers

/** Result of checking a DIF assemble response. */
type AssembleCheckResult = {
Expand Down Expand Up @@ -110,7 +110,7 @@ function checkAssembleResponse(
return { allDone: false, missingChecksums };
}

// ── API Function ────────────────────────────────────────────────────
// API Function

/**
* Upload a dart symbol map to Sentry via the DIF chunk-upload protocol.
Expand Down
44 changes: 44 additions & 0 deletions src/lib/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,3 +228,47 @@

return;
}

/**
* Infer the repository name from local git remotes.
*
* Tries remotes in priority order: upstream → origin. Falls back to the
* next remote if the URL can't be parsed.
*
* @param cwd - Working directory
* @returns The repo name and which remote it came from, or undefined
*/
export function inferRepositoryName(
cwd?: string
): { name: string; remote: string } | undefined {
for (const remote of ["upstream", "origin"]) {
try {
const url = git(["remote", "get-url", remote], cwd);
const name = parseRemoteUrl(url);
if (name) {
return { name, remote };
}
} catch {
// Remote doesn't exist, try next
}
}
return;
}

/**
* Infer the default branch from a git remote's HEAD ref.
*
* @param remote - The remote name (e.g., "origin", "upstream")
* @param cwd - Working directory
* @returns The branch name, or "main" as fallback
*/
export function inferDefaultBranch(remote: string, cwd?: string): string {
try {
const output = git(["symbolic-ref", `refs/remotes/${remote}/HEAD`], cwd);
// refs/remotes/origin/main → main
const parts = output.split("/");
return parts.at(-1) ?? "main";
} catch {
return "main";
}
}

Check warning on line 274 in src/lib/git.ts

View check run for this annotation

@sentry/warden / warden: find-bugs

inferDefaultBranch truncates branch names containing '/'

Splitting the ref by `/` and taking `.at(-1)` discards all but the last path segment, so a default branch like `release/2.0` (output: `refs/remotes/origin/release/2.0`) would be returned as `"2.0"` instead of `"release/2.0"`. Use `output.slice(\`refs/remotes/${remote}/\`.length) || "main"` instead.
Comment on lines +272 to +274

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inferDefaultBranch truncates branch names containing '/'

Splitting the ref by / and taking .at(-1) discards all but the last path segment, so a default branch like release/2.0 (output: refs/remotes/origin/release/2.0) would be returned as "2.0" instead of "release/2.0". Use output.slice(\refs/remotes/${remote}/`.length) || "main"` instead.

Evidence
  • git symbolic-ref refs/remotes/origin/HEAD emits refs/remotes/origin/release/2.0 for a default branch named release/2.0.
  • In inferDefaultBranch, output.split("/") produces ["refs", "remotes", "origin", "release", "2.0"] and .at(-1) returns only "2.0", dropping the release/ prefix.
  • The ?? "main" guard is ineffective: split("/") always returns a non-empty array, so .at(-1) is never undefined; an empty output yields "" (falsy, not nullish) and still bypasses the fallback.
  • The truncated branch flows through inferDefaultBranch(repoInfo?.remote ?? "origin", this.cwd) into uploadCodeMappings({ ... }) in src/commands/code-mappings/upload.ts.

Identified by Warden find-bugs · VXV-VGS

Loading