Skip to content

Commit 3af5252

Browse files
authored
fix: use shared git helpers for code-mappings repo inference (#1087)
## Summary Follow-up fix for #1086 — replaces buggy custom git helpers in `code-mappings upload` with the well-tested shared helpers from `src/lib/git.ts`. ## What was broken The `SSH_REMOTE_RE` regex (`:(.+?)(?:\.git)?$`) in `upload.ts` incorrectly matched HTTPS URLs because they contain `:` (in `https:`). This produced garbage repo names: | Remote URL | Expected | Got | |-----------|----------|-----| | `https://github.com/owner/repo.git` | `owner/repo` | `//github.com/owner/repo` | | `ssh://git@github.com:22/owner/repo.git` | `owner/repo` | `//git@github.com:22/owner/repo` | The corrupted name was sent to the Sentry API as the `repository` field. ## What this fixes 1. **Buggy regex replaced** — Uses `parseRemoteUrl()` from `src/lib/git.ts` which correctly tries `new URL()` first (handles https/ssh/git protocols) and only falls back to SCP-style regex when URL parsing fails. 2. **Shell injection risk removed** — Replaced `execSync` (shell) with `execFileSync` (no shell) via the shared `git()` helper. 3. **Code consolidated** — New `inferRepositoryName()` and `inferDefaultBranch()` functions in `git.ts` replace the duplicated logic in `upload.ts`. Both use `this.cwd` for correct working directory. 4. **ASCII art dividers removed** — Per AGENTS.md prohibited comment styles, replaced `// ── Section ───` dividers with plain `// Section` in code-mappings and dart-symbol-map files. ## Files changed | File | Change | |------|--------| | `src/lib/git.ts` | Added `inferRepositoryName()` and `inferDefaultBranch()` | | `src/commands/code-mappings/upload.ts` | Removed custom git helpers, imported from git.ts | | `src/lib/api/code-mappings.ts` | Removed ASCII art dividers | | `src/commands/dart-symbol-map/upload.ts` | Removed ASCII art dividers | | `src/lib/api/dart-symbols.ts` | Removed ASCII art dividers | Net: **66 insertions, 96 deletions** (less code, more correct)
1 parent 8197139 commit 3af5252

6 files changed

Lines changed: 66 additions & 96 deletions

File tree

.lore.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,6 @@
7878

7979
### Preference
8080

81-
<!-- lore:019ead3f-be6c-7168-9b32-e27cc888a801 -->
82-
* **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.
83-
8481
<!-- lore:019ea9dc-bdfe-7a4f-a5f9-f5c4d4acc42e -->
8582
* **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.
8683

@@ -89,3 +86,6 @@
8986

9087
<!-- lore:019e517a-f897-7dbd-8c96-a96ee5b9abaa -->
9188
* **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.
89+
90+
<!-- lore:019ead59-db61-71ae-9e62-24a3b5fd994f -->
91+
* **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.

src/commands/code-mappings/upload.ts

Lines changed: 7 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
* 5. Report created/updated/errors counts
1515
*/
1616

17-
import { execSync } from "node:child_process";
1817
import { readFile } from "node:fs/promises";
1918

2019
import type { SentryContext } from "../../context.js";
@@ -26,12 +25,13 @@ import { buildCommand } from "../../lib/command.js";
2625
import { ContextError, ValidationError } from "../../lib/errors.js";
2726
import { mdKvTable, renderMarkdown } from "../../lib/formatters/markdown.js";
2827
import { CommandOutput } from "../../lib/formatters/output.js";
28+
import { inferDefaultBranch, inferRepositoryName } from "../../lib/git.js";
2929
import { logger } from "../../lib/logger.js";
3030
import { resolveOrgAndProject } from "../../lib/resolve-target.js";
3131

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

34-
// ── Types ───────────────────────────────────────────────────────────
34+
// Types
3535

3636
/** Structured result for the upload command. */
3737
type CodeMappingsUploadResult = {
@@ -50,7 +50,7 @@ type CodeMappingsUploadResult = {
5050
}>;
5151
};
5252

53-
// ── Formatter ───────────────────────────────────────────────────────
53+
// Formatter
5454

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

@@ -82,81 +82,7 @@ function formatUploadResult(data: CodeMappingsUploadResult): string {
8282
return output;
8383
}
8484

85-
/** SSH remote URL pattern: git@host:path.git — captures the full path after `:` */
86-
const SSH_REMOTE_RE = /:(.+?)(?:\.git)?$/;
87-
/** HTTPS remote URL pattern: https://host/path.git — captures the path after the host */
88-
const HTTPS_REMOTE_RE = /^https?:\/\/[^/]+\/(.+?)(?:\.git)?$/;
89-
90-
// ── Helpers ─────────────────────────────────────────────────────────
91-
92-
/**
93-
* Infer the repository name and the remote used from local git remotes.
94-
*
95-
* Tries remotes in order: upstream → origin. Extracts `owner/repo` from
96-
* the remote URL. Falls back to the next remote if the URL can't be parsed.
97-
*
98-
* Returns both the repo name and which remote was used (for branch inference).
99-
*/
100-
function inferRepo(): { name: string; remote: string } | null {
101-
for (const remote of ["upstream", "origin"]) {
102-
try {
103-
const remoteUrl = execSync(`git remote get-url ${remote}`, {
104-
encoding: "utf-8",
105-
stdio: ["pipe", "pipe", "ignore"],
106-
}).trim();
107-
const name = extractRepoName(remoteUrl);
108-
if (name) {
109-
return { name, remote };
110-
}
111-
log.debug(`Could not parse repo name from '${remote}' URL: ${remoteUrl}`);
112-
} catch {
113-
log.debug(`No '${remote}' remote found`);
114-
}
115-
}
116-
return null;
117-
}
118-
119-
/**
120-
* Extract the repository path from a git remote URL.
121-
*
122-
* Handles HTTPS, SSH, and git:// URLs. Supports nested paths for
123-
* GitLab subgroups (e.g., `group/subgroup/project`).
124-
*/
125-
function extractRepoName(url: string): string | null {
126-
// SSH: git@github.com:owner/repo.git or git@gitlab.com:group/sub/project.git
127-
const sshMatch = url.match(SSH_REMOTE_RE);
128-
if (sshMatch?.[1]) {
129-
return sshMatch[1];
130-
}
131-
// HTTPS: https://github.com/owner/repo.git or https://gitlab.com/group/sub/project.git
132-
const httpsMatch = url.match(HTTPS_REMOTE_RE);
133-
if (httpsMatch?.[1]) {
134-
return httpsMatch[1];
135-
}
136-
return null;
137-
}
138-
139-
/**
140-
* Infer the default branch from a git remote's HEAD ref.
141-
*
142-
* @param remote - The remote name to check (e.g., "origin", "upstream")
143-
*/
144-
function inferDefaultBranch(remote: string): string {
145-
try {
146-
const output = execSync(`git symbolic-ref refs/remotes/${remote}/HEAD`, {
147-
encoding: "utf-8",
148-
stdio: ["pipe", "pipe", "ignore"],
149-
}).trim();
150-
// refs/remotes/origin/main → main
151-
const parts = output.split("/");
152-
return parts.at(-1) ?? "main";
153-
} catch {
154-
log.debug(
155-
`Could not infer default branch from '${remote}' remote HEAD, using 'main'`
156-
);
157-
return "main";
158-
}
159-
}
85+
// Helpers
16086

16187
/**
16288
* Read and validate the code mappings JSON file.
@@ -231,7 +157,7 @@ async function readAndValidateMappings(
231157
return mappings;
232158
}
233159

234-
// ── Command ─────────────────────────────────────────────────────────
160+
// Command
235161

236162
export const uploadCommand = buildCommand({
237163
auth: true,
@@ -304,7 +230,7 @@ export const uploadCommand = buildCommand({
304230
const { org, project } = resolved;
305231

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

325251
log.info(
326252
`Uploading ${mappings.length} code mapping(s) for ${org}/${project}${repository}`

src/commands/dart-symbol-map/upload.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import { resolveOrgAndProject } from "../../lib/resolve-target.js";
2323

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

26-
// ── Types ───────────────────────────────────────────────────────────
26+
// Types
2727

2828
/** Structured result for the upload command. */
2929
type DartSymbolMapUploadResult = {
@@ -39,7 +39,7 @@ type DartSymbolMapUploadResult = {
3939
uploaded: boolean;
4040
};
4141

42-
// ── Formatter ───────────────────────────────────────────────────────
42+
// Formatter
4343

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

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

61-
// ── Helpers ─────────────────────────────────────────────────────────
61+
// Helpers
6262

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

155-
// ── Command ─────────────────────────────────────────────────────────
155+
// Command
156156

157157
export const uploadCommand = buildCommand({
158158
// Auth is not required for --no-upload (dry-run mode).

src/lib/api/code-mappings.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { apiRequestToRegion } from "./infrastructure.js";
1717

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

20-
// ── Schemas ─────────────────────────────────────────────────────────
20+
// Schemas
2121

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

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

52-
// ── Constants ───────────────────────────────────────────────────────
52+
// Constants
5353

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

57-
// ── Types ───────────────────────────────────────────────────────────
57+
// Types
5858

5959
/** Options for {@link uploadCodeMappings}. */
6060
export type CodeMappingsUploadOptions = {
@@ -73,7 +73,7 @@ export type MergedCodeMappingsResponse = {
7373
mappings: CodeMappingResult[];
7474
};
7575

76-
// ── API Function ────────────────────────────────────────────────────
76+
// API Function
7777

7878
/**
7979
* Upload code mappings to Sentry via the bulk endpoint.

src/lib/api/dart-symbols.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import { apiRequestToRegion } from "./infrastructure.js";
3030

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

33-
// ── Types ───────────────────────────────────────────────────────────
33+
// Types
3434

3535
/** A single dart symbol map file to upload. */
3636
export type DartSymbolMap = {
@@ -52,7 +52,7 @@ export type DartSymbolMapUploadOptions = {
5252
mapping: DartSymbolMap;
5353
};
5454

55-
// ── Schemas ─────────────────────────────────────────────────────────
55+
// Schemas
5656

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

6363
type DifAssembleResponse = z.infer<typeof DifAssembleResponseSchema>;
6464

65-
// ── Helpers ─────────────────────────────────────────────────────────
65+
// Helpers
6666

6767
/** Result of checking a DIF assemble response. */
6868
type AssembleCheckResult = {
@@ -110,7 +110,7 @@ function checkAssembleResponse(
110110
return { allDone: false, missingChecksums };
111111
}
112112

113-
// ── API Function ────────────────────────────────────────────────────
113+
// API Function
114114

115115
/**
116116
* Upload a dart symbol map to Sentry via the DIF chunk-upload protocol.

src/lib/git.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,3 +228,47 @@ export function parseRemoteUrl(url: string): string | undefined {
228228

229229
return;
230230
}
231+
232+
/**
233+
* Infer the repository name from local git remotes.
234+
*
235+
* Tries remotes in priority order: upstream → origin. Falls back to the
236+
* next remote if the URL can't be parsed.
237+
*
238+
* @param cwd - Working directory
239+
* @returns The repo name and which remote it came from, or undefined
240+
*/
241+
export function inferRepositoryName(
242+
cwd?: string
243+
): { name: string; remote: string } | undefined {
244+
for (const remote of ["upstream", "origin"]) {
245+
try {
246+
const url = git(["remote", "get-url", remote], cwd);
247+
const name = parseRemoteUrl(url);
248+
if (name) {
249+
return { name, remote };
250+
}
251+
} catch {
252+
// Remote doesn't exist, try next
253+
}
254+
}
255+
return;
256+
}
257+
258+
/**
259+
* Infer the default branch from a git remote's HEAD ref.
260+
*
261+
* @param remote - The remote name (e.g., "origin", "upstream")
262+
* @param cwd - Working directory
263+
* @returns The branch name, or "main" as fallback
264+
*/
265+
export function inferDefaultBranch(remote: string, cwd?: string): string {
266+
try {
267+
const output = git(["symbolic-ref", `refs/remotes/${remote}/HEAD`], cwd);
268+
// refs/remotes/origin/main → main
269+
const parts = output.split("/");
270+
return parts.at(-1) ?? "main";
271+
} catch {
272+
return "main";
273+
}
274+
}

0 commit comments

Comments
 (0)